Fix sharing image regression and add support for heic and heif files #20138

geriux · 2024-02-07T09:57:32Z

Related PRs:

MediaUtils - Add support for heic and heif image formats WordPress-Utils-Android#142

Fixes an issue where some shared images into the app aren't being loaded in a post/media library correctly.

To address this problem, it adds a fallback when getRealPathFromURI returns an empty string, where it will return the original uri to the media type validation functions.

It also includes a change in WordPress Utils where it adds the heic and heif formats.

I've also created this #20140 where it captures an issue where in some cases it's not possible to share an image to the app due to permissions denial. To prevent a crash, a try/catch has been added until that issue is solved.

To Test:

Share an image from an image editing app

Open an image editing app like Lightroom
Edit an image and share it to the Jetpack app
Tap on Add to new post
Expect the editor to be opened and the image to be loaded

Share an image from a browser

Open a web browser
Go to any website and long-press on an image
Share the image to the Jetpack app
Tap on Add to new post
Expect the editor to be opened and the image to be loaded

Share an image from the Photos app

Open the Photos app
Select an image or images
Share the image(s) to the Jetpack app
Tap on Add to new post
Expect the editor to be opened and the image(s) to be loaded

Share an image from a Files Explorer app

Open the Files Explorer app
Select an image or images
Share the image(s) to the Jetpack app
Tap on Add to new post
Expect the editor to be opened and the image(s) to be loaded

Share a HEIC image

Open the Gallery/Files app
Select a heic image
Share the image to the Jetpack app
Tap on Add to new post
Expect the editor to be opened and the image to be loaded

Regression Notes

Potential unintended areas of impact
- N/A, it should only affect sharing media through intents.
What I did to test those areas of impact (or what existing automated tests I relied on)
- Manual testing
What automated tests I added (or what prevented me from doing so)
- N/A, relied on manual testing.

PR Submission Checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist:

wpmobilebot · 2024-02-07T10:13:48Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20138-6c13ba8
	Commit	`6c13ba8`
	Direct Download	`wordpress-prototype-build-pr20138-6c13ba8.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-02-07T10:16:46Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20138-6c13ba8
	Commit	`6c13ba8`
	Direct Download	`jetpack-prototype-build-pr20138-6c13ba8.apk`

Note: Google Login is not supported on these builds.

…d crashes

peril-wordpress-mobile · 2024-02-07T14:27:16Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

dangermattic · 2024-02-07T14:27:40Z

	1 Warning
⚠️	This PR is assigned to the milestone 24.2 ❄️. The due date for this milestone has already passed. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

fluiddot · 2024-02-07T18:05:52Z

WordPress/src/main/java/org/wordpress/android/ui/ShareIntentReceiverActivity.java

+        // For cases when getRealPathFromURI returns an empty string
+        if (TextUtils.isEmpty(filePath)) {
+            filePath = String.valueOf(uri);
+        }


I wonder why the URI is not properly parsed. I debugged the code and found that it's caused by this line returning false. My presumption is that for URIs like the following it should work:

content://media/external/images/media/1000000246 (media file from Gallery app)

content://com.adobe.lrmobile.provider/myimages/20231013_1723402258340274679050296%20(17).jpg (media file from Adobe Lightroom app)

Might be related to the Android version, although I haven't found any references in the documentation.

This code was introduced in 2017, so it might be a long shot, but I wonder @daniloercoli as author, if you could provide any info about this logic. Thanks for the help 🙇 !

It might not be critical, but the fact that we extract the path from the URI makes me hesitant about whether we can simply convert it to string. Not sure if this might introduce undesired side-effects.

I realize that the function isAllowedMediaType was introduced recently in #19754:

WordPress-Android/WordPress/src/main/java/org/wordpress/android/ui/ShareIntentReceiverActivity.java

Lines 119 to 126 in 0c98841

private boolean isAllowedMediaType(@NonNull Uri uri) {

String filePath = MediaUtils.getRealPathFromURI(this, uri);

// For cases when getRealPathFromURI returns an empty string

if (TextUtils.isEmpty(filePath)) {

filePath = String.valueOf(uri);

}

return MediaUtils.isValidImage(filePath) || MediaUtils.isVideo(filePath);

}

This reduces my concern about providing a fallback in case the URI can't be processed, as mentioned in #20138 (comment). Hence, I think we can proceed with this approach.

Yeah I think the whole code that handles this should be revisited with the app permissions logic as well, which is something that was changed recently. The sharing media behavior is also different between OS versions and apps, so definitely worth looking into it to fix issues like this one #20140

fluiddot

@geriux I tested the different test cases and confirmed it fixes the issue and works as expected. From my side, these changes look good. However, I'd like to defer my approval to @wordpress-mobile/android-developers, especially due to the concerns raised in https://github.com/wordpress-mobile/WordPress-Android/pull/20138/files#r1481890026.

fluiddot

LGTM 🎊 !

As shared in #20138 (comment), I'm now more confident about this solution.

antonis · 2024-02-08T10:37:17Z

WordPress/src/main/java/org/wordpress/android/ui/ShareIntentReceiverActivity.java

-                mLocalMediaUris.add(MediaUtils.downloadExternalMedia(this, externalUri));
-            }
+        } catch (Exception e) {
+            AppLog.e(T.MEDIA, "ShareIntentReceiver failed to download media ", e);


It would be nice to communicate the error to the user here (e.g. with ToastUtils.showToast)
Note that I wasn't able to reach this Exception flow in my tests.

Good idea! I'll add it, thank you!

As a reference, this is how it will look like:

LoadMediaMessage.mov

I wonder if the error message could mention the sharing action, Unable to load media might be a bit cryptic to users. What about something like Unable to share media due to an error when loading, please try sharing other media files.. WDYT?

I thought about it but since this is to be included in the beta, I'm not sure how we usually handle new strings. Do you know if we are still in time to get it translated?

If the user encounters the permissions issue, any file will give an error by the way 😅

I thought about it but since this is to be included in the beta, I'm not sure how we usually handle new strings. Do you know if we are still in time to get it translated?

Ah, good point. I saw that the PR #20156 recently updated the translations, so maybe we are still on time. cc @spencertransier

If the user encounters the permissions issue, any file will give an error by the way 😅

True. I presume we'd need to check the error type to provide the correct details in the error message. In that case, we could either expand the message to include potential permissions issues or make it more generic:

Option 1: Unable to share media.

Option 2: Unable to share media due to an error when loading.

Option 3: Unable to share media due to an error when loading, please try sharing other media files or check app permissions.

Option 3: Unable to share media due to an error when loading, please try sharing other media files or check app permissions.

If we choose a new resource option 3 sounds better to me since it provides some guidance to the user. I'm not sure if the please try sharing other media files is helpful though if the user tries sharing with the same method/app. Wdyt of:
Unable to load the media for sharing. Please check the app's permissions or use the app's media library.?
If we go with a long text we should also pass the ToastUtils.Duration.LONG parameter.

Thank you both for the suggestions! 🙇‍♂️ I have updated in 42ad16f Once CI checks pass I'll merge this PR and ask for the translations process internally.

antonis

Thank you for your work on this @geriux 🙇
I was able to reproduce the image loading failure with Adobe Lightroom on a Pixel 5 (Android 14) and confirmed that the introduced fix works.
The code changes also look consistent 🎉
I left a minor non blocking suggestion with https://github.com/wordpress-mobile/WordPress-Android/pull/20138/files#r1482753237

wpmobilebot · 2024-02-12T07:55:54Z

Found 1 violations:

The PR caused the following dependency changes:

-\--- org.wordpress:utils:{strictly 3.12.0} -> 3.12.0
+\--- org.wordpress:utils:{strictly 3.13.0} -> 3.13.0

Please review and act accordingly

Gerardo added 2 commits February 7, 2024 10:51

ShareIntent - Use the original uri if getRealPathFromURI returns empty

50b98f2

Update WordPressUtils ref

abc7992

geriux added the [Type] Bug label Feb 7, 2024

geriux added this to the 24.2 ❄️ milestone Feb 7, 2024

Gerardo added 3 commits February 7, 2024 13:29

Send uri path directly and add try/catch to prevent permission relate…

9d3e3e2

…d crashes

Update log message

daccf7e

Add uri as fallback

0c98841

geriux marked this pull request as ready for review February 7, 2024 14:27

fluiddot self-requested a review February 7, 2024 16:23

fluiddot reviewed Feb 7, 2024

View reviewed changes

fluiddot approved these changes Feb 8, 2024

View reviewed changes

antonis reviewed Feb 8, 2024

View reviewed changes

antonis approved these changes Feb 8, 2024

View reviewed changes

Gerardo added 2 commits February 9, 2024 09:39

ShareIntentReceiver - Add Toast message when media can be loaded

a41e57d

Update WordPressUtils to 3.13.0

47b2fc5

Gerardo added 3 commits February 12, 2024 13:10

Update error message when sharing a media file to the app fails

42ad16f

Split long string

1d0c632

Fix style issues with long line

6c13ba8

geriux merged commit 9972cb5 into release/24.2 Feb 12, 2024
20 checks passed

geriux deleted the fix/sharing-image-regression branch February 12, 2024 14:04

mokagio mentioned this pull request Feb 12, 2024

Merge release/24.2 to trigger strings.xml update in GlotPress #20177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sharing image regression and add support for heic and heif files #20138

Fix sharing image regression and add support for heic and heif files #20138

geriux commented Feb 7, 2024 •

edited

Loading

wpmobilebot commented Feb 7, 2024 •

edited

Loading

wpmobilebot commented Feb 7, 2024 •

edited

Loading

peril-wordpress-mobile bot commented Feb 7, 2024

dangermattic commented Feb 7, 2024 •

edited

Loading

fluiddot Feb 7, 2024

fluiddot Feb 7, 2024

fluiddot Feb 8, 2024

geriux Feb 8, 2024

fluiddot left a comment

fluiddot left a comment

antonis Feb 8, 2024

geriux Feb 9, 2024

geriux Feb 9, 2024

fluiddot Feb 9, 2024

geriux Feb 9, 2024

geriux Feb 9, 2024

fluiddot Feb 9, 2024

antonis Feb 9, 2024

geriux Feb 12, 2024

antonis left a comment

wpmobilebot commented Feb 12, 2024

	private boolean isAllowedMediaType(@NonNull Uri uri) {
	String filePath = MediaUtils.getRealPathFromURI(this, uri);
	// For cases when getRealPathFromURI returns an empty string
	if (TextUtils.isEmpty(filePath)) {
	filePath = String.valueOf(uri);
	}
	return MediaUtils.isValidImage(filePath) \|\| MediaUtils.isVideo(filePath);
	}

Fix sharing image regression and add support for heic and heif files #20138

Fix sharing image regression and add support for heic and heif files #20138

Conversation

geriux commented Feb 7, 2024 • edited Loading

To Test:

Share an image from an image editing app

Share an image from a browser

Share an image from the Photos app

Share an image from a Files Explorer app

Share a HEIC image

Regression Notes

PR Submission Checklist:

Testing Checklist:

wpmobilebot commented Feb 7, 2024 • edited Loading

wpmobilebot commented Feb 7, 2024 • edited Loading

peril-wordpress-mobile bot commented Feb 7, 2024

dangermattic commented Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonis left a comment

Choose a reason for hiding this comment

wpmobilebot commented Feb 12, 2024

The PR caused the following dependency changes:

geriux commented Feb 7, 2024 •

edited

Loading

wpmobilebot commented Feb 7, 2024 •

edited

Loading

wpmobilebot commented Feb 7, 2024 •

edited

Loading

dangermattic commented Feb 7, 2024 •

edited

Loading