Offline Pages : PageList Item Progress State

[jd-alexander]

Fixes #11127

Findings

The Post List support showing progress for uploads and since pages are posts then the implementation for the Post List can be adapted for the Page List.

Solution

To utilize existing classes and logic that's used for the Post List progress state to power the Page List. This primarily involves creating new classes for this behavior since refactoring the logic for the Post List to support Pages might not be in our best interest at this point since we aren't sure what will need to be shared.

An additional improvement that was made in this PR was to create a component similar to PostListEventListener called PageListEventListener so that the calls and events from the dispatcher are encapsulated in a central component to reduce the clutter in the PagesViewModel. While doing the review, I would appreciate if the coroutine and overall thread usage could be checked to ensure it is correct.

Testing

For all these tests it's best to change the device's network type to GSM and it's signal strength to poor to observe the state changes in the list.

Upload Page Offline

1. Turn on airplane mode.
2. Create a new post.
3. Click publish.
4. Ensure the post you published is visible and turn airplane mode off.
5. Observe that the state changes cause the Page List Item to display an indeterminate ProgressBar and an overlay on the row contents.

Before	After

Upload Page with Image/Video Offline

1. Turn on airplane mode.
2. Create a new post and add an image or video.
3. Click publish.
4. Ensure the post you published is visible and turn airplane mode off.
5. Observe that the state changes cause the Page List Item to display a determinate ProgressBar and an overlay on the row contents.
6. The ProgressBar should show the percentage of the current upload.
7. Also, observe that when the media upload is complete the ProgressBar switches back to Indeterminate to finish the upload.

Before	After

Other testing considerations

1. Ensure that both cases above work when the device is online and didn't go into airplane mode.
2. While the upload is taking place, navigate back to the My Site page and then click the Pages button and ensure the correct state is being shown.
3. Also, ensure that the progress state is always Hidden once the upload is complete or an error takes place. (Error wasn't implemented in this PR, it possibly just switches back to the local changes label and shows the error in the notification section.)
4. There are several events within PostListEventListener that inspired the PageListEventListenerIt's important that a comparison between both behaviors are done to ensure that the upload state is reacting effectively to this. Some of the events that weren't copied over were related to showing error and success states so those were left for future PRs. Only events that modify the upload state were utilized.
5. The layout wasn't modified to support the status label being above the overlay since it was outside the scope of this PR, so in the tests that will be noticeable.
6. When testing with video, the video optimization can be enabled in the App Settings so that the progress from that can also be propagated and verified.

Reviewing

Only 1 reviewer is needed but anyone can review.

Submitter Checklist

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
• I have considered adding accessibility improvements for my changes.
• If it's feasible, I have added unit tests.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[malinajirka]

Thanks @jd-alexander! I've reviewed the code again and noticed a few more things. Let me know what you think.

I also tested the app and I encountered following issues:
1.

• Open Page List
• Click on the overflow menu on one of the items
• Click eg. on "Move to drafts" or "Publish Now"
• Boom

2020-02-06 13:55:42.744 E: FATAL EXCEPTION: DefaultDispatcher-worker-5
    Process: org.wordpress.android.beta, PID: 17352
    java.lang.IllegalStateException: post must not be null
        at org.wordpress.android.viewmodel.pages.PageItemUploadProgressHelper.getProgressStateForPage(PageItemUploadProgressHelper.kt:43)
        at org.wordpress.android.viewmodel.pages.PageListViewModel.preparePublishedPages(PageListViewModel.kt:253)
        at org.wordpress.android.viewmodel.pages.PageListViewModel.access$preparePublishedPages(PageListViewModel.kt:48)
        at org.wordpress.android.viewmodel.pages.PageListViewModel$loadPagesAsync$1.invokeSuspend(PageListViewModel.kt:170)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:561)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:727)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:667)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:655)

2. When I open a page -> change title -> click on back -> the progress indicator never disappears.

3. Probably not related to this PR, but I'm curious if you know why. We hide the progress indicator when the upload finishes -> how come the "local changes" label disappears after another +- 3 seconds?

[jd-alexander]

Thanks for the review!

1. I was able to reproduce the crash after a few tries. I will look into it.
2. Reproduced and looking into it.
3. It seems that because the page is being reloaded from the server there is a delay before the statuses are updated.

[jd-alexander]

I fixed the issues below :

1. Open Page List
   Click on the overflow menu on one of the items
   Click eg. on "Move to drafts" or "Publish Now"
   Boom
   ebc2cd6

2. When I open a page -> change title -> click on back -> the progress indicator never disappears.
   22525dd

3. Probably not related to this PR, but I'm curious if you know why. We hide the progress indicator when the upload finishes -> how come the "local changes" label disappears after another +- 3 seconds?
   42faec8

[malinajirka]

Thanks for the changes @jd-alexander! I know you said you haven't had a chance to do thorough tests after the latest changes. I decided to review the PR anyway so we speed things up a bit.

1. ebc2cd6

Have you investigated how come the post is null? I wouldn't expect it to be null after clicking on the overflow menu.
I'm a bit worried the null check might just hide a potential issue. The database just returned us a list of pages and a few ms later it seems the page is not in the database anymore. Do we remove the page from the database before we move it to draft/publish it?

👍

3. 42faec8

If I understand the fix correctly, we won't hide the progress indicator when the upload completes and we let the existing logic handle the refresh of the item, right? Tbh I'm getting a bit lost in the logic - it feels weird that we don't need to invalidate the status when upload completes successfully. Would you mind discussing this on a call. Thanks!

I've encountered the following crash

• I'm not able to reproduce it. I created a post, added a title and I clicked on "Up" button in the editor.

2020-02-07 11:45:13.904 E: FATAL EXCEPTION: DefaultDispatcher-worker-4
    Process: org.wordpress.android.beta, PID: 5736
    java.lang.IllegalStateException: Already resumed, but proposed with update kotlin.Unit
        at kotlinx.coroutines.CancellableContinuationImpl.alreadyResumedError(CancellableContinuationImpl.kt:332)
        at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl(CancellableContinuationImpl.kt:327)
        at kotlinx.coroutines.CancellableContinuationImpl.resumeWith(CancellableContinuationImpl.kt:247)
        at org.wordpress.android.viewmodel.pages.PagesViewModel.handlePostUploadedWithoutError(PagesViewModel.kt:701)
        at org.wordpress.android.viewmodel.pages.PagesViewModel$start$1.invoke(PagesViewModel.kt:185)
        at org.wordpress.android.viewmodel.pages.PagesViewModel$start$1.invoke(PagesViewModel.kt:76)
        at org.wordpress.android.viewmodel.pages.PageListEventListener$onPostChanged$1.invokeSuspend(PageListEventListener.kt:100)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:561)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:727)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:667)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:655)

[jd-alexander]

Thanks for the review!

Have you investigated how come the post is null? I wouldn't expect it to be null after clicking on the overflow menu.
I'm a bit worried the null check might just hide a potential issue. The database just returned us a list of pages and a few ms later it seems the page is not in the database anymore. Do we remove the page from the database before we move it to draft/publish it?

It doesn't seem like this is the case. Based on a glance I just did at the Flux C code, an update query is run. Nonetheless, I created an issue to track this and continue the investigation. #11260

If I understand the fix correctly, we won't hide the progress indicator when the upload completes and we let the existing logic handle the refresh of the item, right? Tbh I'm getting a bit lost in the logic - it feels weird that we don't need to invalidate the status when upload completes successfully. Would you mind discussing this on a call. Thanks!

I understand this clearly. I did the invalidation but I am seeing what is causing the error now. Will share on the call.

[malinajirka]

Thanks @jd-alexander! As we discussed I'm approving this PR even though it still contains some issues. We are merging it into a feature-branch, so it shouldn't matter. We'll be able to create more smaller PRs.

I can't merge since the connected test failed. Feel free to merge it when the issue is fixed (it's probably just a flaky test and restart of the flow should fix it).

[jd-alexander]

Thanks @malinajirka I created issues for all the outstanding bugs that need to be resolved based on our exploration. I will merge this now since they are being tracked. Thanks for the reviews! Now time to add the other PRs that depend on the UploadStatus 🙏