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

[Gutenberg] Fix media upload progress indicators #20426

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

derekblank
Copy link
Member

WIP: Fixes media upload progress indicators not incrementing

Addresses:


To Test:

Current Behavior:

  1. Open a Post in the Editor
  2. Add an Image or Video block and select Choose From Device
  3. Observe progress indicator does not increment

Expected Behavior:

  1. Open a Post in the Editor
  2. Add an Image or Video block and select Choose From Device
  3. Observe progress indicator increments as media is uploaded

Regression Notes

  1. Potential unintended areas of impact

    • TODO
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • TODO
  3. What automated tests I added (or what prevented me from doing so)

    • TODO

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 (strike-out the not-applying and unnecessary ones):

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@derekblank derekblank added [Type] Bug Gutenberg Editing and display of Gutenberg blocks. labels Mar 7, 2024
@dangermattic
Copy link
Collaborator

1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

Copy link

sonarcloud bot commented Mar 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@derekblank
Copy link
Member Author

derekblank commented Mar 7, 2024

@fluiddot / @antonis We've noted that the horizontal media progress indicators within the Editor no longer increment in WP/JPAndroid 24.4-rc-1. This appears to affect Android only, and has been partially captured in wordpress-mobile/gutenberg-mobile#6667.

In my debugging of that issue, I noted that the value of progress is not updated, tracing all the way back to the implementation of updateMediaProgress within WordPress-Android itself:

private void updateMediaProgress() {
for (String mediaId : mUploadingMediaProgressMax.keySet()) {
// upload progress should work on numeric mediaIds only
if (!TextUtils.isEmpty(mediaId) && TextUtils.isDigitsOnly(mediaId)) {
getGutenbergContainerFragment().mediaFileUploadProgress(Integer.valueOf(mediaId),
mUploadingMediaProgressMax.get(mediaId));
} else {
getGutenbergContainerFragment().mediaFileSaveProgress(mediaId,
mUploadingMediaProgressMax.get(mediaId));
}
}
}

When logging out the values of updateMediaProgress for an active upload, the progress value appears to never increment past its initial value.

In this PR, I'm exploring that perhaps some of the media upload progress logic was unintentionally removed when removing Story code, possibly within:

I'll continue to explore ideas in how the methods in this draft PR would be invoked to update media progress and send it to the Gutenberg Editor -- If you have any further thoughts on this approach, or other areas to explore that may be helpful, please let me know.

@antonis
Copy link
Member

antonis commented Mar 7, 2024

Thank you for the ping @derekblank 🙇

My understanding was that the removed StorySaveMediaListener interface affected only stories due to the naming but probably this is not the case 🤔
Note also that wordpress-mobile/gutenberg-mobile#6667 preceded the stories removal PRs.
Another interface that tracks the upload progress is the EditorMediaUploadListener that is implemented by the GutenbergEditorFragment

Let me know if I can help on this investigation and feel free to add me as a reviewer.

@derekblank
Copy link
Member Author

@antonis Thanks for the extra info and context -- it helps to rule things out. 🙇 Especially, noting that wordpress-mobile/gutenberg-mobile#6667 preceded the stories removal PRs, which I hadn't yet noted. This helps to widen the debugging timeline.

@antonis
Copy link
Member

antonis commented Mar 7, 2024

I had a 2nd look on how the video upload progress is communicated and noticed that the progress is passing to the gluecode.

Note on the recording below I initially did an upload with no breakpoints and no progress was displayed. On the second attempt I added a breakpoint on the line above and some progress was displayed on the screen. A hypothesis may be that too many events are emitted and dropped (in that case maybe we can throttle them similar to this) or the progress values are inconsistent 🤔

Screen_recording_20240307_120632.mp4

@derekblank
Copy link
Member Author

I had a 2nd look on how the video upload progress is communicated and noticed that the progress is passing to the gluecode.

Thanks for taking a second look, and good catch on the progress making it to the glue code. I noted that the progress value increments to a very small number (like 0.39179655723273754), and then never updates again. I'll try to bisect where another change may have been introduced.

@derekblank
Copy link
Member Author

On investigating further, indeed the onMediaUploadProgress events are still retained from from the EditorMediaUploadListener Interface. With the additional context of the timeline, I agree the Story removals code is not likely to have affected the progress event.

But, I have not yet been able to determine what else may have changed between 24.3 and 24.4 that may have affected the progress events. I had considered if it could have been related to Jetpack plugin updates, but the issue still occurs for Image and Video blocks on WP.com and self-hosted sites, with and without the Jetpack plugin installed. Possibly something external, like changes to the REST client?

From logging out progress values in the MediaUploadHandler, the progress value appears to increment only once with a very small initial (but it does send this value to the Glue Code and the Editor UI, as you can see a very small increment in the Progress Bar indicator component.

Just recapping my findings and will continue to investigate further. Open to ideas for leads.

Screenshot_2024-03-08_at_12_13_14 pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Editing and display of Gutenberg blocks. [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants