Bug 1924196 - Fix delete item on HistoryFragment when device theme has changed r=android-reviewers,rsainani

Differential Revision: https://phabricator.services.mozilla.com/D228824

mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/history/HistoryAdapter.kt

@@ -18,6 +18,7 @@ class HistoryAdapter(
     private val store: HistoryFragmentStore,
     private val onRecentlyClosedClicked: () -> Unit,
     private val onHistoryItemClicked: (History) -> Unit,
+    private val onDeleteInitiated: (Set<History>) -> Unit,
     private val onEmptyStateChanged: (Boolean) -> Unit,
 ) : PagingDataAdapter<History, HistoryListItemViewHolder>(historyDiffCallback),
     SelectionHolder<History> {
@@ -46,6 +47,7 @@ class HistoryAdapter(
             store = store,
             onHistoryItemClicked = onHistoryItemClicked,
             onRecentlyClosedClicked = onRecentlyClosedClicked,
+            onDeleteInitiated = onDeleteInitiated,
         )
     }
 

mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt

@@ -27,11 +27,12 @@ import androidx.navigation.fragment.findNavController
 import androidx.paging.Pager
 import androidx.paging.PagingConfig
 import androidx.paging.PagingData
-import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers.IO
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.mapNotNull
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
+import mozilla.components.browser.state.action.HistoryMetadataAction
 import mozilla.components.concept.engine.prompt.ShareData
 import mozilla.components.lib.state.ext.consumeFrom
 import mozilla.components.lib.state.ext.flowScoped
@@ -48,6 +49,7 @@ import org.mozilla.fenix.R
 import org.mozilla.fenix.addons.showSnackBar
 import org.mozilla.fenix.browser.browsingmode.BrowsingMode
 import org.mozilla.fenix.components.StoreProvider
+import org.mozilla.fenix.components.appstate.AppAction
 import org.mozilla.fenix.components.history.DefaultPagedHistoryProvider
 import org.mozilla.fenix.databinding.FragmentHistoryBinding
 import org.mozilla.fenix.ext.components
@@ -123,18 +125,15 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler,
                         scope = lifecycleScope,
                     ),
                     HistoryStorageMiddleware(
-                        appStore = requireContext().components.appStore,
                         browserStore = requireContext().components.core.store,
-                        historyProvider = historyProvider,
                         historyStorage = requireContext().components.core.historyStorage,
-                        undoDeleteSnackbar = ::showDeleteSnackbar,
                         onTimeFrameDeleted = ::onTimeFrameDeleted,
                     ),
                 ),
             )
         }
         _historyView = HistoryView(
-            binding.historyLayout,
+            container = binding.historyLayout,
             onZeroItemsLoaded = {
                 historyStore.dispatch(
                     HistoryFragmentAction.ChangeEmptyState(isEmpty = true),
@@ -148,6 +147,7 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler,
             },
             onRecentlyClosedClicked = ::navigateToRecentlyClosed,
             onHistoryItemClicked = ::openItem,
+            onDeleteInitiated = ::onDeleteInitiated,
         )
 
         return view
@@ -175,14 +175,12 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler,
 
     private fun showDeleteSnackbar(
         items: Set<History>,
-        undo: suspend (Set<History>) -> Unit,
-        delete: suspend (Set<History>) -> Unit,
     ) {
-        CoroutineScope(IO).allowUndo(
+        lifecycleScope.allowUndo(
             view = requireActivity().getRootView()!!,
             message = getMultiSelectSnackBarMessage(items),
             undoActionTitle = getString(R.string.snackbar_deleted_undo),
-            onCancel = { undo.invoke(items) },
+            onCancel = { undo(items) },
             operation = { delete(items) },
         )
     }
@@ -294,6 +292,7 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler,
         R.id.delete_history_multi_select -> {
             with(historyStore) {
                 dispatch(HistoryFragmentAction.DeleteItems(state.mode.selectedItems))
+                onDeleteInitiated(state.mode.selectedItems)
                 dispatch(HistoryFragmentAction.ExitEditMode)
             }
             true
@@ -413,6 +412,13 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler,
         )
     }
 
+    private fun onDeleteInitiated(items: Set<History>) {
+        val appStore = requireContext().components.appStore
+
+        appStore.dispatch(AppAction.AddPendingDeletionSet(items.toPendingDeletionHistory()))
+        showDeleteSnackbar(items)
+    }
+
     private fun share(data: List<ShareData>) {
         GleanHistory.shared.record(NoExtras())
         val directions = HistoryFragmentDirections.actionGlobalShareFragment(
@@ -435,6 +441,34 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler,
         )
     }
 
+    private suspend fun undo(items: Set<History>) = withContext(IO) {
+        val appStore = requireContext().components.appStore
+        val pendingDeletionItems = items.map { it.toPendingDeletionHistory() }.toSet()
+        appStore.dispatch(AppAction.UndoPendingDeletionSet(pendingDeletionItems))
+    }
+
+    private suspend fun delete(items: Set<History>) = withContext(IO) {
+        val browserStore = requireContext().components.core.store
+        val historyStorage = requireContext().components.core.historyStorage
+
+        historyStore.dispatch(HistoryFragmentAction.EnterDeletionMode)
+        for (item in items) {
+            when (item) {
+                is History.Regular -> historyStorage.deleteVisitsFor(item.url)
+                is History.Group -> {
+                    // NB: If we have non-search groups, this logic needs to be updated.
+                    historyProvider.deleteMetadataSearchGroup(item)
+                    browserStore.dispatch(
+                        HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = item.title),
+                    )
+                }
+                // We won't encounter individual metadata entries outside of groups.
+                is History.Metadata -> {}
+            }
+        }
+        historyStore.dispatch(HistoryFragmentAction.ExitDeletionMode)
+    }
+
     @Suppress("UnusedPrivateMember")
     private suspend fun syncHistory() {
         val accountManager = requireComponents.backgroundServices.accountManager

mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/history/HistoryView.kt

@@ -21,12 +21,14 @@ import org.mozilla.fenix.theme.ThemeManager
 /**
  * View that contains and configures the History List
  */
+@Suppress("LongParameterList")
 class HistoryView(
     container: ViewGroup,
     val store: HistoryFragmentStore,
     val onZeroItemsLoaded: () -> Unit,
     val onRecentlyClosedClicked: () -> Unit,
     val onHistoryItemClicked: (History) -> Unit,
+    val onDeleteInitiated: (Set<History>) -> Unit,
     val onEmptyStateChanged: (Boolean) -> Unit,
 ) : LibraryPageView(container) {
 
@@ -43,6 +45,7 @@ class HistoryView(
         store = store,
         onHistoryItemClicked = onHistoryItemClicked,
         onRecentlyClosedClicked = onRecentlyClosedClicked,
+        onDeleteInitiated = onDeleteInitiated,
     ) { isEmpty ->
         onEmptyStateChanged(isEmpty)
     }.apply {

mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/history/state/HistoryStorageMiddleware.kt

@@ -8,45 +8,27 @@ import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import mozilla.components.browser.state.action.EngineAction
-import mozilla.components.browser.state.action.HistoryMetadataAction
 import mozilla.components.browser.state.action.RecentlyClosedAction
 import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.browser.storage.sync.PlacesHistoryStorage
 import mozilla.components.lib.state.Middleware
 import mozilla.components.lib.state.MiddlewareContext
-import mozilla.components.lib.state.Store
-import org.mozilla.fenix.components.AppStore
-import org.mozilla.fenix.components.appstate.AppAction
-import org.mozilla.fenix.components.history.DefaultPagedHistoryProvider
-import org.mozilla.fenix.library.history.History
 import org.mozilla.fenix.library.history.HistoryFragmentAction
 import org.mozilla.fenix.library.history.HistoryFragmentState
 import org.mozilla.fenix.library.history.HistoryFragmentStore
-import org.mozilla.fenix.library.history.toPendingDeletionHistory
 
 /**
  * A [Middleware] for initiating storage side-effects based on [HistoryFragmentAction]s that are
  * dispatched to the [HistoryFragmentStore].
  *
- * @param appStore To dispatch Actions to update global state.
  * @param browserStore To dispatch Actions to update global state.
- * @param historyProvider To update storage as a result of some Actions.
  * @param historyStorage To update storage as a result of some Actions.
- * @param undoDeleteSnackbar Called when items are deleted to offer opportunity of undoing before
- *  deletion is fully completed.
  * @param onTimeFrameDeleted Called when a time range of items is successfully deleted.
  * @param scope CoroutineScope to launch storage operations into.
  */
 class HistoryStorageMiddleware(
-    private val appStore: AppStore,
     private val browserStore: BrowserStore,
-    private val historyProvider: DefaultPagedHistoryProvider,
     private val historyStorage: PlacesHistoryStorage,
-    private val undoDeleteSnackbar: (
-        items: Set<History>,
-        undo: suspend (Set<History>) -> Unit,
-        delete: suspend (Set<History>) -> Unit,
-    ) -> Unit,
     private val onTimeFrameDeleted: () -> Unit,
     private val scope: CoroutineScope = CoroutineScope(Dispatchers.IO),
 ) : Middleware<HistoryFragmentState, HistoryFragmentAction> {
@@ -57,10 +39,6 @@ class HistoryStorageMiddleware(
     ) {
         next(action)
         when (action) {
-            is HistoryFragmentAction.DeleteItems -> {
-                appStore.dispatch(AppAction.AddPendingDeletionSet(action.items.toPendingDeletionHistory()))
-                undoDeleteSnackbar(action.items, ::undo, delete(context.store))
-            }
             is HistoryFragmentAction.DeleteTimeRange -> {
                 context.store.dispatch(HistoryFragmentAction.EnterDeletionMode)
                 scope.launch {
@@ -85,32 +63,4 @@ class HistoryStorageMiddleware(
             else -> Unit
         }
     }
-
-    private fun undo(items: Set<History>) {
-        val pendingDeletionItems = items.map { it.toPendingDeletionHistory() }.toSet()
-        appStore.dispatch(AppAction.UndoPendingDeletionSet(pendingDeletionItems))
-    }
-
-    private fun delete(store: Store<HistoryFragmentState, HistoryFragmentAction>): suspend (Set<History>) -> Unit {
-        return { items ->
-            scope.launch {
-                store.dispatch(HistoryFragmentAction.EnterDeletionMode)
-                for (item in items) {
-                    when (item) {
-                        is History.Regular -> historyStorage.deleteVisitsFor(item.url)
-                        is History.Group -> {
-                            // NB: If we have non-search groups, this logic needs to be updated.
-                            historyProvider.deleteMetadataSearchGroup(item)
-                            browserStore.dispatch(
-                                HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = item.title),
-                            )
-                        }
-                        // We won't encounter individual metadata entries outside of groups.
-                        is History.Metadata -> {}
-                    }
-                }
-                store.dispatch(HistoryFragmentAction.ExitDeletionMode)
-            }
-        }
-    }
 }

mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/history/viewholders/HistoryListItemViewHolder.kt

@@ -25,6 +25,7 @@ class HistoryListItemViewHolder(
     private val store: HistoryFragmentStore,
     private val onHistoryItemClicked: (History) -> Unit,
     private val onRecentlyClosedClicked: () -> Unit,
+    private val onDeleteInitiated: (Set<History>) -> Unit,
 ) : RecyclerView.ViewHolder(view) {
 
     private var item: History? = null
@@ -41,6 +42,7 @@ class HistoryListItemViewHolder(
             setOnClickListener {
                 val item = item ?: return@setOnClickListener
                 store.dispatch(HistoryFragmentAction.DeleteItems(setOf(item)))
+                onDeleteInitiated(setOf(item))
             }
         }
     }

mobile/android/fenix/app/src/test/java/org/mozilla/fenix/library/history/state/HistoryStorageMiddlewareTest.kt

@@ -4,11 +4,9 @@
 
 package org.mozilla.fenix.library.history.state
 
-import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.test.advanceUntilIdle
 import mozilla.components.browser.state.action.BrowserAction
 import mozilla.components.browser.state.action.EngineAction
-import mozilla.components.browser.state.action.HistoryMetadataAction
 import mozilla.components.browser.state.action.RecentlyClosedAction
 import mozilla.components.browser.state.state.BrowserState
 import mozilla.components.browser.state.store.BrowserStore
@@ -28,25 +26,17 @@ import org.junit.Rule
 import org.junit.Test
 import org.mockito.Mockito.anyLong
 import org.mockito.Mockito.verify
-import org.mozilla.fenix.components.AppStore
-import org.mozilla.fenix.components.appstate.AppAction
-import org.mozilla.fenix.components.history.DefaultPagedHistoryProvider
-import org.mozilla.fenix.library.history.History
 import org.mozilla.fenix.library.history.HistoryFragmentAction
 import org.mozilla.fenix.library.history.HistoryFragmentState
 import org.mozilla.fenix.library.history.HistoryFragmentStore
-import org.mozilla.fenix.library.history.HistoryItemTimeGroup
 import org.mozilla.fenix.library.history.RemoveTimeFrame
-import org.mozilla.fenix.library.history.toPendingDeletionHistory
 
 class HistoryStorageMiddlewareTest {
     @get:Rule
     val coroutinesTestRule = MainCoroutineRule()
 
-    private val appStore = mock<AppStore>()
     private lateinit var captureBrowserActions: CaptureActionsMiddleware<BrowserState, BrowserAction>
     private lateinit var browserStore: BrowserStore
-    private val provider = mock<DefaultPagedHistoryProvider>()
     private lateinit var storage: PlacesHistoryStorage
 
     @Before
@@ -56,127 +46,12 @@ class HistoryStorageMiddlewareTest {
         browserStore = BrowserStore(middleware = listOf(captureBrowserActions))
     }
 
-    @Test
-    fun `WHEN items are deleted THEN action to add to pending deletion is dispatched and snackbar is shown`() = runTestOnMain {
-        var snackbarCalled = false
-        val history = setOf(History.Regular(0, "title", "url", 0, HistoryItemTimeGroup.timeGroupForTimestamp(0)))
-        val middleware = HistoryStorageMiddleware(
-            appStore = appStore,
-            browserStore = browserStore,
-            historyProvider = provider,
-            historyStorage = storage,
-            undoDeleteSnackbar = { _, _, _ -> snackbarCalled = true },
-            onTimeFrameDeleted = { },
-            scope = this,
-        )
-        val store = HistoryFragmentStore(
-            initialState = HistoryFragmentState.initial,
-            middleware = listOf(middleware),
-        )
-
-        store.dispatch(HistoryFragmentAction.DeleteItems(history)).joinBlocking()
-        store.waitUntilIdle()
-
-        assertTrue(snackbarCalled)
-        verify(appStore).dispatch(AppAction.AddPendingDeletionSet(history.toPendingDeletionHistory()))
-    }
-
-    @Test
-    fun `WHEN items are deleted THEN undo dispatches action to remove them from pending deletion state`() = runTestOnMain {
-        var snackbarCalled = false
-        val history = setOf(History.Regular(0, "title", "url", 0, HistoryItemTimeGroup.timeGroupForTimestamp(0)))
-        val middleware = HistoryStorageMiddleware(
-            appStore = appStore,
-            browserStore = browserStore,
-            historyProvider = provider,
-            historyStorage = storage,
-            undoDeleteSnackbar = { _, undo, _ ->
-                runBlocking { undo(history) }
-                snackbarCalled = true
-            },
-            onTimeFrameDeleted = { },
-            scope = this,
-        )
-        val store = HistoryFragmentStore(
-            initialState = HistoryFragmentState.initial,
-            middleware = listOf(middleware),
-        )
-
-        store.dispatch(HistoryFragmentAction.DeleteItems(history)).joinBlocking()
-        store.waitUntilIdle()
-
-        assertTrue(snackbarCalled)
-        verify(appStore).dispatch(AppAction.AddPendingDeletionSet(history.toPendingDeletionHistory()))
-        verify(appStore).dispatch(AppAction.UndoPendingDeletionSet(history.toPendingDeletionHistory()))
-    }
-
-    @Test
-    fun `WHEN regular items are deleted and not undone THEN items are removed from storage`() = runTestOnMain {
-        var snackbarCalled = false
-        val history = setOf(History.Regular(0, "title", "url", 0, HistoryItemTimeGroup.timeGroupForTimestamp(0)))
-        val middleware = HistoryStorageMiddleware(
-            appStore = appStore,
-            browserStore = browserStore,
-            historyProvider = provider,
-            historyStorage = storage,
-            undoDeleteSnackbar = { _, _, delete ->
-                runBlocking { delete(history) }
-                snackbarCalled = true
-            },
-            onTimeFrameDeleted = { },
-            scope = this,
-        )
-        val store = HistoryFragmentStore(
-            initialState = HistoryFragmentState.initial,
-            middleware = listOf(middleware),
-        )
-
-        store.dispatch(HistoryFragmentAction.DeleteItems(history)).joinBlocking()
-        store.waitUntilIdle()
-
-        assertTrue(snackbarCalled)
-        verify(storage).deleteVisitsFor(history.first().url)
-    }
-
-    @Test
-    fun `WHEN group items are deleted and not undone THEN items are removed from provider and browser store updated`() = runTestOnMain {
-        var snackbarCalled = false
-        val history = setOf(History.Group(0, "title", 0, HistoryItemTimeGroup.timeGroupForTimestamp(0), listOf()))
-        val middleware = HistoryStorageMiddleware(
-            appStore = appStore,
-            browserStore = browserStore,
-            historyProvider = provider,
-            historyStorage = storage,
-            undoDeleteSnackbar = { _, _, delete ->
-                runBlocking { delete(history) }
-                snackbarCalled = true
-            },
-            onTimeFrameDeleted = { },
-            scope = this,
-        )
-        val store = HistoryFragmentStore(
-            initialState = HistoryFragmentState.initial,
-            middleware = listOf(middleware),
-        )
-
-        store.dispatch(HistoryFragmentAction.DeleteItems(history)).joinBlocking()
-        store.waitUntilIdle()
-        browserStore.waitUntilIdle()
-
-        assertTrue(snackbarCalled)
-        captureBrowserActions.assertFirstAction(HistoryMetadataAction.DisbandSearchGroupAction::class)
-        verify(provider).deleteMetadataSearchGroup(history.first())
-    }
-
     @Test
     fun `WHEN a null time frame is deleted THEN browser store is informed, storage deletes everything, and callback is invoked`() = runTestOnMain {
         var callbackInvoked = false
         val middleware = HistoryStorageMiddleware(
-            appStore = appStore,
             browserStore = browserStore,
-            historyProvider = provider,
             historyStorage = storage,
-            undoDeleteSnackbar = mock(),
             onTimeFrameDeleted = { callbackInvoked = true },
             scope = this,
         )
@@ -201,11 +76,8 @@ class HistoryStorageMiddlewareTest {
     fun `WHEN a specified time frame is deleted THEN browser store is informed, storage deletes time frame, and callback is invoked`() = runTestOnMain {
         var callbackInvoked = false
         val middleware = HistoryStorageMiddleware(
-            appStore = appStore,
             browserStore = browserStore,
-            historyProvider = provider,
             historyStorage = storage,
-            undoDeleteSnackbar = mock(),
             onTimeFrameDeleted = { callbackInvoked = true },
             scope = this,
         )