From a9e598b6b6c23b5fc5cd981fddf132b89fd0f209 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Wed, 13 Oct 2021 01:54:30 -0700 Subject: [PATCH] Closes #21871 - Eagerly update UI state after search group removal Before this patch, this was the behavior - 'remove' button is clicked, we'd ask the storage to remove metadata (on its IO thread), then navigate to Home Screen. This resulted in a race we could end-up on the Home Screen before delete finishes, so the search groups do not appear to be removed (but, refreshing the Home Screen again shows that they are removed). This also resulted in an unnecessary navigation which felt very janky (screen will "scroll" to the top) and was way more work than necessary. After this patch, we: - dispatch two actions (on browserstore, on homefragmentstore) which remove the search groups from any relevant in-memory state; any UI bound to this state will be automatically "refreshed" - no longer navigate as part of the remove action, so the UI doesn't move and removal happens "in-place" --- .../controller/HistoryMetadataController.kt | 16 ++++++++++++---- .../java/org/mozilla/fenix/home/HomeFragment.kt | 4 +++- .../org/mozilla/fenix/home/HomeFragmentStore.kt | 4 ++++ .../mozilla/fenix/home/HomeFragmentStoreTest.kt | 12 ++++++++++++ 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/historymetadata/controller/HistoryMetadataController.kt b/app/src/main/java/org/mozilla/fenix/historymetadata/controller/HistoryMetadataController.kt index af8cd6f031..704db99aad 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/controller/HistoryMetadataController.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/controller/HistoryMetadataController.kt @@ -9,12 +9,15 @@ import androidx.annotation.VisibleForTesting.PRIVATE import androidx.navigation.NavController import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch +import mozilla.components.browser.state.action.HistoryMetadataAction +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.storage.HistoryMetadataStorage import org.mozilla.fenix.R -import org.mozilla.fenix.browser.BrowserFragmentDirections import org.mozilla.fenix.historymetadata.HistoryMetadataGroup import org.mozilla.fenix.historymetadata.interactor.HistoryMetadataInteractor +import org.mozilla.fenix.home.HomeFragmentAction import org.mozilla.fenix.home.HomeFragmentDirections +import org.mozilla.fenix.home.HomeFragmentStore import org.mozilla.fenix.library.history.toHistoryMetadata /** @@ -42,6 +45,8 @@ interface HistoryMetadataController { * The default implementation of [HistoryMetadataController]. */ class DefaultHistoryMetadataController( + private val store: BrowserStore, + private val homeStore: HomeFragmentStore, private val navController: NavController, private val storage: HistoryMetadataStorage, private val scope: CoroutineScope @@ -65,12 +70,15 @@ class DefaultHistoryMetadataController( } override fun handleRemoveGroup(searchTerm: String) { + // We want to update the UI right away in response to user action without waiting for the IO. + // First, dispatch actions that will clean up search groups in the two stores that have + // metadata-related state. + store.dispatch(HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = searchTerm)) + homeStore.dispatch(HomeFragmentAction.DisbandSearchGroupAction(searchTerm = searchTerm)) + // Then, perform the expensive IO work of removing search groups from storage. scope.launch { storage.deleteHistoryMetadata(searchTerm) } - navController.navigate( - BrowserFragmentDirections.actionGlobalHome() - ) } @VisibleForTesting(otherwise = PRIVATE) 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 c2998bc21e..3bea7b450b 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -349,8 +349,10 @@ class HomeFragment : Fragment() { ), historyMetadataController = DefaultHistoryMetadataController( navController = findNavController(), + homeStore = homeFragmentStore, storage = components.core.historyStorage, - scope = viewLifecycleOwner.lifecycleScope + scope = viewLifecycleOwner.lifecycleScope, + store = components.core.store ), pocketStoriesController = DefaultPocketStoriesController( homeActivity = activity, diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt index dd18e0d121..61e597ac04 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt @@ -97,6 +97,7 @@ sealed class HomeFragmentAction : Action { data class RecentTabsChange(val recentTabs: List) : HomeFragmentAction() data class RecentBookmarksChange(val recentBookmarks: List) : HomeFragmentAction() data class HistoryMetadataChange(val historyMetadata: List) : HomeFragmentAction() + data class DisbandSearchGroupAction(val searchTerm: String) : HomeFragmentAction() data class SelectPocketStoriesCategory(val categoryName: String) : HomeFragmentAction() data class DeselectPocketStoriesCategory(val categoryName: String) : HomeFragmentAction() data class PocketStoriesShown(val storiesShown: List) : HomeFragmentAction() @@ -150,6 +151,9 @@ private fun homeFragmentStateReducer( is HomeFragmentAction.RecentTabsChange -> state.copy(recentTabs = action.recentTabs) is HomeFragmentAction.RecentBookmarksChange -> state.copy(recentBookmarks = action.recentBookmarks) is HomeFragmentAction.HistoryMetadataChange -> state.copy(historyMetadata = action.historyMetadata) + is HomeFragmentAction.DisbandSearchGroupAction -> state.copy( + historyMetadata = state.historyMetadata.filter { it.title.lowercase() != action.searchTerm.lowercase() } + ) is HomeFragmentAction.SelectPocketStoriesCategory -> { val updatedCategoriesState = state.copy( pocketStoriesCategoriesSelections = diff --git a/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt index f5bb4db992..2d4c026514 100644 --- a/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt @@ -128,6 +128,18 @@ class HomeFragmentStoreTest { assertEquals(historyMetadata, homeFragmentStore.state.historyMetadata) } + @Test + fun `Test disbanding search group in HomeFragmentStore`() = runBlocking { + val g1 = HistoryMetadataGroup(title = "test One") + val g2 = HistoryMetadataGroup(title = "test two") + val historyMetadata: List = listOf(g1, g2) + homeFragmentStore.dispatch(HomeFragmentAction.HistoryMetadataChange(historyMetadata)).join() + assertEquals(historyMetadata, homeFragmentStore.state.historyMetadata) + + homeFragmentStore.dispatch(HomeFragmentAction.DisbandSearchGroupAction("Test one")).join() + assertEquals(listOf(g2), homeFragmentStore.state.historyMetadata) + } + @Test fun `Test changing hiding collections placeholder`() = runBlocking { assertTrue(homeFragmentStore.state.showCollectionPlaceholder)