Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media is not attached after relaunching the app #12060

Closed
shiki opened this issue Jul 3, 2019 · 8 comments
Closed

Media is not attached after relaunching the app #12060

shiki opened this issue Jul 3, 2019 · 8 comments

Comments

@shiki
Copy link
Member

shiki commented Jul 3, 2019

Found in #11839 (comment) by @yaelirub.

Expected behavior

Media is attached to the post when the app is relaunched.

Actual behavior

The media upload resumes when the app was relaunched but the image was never attached to the post and only a placeholder is visible.

Steps to reproduce the behavior

This will be easier to reproduce if running the app from Xcode.

  1. Go offline.
  2. Run the app in Xcode.
  3. Create a post with an image.
  4. Exit the Editor and save the draft.
  5. Stop app by stopping Xcode.
  6. Run the app in Xcode again.
  7. Open the post again. Notice that the image will no longer be shown, just the placeholder.
@shiki
Copy link
Member Author

shiki commented Jul 5, 2019

@diegoreymendez After some more investigation, I found that there are 2 main problems here. They are the usage of full path URLs and using the cache directory.

Problem A: Full Path URLs

After we attach an image and we save the post, the content will look like this:

<p>
    <img
        src="file:///var/mobile/Containers/Data/Application/92BED2B8-0498-41DF-BE0B-D0B24F3EAC9F/Library/Caches/Media/thumbnail-p3-2436x2436.jpeg"
        class="size-full"
        data-wp_upload_id="x-coredata://D5CF762D-7FB1-450D-B5A2-989DC0FC70B9/Media/p3"
    >
</p>

Notice that the <img> src is the full path to the image in the file system. The issue here is that we cannot fully rely on the application data path represented by this part of that path:

/var/mobile/Containers/Data/Application/92BED2B8-0498-41DF-BE0B-D0B24F3EAC9F/

That UUID folder will change every time we run the app on Xcode. For example, when I run the app in Xcode again and create a new draft, the post content will look like this:

<p>
    <img
        src="file:///var/mobile/Containers/Data/Application/6B2A2AF7-40B1-4ACB-BE59-A4D0095B288F/Library/Caches/Media/thumbnail-p4-2436x2436.jpeg"
        class="size-full"
        data-wp_upload_id="x-coredata://D5CF762D-7FB1-450D-B5A2-989DC0FC70B9/Media/p4"
    >
</p>

Notice that the src is pointing to a different base path with a different UUID folder. This is the reason why the image is never shown again when the user reopens the post. Contrary to my previous guess, the image file was not deleted, the base folder (UUID) was just renamed to a different one. We were just pointing to the old file path.

Based on some light testing, it looks like this happens when the user upgrades the app from the App Store too. This does not happen when the user just force closes the app or the app crashes.

References

These observations are corroborated by these references:

Possible Solution

It looks like the right solution would be to not rely on the full path and use a relative path. For example, for this post content:

<img
    src="file:///var/mobile/Containers/Data/Application/6B2A2AF7-40B1-4ACB-BE59-A4D0095B288F/Library/Caches/Media/thumbnail-p4-2436x2436.jpeg"
>

We will replace the src with:

<img src="file://Library/Caches/Media/thumbnail-p4-2436x2436.jpeg" >

We will then convert this back to the full path when the post is reopened in the Editor. This will guarantee that we will be using the current application data path. This type of reference will survive across app upgrades.

It does not stop here though. It looks like the Post List also relies on the full path to show the image by using Post.pathForDisplayImage which is filled in by updatePathForDisplayImageBasedOnContent:

- (void)updatePathForDisplayImageBasedOnContent
{
// First lets check the post content for a suitable image
NSString *result = [DisplayableImageHelper searchPostContentForImageToDisplay:self.content];
if (result.length > 0) {
self.pathForDisplayImage = result;
}
// If none found let's see if some galleries are available
NSSet *mediaIDs = [DisplayableImageHelper searchPostContentForAttachmentIdsInGalleries:self.content];
for (Media *media in self.blog.media) {
NSNumber *mediaID = media.mediaID;
if (mediaID && [mediaIDs containsObject:mediaID]) {
result = media.remoteURL;
}
}
self.pathForDisplayImage = result;
}

There might be more so we'll have to make adjustments to those places too.

Problem B: Cache Directory Usage

The usage of Library/Cache can still be a problem. The docs say:

Put data cache files in the Library/Caches/ directory. Cache data can be used for any data that needs to persist longer than temporary data, but not as long as a support file. Generally speaking, the application does not require cache data to operate properly, but it can use cache data to improve performance. Examples of cache data include (but are not limited to) database cache files and transient, downloadable content. Note that the system may delete the Caches/ directory to free up disk space, so your app must be able to re-create or download these files as needed.

It looks like the Library/Cache has a tendency to survive longer than the temporary folder. Still, we have to consider it as unreliable.

Possible Solutions

Use a Persistent Folder

In the case of "thumbnails" generated for use in the Editor and Post List, I believe they shouldn't be in the Library/Cache but in Documents instead. They should:

  1. Be created when a media is attached.
  2. Deleted once the media is uploaded to the server.

It looks like this can be done by changing the MediaThumbnailExporter.mediaDirectoryType to .uploads so the storage is persistent.

// Configure a thumbnail exporter.
let exporter = MediaThumbnailExporter()
exporter.mediaDirectoryType = .cache

I have not fully explored the repercussions of this. There might be other places that are using that method.

On-the-fly Generation

Another possible solution is if we can solve Problem A, we can try having an on-the-fly generation of the thumbnail. For example, I discussed above that the URL should be changed to this:

<img src="file://Library/Caches/Media/thumbnail-p4-2436x2436.jpeg">

Since that is technically a placeholder now, we could insert additional data in there, like the mediaID:

<img src="file://Library/Caches/Media/thumbnail-p4-2436x2436.jpeg?mediaID=1234">

We can then use that mediaID to generate the thumbnail if it is no longer available in Library/Cache. With this, we can keep the thumbnails in Library/Cache and do not have to worry about when to delete them.

Final Thoughts

I'll stop here for now until I hear your opinions. 🙂

I also have some questions for you to ponder:

  • Now that we've found that this will most probably happen on app upgrades only, will this change the priority?
  • The effort required to fix this is quite huge. Is it worth it to proceed?

@yaelirub and @jklausa, please let me know if you have any ideas too.

@shiki shiki added 13 and removed 2 labels Jul 5, 2019
@yaelirub
Copy link
Contributor

yaelirub commented Jul 9, 2019

Great research, @shiki!
I can't believe we don't run into more issues (or maybe we do?) by caching the full path. Since the solution to Problem A is pretty clear and needs to be done regardless I think we should split this ticket.
Updating to relative paths shouldn't be too hard and is something we should do imo.

Is Problem B a current issue?

How do you feel about splitting the ticket into an implementation for the relative paths (should probably be estimated between 3-5 imo) and a discussion ticket for the second issue that doesn't necessarily have to live in the offline support project board but instead maybe in Groundskeeping ?

@shiki
Copy link
Member Author

shiki commented Jul 9, 2019

Thank you for responding, @yaelirub.

Is Problem B a current issue?

It is an issue but my guess is it's not likely to happen. It's probably okay to move it to the Groundskeeping backlog.

@yaelirub
Copy link
Contributor

yaelirub commented Jul 9, 2019

Great! Let's create a new ticket for the relative path implementation and move it to the backlog with high priority.

And another one for the cache usage to the backlog in Groundskeeping.

I believe we can remove the 13 from this one and close #11846 once the relative path implementation is done

Thank you, @shiki

@yaelirub
Copy link
Contributor

@shiki , any updates on this ^ ?

@shiki
Copy link
Member Author

shiki commented Jul 16, 2019

@yaelirub Thanks for the nudge. It's been in my list of things to do. Since we're revisiting our list of issues to tackle though, do you think we should still work on this as part of Offline Posting or should we keep this as is and move everything to Groundskeeping?

@yaelirub
Copy link
Contributor

yaelirub commented Jul 16, 2019

@shiki , I still stand by my previous suggestion:

How do you feel about splitting the ticket into an implementation for the relative paths (should probably be estimated between 3-5 imo) and a discussion ticket for the second issue that doesn't necessarily have to live in the offline support project board but instead maybe in Groundskeeping ?

@shiki
Copy link
Member Author

shiki commented Jul 16, 2019

Split this issue into #12142 and #12143.

@shiki shiki closed this as completed Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants