From 3f083beeb49cf39465d6ca99cd4973b3a9fee391 Mon Sep 17 00:00:00 2001 From: Alexandru2909 Date: Thu, 31 Mar 2022 15:48:15 +0300 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/24214 - Move bookmark removal events to BookmarkController --- .../mozilla/fenix/components/metrics/Event.kt | 3 --- .../components/metrics/GleanMetricsService.kt | 1 - .../library/bookmarks/BookmarkController.kt | 24 +++++++++++++----- .../library/bookmarks/BookmarkFragment.kt | 25 ++++++++----------- .../bookmarks/BookmarkFragmentInteractor.kt | 9 +++---- .../bookmarks/BookmarkControllerTest.kt | 15 ++++++----- .../BookmarkFragmentInteractorTest.kt | 5 ++-- 7 files changed, 41 insertions(+), 41 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt index cedd7d98a1..a055b6f372 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt @@ -29,9 +29,6 @@ sealed class Event { object DismissedOnboarding : Event() object ClearedPrivateData : Event() object AddBookmark : Event() - object RemoveBookmark : Event() - object RemoveBookmarkFolder : Event() - object RemoveBookmarks : Event() object CustomTabsClosed : Event() object CustomTabsActionTapped : Event() object CustomTabsMenuOpened : Event() diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt index b9eb340447..e5d9158520 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt @@ -778,7 +778,6 @@ private val Event.wrapper: EventWrapper<*>? is Event.AddonInstalled -> null is Event.SearchWidgetInstalled -> null is Event.SyncAuthFromSharedReuse, Event.SyncAuthFromSharedCopy -> null - else -> null } /** diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt index 47c150c148..1e45c8beed 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt @@ -21,7 +21,6 @@ import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.bookmarkStorage import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.nav @@ -44,14 +43,27 @@ interface BookmarkController { fun handleCopyUrl(item: BookmarkNode) fun handleBookmarkSharing(item: BookmarkNode) fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode) - fun handleBookmarkDeletion(nodes: Set, eventType: Event) + + /** + * Handle bookmark nodes deletion + * @param nodes The set of nodes to be deleted. + * @param removeType Type of removal. + */ + fun handleBookmarkDeletion(nodes: Set, removeType: BookmarkRemoveType) fun handleBookmarkFolderDeletion(nodes: Set) fun handleRequestSync() fun handleBackPressed() fun handleSearch() } -@Suppress("TooManyFunctions") +/** + * Type of bookmark nodes deleted. + */ +enum class BookmarkRemoveType { + SINGLE, MULTIPLE, FOLDER +} + +@Suppress("TooManyFunctions", "LongParameterList") class DefaultBookmarkController( private val activity: HomeActivity, private val navController: NavController, @@ -62,7 +74,7 @@ class DefaultBookmarkController( private val tabsUseCases: TabsUseCases?, private val loadBookmarkNode: suspend (String) -> BookmarkNode?, private val showSnackbar: (String) -> Unit, - private val deleteBookmarkNodes: (Set, Event) -> Unit, + private val deleteBookmarkNodes: (Set, BookmarkRemoveType) -> Unit, private val deleteBookmarkFolder: (Set) -> Unit, private val invokePendingDeletion: () -> Unit, private val showTabTray: () -> Unit @@ -146,8 +158,8 @@ class DefaultBookmarkController( showTabTray() } - override fun handleBookmarkDeletion(nodes: Set, eventType: Event) { - deleteBookmarkNodes(nodes, eventType) + override fun handleBookmarkDeletion(nodes: Set, removeType: BookmarkRemoveType) { + deleteBookmarkNodes(nodes, removeType) } override fun handleBookmarkFolderDeletion(nodes: Set) { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt index a968ad0b15..79879ce6ae 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt @@ -43,7 +43,6 @@ import org.mozilla.fenix.NavHostActivity import org.mozilla.fenix.R import org.mozilla.fenix.components.FenixSnackbar import org.mozilla.fenix.components.StoreProvider -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.databinding.FragmentBookmarkBinding import org.mozilla.fenix.ext.bookmarkStorage import org.mozilla.fenix.ext.components @@ -306,7 +305,7 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan } } - private fun deleteMulti(selected: Set, eventType: Event = Event.RemoveBookmarks) { + private fun deleteMulti(selected: Set, eventType: BookmarkRemoveType = BookmarkRemoveType.MULTIPLE) { selected.iterator().forEach { if (it.type == BookmarkNodeType.FOLDER) { showRemoveFolderDialog(selected) @@ -318,18 +317,17 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan pendingBookmarkDeletionJob = getDeleteOperation(eventType) val message = when (eventType) { - is Event.RemoveBookmarks -> { + BookmarkRemoveType.MULTIPLE -> { getRemoveBookmarksSnackBarMessage(selected, containsFolders = false) } - is Event.RemoveBookmarkFolder, - is Event.RemoveBookmark -> { + BookmarkRemoveType.FOLDER, + BookmarkRemoveType.SINGLE -> { val bookmarkNode = selected.first() getString( R.string.bookmark_deletion_snackbar_message, bookmarkNode.url?.toShortUrl(requireContext().components.publicSuffixList) ?: bookmarkNode.title ) } - else -> throw IllegalStateException("Illegal event type in onDeleteSome") } viewLifecycleOwner.lifecycleScope.allowUndo( @@ -386,7 +384,7 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan } setPositiveButton(R.string.delete_browsing_data_prompt_allow) { dialog: DialogInterface, _ -> updatePendingBookmarksToDelete(selected) - pendingBookmarkDeletionJob = getDeleteOperation(Event.RemoveBookmarkFolder) + pendingBookmarkDeletionJob = getDeleteOperation(BookmarkRemoveType.FOLDER) dialog.dismiss() val snackbarMessage = getRemoveBookmarksSnackBarMessage(selected, containsFolders = true) // Use fragment's lifecycle; the view may be gone by the time dialog is interacted with. @@ -397,7 +395,7 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan { undoPendingDeletion(selected) }, - operation = getDeleteOperation(Event.RemoveBookmarkFolder) + operation = getDeleteOperation(BookmarkRemoveType.FOLDER) ) } create() @@ -418,20 +416,17 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan refreshBookmarks() } - private fun getDeleteOperation(event: Event): (suspend () -> Unit) { + private fun getDeleteOperation(event: BookmarkRemoveType): (suspend () -> Unit) { return { deleteSelectedBookmarks(pendingBookmarksToDelete) pendingBookmarkDeletionJob = null when (event) { - is Event.RemoveBookmarkFolder -> + BookmarkRemoveType.FOLDER -> BookmarksManagement.folderRemove.record(NoExtras()) - is Event.RemoveBookmarks -> + BookmarkRemoveType.MULTIPLE -> BookmarksManagement.multiRemoved.record(NoExtras()) - is Event.RemoveBookmark -> + BookmarkRemoveType.SINGLE -> BookmarksManagement.removed.record(NoExtras()) - else -> { - throw IllegalArgumentException("Illegal event type in getDeleteOperation") - } } refreshBookmarks() } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt index d6305745ed..2c6ccd2f92 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt @@ -9,7 +9,6 @@ import mozilla.components.concept.storage.BookmarkNodeType import mozilla.telemetry.glean.private.NoExtras import org.mozilla.fenix.GleanMetrics.BookmarksManagement import org.mozilla.fenix.browser.browsingmode.BrowsingMode -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.utils.Do @@ -89,11 +88,11 @@ class BookmarkFragmentInteractor( } val eventType = when (nodes.singleOrNull()?.type) { BookmarkNodeType.ITEM, - BookmarkNodeType.SEPARATOR -> Event.RemoveBookmark - BookmarkNodeType.FOLDER -> Event.RemoveBookmarkFolder - null -> Event.RemoveBookmarks + BookmarkNodeType.SEPARATOR -> BookmarkRemoveType.SINGLE + BookmarkNodeType.FOLDER -> BookmarkRemoveType.FOLDER + null -> BookmarkRemoveType.MULTIPLE } - if (eventType == Event.RemoveBookmarkFolder) { + if (eventType == BookmarkRemoveType.FOLDER) { bookmarksController.handleBookmarkFolderDeletion(nodes) } else { bookmarksController.handleBookmarkDeletion(nodes, eventType) diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt index 663fffbe76..a683353d9a 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt @@ -38,7 +38,6 @@ import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.components.Services -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.bookmarkStorage import org.mozilla.fenix.ext.components @@ -396,12 +395,12 @@ class BookmarkControllerTest { fun `handleBookmarkDeletion for an item should properly call a passed in delegate`() { var deleteBookmarkNodesInvoked = false createController( - deleteBookmarkNodes = { nodes, event -> + deleteBookmarkNodes = { nodes, removeEvent -> assertEquals(setOf(item), nodes) - assertEquals(Event.RemoveBookmark, event) + assertEquals(BookmarkRemoveType.SINGLE, removeEvent) deleteBookmarkNodesInvoked = true } - ).handleBookmarkDeletion(setOf(item), Event.RemoveBookmark) + ).handleBookmarkDeletion(setOf(item), BookmarkRemoveType.SINGLE) assertTrue(deleteBookmarkNodesInvoked) } @@ -410,12 +409,12 @@ class BookmarkControllerTest { fun `handleBookmarkDeletion for multiple bookmarks should properly call a passed in delegate`() { var deleteBookmarkNodesInvoked = false createController( - deleteBookmarkNodes = { nodes, event -> + deleteBookmarkNodes = { nodes, removeEvent -> assertEquals(setOf(item, subfolder), nodes) - assertEquals(Event.RemoveBookmarks, event) + assertEquals(BookmarkRemoveType.MULTIPLE, removeEvent) deleteBookmarkNodesInvoked = true } - ).handleBookmarkDeletion(setOf(item, subfolder), Event.RemoveBookmarks) + ).handleBookmarkDeletion(setOf(item, subfolder), BookmarkRemoveType.MULTIPLE) assertTrue(deleteBookmarkNodesInvoked) } @@ -481,7 +480,7 @@ class BookmarkControllerTest { private fun createController( loadBookmarkNode: suspend (String) -> BookmarkNode? = { _ -> null }, showSnackbar: (String) -> Unit = { _ -> }, - deleteBookmarkNodes: (Set, Event) -> Unit = { _, _ -> }, + deleteBookmarkNodes: (Set, BookmarkRemoveType) -> Unit = { _, _ -> }, deleteBookmarkFolder: (Set) -> Unit = { _ -> }, invokePendingDeletion: () -> Unit = { }, showTabTray: () -> Unit = { } diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt index bb3a5e700c..b6164c55cf 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt @@ -19,7 +19,6 @@ import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.GleanMetrics.BookmarksManagement import org.mozilla.fenix.browser.browsingmode.BrowsingMode -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -174,7 +173,7 @@ class BookmarkFragmentInteractorTest { interactor.onDelete(setOf(item)) verify { - bookmarkController.handleBookmarkDeletion(setOf(item), Event.RemoveBookmark) + bookmarkController.handleBookmarkDeletion(setOf(item), BookmarkRemoveType.SINGLE) } } @@ -197,7 +196,7 @@ class BookmarkFragmentInteractorTest { interactor.onDelete(setOf(item, subfolder)) verify { - bookmarkController.handleBookmarkDeletion(setOf(item, subfolder), Event.RemoveBookmarks) + bookmarkController.handleBookmarkDeletion(setOf(item, subfolder), BookmarkRemoveType.MULTIPLE) } }