Async media: Publish/Save while uploading media #5880

aforcier · 2017-05-11T12:35:25Z

Adds support for publishing/saving posts while they have media uploads in progress.

Caveats:

Doesn't handle cancelled images (this could only happen if the MediaUploadService were destroyed while uploading media) - will be handled in a future PR
There's no messaging of what's going on (other than the 'Uploading...' state of the post card in the list) until the post upload completes and we see a notification. The upload UI is getting an overhaul as part of the async experience (in future PRs). In the meantime, I'll open a separate PR adding plain notifications for async uploads for demonstration purposes.
This breaks reattachment, in the sense that you can't re-open a post once you've exited the editor, until it finishes uploading

To test:

A:

Create or edit a post, and add media
While media are uploading, press Publish (or Update)
The post should be in 'Uploading' state in the post list
Wait for the post to finish uploading (the state will update, and you'll get a 'Published' notification)
Open the post, and confirm that all media are uploaded and not local

B:

Create or edit a post, and add media
While media are uploading, exit the Editor
The post should be in 'Uploading' state in the post list
Wait for the post to finish uploading (the state will update to Draft, and you'll get a 'Published' notification)
Open the post, and confirm that all media are uploaded and not local

C:

Repeat A and B, but induce an error in the image upload
The post should be saved as a local draft
Opening the post should have the failed media in 'failed' status

Also check that uploading/closing the editor with already failed media behaves the same as before, and that non-media uploads work correctly.

cc @mzorz and @daniloercoli

mzorz · 2017-05-11T17:17:39Z

WordPress/src/main/java/org/wordpress/android/ui/media/services/MediaUploadService.java

    }
-}
+
+    public static boolean hasPendingMediaUploadsForPost(PostModel postModel) {


can we pass the postID here instead of the Model? given that's the only thing being accessed within this method after all

I don't think we should - a style rule of using FluxC is to always use models wherever possible. This avoids ambiguities we used to have where you get passed an id that could represent either a local id or a remote id.

👍 for now ;)

mzorz · 2017-05-11T17:21:36Z

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

+    */
+    @SuppressWarnings("unused")
+    public void onEventMainThread(PostEvents.PostUploadCanceled event) {
+        if (isAdded() && mSite.getId() == event.localSiteId) {


I understand mSite shouldn't be null, but reading the onCreate sequence it seems this class is registered for FLuxC event listening before the mSite property is initialized in updateSiteOrFinishActivity(). While that might deserve further analysis, I'd say at least for the sake of this PR we check mSite is different than null here to avoid a NPE.

Agreed with the null check - note though that this isn't a FluxC callback - it's the old WordPress-internal event bus we still use for some internal actions.

oops meant that! thanks :)

Done in f18488f (I also made updateSiteOrFinishActivity() get called first, which should prevent any events being handled before we've finished setting up mSite).

mzorz · 2017-05-11T17:25:49Z

Minor comments left here @aforcier - checking functionality now

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListFragment.java # WordPress/src/main/res/values/strings.xml

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/media/services/MediaUploadService.java

mzorz · 2017-06-02T12:33:20Z

WordPress/src/main/java/org/wordpress/android/ui/posts/services/PostUploadService.java

+
+        if (event.canceled) {
+            // TODO: If a media upload for a post was cancelled, we might want to cancel the post upload, too
+            // Not implemented


Should we cancel the post upload here then? If this event only means the user triggered the cancel action, then I think we're safe to cancel the post upload as well

The reason I added a TODO was so we could think out this use case a bit more - currently there's no way for this to be called (can't cancel a media upload outside the editor, and re-opening a post with uploading media will cancel the post upload, so cancelling at that point will also not call this).

But I think it makes sense to just add the behavior with some logging so we can tell this is happening if we're confused about something in the future.

Done in 3d25a37 and 962627f.

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListFragment.java

aforcier · 2017-06-05T13:33:24Z

Ready for another look, @mzorz!

mzorz · 2017-06-06T19:01:37Z

Case A: OK
Case B: OK
Case C (B): turning airplane mode ON after exiting the Editor leaves the post list with the new post with the spinning wheel forever, see video here https://cloudup.com/ct3G73l8-Hd

aforcier · 2017-06-06T19:56:37Z

Turning airplane mode ON after exiting the Editor leaves the post list with the new post with the spinning wheel forever, see video here https://cloudup.com/ct3G73l8-Hd

Eeek. Doesn't look async related though, I'm able to reproduce this in develop - shall we open a new issue (if you confirm it's the same bug in develop)?

mzorz · 2017-06-08T12:26:11Z

Eeek. Doesn't look async related though, I'm able to reproduce this in develop - shall we open a new issue (if you confirm it's the same bug in develop)?

👌 Opened issue #6026

aforcier added 10 commits May 10, 2017 20:08

Enable editor Publish button while uploading media

eb0507b

Support checking for pending media uploads for posts

b5a86cb

Skip posts in upload queue with pending media uploads

984b57a

Re-check post upload queue when relevant media uploads complete

90c6bbe

Make sure PostModel is saved to DB before uploading

9a4351e

Clean up some duplication in PostUploadService

23ff639

Clean up MediaUploadService logging

0980a41

Stop uploading posts if their media uploads fail

dc9c3a5

Refresh the post list when an uploading post is cancelled

50585df

Auto-save posts remotely after media uploads complete

d38326e

aforcier added the Media label May 11, 2017

aforcier mentioned this pull request May 11, 2017

Async media: Notifications for async publish #5881

Merged

mzorz self-assigned this May 11, 2017

mzorz reviewed May 11, 2017

View reviewed changes

aforcier added 2 commits May 24, 2017 12:41

Merge branch 'feature/async-media' into feature/async-media-plus-publish

34f6e1f

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListFragment.java # WordPress/src/main/res/values/strings.xml

Safeguard against receiving events before mSite inits

f18488f

aforcier force-pushed the feature/async-media-plus-publish branch from 60d765c to f18488f Compare May 24, 2017 17:30

aforcier added 2 commits May 26, 2017 18:48

Merge branch 'feature/async-media' into feature/async-media-plus-publish

f7265cd

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/media/services/MediaUploadService.java

Merge branch 'feature/async-media' into feature/async-media-plus-publish

e1156e7

mzorz reviewed Jun 2, 2017

View reviewed changes

aforcier added 3 commits June 5, 2017 08:47

Cancel post uploads that have cancelled media

3d25a37

Extract duplicated code to cancelPostUploadMatchingMedia()

962627f

Merge branch 'feature/async-media' into feature/async-media-plus-publish

95254e3

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListFragment.java

mzorz mentioned this pull request Jun 8, 2017

Losing connectivity while post is uploading leaves it in uploading state forever #6026

Closed

mzorz merged commit f149062 into feature/async-media Jun 8, 2017

aforcier deleted the feature/async-media-plus-publish branch June 8, 2017 12:32

aforcier mentioned this pull request Jun 15, 2017

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

Merged

Async media: Publish/Save while uploading media #5880

Async media: Publish/Save while uploading media #5880

Uh oh!

Conversation

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

Uh oh!

Caveats:

To test:

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!

mzorz commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aforcier Jun 5, 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!

aforcier commented Jun 5, 2017

Uh oh!

mzorz commented Jun 6, 2017

Uh oh!

aforcier commented Jun 6, 2017

Uh oh!

mzorz commented Jun 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aforcier commented May 11, 2017 •

edited

Loading

aforcier Jun 5, 2017 •

edited

Loading