Add discrete onMediaUploadPaused handler to media uploads when offline #19884

derekblank · 2024-01-04T06:31:40Z

Augments the work in #19878 to pause media upload when internet connection is unavailable.

This PR introduces a discrete onMediaUploadPaused handler to separate concerns and avoid invoking mediaFileUploadPaused from within onMediaUploadFailed.

To Test:

Regression Notes

Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)

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.

UI Changes Testing Checklist:

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 · 2024-01-04T06:39:39Z

libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java

+
+    @Override
+    public void onMediaUploadPaused(final String localMediaId) {
+        getGutenbergContainerFragment().mediaFileUploadPaused(Integer.valueOf(localMediaId));
        mFailedMediaIds.add(localMediaId);


I don't really wish to add these localMediaIds to mFailedMediaIds, but it is necessary to automatically restart media uploads:

WordPress-Android/libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java

Lines 1591 to 1596 in 234db29

public void onConnectionStatusChange(boolean isConnected) {

getGutenbergContainerFragment().onConnectionStatusChange(isConnected);

if (BuildConfig.DEBUG && isConnected && hasFailedMediaUploads()) {

mEditorFragmentListener.onMediaRetryAll(mFailedMediaIds);

}

}

Semantically, this should probably be called mFailedOrPausedMediaIds (including related artifacts
like hasFailedMediaUploads -> hasFailedOrPausedMediaUploads). The concept of failed uploads could be rethought of entirely with the introduction of MEDIA_UPLOAD_STATE_PAUSED. That feels well outside the scope of this change, as it has quite a large surface area. If there is a more elegant way to restart media uploads without adding these to mFailedMediaIds, I am open to it.

I agree a refactor is likely beneficial but unnecessary at this time.

That said, I do not believe overloading mFailedMediaIds to include "paused" media is all that cryptic or misleading, particularly if we only consider media to be "paused" if it also contains an error, as I noted in a separate comment.

wpmobilebot · 2024-01-04T06:51:33Z

📲 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	pr19884-b22021a
	Commit	`b22021a`
	Direct Download	`jetpack-prototype-build-pr19884-b22021a.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-01-04T06:52:10Z

📲 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	pr19884-b22021a
	Commit	`b22021a`
	Direct Download	`wordpress-prototype-build-pr19884-b22021a.apk`

Note: Google Login is not supported on these builds.

dcalhoun

I like your proposed changes. I plan to checkout the branch locally and implement some of my suggested changes. I am still interested in your responses to my suggestions, though.

dcalhoun · 2024-01-04T14:03:23Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

@@ -3678,6 +3678,11 @@ public void onMediaUploaded(OnMediaUploaded event) {
            return;
        }

+        if (!NetworkUtils.isNetworkAvailable(this)) {


I worry that if we do not check for an error as well, we might erroneously mark successfully uploaded media as paused. WDYT?

dcalhoun · 2024-01-04T14:05:49Z

WordPress/src/main/java/org/wordpress/android/ui/posts/editor/media/EditorMedia.kt

+    fun onMediaUploadPaused(listener: EditorMediaUploadListener, media: MediaModel) = launch {
+        listener.onMediaUploadPaused(media.id.toString())
+    }


I imagine we should still track an event of EDITOR_UPLOAD_MEDIA_FAILED for paused media, or implement a new event. WDYT?

dcalhoun · 2024-01-04T14:07:05Z

libs/editor/src/main/java/org/wordpress/android/editor/EditorMediaUploadListener.java

@@ -9,4 +9,5 @@ public interface EditorMediaUploadListener {
    void onMediaUploadProgress(String localId, float progress);
    void onMediaUploadFailed(String localId, String errorType);
    void onGalleryMediaUploadSucceeded(long galleryId, long remoteId, int remaining);
+    void onMediaUploadPaused(String toString);


This is an odd parameter name, presumably a typo.

Suggested change

void onMediaUploadPaused(String toString);

void onMediaUploadPaused(String localId);

dcalhoun · 2024-01-04T14:11:06Z

libs/editor/src/main/java/org/wordpress/android/editor/AztecEditorFragment.java

+    public void onMediaUploadPaused(final String localMediaId) {
+        mUploadingMediaProgressMax.remove(localMediaId);
+    }


I like avoiding a need to pass the error type and entangle the paused and error methods, but I worry about modify Aztec. We likely need to test Aztec to avoid regressions.

Additionally, I note this method lacks an @Override annotation that surround methods possess. We should likely follow the existing code pattern.

Lastly, how confident are we that we only need to remove the localMediaId? Do we need the guard clause or other logic from the sibling failed method?

Adding onMediaUploadPaused to Aztec was done quickly to get around some build errors from Android Studio. I also am not keen on modifying Aztec. I'll do some further testing to see if this is actually needed.

My understanding is that it is necessary, as both Aztec and Gutenberg share a common interface to which they must adhere. For Aztec, the latest implementation of this merely passes the invocation onto the existing failed method. So, the regression risk should be fairly low. It is likely still worth testing the Aztec editor via editing classic posts in the app.

WordPress-Android/libs/editor/src/main/java/org/wordpress/android/editor/AztecEditorFragment.java

Lines 1434 to 1438 in b97dbec

@Override

public void onMediaUploadPaused(final String localMediaId) {

// Aztec does not leverage the paused media state, only the Gutenberg editor

onMediaUploadFailed(localMediaId);

}

dcalhoun · 2024-01-04T14:20:17Z

libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java

@@ -1488,14 +1488,14 @@ public void onMediaUploadProgress(final String localMediaId, final float progres

    @Override
    public void onMediaUploadFailed(final String localMediaId, String errorType) {


With this approach, we no longer require the errorType parameter and should remove it.

Suggested change

public void onMediaUploadFailed(final String localMediaId, String errorType) {

public void onMediaUploadFailed(final String localMediaId) {

dcalhoun · 2024-01-04T14:23:05Z

libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java

+
+    @Override
+    public void onMediaUploadPaused(final String localMediaId) {
+        getGutenbergContainerFragment().mediaFileUploadPaused(Integer.valueOf(localMediaId));
        mFailedMediaIds.add(localMediaId);


I agree a refactor is likely beneficial but unnecessary at this time.

That said, I do not believe overloading mFailedMediaIds to include "paused" media is all that cryptic or misleading, particularly if we only consider media to be "paused" if it also contains an error, as I noted in a separate comment.

Ensure the implementation adheres to the relevant interface.

With the addition of a discrete paused method, we no longer utilize this parameter in the failed method.

We do not plan to add paused media support to the Aztec editor. Therefore, it likely makes more sense for the `onMediaUploadPaused` implementation to merely invoke the existing `onMediaUploadFailed` method.

Without checking for an error, there may be a possibility that successfully uploaded media is marked as "paused."

Even though we communicate a nuanced paused state to the user, we likely need to continue tracking this event as a failure, or consider adding a discrete pause analytic event.

Based upon existing code, we should consider completed uploads lacking remote URLs to be failures. Therefore, we should apply the new pause state to this context.

peril-wordpress-mobile · 2024-01-04T15:22:23Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

dcalhoun · 2024-01-04T15:26:33Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

+            if (TextUtils.isEmpty(event.media.getUrl()) && !NetworkUtils.isNetworkAvailable(this)) {
+                MediaError error = new MediaError(MediaErrorType.GENERIC_ERROR);
+                mEditorMedia.onMediaUploadPaused(mEditorMediaUploadListener, event.media, error);


Noting that I added the paused media implementation here. Based upon the existing code, it appears that completed media should be considered failed if it possesses a null remote URL.

wpmobilebot · 2024-01-04T15:29:11Z

	1 Warning
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

Add discrete onMediaUploadPaused handler to media uploads when offline

b22021a

derekblank requested a review from dcalhoun January 4, 2024 06:31

derekblank commented Jan 4, 2024

View reviewed changes

derekblank added the Gutenberg Editing and display of Gutenberg blocks. label Jan 4, 2024

derekblank marked this pull request as ready for review January 4, 2024 06:41

derekblank mentioned this pull request Jan 4, 2024

feat: Pause media upload when internet connection unavailable #19878

Merged

12 tasks

dcalhoun reviewed Jan 4, 2024

View reviewed changes

dcalhoun added 7 commits January 4, 2024 09:31

refactor: Fix parameter name typo

d8a7bff

refactor: Annotate override of onMediaUploadPaused method

8d46802

Ensure the implementation adheres to the relevant interface.

refactor: Remove unused onMediaUploadFailed error type parameter

8ed6850

With the addition of a discrete paused method, we no longer utilize this parameter in the failed method.

feat: Aztec onMediaUploadPaused passes onto onMediaUploadFailed

ad1ca04

We do not plan to add paused media support to the Aztec editor. Therefore, it likely makes more sense for the `onMediaUploadPaused` implementation to merely invoke the existing `onMediaUploadFailed` method.

feat: Only consider failed media uploads to be paused

594f989

Without checking for an error, there may be a possibility that successfully uploaded media is marked as "paused."

feat: Track failed media upload event when pausing media uploads

00df84c

Even though we communicate a nuanced paused state to the user, we likely need to continue tracking this event as a failure, or consider adding a discrete pause analytic event.

feat: When lacking connectivity, pause completed upload w/o remote URLs

b1f7e3b

Based upon existing code, we should consider completed uploads lacking remote URLs to be failures. Therefore, we should apply the new pause state to this context.

dcalhoun approved these changes Jan 4, 2024

View reviewed changes

dcalhoun merged commit 2a5075d into feat/update-image-block-upload-visuals Jan 4, 2024
12 of 20 checks passed

dcalhoun deleted the feat/update-image-block-upload-visuals__discrete-paused-state branch January 4, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add discrete onMediaUploadPaused handler to media uploads when offline #19884

Add discrete onMediaUploadPaused handler to media uploads when offline #19884

derekblank commented Jan 4, 2024

derekblank Jan 4, 2024 •

edited

Loading

dcalhoun Jan 4, 2024

wpmobilebot commented Jan 4, 2024

wpmobilebot commented Jan 4, 2024

dcalhoun left a comment •

edited

Loading

dcalhoun Jan 4, 2024

dcalhoun Jan 4, 2024

dcalhoun Jan 4, 2024

dcalhoun Jan 4, 2024

derekblank Jan 4, 2024

dcalhoun Jan 5, 2024

dcalhoun Jan 4, 2024

dcalhoun Jan 4, 2024

peril-wordpress-mobile bot commented Jan 4, 2024

dcalhoun Jan 4, 2024

wpmobilebot commented Jan 4, 2024

	public void onConnectionStatusChange(boolean isConnected) {
	getGutenbergContainerFragment().onConnectionStatusChange(isConnected);
	if (BuildConfig.DEBUG && isConnected && hasFailedMediaUploads()) {
	mEditorFragmentListener.onMediaRetryAll(mFailedMediaIds);
	}
	}

	void onMediaUploadPaused(String toString);
	void onMediaUploadPaused(String localId);

	@Override
	public void onMediaUploadPaused(final String localMediaId) {
	// Aztec does not leverage the paused media state, only the Gutenberg editor
	onMediaUploadFailed(localMediaId);
	}

		@@ -1488,14 +1488,14 @@ public void onMediaUploadProgress(final String localMediaId, final float progres

		@Override
		public void onMediaUploadFailed(final String localMediaId, String errorType) {

Add discrete onMediaUploadPaused handler to media uploads when offline #19884

Add discrete onMediaUploadPaused handler to media uploads when offline #19884

Conversation

derekblank commented Jan 4, 2024

To Test:

Regression Notes

PR Submission Checklist:

UI Changes Testing Checklist:

derekblank Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Jan 4, 2024

wpmobilebot commented Jan 4, 2024

dcalhoun left a comment • 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

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

peril-wordpress-mobile bot commented Jan 4, 2024

Choose a reason for hiding this comment

wpmobilebot commented Jan 4, 2024

derekblank Jan 4, 2024 •

edited

Loading

dcalhoun left a comment •

edited

Loading