Skip to content

Commit

Permalink
Closes mozilla-mobile#21871 - Eagerly update UI state after search gr…
Browse files Browse the repository at this point in the history
…oup 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"
  • Loading branch information
Grisha Kruglov authored and grigoryk committed Oct 13, 2021
1 parent 135f8a3 commit a9e598b
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
4 changes: 3 additions & 1 deletion app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions app/src/main/java/org/mozilla/fenix/home/HomeFragmentStore.kt
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ sealed class HomeFragmentAction : Action {
data class RecentTabsChange(val recentTabs: List<RecentTab>) : HomeFragmentAction()
data class RecentBookmarksChange(val recentBookmarks: List<BookmarkNode>) : HomeFragmentAction()
data class HistoryMetadataChange(val historyMetadata: List<HistoryMetadataGroup>) : 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<PocketRecommendedStory>) : HomeFragmentAction()
Expand Down Expand Up @@ -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 =
Expand Down
12 changes: 12 additions & 0 deletions app/src/test/java/org/mozilla/fenix/home/HomeFragmentStoreTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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<HistoryMetadataGroup> = 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)
Expand Down

0 comments on commit a9e598b

Please sign in to comment.