-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: Retry failed media uploads when re-establishing network connection #19803
feat: Retry failed media uploads when re-establishing network connection #19803
Conversation
Improve the media upload UX by automatically retrying failed uploads.
@@ -1583,5 +1583,9 @@ public void onGutenbergDialogNegativeClicked(@NonNull String instanceTag) { | |||
@Override | |||
public void onConnectionStatusChange(boolean isConnected) { | |||
getGutenbergContainerFragment().onConnectionStatusChange(isConnected); | |||
if (hasFailedMediaUploads()) { | |||
mEditorFragmentListener.onMediaRetryAll(mFailedMediaIds); | |||
mFailedMediaIds.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure clearing this is necessary, it does not appear to be done elsewhere, e.g., showRetryMediaUploadDialog
. This clear was added in an attempt to fix erroneously displayed errors after saving or publishing a post, but it does not appear to work.
Steps to reproduce
- Use the prototype build on this PR.
- Disable network connectivity.
- Add an Image block and attach media.
- Enable network connectivity.
- Allow the automatic retry of media upload to finish successfully.
- Save and close the post.
- Note the failed media upload notification.
It appears as though a lingering error state/message is in place, but I am unable to find where that is or how to clear it. I expected onMediaRetryAll
to clear it, but it doesn't appear to be doing so.
👋🏻 @antonis. Randomly pinging you for Android expertise and help. 😄 Would you be willing to help me debug why I might see this error when the media attached to the post actually successfully uploads with the new retry logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing the existing functionality of retrying failing media uploads via the dialog presented when tapping a failed image, it appears the lingering error message is a preexisting issue. Semi-related issues are reported in #11039, #15897, and #13427.
Steps to reproduce
- Use the latest Play Store app version.
- Disable network connectivity.
- Add an Image block and attach media.
- Enable network connectivity.
- Tap the Image block with a failed upload.
- Tap "Retry."
- After the media finishes uploading successfully, save and close the post.
- Note the failed media upload notification.
Therefore, I do not consider this issue blocking. It'd be nice to resolve, but could be done so in a separate PR later.
@antonis please do not feel like it is a priority to help debug this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure clearing this is necessary
Removing the clearing with c114528 and the reasoning for it sounds good 👍
Therefore, I do not consider this issue blocking. It'd be nice to resolve, but could be done so in a separate PR later.
I was able to reproduce this issue and tried to figure out why this happens. My understanding is that there is a limitation on how the SnackbarSequencer
works were this kind of internal notifications are not really canceled once issued. I agree that this can be investigated further and handled in a follow up PR 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for investigating and sharing your insights and perspective. Very helpful! 🙇🏻♂️
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
We will enable the feature in production once all project work is completed.
This was originally added in an attempt to clear an unexpected "failed media upload" notification displayed after closing the editor. However, it did not resolve that issue, it must originate from some other location. Additionally, it would appears successful media uploads already manage removing media IDs from the `mFailedMediaIds` collection.
libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrying media uploads without a network connection will undoubtedly fail again. Co-authored-by: Antonis Lilis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this feature successfully on an internal build, and automated retries are working. Also confirmed that the feature is not active on non-debug/internal builds. 🚀
I removed the RELEASE-NOTES entry, keeping with the precedent that we will add public-facing release notes when the DEBUG flag is removed for this feature.
Description
Improve the media upload experience in the post editor by automatically retrying
failed uploads when re-establishing network connectivity.
Fixes wordpress-mobile/gutenberg-mobile#6406.
Testing Instructions
When network connectivity is available
When network connectivity is unavailable
Regression Notes
Editor media uploads, editor launch, writing flow.
Insert and interact with various blocks using the editor.
None, lack of familiarity with the testing stack.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: