For #26511: load homescreen wallpaper in blocking coroutine

[MatthewTighe]

Pull Request checklist

• Tests: This PR includes thorough tests or an explanation of why it does not
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

• QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

1. Click on Checks at the top of the PR page.
2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
3. Click on the build-debug task.
4. Click on View task in Taskcluster in the new DETAILS section.
5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #26511

[jonalmeida]

This read like it's a hack for something - we're reading from the store first and setting the wallpaper, before we start to observe the store and set the wallpaper again? What is happening that makes it slow to just have the observer which gives you the initial state?

[MatthewTighe]

Ah, you're right. The repetition there shouldn't actually be necessary. Thanks!

[MatthewTighe]

Actually, it looks like it is required to get the response time we'd like. I could only make guesses as to why the observation seems to take longer. I would guess it might relate to the flow being collected in a coroutine that is launched, which is why I had to switch to this runBlocking implementation in the first place.

I believe we'll still want the observation though, as we will want the home screen to react to changes that are made from the upcoming wallpaper selection tool #26216. I don't have a good way to avoid these repeated calls of the top of my head, unless we want to start the observation from wherever we launch the selection tool from instead of from onCreate

[MatthewTighe]

Come to think of it, this may just be related to viewLifecycleOwner.lifecycleScope. IIRC it doesn't run coroutines until after onViewCreated

[MatthewTighe]

Changing it to the fragment's direct lifecycleScope or even the activity's lifecycleScope did not seem to improve the issue.

I would be okay with removing the observation here for now and punting on finding the right place for it until we do the selection tool. The wallpaper should only ever be updated while the fragment is destroyed, since it can only be currently changed from the wallpaper settings. Previously, we had the additional constraint of it being changed when the logo on the home screen was clicked but we have removed that feature

[gabrielluong]

One way we can proceed here is to comment out the observeWallpaperChanges code for now and file a follow up issue to investigate. Perhaps @Mugurell or @mcarare will have a cycle to dig deeper and suggest a solution. Other than resolving the discussion here, I think the changes looks good from my perspective.

[MatthewTighe]

I think that's a good idea. I will file a follow up

[gabrielluong]

Cool, I will pre-attemptively approve this for now since I don't need to re-review commented out changes. My only ask is that you throw it the newly filed issue into the current sprint.

[gabrielluong]

Curious why we chose to use runBlockingIncrement here instead of launching a coroutine like lifecycleScope.launch(IO) {}?

[MatthewTighe]

Launching it in a coroutine is actually what causes the bug. I believe that is because lifecycleScopes do not become active until the Fragment is already passed its onViewCreated lifecycle state as described in this comment #26515 (comment)

[gabrielluong]

Since we are calling setWallpaperToCurrent() and observeWallpaperChanges(), is it possible for the same wallpaper to be shown twice via this showWallpaper call? If so, can we add a check to not do anything if the wallpaper to be shown is already the currently shown wallpaper?

Normally, I don't think this should be a problem, but it's my reaction to seeing the discussion above.

[MatthewTighe]

Since we are calling setWallpaperToCurrent() and observeWallpaperChanges()

There is some conversation above about whether it might even be correct for now to remove observeWallpaperChanges. Doing so could clean this up, but it might add extra work to the wallpaper selection tool. The wallpaper would indeed get shown twice using the current strategy, but there is no visual interruption as far as I can tell. It's possible the platform does something smart in terms of caching ImageViews, but I don't know for sure.

If so, can we add a check to not do anything if the wallpaper to be shown is already the currently shown wallpaper?

I would be open to this strategy. I generally try to avoid introducing mutable state where possible, but this sounds like it might be the simplest solution.

[gabrielluong]

Ya, I would want to avoid introducing a mutable state as well. See #26515 (comment) if this could be a potential solution with move forward with to unblock this.

[gabrielluong]

I would prefer if we landed this after the Beta cut on Monday, and uplifting this after we get QA verified if there is an a demand to get this fix into 105. Otherwise, ignore the uplift comment if we don't expect this fix to be urgently released in 105. Added a do-not-land for this reasoning.

[MatthewTighe]

I would prefer if we landed this after the Beta cut on Monday, and uplifting this after we get QA verified if there is an a demand to get this fix into 105. Otherwise, ignore the uplift comment if we don't expect this fix to be urgently released in 105.

Cool, that sounds good to me. I think this is worth uplifting since the bug is pretty noticeable and I would not like it to go to release, but I will follow up with the relevant parties to see if there is agreement once the QA cycle is finished.

[MatthewTighe]

Filed #26555 as a follow up

[Mugurell]

Investigating #26555 I see cases in which with this code applied the wallpaper would not get set - #26555 (comment).
Not sure if this is a new or pre-existing issue.

This seems to be a bug introduced by these changes which results in a race with wallpaperUseCases.initialize for reading an "initialized" wallpaper.

Posted my recommendations in #26555 (comment). Curious about your thoughts.

[MatthewTighe]

I've rebased this over some other wallpaper work and included some changes to cache metadata properties as described in the comments of #26555 (comment). Unfortunately, I was not address @Mugurell's concerns. The old version of this patch still exists at a branch on my fork here if anyone needs it https://github.com/MatthewTighe/fenix/tree/old-wallpapers-on-main

[MozillaNoah]

@Mugurell @gabrielluong Were there any outstanding changes being requested here or any leftover gotchas, or is this clear for landing to move along #26562?

[gabrielluong]

@Mugurell @gabrielluong Were there any outstanding changes being requested here or any leftover gotchas, or is this clear for landing to move along #26562?

I am gonna defer to @Mugurell for the final decision since he has investigated this more thoroughly than I have. The changes have changed quite a bit since my approval as well. @Mugurell can you also do a quick review and provide your recommendation on how we can best unblock this?

[Mugurell]

@MozillaNoah @gabrielluong
Both the original and the subsequent implementation had the bug about only starting to observe the store in onStart - after the screen is already visible so the issue would not be resolved.
This was discussed in #26555.
The latest version put by Matt came to help speed setting the wallpaper for the first time by loading it from settings - and not wait to download it. It removed the race that previously existed with reading directly from the state in onCreateView but kept the same observing style that would mean the originally reported bug is still there.

I would propose landing this as part of #26647 in one go, with this commit serving as a good base towards faster dispatching of the initial wallpaper to store and then my second commit coming to observe the store and update the wallpaper before HomeFragment is displayed and so fixing the original bug.
Matt left me a message on Slack that a fix should be uplifted to beta so uplifting both commits in one go would ensure a smooth process.

[MozillaNoah]

I would propose landing this as part of #26647 in one go, with this commit serving as a good base towards faster dispatching of the initial wallpaper to store and then my second commit coming to observe the store and update the wallpaper before HomeFragment is displayed and so fixing the original bug. Matt left me a message on Slack that a fix should be uplifted to beta so uplifting both commits in one go would ensure a smooth process.

@Mugurell Gotcha gotcha. Should we aim to land these on Monday to better align their readiness? Since this PR needs its unit tests fixed, #26647 is still a draft, and I want to be respectful to your time zone.

[Mugurell]

I would propose landing this as part of #26647 in one go, with this commit serving as a good base towards faster dispatching of the initial wallpaper to store and then my second commit coming to observe the store and update the wallpaper before HomeFragment is displayed and so fixing the original bug. Matt left me a message on Slack that a fix should be uplifted to beta so uplifting both commits in one go would ensure a smooth process.

@Mugurell Gotcha gotcha. Should we aim to land these on Monday to better align their readiness? Since this PR needs its unit tests fixed, #26647 is still a draft, and I want to be respectful to your time zone.

Thank you!
Updated #26647, tested more, for me the combined changes seems to work great. If it gets approved I'd say that can be landed today so that QA can test on Monday and uplift to beta then. Landing on Monday sounds good also.

[MozillaNoah]

Thank you! Updated #26647, tested more, for me the combined changes seems to work great. If it gets approved I'd say that can be landed today so that QA can test on Monday and uplift to beta then. Landing on Monday sounds good also.

OK awesome! Let's land on Monday, just since this particular PR isn't my own code, so it won't be as easy to troubleshoot if anything bad happens. I feel comfortable having you land this, so you can land #26647 immediately after. Otherwise, we can just coordinate when my day starts Pacific time.