Skip to content

Conversation

@aforcier
Copy link
Contributor

Bridges async publishing (#5880) with progress reattachment (#5780). Without this PR, exiting a post while media are uploading will queue the post for upload, and save it as a remote draft (where applicable) once the media complete. During this time, the 'Uploading' overlay is present, blocking the post from being edited, even though that is now supported thanks to the reattachment work.

With this PR, we only show the overlay while the post content itself is uploading (and not while its media are uploading). The post is removed from the upload queue if it's re-opened, allowing the media to complete but preventing the post from being uploaded while it's being edited.

Note: The UI for the post list with uploading posts will be overhauled as part of the async redesign - I just made the minimal changes necessary to support reattachment for the moment.

To test:

  1. Make sure Aztec is enabled
  2. Add media to a post, and exit while they are progressing
  3. Notice that the post in the post list is marked 'Uploading', but is tappable
  4. Edit the post, observe that the progress of the media is updated
  5. Repeat 2-3, but this time wait until media complete (and the post content itself starts uploading)
  6. Observe that the grayed-out overlay is shown above the post item in the list, making it untappable

cc @mzorz

aforcier added 5 commits June 15, 2017 11:36
Previously, we blocked editing of posts even while they were queued
for upload. Since we can now re-attach the progress of any media in
the editor, we can allow posts to be edited at any time except when
the actual post content is being uploaded.

// prevent deleting post while it's being uploaded
if (!PostUploadService.isPostUploading(post)) {
if (!PostUploadService.isPostUploadingOrQueued(post)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a Toast message here in the else condition? Saying something like This post is either being uploaded or in the queue to upload and can't be trashed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this needs design input and we're putting that off for now, I'd rather leave it obviously not working than add what looks like a finished solution - and rely on that plus the TODO and our own list of tasks to track updating this once we have a final design to implement for the post list. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally! As discussed, we'll treat this particular behavior separately 👍

if (event.media != null && event.completed) {
PostModel post = mPostStore.getPostByLocalPostId(event.media.getLocalPostId());
if (post != null && PostUploadService.isPostUploadingOrQueued(post)) {
loadPosts(LoadMode.FORCED);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice addition here - refresh post list when a new post has just been uploaded. Do you know if this code is run as well when some other activity is in the foreground, i.e. the Post Preview or the Editor for a post other than the one that has just been finished uploading ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Disregard this comment - performed some tests to realize how it's actually working 👍

// no "..." more button when uploading
pageHolder.btnMore.setVisibility(PostUploadService.isPostUploading(post) ? View.GONE : View.VISIBLE);
pageHolder.btnMore.setVisibility(PostUploadService.isPostUploadingOrQueued(post) ? View.GONE :
View.VISIBLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is interesting! maybe we can do the same for the trash icon instead of adding the Toast I suggested above?


public static void cancelQueuedPostUpload(PostModel post) {
synchronized (mPostsList) {
Iterator<PostModel> iterator = mPostsList.iterator();
Copy link
Contributor

@mzorz mzorz Jun 15, 2017

Choose a reason for hiding this comment

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

Wouldn't it be better to rename mPostsList to something like mQueuedPostsList now that this PR makes a clearer distinction between something being currently uploaded as opposed to something being in a queue to upload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent idea, updated in c007c57.

@mzorz
Copy link
Contributor

mzorz commented Jun 15, 2017

Very nice @aforcier ! Tested it with multiple images, on wp.com and self-hosted sites, rotating the screen, etc. and it all worked as expected. Left some minor comments only. Good job!

@mzorz
Copy link
Contributor

mzorz commented Jun 15, 2017

:shipit:

@mzorz mzorz merged commit 4393490 into feature/async-media Jun 15, 2017
@aforcier aforcier deleted the issue/async-media-fix-reattachment branch June 15, 2017 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants