From d6330b6dacc405cfd57618648537e44fb1b37ce0 Mon Sep 17 00:00:00 2001 From: Mugurell Date: Fri, 2 Sep 2022 13:39:56 +0300 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/26511: Parallelize work for setting the wallpaper Split loading the bitmap from storage and actually setting it in two operations with one that can run in parallel with onCreateView for HomeFragment and one that can be used serially on the main thread to actually set the wallpaper. This seems like the best compromise to ensure that everytime the homescreen is shown it will have the wallpaper set but does affect the performance - there is a delay in showing HomeFragment to account for waiting for the wallpaper to be set. In testing the new delay seems close to the one from the initial wallpapers implementation. See more in https://github.com/mozilla-mobile/fenix/pull/26794. --- .../org/mozilla/fenix/home/HomeFragment.kt | 8 +- .../mozilla/fenix/home/WallpapersObserver.kt | 91 ++++++-- .../fenix/home/WallpapersObserverTest.kt | 195 +++++++++--------- 3 files changed, 179 insertions(+), 115 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index d88c4b7430..8b32ed7fbc 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -213,6 +213,7 @@ class HomeFragment : Fragment() { if (shouldEnableWallpaper()) { wallpapersObserver = WallpapersObserver( appStore = components.appStore, + settings = requireContext().settings(), wallpapersUseCases = components.useCases.wallpaperUseCases, wallpaperImageView = binding.wallpaperImageView, ).also { @@ -412,7 +413,6 @@ class HomeFragment : Fragment() { getMenuButton()?.dismissMenu() if (shouldEnableWallpaper()) { - // Setting the wallpaper is a potentially expensive operation - can take 100ms. // Running this on the Main thread helps to ensure that the just updated configuration // will be used when the wallpaper is scaled to match. // Otherwise the portrait wallpaper may remain shown on landscape, @@ -757,6 +757,12 @@ class HomeFragment : Fragment() { lifecycleScope.launch(IO) { requireComponents.reviewPromptController.promptReview(requireActivity()) } + + if (shouldEnableWallpaper()) { + runBlockingIncrement { + wallpapersObserver?.applyCurrentWallpaper() + } + } } private fun dispatchModeChanges(mode: Mode) { diff --git a/app/src/main/java/org/mozilla/fenix/home/WallpapersObserver.kt b/app/src/main/java/org/mozilla/fenix/home/WallpapersObserver.kt index 25456f43ff..83d7bb9afc 100644 --- a/app/src/main/java/org/mozilla/fenix/home/WallpapersObserver.kt +++ b/app/src/main/java/org/mozilla/fenix/home/WallpapersObserver.kt @@ -4,21 +4,27 @@ package org.mozilla.fenix.home +import android.graphics.Bitmap import android.widget.ImageView import androidx.annotation.VisibleForTesting +import androidx.annotation.WorkerThread import androidx.core.view.isVisible import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleObserver import androidx.lifecycle.LifecycleOwner +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.cancel import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import mozilla.components.lib.state.Store import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction import org.mozilla.fenix.components.appstate.AppState import org.mozilla.fenix.ext.scaleToBottomOfView +import org.mozilla.fenix.utils.Settings import org.mozilla.fenix.wallpapers.Wallpaper import org.mozilla.fenix.wallpapers.WallpapersUseCases @@ -28,18 +34,44 @@ import org.mozilla.fenix.wallpapers.WallpapersUseCases * when the [LifecycleOwner] is destroyed. * * @param appStore Holds the details abut the current wallpaper. + * @param settings Used for checking user's option for what wallpaper to use. * @param wallpapersUseCases Used for interacting with the wallpaper feature. * @param wallpaperImageView Serves as the target when applying wallpapers. + * @param backgroundWorkDispatcher Used for scheduling the wallpaper update when the state is updated + * with a new wallpaper. */ class WallpapersObserver( private val appStore: AppStore, + private val settings: Settings, private val wallpapersUseCases: WallpapersUseCases, private val wallpaperImageView: ImageView, + backgroundWorkDispatcher: CoroutineDispatcher = Dispatchers.IO, ) : DefaultLifecycleObserver { @VisibleForTesting internal var observeWallpapersStoreSubscription: Store.Subscription? = null + + /** + * Coroutine scope for updating the wallpapers when an update is observed. + * Allows for easy cleanup when the client of this is destroyed. + */ + @VisibleForTesting + internal var wallpapersScope = CoroutineScope(backgroundWorkDispatcher) + + /** + * Setting the wallpaper assumes two steps: + * - load - running on IO + * - set - running on Main + * This property caches the result of [loadWallpaper] to be later used by [applyCurrentWallpaper]. + */ + @Volatile @VisibleForTesting - internal var wallpapersScope = CoroutineScope(Dispatchers.Main.immediate) + internal var currentWallpaperImage: Bitmap? = null + + /** + * Listener for when the first observed wallpaper is loaded and available to be set. + */ + @VisibleForTesting + internal val isWallpaperLoaded = CompletableDeferred() init { observeWallpaperUpdates() @@ -49,8 +81,20 @@ class WallpapersObserver( * Immediately apply the current wallpaper automatically adjusted to support * the current configuration - portrait or landscape. */ - fun applyCurrentWallpaper() { - showWallpaper() + internal suspend fun applyCurrentWallpaper() { + isWallpaperLoaded.await() + + withContext(Dispatchers.Main.immediate) { + with(currentWallpaperImage) { + when (this) { + null -> wallpaperImageView.isVisible = false + else -> { + scaleToBottomOfView(wallpaperImageView) + wallpaperImageView.isVisible = true + } + } + } + } } override fun onDestroy(owner: LifecycleOwner) { @@ -63,37 +107,42 @@ class WallpapersObserver( var lastObservedValue: Wallpaper? = null observeWallpapersStoreSubscription = appStore.observeManually { state -> val currentValue = state.wallpaperState.currentWallpaper + + // Use the persisted wallpaper name to wait until a state update + // that contains the wallpaper that the user chose. + // Avoids setting the AppState default wallpaper if we know that another wallpaper is chosen. + if (currentValue.name != settings.currentWallpaperName) { + return@observeManually + } + // Use the wallpaper name to differentiate between updates to properly support // the restored from settings wallpaper being the same as the one downloaded // case in which details like "collection" may be different. + // Avoids setting the same wallpaper twice. if (currentValue.name != lastObservedValue?.name) { lastObservedValue = currentValue - showWallpaper(currentValue) + wallpapersScope.launch { + loadWallpaper(currentValue) + applyCurrentWallpaper() + } } }.also { it.resume() } } + /** + * Load the bitmap of [wallpaper] and cache it in [currentWallpaperImage]. + */ + @WorkerThread @VisibleForTesting - internal fun showWallpaper(wallpaper: Wallpaper = appStore.state.wallpaperState.currentWallpaper) { - wallpapersScope.launch { - when (wallpaper) { - // We only want to update the wallpaper when it's different from the default one - // as the default is applied already on xml by default. - Wallpaper.Default -> { - wallpaperImageView.isVisible = false - } - else -> { - val bitmap = wallpapersUseCases.loadBitmap(wallpaper) - - bitmap?.let { - it.scaleToBottomOfView(wallpaperImageView) - wallpaperImageView.isVisible = true - } - } - } + internal suspend fun loadWallpaper(wallpaper: Wallpaper) { + currentWallpaperImage = when (wallpaper) { + Wallpaper.Default -> null + else -> wallpapersUseCases.loadBitmap(wallpaper) } + + isWallpaperLoaded.complete(Unit) } } diff --git a/app/src/test/java/org/mozilla/fenix/home/WallpapersObserverTest.kt b/app/src/test/java/org/mozilla/fenix/home/WallpapersObserverTest.kt index dfb9fe0a16..08aff3c0c6 100644 --- a/app/src/test/java/org/mozilla/fenix/home/WallpapersObserverTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/WallpapersObserverTest.kt @@ -7,177 +7,186 @@ package org.mozilla.fenix.home import android.graphics.Bitmap import android.widget.ImageView import androidx.core.view.isVisible -import io.mockk.Called import io.mockk.Runs import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk import io.mockk.spyk import io.mockk.verify +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.cancel +import mozilla.components.support.test.ext.joinBlocking import mozilla.components.support.test.libstate.ext.waitUntilIdle +import mozilla.components.support.test.rule.MainCoroutineRule +import mozilla.components.support.test.rule.runTestOnMain +import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull -import org.junit.Assert.assertTrue -import org.junit.Ignore +import org.junit.Assert.assertNull +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction.WallpaperAction.UpdateCurrentWallpaper import org.mozilla.fenix.helpers.FenixRobolectricTestRunner +import org.mozilla.fenix.utils.Settings import org.mozilla.fenix.wallpapers.Wallpaper import org.mozilla.fenix.wallpapers.WallpapersUseCases @RunWith(FenixRobolectricTestRunner::class) class WallpapersObserverTest { + @get:Rule + val coroutinesTestRule = MainCoroutineRule() + @Test fun `WHEN the observer is created THEN start observing the store`() { val appStore: AppStore = mockk(relaxed = true) { every { observeManually(any()) } answers { mockk(relaxed = true) } } - val observer = WallpapersObserver(appStore, mockk(), mockk()) + val observer = getObserver(appStore) assertNotNull(observer.observeWallpapersStoreSubscription) } @Test - fun `WHEN asked to apply the wallpaper THEN show it`() { + fun `GIVEN a certain wallpaper is chosen WHEN the state is updated with that wallpaper THEN load it it`() = runTestOnMain { val appStore = AppStore() - val observer = spyk(WallpapersObserver(appStore, mockk(), mockk())) { - every { showWallpaper(any()) } just Runs - } - - observer.applyCurrentWallpaper() - - verify { observer.showWallpaper(any()) } - } - - @Test - fun `GIVEN the store was observed for updates WHEN the lifecycle owner is destroyed THEN stop observing the store`() { - val observer = WallpapersObserver(mockk(relaxed = true), mockk(), mockk()) - observer.observeWallpapersStoreSubscription = mockk(relaxed = true) - observer.wallpapersScope = mockk { - every { cancel() } just Runs - } + val settings: Settings = mockk { every { currentWallpaperName } returns "Test" } + val wallpaper: Wallpaper = mockk { every { name } returns "Test" } + val observer = spyk( + getObserver(appStore, settings, mockk(relaxed = true), mockk(relaxed = true)), + ) - observer.onDestroy(mockk()) + // Ignore the call on the real instance and call again "observeWallpaperUpdates" + // on the spy to be able to verify the "showWallpaper" call in the spy. + observer.observeWallpaperUpdates() + appStore.dispatch(UpdateCurrentWallpaper(wallpaper)).joinBlocking() + appStore.waitUntilIdle() - verify { observer.wallpapersScope.cancel() } - verify { observer.observeWallpapersStoreSubscription!!.unsubscribe() } + coVerify { observer.loadWallpaper(any()) } + coVerify { observer.applyCurrentWallpaper() } } @Test - fun `WHEN the wallpaper is updated THEN show the wallpaper`() { + fun `GIVEN a certain wallpaper is chosen WHEN the state updated with another wallpaper THEN avoid loading it`() = runTestOnMain { val appStore = AppStore() - val observer = spyk(WallpapersObserver(appStore, mockk(relaxed = true), mockk(relaxed = true))) { - every { showWallpaper(any()) } just Runs - } + val settings: Settings = mockk { every { currentWallpaperName } returns "Test" } + val otherWallpaper: Wallpaper = mockk { every { name } returns "Other" } + val observer = spyk( + getObserver(appStore, settings, mockk(relaxed = true), mockk(relaxed = true)), + ) // Ignore the call on the real instance and call again "observeWallpaperUpdates" // on the spy to be able to verify the "showWallpaper" call in the spy. observer.observeWallpaperUpdates() - - val newWallpaper: Wallpaper = mockk(relaxed = true) - appStore.dispatch(UpdateCurrentWallpaper(newWallpaper)) + appStore.dispatch(UpdateCurrentWallpaper(otherWallpaper)).joinBlocking() appStore.waitUntilIdle() - verify { observer.showWallpaper(newWallpaper) } + + coVerify(exactly = 0) { observer.loadWallpaper(any()) } + coVerify(exactly = 0) { observer.applyCurrentWallpaper() } } @Test - @Ignore("Intermittent test: https://github.com/mozilla-mobile/fenix/issues/26760") - fun `WHEN the wallpaper is updated to a new one THEN show the wallpaper`() { + fun `GIVEN a wallpaper is SHOWN WHEN the wallpaper is updated to the current one THEN don't try showing the same wallpaper again`() { val appStore = AppStore() - val wallpapersUseCases: WallpapersUseCases = mockk { - coEvery { loadBitmap(any()) } returns null - } - val observer = spyk(WallpapersObserver(appStore, wallpapersUseCases, mockk(relaxed = true))) { - every { showWallpaper(any()) } just Runs + val settings: Settings = mockk { every { currentWallpaperName } returns "Test" } + val wallpaper: Wallpaper = mockk { every { name } returns "Test" } + val wallpapersUseCases: WallpapersUseCases = mockk { coEvery { loadBitmap(any()) } returns null } + val observer = spyk(getObserver(appStore, settings, wallpapersUseCases, mockk(relaxed = true))) { + coEvery { loadWallpaper(any()) } just Runs + coEvery { applyCurrentWallpaper() } just Runs } // Ignore the call on the real instance and call again "observeWallpaperUpdates" // on the spy to be able to verify the "showWallpaper" call in the spy. observer.observeWallpaperUpdates() - verify { observer.showWallpaper(Wallpaper.Default) } + appStore.dispatch(UpdateCurrentWallpaper(wallpaper)).joinBlocking() + appStore.waitUntilIdle() + coVerify(exactly = 1) { observer.loadWallpaper(any()) } + coVerify(exactly = 1) { observer.applyCurrentWallpaper() } - val wallpaper: Wallpaper = mockk(relaxed = true) - appStore.dispatch(UpdateCurrentWallpaper(wallpaper)) + appStore.dispatch(UpdateCurrentWallpaper(wallpaper)).joinBlocking() appStore.waitUntilIdle() - verify { observer.showWallpaper(wallpaper) } + coVerify(exactly = 1) { observer.loadWallpaper(any()) } + coVerify(exactly = 1) { observer.applyCurrentWallpaper() } } @Test - fun `WHEN the wallpaper is updated to the current one THEN don't try showing the same wallpaper again`() { - val appStore = AppStore() - val wallpapersUseCases: WallpapersUseCases = mockk { - coEvery { loadBitmap(any()) } returns null - } - val observer = spyk(WallpapersObserver(appStore, wallpapersUseCases, mockk(relaxed = true))) { - every { showWallpaper(any()) } just Runs + fun `GIVEN the store was observed for updates WHEN the lifecycle owner is destroyed THEN stop observing the store`() { + val observer = getObserver(mockk(relaxed = true)) + observer.observeWallpapersStoreSubscription = mockk(relaxed = true) + observer.wallpapersScope = mockk { + every { cancel() } just Runs } - // Ignore the call on the real instance and call again "observeWallpaperUpdates" - // on the spy to be able to verify the "showWallpaper" call in the spy. - observer.observeWallpaperUpdates() - val wallpaper: Wallpaper = mockk(relaxed = true) - appStore.dispatch(UpdateCurrentWallpaper(wallpaper)) - appStore.waitUntilIdle() - verify { observer.showWallpaper(wallpaper) } + observer.onDestroy(mockk()) - appStore.dispatch(UpdateCurrentWallpaper(wallpaper)) - appStore.waitUntilIdle() - verify(exactly = 1) { observer.showWallpaper(wallpaper) } + verify { observer.wallpapersScope.cancel() } + verify { observer.observeWallpapersStoreSubscription!!.unsubscribe() } } @Test - fun `GIVEN no wallpaper is provided WHEN asked to show the wallpaper THEN show the current one`() { - val wallpaper: Wallpaper = mockk() - val appStore: AppStore = mockk(relaxed = true) { - every { state.wallpaperState.currentWallpaper } returns wallpaper - } - val observer = spyk(WallpapersObserver(appStore, mockk(relaxed = true), mockk(relaxed = true))) + fun `GIVEN a wallpaper image is available WHEN asked to apply the current wallpaper THEN show set it to the wallpaper ImageView`() = runTestOnMain { + val imageView: ImageView = mockk(relaxed = true) + val observer = getObserver(wallpaperImageView = imageView) + val image: Bitmap = mockk() + observer.currentWallpaperImage = image + observer.isWallpaperLoaded.complete(Unit) - observer.showWallpaper() + observer.applyCurrentWallpaper() - verify { observer.showWallpaper(wallpaper) } + verify { imageView.setImageBitmap(image) } + verify { imageView.isVisible = true } } - fun `GiVEN the current wallpaper is the default one WHEN showing it THEN hide the wallpaper view`() { - val wallpapersUseCases: WallpapersUseCases = mockk() - val wallpaperView: ImageView = mockk(relaxed = true) - val observer = WallpapersObserver(mockk(relaxed = true), wallpapersUseCases, wallpaperView) + fun `GIVEN no wallpaper image is available WHEN asked to apply the current wallpaper THEN hide the wallpaper ImageView`() = runTestOnMain { + val imageView: ImageView = mockk() + val observer = getObserver(wallpaperImageView = imageView) + observer.isWallpaperLoaded.complete(Unit) - observer.showWallpaper(Wallpaper.Default) + observer.applyCurrentWallpaper() - verify { wallpaperView.isVisible = false } - verify { wallpapersUseCases wasNot Called } + verify { imageView.isVisible = false } + verify(exactly = 0) { imageView.setImageBitmap(any()) } } @Test - fun `GiVEN the current wallpaper is different than the default one WHEN showing it THEN load it's bitmap in the visible wallpaper view`() { - val wallpaper: Wallpaper = mockk() - val bitmap: Bitmap = mockk() - val wallpapersUseCases: WallpapersUseCases = mockk { - coEvery { loadBitmap(any()) } returns bitmap - } - val wallpaperView: ImageView = mockk(relaxed = true) - val observer = WallpapersObserver(mockk(relaxed = true), wallpapersUseCases, wallpaperView) + fun `GIVEN the default wallpaper WHEN asked to load it THEN cache that the current image is null`() = runTestOnMain { + val observer = getObserver() + observer.currentWallpaperImage = mockk() - observer.showWallpaper(wallpaper) + observer.loadWallpaper(Wallpaper.Default) - verify { wallpaperView.isVisible = true } - verify { wallpaperView.setImageBitmap(bitmap) } + assertNull(observer.currentWallpaperImage) } @Test - fun `GIVEN the observer THEN use the main thread for showing the wallpaper`() { - val wallpapersUseCases: WallpapersUseCases = mockk() - val wallpaperView: ImageView = mockk(relaxed = true) + fun `GIVEN a custom wallpaper WHEN asked to load it THEN cache it's bitmap`() = runTestOnMain { + val bitmap: Bitmap = mockk() + val wallpaper: Wallpaper = mockk() + val usecases: WallpapersUseCases = mockk { + coEvery { loadBitmap(wallpaper) } returns bitmap + } + val observer = getObserver(wallpapersUseCases = usecases) - val observer = WallpapersObserver(mockk(relaxed = true), wallpapersUseCases, wallpaperView) + observer.loadWallpaper(wallpaper) - // Check that the context that would be used is Dispatchers.Main.immediate - // Unfortunately I could not also test that this is actually used when "showWallpaper" is called. - assertTrue(observer.wallpapersScope.toString().contains("Dispatchers.Main.immediate")) + assertEquals(bitmap, observer.currentWallpaperImage) } + + private fun getObserver( + appStore: AppStore = mockk(relaxed = true), + settings: Settings = mockk(), + wallpapersUseCases: WallpapersUseCases = mockk(), + wallpaperImageView: ImageView = mockk(), + backgroundWorkDispatcher: CoroutineDispatcher = coroutinesTestRule.testDispatcher, + ) = WallpapersObserver( + appStore = appStore, + settings = settings, + wallpapersUseCases = wallpapersUseCases, + wallpaperImageView = wallpaperImageView, + backgroundWorkDispatcher = backgroundWorkDispatcher, + ) }