diff --git a/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt b/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt index d4dfd5237..5da8c342d 100644 --- a/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt +++ b/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt @@ -16,7 +16,6 @@ import org.mozilla.fenix.library.history.History import org.mozilla.fenix.library.history.HistoryItemTimeGroup import org.mozilla.fenix.perf.runBlockingIncrement import org.mozilla.fenix.utils.Settings.Companion.SEARCH_GROUP_MINIMUM_SITES -import kotlin.math.abs private const val BUFFER_TIME = 15000 /* 15 seconds in ms */ @@ -155,37 +154,17 @@ class DefaultPagedHistoryProvider( * Removes [group] and any corresponding history visits. */ suspend fun deleteMetadataSearchGroup(group: History.Group) { + // The intention is to delete items from history for good. + // Corresponding metadata items would also be removed, + // because of ON DELETE CASCADE relation in DB schema. for (historyMetadata in group.items) { - getMatchingHistory(historyMetadata)?.let { - historyStorage.deleteVisit( - url = it.url, - timestamp = it.visitTime - ) - } + historyStorage.deleteVisitsFor(historyMetadata.url) } - historyStorage.deleteHistoryMetadata( - searchTerm = group.title - ) - // Force a re-fetch of the groups next time we go through #getHistory. historyGroups = null } - /** - * Returns the [History.Regular] corresponding to the given [History.Metadata] item. - */ - private suspend fun getMatchingHistory(historyMetadata: History.Metadata): VisitInfo? { - val history = historyStorage.getDetailedVisits( - start = historyMetadata.visitedAt - BUFFER_TIME, - end = historyMetadata.visitedAt + BUFFER_TIME, - excludeTypes = excludedVisitTypes - ) - return history - .filter { it.url == historyMetadata.url } - .minByOrNull { abs(historyMetadata.visitedAt - it.visitTime) } - } - @Suppress("MagicNumber") private suspend fun getHistoryAndSearchGroups( offset: Int, diff --git a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt index 48f05dd5e..478120132 100644 --- a/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt @@ -367,7 +367,7 @@ class HistoryFragment : LibraryPageFragment(), UserInteractionHandler { analytics.metrics.track(Event.HistoryItemRemoved) when (item) { - is History.Regular -> core.historyStorage.deleteVisit(item.url, item.visitedAt) + is History.Regular -> core.historyStorage.deleteVisitsFor(item.url) is History.Group -> { // NB: If we have non-search groups, this logic needs to be updated. historyProvider.deleteMetadataSearchGroup(item) diff --git a/app/src/main/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragment.kt b/app/src/main/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragment.kt index c22f1f575..e93af7479 100644 --- a/app/src/main/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragment.kt @@ -26,6 +26,7 @@ import org.mozilla.fenix.ext.nav import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.setTextColor import org.mozilla.fenix.ext.showToolbar +import org.mozilla.fenix.ext.components import org.mozilla.fenix.library.LibraryPageFragment import org.mozilla.fenix.library.history.History import org.mozilla.fenix.library.historymetadata.controller.DefaultHistoryMetadataGroupController @@ -72,7 +73,8 @@ class HistoryMetadataGroupFragment : interactor = DefaultHistoryMetadataGroupInteractor( controller = DefaultHistoryMetadataGroupController( - activity = activity as HomeActivity, + historyStorage = (activity as HomeActivity).components.core.historyStorage, + browserStore = (activity as HomeActivity).components.core.store, store = historyMetadataGroupStore, selectOrAddUseCase = requireComponents.useCases.tabsUseCases.selectOrAddTab, metrics = requireComponents.analytics.metrics, diff --git a/app/src/main/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupController.kt b/app/src/main/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupController.kt index 328efd499..f273a77ee 100644 --- a/app/src/main/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupController.kt +++ b/app/src/main/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupController.kt @@ -7,13 +7,14 @@ package org.mozilla.fenix.library.historymetadata.controller 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.browser.storage.sync.PlacesHistoryStorage import mozilla.components.concept.engine.prompt.ShareData import mozilla.components.feature.tabs.TabsUseCases -import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.R -import org.mozilla.fenix.ext.components import org.mozilla.fenix.library.history.History import org.mozilla.fenix.library.historymetadata.HistoryMetadataGroupFragmentAction import org.mozilla.fenix.library.historymetadata.HistoryMetadataGroupFragmentDirections @@ -75,7 +76,8 @@ interface HistoryMetadataGroupController { * The default implementation of [HistoryMetadataGroupController]. */ class DefaultHistoryMetadataGroupController( - private val activity: HomeActivity, + private val historyStorage: PlacesHistoryStorage, + private val browserStore: BrowserStore, private val store: HistoryMetadataGroupFragmentStore, private val selectOrAddUseCase: TabsUseCases.SelectOrAddUseCase, private val metrics: MetricController, @@ -117,18 +119,31 @@ class DefaultHistoryMetadataGroupController( override fun handleDelete(items: Set) { scope.launch { + val isDeletingLastItem = store.state.items.size == items.size items.forEach { store.dispatch(HistoryMetadataGroupFragmentAction.Delete(it)) - activity.components.core.historyStorage.deleteHistoryMetadata(it.historyMetadataKey) + historyStorage.deleteVisitsFor(it.url) metrics.track(Event.HistorySearchTermGroupRemoveTab) } + // The method is called for both single and multiple items. + // In case all items have been deleted, we have to disband the search group. + if (isDeletingLastItem) { + browserStore.dispatch( + HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = searchTerm) + ) + } } } override fun handleDeleteAll() { scope.launch { store.dispatch(HistoryMetadataGroupFragmentAction.DeleteAll) - activity.components.core.historyStorage.deleteHistoryMetadata(searchTerm) + store.state.items.forEach { + historyStorage.deleteVisitsFor(it.url) + } + browserStore.dispatch( + HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = searchTerm) + ) metrics.track(Event.HistorySearchTermGroupRemoveAll) } } diff --git a/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt b/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt index 28f2fd875..705e0855b 100644 --- a/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt @@ -11,6 +11,7 @@ import io.mockk.mockk import io.mockk.verify import kotlinx.coroutines.test.TestCoroutineScope import kotlinx.coroutines.test.runBlockingTest +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.browser.storage.sync.PlacesHistoryStorage import mozilla.components.concept.engine.prompt.ShareData import mozilla.components.concept.storage.HistoryMetadataKey @@ -46,6 +47,7 @@ class HistoryMetadataGroupControllerTest { private val activity: HomeActivity = mockk(relaxed = true) private val store: HistoryMetadataGroupFragmentStore = mockk(relaxed = true) + private val browserStore: BrowserStore = mockk(relaxed = true) private val selectOrAddUseCase: TabsUseCases.SelectOrAddUseCase = mockk(relaxed = true) private val metrics: MetricController = mockk(relaxed = true) private val navController: NavController = mockk(relaxed = true) @@ -74,10 +76,13 @@ class HistoryMetadataGroupControllerTest { private lateinit var controller: DefaultHistoryMetadataGroupController + private fun getMetadataItemsList() = listOf(mozillaHistoryMetadataItem, firefoxHistoryMetadataItem) + @Before fun setUp() { controller = DefaultHistoryMetadataGroupController( - activity = activity, + historyStorage = historyStorage, + browserStore = browserStore, store = store, selectOrAddUseCase = selectOrAddUseCase, metrics = metrics, @@ -87,6 +92,7 @@ class HistoryMetadataGroupControllerTest { ) every { activity.components.core.historyStorage } returns historyStorage + every { store.state.items } returns getMetadataItemsList() } @After @@ -99,7 +105,10 @@ class HistoryMetadataGroupControllerTest { controller.handleOpen(mozillaHistoryMetadataItem) verify { - selectOrAddUseCase.invoke(mozillaHistoryMetadataItem.url, mozillaHistoryMetadataItem.historyMetadataKey) + selectOrAddUseCase.invoke( + mozillaHistoryMetadataItem.url, + mozillaHistoryMetadataItem.historyMetadataKey + ) navController.navigate(R.id.browserFragment) metrics.track(Event.HistorySearchTermGroupOpenTab) } @@ -158,13 +167,13 @@ class HistoryMetadataGroupControllerTest { @Test fun handleDelete() = testDispatcher.runBlockingTest { - controller.handleDelete(setOf(mozillaHistoryMetadataItem, firefoxHistoryMetadataItem)) + controller.handleDelete(getMetadataItemsList().toSet()) coVerify { - store.dispatch(HistoryMetadataGroupFragmentAction.Delete(mozillaHistoryMetadataItem)) - store.dispatch(HistoryMetadataGroupFragmentAction.Delete(firefoxHistoryMetadataItem)) - historyStorage.deleteHistoryMetadata(mozillaHistoryMetadataItem.historyMetadataKey) - historyStorage.deleteHistoryMetadata(firefoxHistoryMetadataItem.historyMetadataKey) + getMetadataItemsList().forEach { + store.dispatch(HistoryMetadataGroupFragmentAction.Delete(it)) + historyStorage.deleteVisitsFor(it.url) + } metrics.track(Event.HistorySearchTermGroupRemoveTab) } } @@ -175,7 +184,9 @@ class HistoryMetadataGroupControllerTest { coVerify { store.dispatch(HistoryMetadataGroupFragmentAction.DeleteAll) - historyStorage.deleteHistoryMetadata(searchTerm) + getMetadataItemsList().forEach { + historyStorage.deleteVisitsFor(it.url) + } metrics.track(Event.HistorySearchTermGroupRemoveAll) } }