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

Refactor Image Loading: Use Internal Storage with Content URI for Enhanced Security and Compatibility #17846

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

Scapesfear
Copy link
Contributor

@Scapesfear Scapesfear commented Jan 20, 2025

Purpose / Description

  • Internalizing Shared Images: Temporarily storing shared images in the app's internal cache for secure handling.
  • Using Content:// URIs: Leveraging FileProvider to generate secure, modern, and Android-compliant URIs instead of direct file paths.
  • Secure WebView Loading: Loading images in WebView using content:// URIs with a controlled HTML structure, ensuring compatibility.

Fixes

Fixes #17776
Superseeds and closes #17778

Approach

Internalizing Shared Images:
The shared image is first internalized, ensuring it’s stored temporarily within AnkiDroid's private storage, making it accessible only to the app. This resolves any issues related to image accessibility or permission errors.

Using content:// URIs:
By utilizing Android's FileProvider to generate content:// URIs, the PR avoids direct file path access, which may fail on newer Android versions (Android 10 and above). The content:// URI provides a safe, standardized way to share and access files between apps, complying with Android’s modern file access restrictions.

WebView Compatibility:
The PR fixes the issue where images weren't displaying by using the content:// URI format in WebView. This ensures proper loading of images in the app without needing risky settings, which could expose sensitive data or create security vulnerabilities.

How Has This Been Tested?

Manual Testing on Samsung F23 5G

test_loadimage.mp4

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it should work, and this will be a nice improvement, thank you

Left a style note about shifting to a quick-return vs deep if/else nesting

Left a note that's a must-change (but an easy one) about log levels

@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jan 20, 2025
@Scapesfear Scapesfear requested a review from mikehardy January 21, 2025 05:09
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me - it.exists() may not be strictly necessary but a little extra checking can't hurt here

Timber.w now - thank you, early returns so nesting is avoided but error conditions are cleanly dealt with one by one in an obvious way - thank you

Good call-out from David to indicate to the user something went wrong, that's a good addition

This will be a nice fix for the share case, and now in combination with the crop fix where png/webp go through, we should have a "really, it works!" feature for sharing images to ankidroid from the context menu.

That add image via share feature broke when we started using the WebView in order to add sharing an SVG as a feature - which is a lesson in unintended consequences! But now all working I think, jpg, svg, png, webp

@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author labels Jan 21, 2025
@Scapesfear Scapesfear requested a review from criticalAY January 21, 2025 18:13
@mikehardy mikehardy added the 2.21 label Jan 22, 2025
@mikehardy mikehardy added this to the 2.21 release milestone Jan 22, 2025
@mikehardy mikehardy added the Review High Priority Request for high priority review label Jan 22, 2025
@mikehardy
Copy link
Member

mikehardy commented Jan 22, 2025

Marked for high priority review so we can get this in (hopefully)

QA ✅ - I have pulled the PR locally and confirm that images shared (from, for instance, android files app or gallery) are now correctly displayed in the previewer when you use "AnkiDroid - Add Image" system share target, so this passes testing

Still doesn't preserve transparency on crop but that's separate of course - I'm surprised that #17840 isn't in yet! - will push on that one

@mikehardy mikehardy added the squash-merge The pull request currently requires maintainers to "Squash Merge" label Jan 22, 2025
Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM then

Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!
Please squash the two commits.

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Jan 23, 2025
@lukstbit
Copy link
Member

Thanks!

@lukstbit lukstbit enabled auto-merge January 23, 2025 12:33
@lukstbit lukstbit added this pull request to the merge queue Jan 23, 2025
@lukstbit lukstbit removed the squash-merge The pull request currently requires maintainers to "Squash Merge" label Jan 23, 2025
Merged via the queue into ankidroid:main with commit b0d3758 Jan 23, 2025
9 checks passed
@github-actions github-actions bot removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) Review High Priority Request for high priority review labels Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image Shared to AnkiDroid Doesn't Load
5 participants