[Site design revamp] Preload MShots when the site creation activity starts #16584

mkevins · 2022-05-18T09:35:53Z

Description

This PR implements a preloading function in the main site creation view model, and invokes it when the site creation activity starts. It cancels the preloading before displaying the theme picker fragment so that the remaining images can load normally.

Preloading is done in the background via Glide, without layout specified (so only the original source image would be cached). This seemed a decent middle ground, imo, since it seeks to mitigate potential network performance issues (image resizing is done locally). If it is desirable to also prepopulate the resized results, an alternative to explore would be loading the entire fragment, but I have not attempted this.

To test:

Feature flag for convenience

Testing the behavior of this flow with and without the preloading can be tedious, since it requires clearing the cache every time. Also, there are multiple levels of cache, and it is difficult to get a fresh start without reinstalling the app for each test run. For convenience, I have added a temporary feature flag to toggle the preloading on and off. Also, each time the preloading (actually any feature flag is toggled 😄 ), the Glide cache is cleared (both memory and disk). It is also necessary to clear the app cache manually from the app info screen.

Precondition:

Start the app with the app inspection tab open, and the network inspector selected in Android Studio. It will be useful to monitor the traffic during the site creation flow.

Network inspector

It can also be useful to perform these steps in an emulator, with a low data rate, e.g. GPRS & Poor.

Changing the network type and strength

Clearing the app cache

clear-app-cache.mp4

Preloading off

Disable preloading in the app debug settings (and restart the app)
Clear the app cache from the app info screen
Start the site creation flow: Sites list -> "+" -> "Create WordPress.com site"
Skip the intent screen
Expect to land on the theme picker screen
Highlight the traffic generated when this screen loads
Expect that the layouts are fetched first, followed by the MShots thumbnails

Preloading on

Enable preloading in the app debug settings (and restart the app)
Clear the app cache from the app info screen
Start the site creation flow: Sites list -> "+" -> "Create WordPress.com site"
Highlight the traffic generated when this screen loads
Expect that the layouts are fetched, followed by the MShots thumbnails
Skip the intent screen
Expect to land on the theme picker screen
Expect that the layouts are fetched, but that the MShots thumbnails are not fetched

Preloading interrupted

Enable preloading in the app debug settings (and restart the app)
Clear the app cache from the app info screen
Start the site creation flow: Sites list -> "+" -> "Create WordPress.com site"
Note the time of the traffic generated when this screen loads
Quickly skip the intent screen (before all the images are finished preloading)
Expect to land on the theme picker screen
Highlight the traffic generated while on both the intent screen and the theme picker screen
Observe the requests in the highlighted traffic:
- Expect that the layouts are requested first, followed by some (not all) of the MShots
- Expect that the layouts are requested again, followed by the remaining MShots that were not precached by this mechanism

Regression Notes

Potential unintended areas of impact
Site creation flow
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
We don't currently have the needed integration test harness established to test this.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

In order to make testing more convenient, this adds a temporary flag that can be toggled in the debug settings of the app. It also adds a hard-coded behavior to clear the Glide cache (memory and disk), for the purpose of testing the flow with a fresh start each time.

For some reason, deleting the Glide cache was not enough, and disabling the volley cache was still not enough to prevent a cache hit. I suspect that there is an additional cache later on the network side of things. Nevertheless, we may want to keep the volley cache disabled, to prevent 3 layers of duplicate cache.

The nested recycler views can only utilized a shared pool if they are given the same instance of the pool. We can either remove the line, or keep a reference to the instance at a higher level in the hierarchy, so that the children recycler views can share a pool of view holders.

…vamp--preload-mshots

peril-wordpress-mobile · 2022-05-18T09:35:59Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2022-05-18T09:57:05Z

You can test the changes on this Pull Request by downloading the APKs:

Jetpack-pr-16584-build-123979.apk

WordPress-pr-16584-build-123979.apk

mkevins · 2022-05-19T05:47:25Z

Some thoughts about other approaches

It occurs to me that with this approach, we are submitting the request for site designs (themes) and their categories twice: once at the start of the activity, in order to obtain the list of thumbnail URLs to prefetch, and once when the fragment loads. There is an opportunity for optimization here: we can save the result from the first call and avoid the second call if the first call is received in time (i.e. before the theme picker fragment is loaded).

Preload the whole fragment?

Another possibility to explore is to load the entire theme picker fragment invisibly at the start of the activity. Since we don't have the intent information at that time, the fragment would not in an identical ui state when it is finally displayed, but it might be possible, nevertheless, to populate both the Glide original source cache as well as many of the relevant resized caches (except the recommended row, since we can't know that ahead of time as it depends on the intent). Such an approach may have implications for the lifecycle of the underlying fragment (and view model).

One of the advantages of this approach is that we could be prepopulating both the original source image and the resized images generated by Glide to fit the given views. One limitation, however, is that we'd only be preloading items in the recycler view (and nested recycler views) that are "visible", and this would almost certainly be different after intent selection.

We could delay preloading until the intent is selected, but this leaves little time for preloading to be beneficial, since the picker is the very next screen shown. Later, with the site name screen re-enabled, this may put more weight towards such an approach.

antonis · 2022-05-19T10:11:22Z

Great work @mkevins 👍
Your approach looks good so far :)

There is an opportunity for optimization here: we can save the result from the first call and avoid the second call if the first call is received in time (i.e. before the theme picker fragment is loaded).

Maybe we can implement a caching mechanism on the network level similar to the one we have for the page layouts. I remember that we skipped this part in the original implementation because the users usually run this flow one. With the prefetching call though it makes sense to cache things.

…vamp--preload-mshots

WordPress/src/main/java/org/wordpress/android/ui/layoutpicker/LayoutsRowViewHolder.kt

WordPress/src/main/java/org/wordpress/android/networking/GlideMShotsLoader.kt

WordPress/src/main/java/org/wordpress/android/ui/sitecreation/SiteCreationActivity.kt

mkevins · 2022-05-20T07:00:32Z

Thanks for taking a look at this Antonis!

Maybe we can implement a caching mechanism on the network level similar to the one we have for the page layouts.

Good idea! I think this can be done in a followup PR. Wdyt?

antonis · 2022-05-20T07:08:32Z

Good idea! I think this can be done in a followup PR. Wdyt?

I agree since it would require a non-trivial amount of code and testing 👍

…vamp--preload-mshots # Conflicts: # WordPress/build.gradle

antonis

Great work @mkevins 👍
I've tested the build on an emulator (api level 32) and everything worked as described. The code changes look concise to me 🎉
Added a few suggestions and responses to your comments but nothing blocking for this PR 🙏

ps. I took the liberty to push minor changes to pash the CI test (lint, tests) 🙇

WordPress/src/main/java/org/wordpress/android/ui/sitecreation/SiteCreationMainVM.kt

…om:wordpress-mobile/WordPress-Android into feature/site-design-revamp--preload-mshots

This reverts commit 2383dc9.

This reverts commit aa00844.

…vamp--preload-mshots

mkevins · 2022-05-23T05:56:30Z

WordPress/src/main/java/org/wordpress/android/ui/sitecreation/SiteCreationMainVM.kt

+    private val fetchHomePageLayoutsUseCase: FetchHomePageLayoutsUseCase
 ) : ViewModel() {
+    init {
+        // TODO: Remove the duplicate {,un}registration in the picker view model


With the cancellation (interruption) flow, we need to have this in both places. The only thing left to remove is this comment, which I forgot to remove before merging 🤦‍♂️ .

Removed here 5351d73.

Part of PR: #16619

Thanks Ovi!

ovitrif · 2022-05-23T12:08:47Z

We could also remove the if check inside the when case

I agree, addressed here:
0d522e6

Part of PR: #16619

mkevins · 2022-05-23T22:27:32Z

We could also remove the if check inside the when case

🤦‍♂️ why did I not see this 😅

Thanks for fixing it @ovitrif 👍 🙇‍♂️

mkevins added 6 commits May 18, 2022 18:41

Implement preloader function in main site creation view model

46cd6c0

Invoke preloading at the start, and cancel before theme picker loads

04a4cdb

Merge branch 'feature/site-design-revamp' into feature/site-design-re…

4ad7aff

…vamp--preload-mshots

mkevins added [Type] Enhancement Site Creation Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. labels May 18, 2022

mkevins requested review from antonis and ovitrif May 18, 2022 09:36

Merge branch 'feature/site-design-revamp' into feature/site-design-re…

fe52805

…vamp--preload-mshots

mkevins commented May 20, 2022

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/layoutpicker/LayoutsRowViewHolder.kt Outdated Show resolved Hide resolved

mkevins commented May 20, 2022

View reviewed changes

WordPress/src/main/java/org/wordpress/android/networking/GlideMShotsLoader.kt Outdated Show resolved Hide resolved

mkevins commented May 20, 2022

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/sitecreation/SiteCreationActivity.kt Outdated Show resolved Hide resolved

mkevins marked this pull request as ready for review May 20, 2022 06:59

antonis added 2 commits May 20, 2022 15:04

Fixes Lint issue

12f8f4c

Merge branch 'feature/site-design-revamp' into feature/site-design-re…

5130cbd

…vamp--preload-mshots # Conflicts: # WordPress/build.gradle

antonis approved these changes May 20, 2022

View reviewed changes

Updates view model test with the new parameters

86f014e

antonis reviewed May 20, 2022

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/sitecreation/SiteCreationMainVM.kt Outdated Show resolved Hide resolved

mkevins added 3 commits May 23, 2022 11:43

Merge branch 'feature/site-design-revamp--preload-mshots' of github.c…

28900f7

…om:wordpress-mobile/WordPress-Android into feature/site-design-revamp--preload-mshots

Move preload cancellation inside when expression

ba5f8c1

Set preloading null outside of network check

290593e

mkevins added 4 commits May 23, 2022 15:21

Revert "Add temporary feature flag to facilitate testing"

1a67643

This reverts commit 2383dc9.

Revert "Disable volley cache for MShots loader"

0cfce3d

This reverts commit aa00844.

Merge branch 'feature/site-design-revamp' into feature/site-design-re…

e9b9a3d

…vamp--preload-mshots

Remove instantiation of nested recycledViewPools

b2f99c7

mkevins merged commit 29c2398 into feature/site-design-revamp May 23, 2022

mkevins deleted the feature/site-design-revamp--preload-mshots branch May 23, 2022 05:53

mkevins commented May 23, 2022

View reviewed changes

ovitrif mentioned this pull request May 23, 2022

[Site Creation] Tweak UI of Site Theme to match design specs #16619

Merged

3 tasks

This was referenced May 24, 2022

[Site design revamp] Use the custom Glide loader for preloading the thumbnails #16624

Merged

Site Design Screen Improvements #16468

Merged

[Site design revamp] Preload MShots when the site creation activity starts #16584

[Site design revamp] Preload MShots when the site creation activity starts #16584

Uh oh!

Conversation

mkevins commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

To test:

Feature flag for convenience

Preloading off

Preloading on

Preloading interrupted

Regression Notes

Uh oh!

peril-wordpress-mobile bot commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkevins commented May 19, 2022

Some thoughts about other approaches

Preload the whole fragment?

Uh oh!

antonis commented May 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkevins commented May 20, 2022

Uh oh!

antonis commented May 20, 2022

Uh oh!

antonis left a comment • 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!

Uh oh!

mkevins May 23, 2022

Choose a reason for hiding this comment

Uh oh!

ovitrif May 23, 2022

Choose a reason for hiding this comment

Uh oh!

mkevins May 23, 2022

Choose a reason for hiding this comment

Uh oh!

ovitrif commented May 23, 2022

Uh oh!

mkevins commented May 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkevins commented May 18, 2022 •

edited

Loading

peril-wordpress-mobile bot commented May 18, 2022 •

edited

Loading

peril-wordpress-mobile bot commented May 18, 2022 •

edited

Loading

antonis left a comment •

edited

Loading