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

Add discrete onMediaUploadPaused handler to media uploads when offline #19884

Merged
merged 8 commits into from
Jan 4, 2024
Merged

Add discrete onMediaUploadPaused handler to media uploads when offline #19884

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b22021a
Add discrete onMediaUploadPaused handler to media uploads when offline
derekblank Jan 4, 2024
d8a7bff
refactor: Fix parameter name typo
dcalhoun Jan 4, 2024
8d46802
refactor: Annotate override of `onMediaUploadPaused` method
dcalhoun Jan 4, 2024
8ed6850
refactor: Remove unused `onMediaUploadFailed` error type parameter
dcalhoun Jan 4, 2024
ad1ca04
feat: Aztec `onMediaUploadPaused` passes onto `onMediaUploadFailed`
dcalhoun Jan 4, 2024
594f989
feat: Only consider failed media uploads to be paused
dcalhoun Jan 4, 2024
00df84c
feat: Track failed media upload event when pausing media uploads
dcalhoun Jan 4, 2024
b1f7e3b
feat: When lacking connectivity, pause completed upload w/o remote URLs
dcalhoun Jan 4, 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
5 changes: 5 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3678,6 +3678,11 @@ public void onMediaUploaded(OnMediaUploaded event) {
return;
}

if (!NetworkUtils.isNetworkAvailable(this)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

mEditorMedia.onMediaUploadPaused(mEditorMediaUploadListener, event.media);
return;
}

// event for unknown media, ignoring
if (event.media == null) {
AppLog.w(AppLog.T.MEDIA, "Media event carries null media object, not recognized");
Expand Down
4 changes: 4 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/editor/media/EditorMedia.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,10 @@ class EditorMedia @Inject constructor(
listener.onMediaUploadFailed(media.id.toString(), error.type.name)
}

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

Choose a reason for hiding this comment

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

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


sealed class AddMediaToPostUiState(
val editorOverlayVisibility: Boolean,
val progressDialogUiState: ProgressDialogUiState
Expand Down
4 changes: 4 additions & 0 deletions libs/editor/src/main/java/org/wordpress/android/editor/AztecEditorFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1431,6 +1431,10 @@ public void onMediaUploadFailed(final String localMediaId, final String errorTyp
mUploadingMediaProgressMax.remove(localMediaId);
}

public void onMediaUploadPaused(final String localMediaId) {
mUploadingMediaProgressMax.remove(localMediaId);
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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);
}


@Override
public void onVideoInfoRequested(final AztecAttributes attrs) {
// VideoPress special case here
Expand Down
1 change: 1 addition & 0 deletions libs/editor/src/main/java/org/wordpress/android/editor/EditorMediaUploadListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

This is an odd parameter name, presumably a typo.

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

}
16 changes: 8 additions & 8 deletions .../editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1488,14 +1488,14 @@ public void onMediaUploadProgress(final String localMediaId, final float progres

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

Choose a reason for hiding this comment

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

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) {

switch (errorType) {
case "CONNECTION_ERROR":
getGutenbergContainerFragment().mediaFileUploadPaused(Integer.valueOf(localMediaId));
break;
default:
getGutenbergContainerFragment().mediaFileUploadFailed(Integer.valueOf(localMediaId));
break;
}
getGutenbergContainerFragment().mediaFileUploadFailed(Integer.valueOf(localMediaId));
mFailedMediaIds.add(localMediaId);
mUploadingMediaProgressMax.remove(localMediaId);
}

@Override
public void onMediaUploadPaused(final String localMediaId) {
getGutenbergContainerFragment().mediaFileUploadPaused(Integer.valueOf(localMediaId));
mFailedMediaIds.add(localMediaId);
Copy link
Contributor Author

@derekblank derekblank Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

mUploadingMediaProgressMax.remove(localMediaId);
}
Expand Down
Loading