Feature/fluxc media upload service multiple uploads reattach progress by mzorz · Pull Request #5780 · wordpress-mobile/WordPress-Android

mzorz · 2017-04-28T18:31:41Z

This PR makes the multiupload capabilities of the MediaUploadService reflect on the UI when going out of the Editor and back (and images in a Post are still being uploaded).

TEST A (plain old single file upload):

open the editor for a new post.
upload one file, choosing it from the media picker (select from gallery)
wait till it is finished uploading
the post should be changed and updated in the DB, the image should unblur.
get out of the editor
get back in
the image should load from the web (see how it slowly forms, on Aztec)

TEST B (background upload):

open the editor for a new post.
start uploading one file, choosing it from the media picker (select from gallery)
while it is uploading (you can see the progress bar), just get out of the editor
wait for a minute or so (whatever you think is enough for the upload to complete)
get back in the editor
the image should load from the web (see how it slowly forms, on Aztec)

TEST C (background upload - reattach progress):

open the editor for a new post.
upload one file, choosing it from the media picker (select from gallery)
while it is uploading (you can see the progress bar), just get out of the editor
wait for a few seconds (just enough for the upload to progress a bit more, but not as much as to let it complete).
get back in the editor
the progress bar should update itself to wherever it is it’s being uploaded
wait till it is finished uploading
the post should be changed and updated in the DB, the image should unblur.
get out of the editor
get back in
the image should load from the web (see how it slowly forms, on Aztec)

TEST D (multiple upload):

open the editor for a new post.
upload one file, choosing it from the media picker (select from gallery)
while it is uploading (you can see the progress bar), pick another file to insert
both files should be updating their progress, independently
wait till both finish and both present the same behaviour as per TEST A.

TEST E (background upload - multiple upload):

open the editor for a new post.
upload one file, choosing it from the media picker (select from gallery)
while it is uploading (you can see the progress bar), pick another file to insert
both files should be updating their progress, independently
while they’re still uploading (you can see the progress bar), just get out of the editor
wait for a minute or so (whatever you think is enough for the upload of both files to complete)
get back in the editor
the images should load from the web (see how they slowly form, on Aztec)

TEST F (background upload - multiple upload - reattach progress):

open the editor for a new post.
upload one file, choosing it from the media picker (select from gallery)
while it is uploading (you can see the progress bar), pick another file to insert
both files should be updating their progress, independently
while they’re still uploading (you can see the progress bar), just get out of the editor
wait for a few seconds (just enough for the uploads to progress a bit more, but not as much as to let both complete).
get back in the editor
the progress bar should update itself to wherever it is it’s being uploaded
wait till both are finished uploading (both images unbar)
get out of the editor, then get back in
the images should load from the web (see how they slowly form, on Aztec)

TEST G (multicancel):

open the editor for a new post.
upload one file, choosing it from the media picker (select from gallery)
while it is uploading (you can see the progress bar), pick another file to insert
both files should be updating their progress, independently
while they’re still uploading (you can see the progress bar), pick a third one
cancel the first one
now cancel the second one
the third one should still be uploading
wait till it finishes, and make sure it’s the only one finishing (wait some minutes and check the state is as expected: 2 marked for retry, third one completed).

Known Issues

(Planning to work on these on separate PRs)

Different working copies of PostModel

It could happen that 2 images are being uploaded, then both finish practically at the same time, then when we try to modify the post’s content (to replace the local image with the remote one) we:

take the post from the db, where 2 images are local and “uploading”
2.a) image A gets replaced with the remote one, and this gets saved into the DB
2.b) image B gets replaced with the remote one, and this gets saved into the DB
We end up having a post with image A local, and image B remote, because what was saved last is 2.b) with a copy of the post that had both local images only.

How to solve this?
We need a way to ensure that all images get saved / replacing the post content.
Fortunately, the service runs as long as there’s stuff enqueued for upload.

In the MediaUploadService, what I’m going to do now is wait for all images in the queue (for a given post) to complete, and only then a) get a copy of the post’s content and b) make all replacements at once, then save.

See #5858

Random issues / enhancements

would be great to get the keyboard already ready and up when creating a new post (see Media editing: show the keyboard as soon as the Aztec editor is shown #5859)
eliminate the mAztecReady flag, might not be necessary
~~The profiling is missing the stop calls~~

                ProfilingUtils.split("EditorFragment.onDomLoaded completed");
                ProfilingUtils.dump();
                ProfilingUtils.stop();

~~(see #5860)~~

~~if I start a draft and do nothing, get out of the editor, it gets saved (it probably shouldn’t, should delete the post entirely). (see Don't keep an empty draft #5861)~~
saveContentFromSource() is not used, can be removed. (see removed unused method #5864)
when uploading several images, the progress upload state refreshes make it nearly impossible to navigate the content (scrolling up/down jumps a lot) (already tracked in Image-loading scrolls the editor to the cursor AztecEditor-Android#313)
if you trash a post the snackbar is shown (and the item in the list disappears), then if you refresh the list before the snackbar is hidden, the post re-appears (because it hasn’t been deleted on the backend yet).
Trashed post reappearing (snackbar / refresh race condition) #5862

cc @aforcier

…updates (reattach)

…y if content available

…h part of it

…to feature/fluxc-media-upload-service-multiple-uploads-reattach-progress

…nt or after view created for new posts - also clearing failed media ids from local array

…Content() then updating it in onMediaUploadProgress()

…gress when exiting it

…to feature/fluxc-media-upload-service-multiple-uploads-reattach-progress

…, to make upload progress visibility show accordingly

…, and passing the new post contents as a return value in the static method implementation in AztecEditorFragment

…g the list so we are sure to re-load posts once media are uploaded there

aforcier · 2017-05-04T16:48:38Z

...or/WordPressEditor/src/main/java/org/wordpress/android/editor/EditorMediaUploadListener.java


 public interface EditorMediaUploadListener {
-    void onMediaUploadSucceeded(String localId, MediaFile mediaFile);
+    void onMediaUploadSucceeded(String localId, EditorFragmentAbstract.MediaType mediaType, MediaFile mediaFile);


Adding an extra param here for the MediaType seems redundant since we're already getting that from the MediaFile, which we're also sending.

Could we make getEditorMimeType() a (static) method of EditorFragmentAbstract instead, and use that from AztecEditorFragment and from EditPostActivity? Then we can drop this parameter (we'll still need it in onMediaUploadFailed below though, I guess).

addressed in 4417a4c

(and thank you! nice improvement there)

aforcier · 2017-05-04T16:49:43Z

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

        removeMediaFromPendingList(media);
    }

+    private EditorFragmentAbstract.MediaType getEditorMimeType(boolean isVideo) {


Seems a bit weird to keep calling getEditorMimeType(mediaFile.getVideo() - it seems more natural to me to have getEditorMimeType(mediaFile), since that's what we're really doing (getting the media type of the media file).

addressed in 4417a4c

daniloercoli · 2017-05-05T11:00:00Z

I tested a variant of TEST B (background upload):

open the editor for a new post
start uploading one file, choosing it from the media picker (select from gallery)
while it is uploading (you can see the progress bar), just get out of the editor
get out of the app. send it background
wait for a minute or so (whatever you think is enough for the upload to complete)
get back in the editor

*The picture appears in the failed mode.

Here is the log:

05-05 12:55:31.689 32488-11155/org.wordpress.android D/WordPress-API: Dispatching action: MediaAction-UPLOADED_MEDIA
05-05 12:55:31.712 32488-32488/org.wordpress.android D/WordPress-MEDIA: 63 - progressing 0.8870163
05-05 12:55:32.775 32488-11155/org.wordpress.android D/WordPress-API: Dispatching action: MediaAction-UPLOADED_MEDIA
05-05 12:55:32.793 32488-32488/org.wordpress.android D/WordPress-MEDIA: 63 - progressing 0.92875826
05-05 12:55:33.864 32488-11155/org.wordpress.android D/WordPress-API: Dispatching action: MediaAction-UPLOADED_MEDIA
05-05 12:55:33.889 32488-32488/org.wordpress.android D/WordPress-MEDIA: 63 - progressing 0.96528244
05-05 12:55:39.227 32488-11155/org.wordpress.android D/WordPress-MEDIA: media upload successful: Response{protocol=h2, code=200, message=, url=https://public-api.wordpress.com/rest/v1.1/sites/15592836/media/new/}
05-05 12:55:39.256 32488-11155/org.wordpress.android D/WordPress-API: Dispatching action: MediaAction-UPLOADED_MEDIA
05-05 12:55:39.260 32488-11155/org.wordpress.android D/WordPress-MEDIA: mediaRestClient: removed id: 63 from current uploads, remaining: 0
05-05 12:55:39.312 32488-32488/org.wordpress.android I/WordPress-MEDIA: Upload completed - localId=63 title=wp-1493895980036
05-05 12:55:39.314 32488-32488/org.wordpress.android D/WordPress-API: Dispatching action: PostAction-UPDATE_POST
05-05 12:55:39.333 32488-7278/org.wordpress.android D/EventBus: No subscribers registered for event class org.wordpress.android.fluxc.store.PostStore$OnPostChanged
05-05 12:55:39.333 32488-7278/org.wordpress.android D/EventBus: No subscribers registered for event class org.greenrobot.eventbus.NoSubscriberEvent
05-05 12:55:39.354 32488-32488/org.wordpress.android V/WordPress-MEDIA: No more media items to upload. Skipping this request - MediaUploadService.
05-05 12:55:39.355 32488-32488/org.wordpress.android I/WordPress-MEDIA: Media Upload Service > completed
05-05 12:55:39.355 32488-32488/org.wordpress.android I/WordPress-MEDIA: No more items pending in queue. Stopping MediaUploadService.
05-05 12:55:39.356 32488-32488/org.wordpress.android I/WordPress-MEDIA: Media Upload Service > destroyed

aforcier · 2017-05-05T13:16:16Z

Confirming what @daniloercoli is seeing - though only if I enable 'Don't keep activities'.

(You're using the visual editor in that screenshot btw @daniloercoli - this branch doesn't have async support for the visual editor, only Aztec.)

aforcier · 2017-05-05T13:26:35Z

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

            onUploadSuccess(event.media);
        }
        else {
+            AppLog.w(AppLog.T.MEDIA, "EDITPOSTACTIVITY: " + event.media.getId() + " - " + event.progress);


This shouldn't be a warning-level log. Also, is this log really necessary? We already generate quite a bit of logging per upload, with this there are three logs per tick:

05-05 08:27:02.034 31057-12701/org.wordpress.android.beta D/WordPress-API: Dispatching action: MediaAction-UPLOADED_MEDIA 05-05 08:27:02.082 31057-31057/org.wordpress.android.beta W/WordPress-MEDIA: EDITPOSTACTIVITY: 65 - 0.9653928 05-05 08:27:02.093 31057-31057/org.wordpress.android.beta D/WordPress-MEDIA: 65 - progressing 0.9653928

If they're helpful while working on async we could keep them but keep track that we should reduce them later - these logs make it into helpshift reports, and I think there's a size limit that can prevent earlier useful logs from being preserved.

Thank you! was just added to guide my own work, ditched it in 0cd3bc9

daniloercoli · 2017-05-05T13:49:49Z

(You're using the visual editor in that screenshot btw @daniloercoli - this branch doesn't have async support for the visual editor, only Aztec.)

Right! I'm using the old visual editor.

daniloercoli · 2017-05-05T14:34:16Z

though only if I enable 'Don't keep activities turned on'.

That's weird. My devices are experiencing some weird issues today due to activities that are not kept. Also, I don't have 'Don't keep activities turned on'. enabled on them.

I think we should move much of the code outside the activity, since adding images to the editor could consume a lot of memory, (not counting when the user has enabled the resize pictures options, and backgrounding the app could lead the system to dispose the activity....

Note: I would encourage to not test the app on emulator or Nexus 6P ;) We should try to test media related improvements on less powerful devices.

…ype from the EditorMediaUploadListener interface

aforcier · 2017-05-09T15:34:01Z

@daniloercoli summarizing the findings from the issue you ran into and what Mario and I discussed:

In general, your steps (start upload, leave editor, background app, wait until media uploads, open post again) should work in this branch with Aztec enabled
Your steps will fail in this branch for the visual editor, because updating posts from outside the visual editor isn't supported. However, they should work in my async visual editor PR: Media Async: Visual editor support #5833

At one point I misunderstood your steps, and looked at exiting the editor while media was uploading (rather than leaving the editor). Trying this again now I realize there is a bug there:

Enable 'don't keep activities'
Start new post, upload media
While media is uploading, background the app (don't leave the editor)
Wait for upload to complete
The post is updated using the async static method @mzorz added in an earlier PR (this will work in the visual editor too with Media Async: Visual editor support #5833)
Re-open the app
The editor is recreated, but uses the PostModel it saved in onSaveInstanceState(), which doesn't have the remote media URL

We should change EditPostActivity to try and fetch the latest copy from the PostStore if it's restoring a post from saved state. ~~I'll open a separate issue for this.~~ Filed in #5863.

@daniloercoli could you confirm that you don't have any issues in this branch if you use Aztec and exit the editor before backgrounding the app?

daniloercoli · 2017-05-10T10:41:31Z

@daniloercoli could you confirm that you don't have any issues in this branch if you use Aztec and exit the editor before backgrounding the app?

Right! everything is working as expected when I use Aztec.

aforcier · 2017-05-10T14:19:17Z

...editor/example/src/main/java/org/wordpress/android/editor/example/EditorExampleActivity.java

                        sleep(500);

-                        ((EditorMediaUploadListener) mEditorFragment).onMediaUploadProgress(mediaId, count);
+                        ((EditorMediaUploadListener) mEditorFragment).onMediaUploadProgress(mediaId, false, count);


Doesn't look like the false param should be here anymore (onMediaUploadProgress wasn't changed) - also in two more places in this class.

correct - addressed in fd1e412

aforcier · 2017-05-10T14:24:49Z

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

+        for (Attributes attrs : content.getAllElementAttributes(uploadingPredicate)) {
+            String localMediaId = attrs.getValue("data-wpid");
+            if (!TextUtils.isEmpty(localMediaId)) {
+                mUploadingMediaProgressMax.put(localMediaId, new Float(0));


We can drop an unnecessary boxing here and make a tiny optimization by just using 0f instead of new Float(0) (also clears a lint warning).

thanks - addressed in 4606f08

aforcier · 2017-05-10T14:25:30Z

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

-                content.refreshText();
-
-                mUploadingMedia.put(localMediaId, MediaType.IMAGE);
+                mUploadingMediaProgressMax.put(localMediaId, new Float(0));


Same as above comment, can use 0f here.

addressed in 4606f08

aforcier · 2017-05-10T14:25:37Z

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

                }
                mFailedMediaIds.remove(localMediaId);
-                mUploadingMedia.put(localMediaId, mediaType);
+                mUploadingMediaProgressMax.put(localMediaId, new Float(0));


Same as above comment, can use 0f here.

addressed in 4606f08

aforcier · 2017-05-10T14:45:08Z

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

-            content.refreshText();
+
+        // first obtain the latest maximum
+        float maxProgressForLocalMediaId = mUploadingMediaProgressMax.get(localMediaId);


This can throw an NPE if the localMediaId doesn't exist in mUploadingMediaProgressMax (HashMap.get() will return a null Float, which will crash when unboxing into a float).

awesome catch - addressed in 1102c4f

aforcier · 2017-05-10T15:01:54Z

Alright @mzorz, did another full pass and went through all your cases. Left a few small comments for things I missed earlier - should be good to go after those are fixed 👍

…ampleActivity

@aforcier

…oad to avoid potential NPE thanks @aforcier

mzorz · 2017-05-10T15:24:16Z

ok! ready again @aforcier 🙇

aforcier · 2017-05-10T15:33:45Z

Nice work @mzorz!

mzorz added 24 commits April 7, 2017 15:32

identified the best place for progress re-attachment logic

8e080a8

added capability to update the UI given the upload listener receives …

9a87dd7

…updates (reattach)

added checks so we make sure to be able to update upload progress onl…

76a0787

…y if content available

only re-add uploading ids if values other than null obtained

8790524

don't cancel uploads when leaving the editor - not anymore

98bbed2

added flag to make sure content is loaded before attempting to refres…

a2a51fa

…h part of it

Merge branch 'feature/fluxc-media-upload-service-multiple-uploads' in…

806a7e9

…to feature/fluxc-media-upload-service-multiple-uploads-reattach-progress

changed mAztecReady flag to be set to true either after setting conte…

cbef415

…nt or after view created for new posts - also clearing failed media ids from local array

null checks added for ease of debugging

86feedf

updated feature branch name

0a3a655

reusing predicate variable

33744a6

refactored attribute handling

91e6d53

refactored overlay methods for reuse and setting everything up on set…

edcf67e

…Content() then updating it in onMediaUploadProgress()

refactoring and handling exceptions as events might come unordered

5a318b1

added comment to clarify code, and pointing to newest FluxC commit hash

f719a77

small temporal change to avoid pushing post if media upload is in pro…

c3cfb48

…gress when exiting it

Merge branch 'feature/fluxc-media-upload-service-multiple-uploads' in…

8b9f541

…to feature/fluxc-media-upload-service-multiple-uploads-reattach-progress

fixed merge conflicts

8e0c0f7

solved more merge conflicts

81c3a4c

fixed listToUpdate, as it was all going to mFailedMediaIds upon setup

be149d2

now taking into account ordering of events in FLuxc is not guaranteed…

b1c20e5

…, to make upload progress visibility show accordingly

now we also remove the class from the image once it's finished uploading

ce2f2d9

removing uploading class when finished uploading in service processor…

ead6b09

…, and passing the new post contents as a return value in the static method implementation in AztecEditorFragment

listening for Post updates from FLuxC in the posts list and refreshin…

adb76f7

…g the list so we are sure to re-load posts once media are uploaded there

aforcier self-assigned this May 1, 2017

solved merge conflicts

161d588

aforcier reviewed May 4, 2017

View reviewed changes

aforcier reviewed May 5, 2017

View reviewed changes

mzorz added 3 commits May 8, 2017 15:13

moved getEditorMimeType() to EditorFragmentAbstract and removed MimeT…

4417a4c

…ype from the EditorMediaUploadListener interface

removed log added while building the reattach progress beahvior

0cd3bc9

using already defined static var for string

89bec62

mzorz mentioned this pull request May 9, 2017

Media Async upload: different working copies of PostModel #5858

Closed

aforcier mentioned this pull request May 9, 2017

Media async upload: Refresh the PostModel when recreating the editor activity #5863

Closed

aforcier added the Media label May 10, 2017

aforcier reviewed May 10, 2017

View reviewed changes

mzorz added 3 commits May 10, 2017 12:09

fixed method signature in onMediaUploadProgress calls within EditorEx…

fd1e412

…ampleActivity

using 0f instead of new Float(0)

4606f08

added check that onMediaUploadProgress corresponds to an existing upl…

1102c4f

…oad to avoid potential NPE thanks @aforcier

aforcier merged commit 70483ab into feature/fluxc-media-upload-service-multiple-uploads May 10, 2017

aforcier deleted the feature/fluxc-media-upload-service-multiple-uploads-reattach-progress branch May 10, 2017 15:33

aforcier mentioned this pull request Jun 15, 2017

Async media: Allow posts to be edited while uploading media #6081

Merged

mzorz mentioned this pull request Jun 29, 2017

Editor: Uploading multiple images/videos does not finish #4664

Closed

mzorz mentioned this pull request Jan 2, 2019

Gb/upload media file #8879

Merged

mzorz mentioned this pull request Jan 28, 2019

Gutenberg - async media upload Post background processing #9132

Merged

1 task

Conversation

mzorz commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TEST A (plain old single file upload):

TEST B (background upload):

TEST C (background upload - reattach progress):

TEST D (multiple upload):

TEST E (background upload - multiple upload):

TEST F (background upload - multiple upload - reattach progress):

TEST G (multicancel):

Known Issues

Different working copies of PostModel

Random issues / enhancements

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniloercoli commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aforcier commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniloercoli commented May 5, 2017

Uh oh!

daniloercoli commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aforcier commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daniloercoli commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aforcier May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aforcier commented May 10, 2017

Uh oh!

mzorz commented May 10, 2017

Uh oh!

aforcier commented May 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mzorz commented Apr 28, 2017 •

edited

Loading

daniloercoli commented May 5, 2017 •

edited

Loading

aforcier commented May 5, 2017 •

edited

Loading

daniloercoli commented May 5, 2017 •

edited

Loading

aforcier commented May 9, 2017 •

edited

Loading

aforcier May 10, 2017 •

edited

Loading