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 all commits
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
10 changes: 9 additions & 1 deletion 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 (event.isError() && !NetworkUtils.isNetworkAvailable(this)) {
mEditorMedia.onMediaUploadPaused(mEditorMediaUploadListener, event.media, event.error);
return;
}

// event for unknown media, ignoring
if (event.media == null) {
AppLog.w(AppLog.T.MEDIA, "Media event carries null media object, not recognized");
Expand All @@ -3696,7 +3701,10 @@ public void onMediaUploaded(OnMediaUploaded event) {
mEditorMedia.onMediaUploadError(mEditorMediaUploadListener, event.media, event.error);
} else if (event.completed) {
// if the remote url on completed is null, we consider this upload wasn't successful
if (TextUtils.isEmpty(event.media.getUrl())) {
if (TextUtils.isEmpty(event.media.getUrl()) && !NetworkUtils.isNetworkAvailable(this)) {
MediaError error = new MediaError(MediaErrorType.GENERIC_ERROR);
mEditorMedia.onMediaUploadPaused(mEditorMediaUploadListener, event.media, error);
Comment on lines +3704 to +3706
Copy link
Member

Choose a reason for hiding this comment

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

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.

} else if (TextUtils.isEmpty(event.media.getUrl())) {
MediaError error = new MediaError(MediaErrorType.GENERIC_ERROR);
mEditorMedia.onMediaUploadError(mEditorMediaUploadListener, event.media, error);
} else {
Expand Down
14 changes: 13 additions & 1 deletion 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 @@ -295,7 +295,19 @@ class EditorMedia @Inject constructor(
}
}
analyticsTrackerWrapper.track(EDITOR_UPLOAD_MEDIA_FAILED, properties)
listener.onMediaUploadFailed(media.id.toString(), error.type.name)
listener.onMediaUploadFailed(media.id.toString())
}

fun onMediaUploadPaused(listener: EditorMediaUploadListener, media: MediaModel, error: MediaError) = launch {
val properties: Map<String, Any?> = withContext(bgDispatcher) {
analyticsUtilsWrapper
.getMediaProperties(media.isVideo, null, media.filePath)
.also {
it["error_type"] = error.type.name
}
}
analyticsTrackerWrapper.track(EDITOR_UPLOAD_MEDIA_FAILED, properties)
listener.onMediaUploadPaused(media.id.toString())
}

sealed class AddMediaToPostUiState(
Expand Down
8 changes: 7 additions & 1 deletion 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 @@ -1413,7 +1413,7 @@ public void onMediaUploadProgress(final String localMediaId, final float progres
}

@Override
public void onMediaUploadFailed(final String localMediaId, final String errorType) {
public void onMediaUploadFailed(final String localMediaId) {
if (!isAdded() || mContent == null) {
return;
}
Expand All @@ -1431,6 +1431,12 @@ public void onMediaUploadFailed(final String localMediaId, final String errorTyp
mUploadingMediaProgressMax.remove(localMediaId);
}

@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
3 changes: 2 additions & 1 deletion 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 @@ -7,6 +7,7 @@ public interface EditorMediaUploadListener {
void onMediaUploadReattached(String localId, float currentProgress);
void onMediaUploadSucceeded(String localId, MediaFile mediaFile);
void onMediaUploadProgress(String localId, float progress);
void onMediaUploadFailed(String localId, String errorType);
void onMediaUploadFailed(String localId);
void onGalleryMediaUploadSucceeded(long galleryId, long remoteId, int remaining);
void onMediaUploadPaused(String localId);
}
18 changes: 9 additions & 9 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 @@ -1487,15 +1487,15 @@ public void onMediaUploadProgress(final String localMediaId, final float progres
}

@Override
public void onMediaUploadFailed(final String localMediaId, String errorType) {
switch (errorType) {
case "CONNECTION_ERROR":
getGutenbergContainerFragment().mediaFileUploadPaused(Integer.valueOf(localMediaId));
break;
default:
getGutenbergContainerFragment().mediaFileUploadFailed(Integer.valueOf(localMediaId));
break;
}
public void onMediaUploadFailed(final String localMediaId) {
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