Actually move images to the intended cache directory and ignore data URIs #6053
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…URIs Just realized that the `moveIfNecessary` function wasn't working as intended, the images never actually gets moved to our intended cache directory because `from.startsWith(cacheDir)` will always return false for those images. Added an extra check to not do it for data URIs as well, to handle images coming from share intents, and images coming from local drafts (which I'm still working on).
|
what’s the user visible consequence of the bug? |
|
Nothing user-visible, we weren't actually deleting the temporary images as they weren't ever moved there to begin with. |
|
sorry, this sounds silly but can you help me understand how to test this? what is the expected and the actual behavior (e.g. if i check the directory on the device). |
|
will find some time to write up a test case tomorrow! It is definitely hard to see what's going on, I'm not sure if file explorer apps on Android are able to peek into our cache directory so we might just have to check by putting console.logs on the before&after |
|
i think i was able to inspect it with Android Studio in the past, just need to better understand what to look for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Need additional verification, cc @gaearon
Just realized that the
moveIfNecessaryfunction wasn't working as intended, the images never actually gets moved to our intended cache directory becausefrom.startsWith(cacheDir)will always return false for those images.Added an extra check to not do it for data URIs as well, to handle images coming from share intents, and images coming from local drafts (which I'm still working on).