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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Feb 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
50b98f2
ShareIntent - Use the original uri if getRealPathFromURI returns empty
Feb 7, 2024
abc7992
Update WordPressUtils ref
Feb 7, 2024
9d3e3e2
Send uri path directly and add try/catch to prevent permission relate…
Feb 7, 2024
daccf7e
Update log message
Feb 7, 2024
0c98841
Add uri as fallback
Feb 7, 2024
a41e57d
ShareIntentReceiver - Add Toast message when media can be loaded
Feb 9, 2024
47b2fc5
Update WordPressUtils to 3.13.0
Feb 12, 2024
42ad16f
Update error message when sharing a media file to the app fails
Feb 12, 2024
1d0c632
Split long string
Feb 12, 2024
6c13ba8
Fix style issues with long line
Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions WordPress/src/main/java/org/wordpress/android/ui/ShareIntentReceiverActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import android.content.SharedPreferences;
import android.net.Uri;
import android.os.Bundle;
import android.text.TextUtils;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
Expand All @@ -21,6 +22,8 @@
import org.wordpress.android.ui.main.WPMainActivity;
import org.wordpress.android.ui.media.MediaBrowserActivity;
import org.wordpress.android.ui.media.MediaBrowserType;
import org.wordpress.android.util.AppLog;
import org.wordpress.android.util.AppLog.T;
import org.wordpress.android.util.FluxCUtils;
import org.wordpress.android.util.MediaUtils;
import org.wordpress.android.util.ToastUtils;
Expand Down Expand Up @@ -94,23 +97,33 @@ private void refreshContent() {
}

private void downloadExternalMedia() {
if (Intent.ACTION_SEND_MULTIPLE.equals(getIntent().getAction())) {
ArrayList<Uri> externalUris = getIntent().getParcelableArrayListExtra((Intent.EXTRA_STREAM));
for (Uri uri : externalUris) {
if (uri != null && isAllowedMediaType(uri)) {
mLocalMediaUris.add(MediaUtils.downloadExternalMedia(this, uri));
try {
if (Intent.ACTION_SEND_MULTIPLE.equals(getIntent().getAction())) {
ArrayList<Uri> externalUris = getIntent().getParcelableArrayListExtra((Intent.EXTRA_STREAM));
for (Uri uri : externalUris) {
if (uri != null && isAllowedMediaType(uri)) {
mLocalMediaUris.add(MediaUtils.downloadExternalMedia(this, uri));
}
}
} else if (Intent.ACTION_SEND.equals(getIntent().getAction())) {
Uri externalUri = getIntent().getParcelableExtra(Intent.EXTRA_STREAM);
if (externalUri != null && isAllowedMediaType(externalUri)) {
mLocalMediaUris.add(MediaUtils.downloadExternalMedia(this, externalUri));
}
}
} else if (Intent.ACTION_SEND.equals(getIntent().getAction())) {
Uri externalUri = getIntent().getParcelableExtra(Intent.EXTRA_STREAM);
if (externalUri != null && isAllowedMediaType(externalUri)) {
mLocalMediaUris.add(MediaUtils.downloadExternalMedia(this, externalUri));
}
} catch (Exception e) {
ToastUtils.showToast(this,
R.string.error_media_could_not_share_media_from_device, ToastUtils.Duration.LONG);
AppLog.e(T.MEDIA, "ShareIntentReceiver failed to download media ", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

LoadMediaMessage.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

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);
}
Comment on lines +123 to +126
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙇 !

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Expand Down
2 changes: 2 additions & 0 deletions WordPress/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2006,6 +2006,8 @@
<string name="error_media_file_type_not_allowed">This file type is not allowed</string>
<string name="error_media_unexpected_empty_media_file_path">Unexpected empty file path for Media</string>
<string name="error_media_could_not_find_media_in_path">Could not find media file in path</string>
<string name="error_media_could_not_share_media_from_device">Unable to load the media for sharing. Please check the app\'s permissions
or use the app\'s media library.</string>
<string name="error_media_path_is_directory">The specified path is a directory instead of a Media file</string>
<string name="error_media_upload_failed_for_reason">Media upload failed.\n%1$s</string>
<string name="error_media_xmlrpc_not_allowed">Your action is not allowed</string>
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ ext {
wordPressFluxCVersion = '2.64.0'
wordPressLoginVersion = '1.12.0'
wordPressPersistentEditTextVersion = '1.0.2'
wordPressUtilsVersion = '3.12.0'
wordPressUtilsVersion = '3.13.0'
indexosMediaForMobileVersion = '43a9026f0973a2f0a74fa813132f6a16f7499c3a'

// debug
Expand Down
Loading