Invalidate Glide disk cache when new images are received #6432
Conversation
grzesiek2010
force-pushed
the
COLLECT-6419
branch
2 times, most recently
from
13620b7 to
0398a4b
Compare
There was a problem hiding this comment.
Those images are pretty big and will forever increase the size of the repo. Could you please replace them with really small images? I don't think they need to be more than 1x1 squares of different colors. You'll need to rebase the commit introducing the images.
Are you sure that update time is stable across updates without changes to media files? While troubleshooting the original issue I noticed that I was always getting updated timestamps for all media files. I didn't investigate why but it seems to me like there might be an issue there.
grzesiek2010
force-pushed
the
COLLECT-6419
branch
from
0398a4b to
22e2c2f
Compare
I thought I was sure, but you're right. Each updated form creates a new directory, so even though we try to optimize the process by not downloading media attachments we already have, we still have to copy the existing ones from the old location to the new one, which changes the timestamp. This isn't a bug, but it forces Glide to load a new image (not ideal, but manageable). I've updated the fix to use an MD5 hash instead. |
|
@getodk/testers I imagine you already have a good protocol for testing this area but just in case you don't have it captured formally anywhere, I think it's important to use as slow of a device as you can and as big of an image as you can. In addition to checking correctness, let's do a brief check of performance. I imagine this will be slightly slower but it shouldn't be very noticeable. |
lognaturel
changed the title
This feels surprising to me. I think it may also explain some unexpected behavior a user recently reported -- they were seeing multiple different form directories with full media even though the form version/definition had never changed. That suggests to me that there might be an issue with cleanup. Is there a reason we can't do the form attachment updating in place if the form definition hasn't changed? |
That's not the scenario I have described I believe because:
It's not possible to update attachments without bumping the form version right? At least it's not possible with Central. |
There are two cases in which it's possible using Central: drafts (that's how this issue was discovered) and Entities. The user issue was reported for a form with Entities. They noticed a few unexpected directories with Entity Lists from different points in time. Given what we've described here, it seems possible that they weren't cleaned up correctly for some reason. Other servers do more liberally allow updating form attachments without changing the form version. |
|
I've tested drafts, and it's true that all files end up with updated timestamps. We have a mechanism in place to prevent downloading media files that are already available. This was originally implemented for form updates, as we would have encountered the issue with drafts otherwise. The mechanism searches for existing media files in other directories created for the same form (but older versions) before downloading a new one. If a matching file is found, it’s copied to the new location instead of being re-downloaded. Unfortunately, it's not optimized, so if a new directory isn't created (such as in the case of drafts or entities) the file is copied regardless. |
|
Tested with Success! Verified on device with Android 10,14 Verified cases:
|
Closes #6419
Why is this the best possible solution? Were any other approaches considered?
According to Glide's documentation, we need to assist Glide in determining when cached images should be invalidated.
There are several options to achieve this, and I chose to base it on the last modification date of the image file which seems to be simple and reliable.Now uses MD5 hash because timestamp updates with each form update.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This pull request should just fix the problem with cached images described in the issue.
Please note that we also support SVG images, which are handled differently from other image types, so it’s important to test them as well. For example, I recently forgot to apply this fix to SVG images: link to PR, but I've now included it.
Do we need any specific form for testing your changes? If so, please attach one.
I used this one:
imgCacheTest.zip
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest