From 3de2fbc15653e09deadad1531e3f5ca7451a44b2 Mon Sep 17 00:00:00 2001 From: Matthew Tighe Date: Fri, 20 Sep 2024 17:03:51 +0000 Subject: [PATCH] Bug 1919620 - Update Tabs Tray to add bookmarks to bookmark folder most recently saved into r=android-reviewers,boek Differential Revision: https://phabricator.services.mozilla.com/D222881 --- .../fenix/tabstray/TabsTrayController.kt | 50 +++++--- .../fenix/tabstray/TabsTrayFragment.kt | 8 +- .../fenix/tabstray/ext/FenixSnackbar.kt | 10 +- .../fenix/app/src/main/res/values/strings.xml | 2 + .../tabstray/DefaultTabsTrayControllerTest.kt | 119 ++++++++++++++++-- .../fenix/tabstray/ext/FenixSnackbarKtTest.kt | 17 +-- 6 files changed, 159 insertions(+), 47 deletions(-) diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt index 0f10200647c5..a917440e5871 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt @@ -10,6 +10,7 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import mozilla.appservices.places.BookmarkRoot import mozilla.components.browser.state.action.DebugAction import mozilla.components.browser.state.action.LastAccessAction import mozilla.components.browser.state.selector.findTab @@ -23,6 +24,7 @@ import mozilla.components.browser.storage.sync.Tab import mozilla.components.concept.base.profiler.Profiler import mozilla.components.concept.engine.mediasession.MediaSession.PlaybackState import mozilla.components.concept.engine.prompt.ShareData +import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.feature.accounts.push.CloseTabsUseCases import mozilla.components.feature.downloads.ui.DownloadCancelDialogFragment import mozilla.components.feature.tabs.TabsUseCases @@ -41,11 +43,9 @@ import org.mozilla.fenix.collections.show import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.TabCollectionStorage import org.mozilla.fenix.components.appstate.AppAction -import org.mozilla.fenix.components.bookmarks.BookmarksUseCase import org.mozilla.fenix.ext.DEFAULT_ACTIVE_DAYS import org.mozilla.fenix.ext.potentialInactiveTabs import org.mozilla.fenix.home.HomeFragment -import org.mozilla.fenix.library.bookmarks.BookmarksSharedViewModel import org.mozilla.fenix.tabstray.browser.InactiveTabsController import org.mozilla.fenix.tabstray.browser.TabsTrayFabController import org.mozilla.fenix.tabstray.ext.getTabSessionState @@ -187,7 +187,7 @@ interface TabsTrayController : SyncedTabsController, InactiveTabsController, Tab * @param profiler [Profiler] used to add profiler markers. * @param navigationInteractor [NavigationInteractor] used to perform navigation actions with side effects. * @param tabsUseCases Use case wrapper for interacting with tabs. - * @param bookmarksUseCase Use case wrapper for interacting with bookmarks. + * @param bookmarksStorage Storage layer for retrieving and saving bookmarks. * @param closeSyncedTabsUseCases Use cases for closing synced tabs. * @param ioDispatcher [CoroutineContext] used for storage operations. * @param collectionStorage Storage layer for interacting with collections. @@ -200,7 +200,6 @@ interface TabsTrayController : SyncedTabsController, InactiveTabsController, Tab * @param showBookmarkSnackbar Lambda used to display a snackbar upon saving tabs as bookmarks. * @param showCollectionSnackbar Lambda used to display a snackbar upon successfully saving tabs * to a collection. - * @param bookmarksSharedViewModel [BookmarksSharedViewModel] used to get currently selected bookmark root. */ @Suppress("TooManyFunctions", "LongParameterList") class DefaultTabsTrayController( @@ -215,7 +214,7 @@ class DefaultTabsTrayController( private val profiler: Profiler?, private val navigationInteractor: NavigationInteractor, private val tabsUseCases: TabsUseCases, - private val bookmarksUseCase: BookmarksUseCase, + private val bookmarksStorage: BookmarksStorage, private val closeSyncedTabsUseCases: CloseTabsUseCases, private val ioDispatcher: CoroutineContext, private val collectionStorage: TabCollectionStorage, @@ -225,12 +224,11 @@ class DefaultTabsTrayController( private val showUndoSnackbarForInactiveTab: (Int) -> Unit, private val showUndoSnackbarForSyncedTab: (CloseTabsUseCases.UndoableOperation) -> Unit, internal val showCancelledDownloadWarning: (downloadCount: Int, tabId: String?, source: String?) -> Unit, - private val showBookmarkSnackbar: (tabSize: Int) -> Unit, + private val showBookmarkSnackbar: (tabSize: Int, parentFolderTitle: String?) -> Unit, private val showCollectionSnackbar: ( tabSize: Int, isNewCollection: Boolean, ) -> Unit, - private val bookmarksSharedViewModel: BookmarksSharedViewModel, ) : TabsTrayController { override fun handleNormalTabsFabClick() { @@ -422,21 +420,35 @@ class DefaultTabsTrayController( TabsTray.bookmarkSelectedTabs.record(TabsTray.BookmarkSelectedTabsExtra(tabCount = tabs.size)) - tabs.forEach { tab -> - // We don't combine the context with lifecycleScope so that our jobs are not cancelled - // if we leave the fragment, i.e. we still want the bookmarks to be added if the - // tabs tray closes before the job is done. - CoroutineScope(ioDispatcher).launch { - bookmarksUseCase.addBookmark( - tab.content.url, - tab.content.title, - parentGuid = bookmarksSharedViewModel.selectedFolder?.guid, - ) + // We don't combine the context with lifecycleScope so that our jobs are not cancelled + // if we leave the fragment, i.e. we still want the bookmarks to be added if the + // tabs tray closes before the job is done. + CoroutineScope(ioDispatcher).launch { + Result.runCatching { + val parentGuid = bookmarksStorage + .getRecentBookmarks(1) + .firstOrNull() + ?.parentGuid + ?: BookmarkRoot.Mobile.id + + val parentNode = bookmarksStorage.getBookmark(parentGuid) + + tabs.forEach { tab -> + bookmarksStorage.addItem( + parentGuid = parentNode!!.guid, + url = tab.content.url, + title = tab.content.title, + position = null, + ) + } + withContext(Dispatchers.Main) { + showBookmarkSnackbar(tabs.size, parentNode?.title) + } + }.getOrElse { + // silently fail } } - showBookmarkSnackbar(tabs.size) - tabsTrayStore.dispatch(TabsTrayAction.ExitSelectMode) } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt index d88c5b47ee51..d219bf2e6220 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt @@ -56,7 +56,6 @@ import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.runIfFragmentIsAttached import org.mozilla.fenix.ext.settings import org.mozilla.fenix.home.HomeScreenViewModel -import org.mozilla.fenix.library.bookmarks.BookmarksSharedViewModel import org.mozilla.fenix.share.ShareFragment import org.mozilla.fenix.tabstray.browser.SelectionBannerBinding import org.mozilla.fenix.tabstray.browser.SelectionBannerBinding.VisibilityModifier @@ -103,7 +102,6 @@ class TabsTrayFragment : AppCompatDialogFragment() { private val tabsFeature = ViewBoundFeatureWrapper() private val tabsTrayInactiveTabsOnboardingBinding = ViewBoundFeatureWrapper() private val syncedTabsIntegration = ViewBoundFeatureWrapper() - private val bookmarksSharedViewModel: BookmarksSharedViewModel by activityViewModels() @VisibleForTesting @Suppress("VariableNaming") @@ -194,7 +192,7 @@ class TabsTrayFragment : AppCompatDialogFragment() { profiler = requireComponents.core.engine.profiler, tabsUseCases = requireComponents.useCases.tabsUseCases, closeSyncedTabsUseCases = requireComponents.useCases.closeSyncedTabsUseCases, - bookmarksUseCase = requireComponents.useCases.bookmarksUseCases, + bookmarksStorage = requireComponents.core.bookmarksStorage, ioDispatcher = Dispatchers.IO, collectionStorage = requireComponents.core.tabCollectionStorage, selectTabPosition = ::selectTabPosition, @@ -205,7 +203,6 @@ class TabsTrayFragment : AppCompatDialogFragment() { showCancelledDownloadWarning = ::showCancelledDownloadWarning, showCollectionSnackbar = ::showCollectionSnackbar, showBookmarkSnackbar = ::showBookmarkSnackbar, - bookmarksSharedViewModel = bookmarksSharedViewModel, ) tabsTrayInteractor = DefaultTabsTrayInteractor( @@ -847,10 +844,11 @@ class TabsTrayFragment : AppCompatDialogFragment() { @VisibleForTesting internal fun showBookmarkSnackbar( tabSize: Int, + parentFolderTitle: String?, ) { FenixSnackbar .make(requireView()) - .bookmarkMessage(tabSize) + .bookmarkMessage(tabSize, parentFolderTitle) .anchorWithAction(getSnackbarAnchor()) { findNavController().navigate( TabsTrayFragmentDirections.actionGlobalBookmarkFragment(BookmarkRoot.Mobile.id), diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt index 8f70afee42a3..d04caa5380c7 100644 --- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt @@ -30,16 +30,18 @@ internal fun FenixSnackbar.collectionMessage( internal fun FenixSnackbar.bookmarkMessage( tabSize: Int, + parentFolderTitle: String?, ): FenixSnackbar { - val stringRes = when { + val displayFolderTitle = parentFolderTitle ?: context.getString(R.string.library_bookmarks) + val displayResId = when { tabSize > 1 -> { - R.string.snackbar_message_bookmarks_saved + R.string.snackbar_message_bookmarks_saved_in } else -> { - R.string.bookmark_saved_snackbar + R.string.bookmark_saved_in_folder_snackbar } } - setText(context.getString(stringRes)) + setText(context.getString(displayResId, displayFolderTitle)) return this } diff --git a/mobile/android/fenix/app/src/main/res/values/strings.xml b/mobile/android/fenix/app/src/main/res/values/strings.xml index 2ba106254e50..e90881dc7d7f 100644 --- a/mobile/android/fenix/app/src/main/res/values/strings.xml +++ b/mobile/android/fenix/app/src/main/res/values/strings.xml @@ -1495,6 +1495,8 @@ Tabs closed: %1$s Bookmarks saved! + + Bookmarks saved in “%s”! Added to shortcuts! diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt index 1248a8fb465d..68a20afb7f1b 100644 --- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt +++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/DefaultTabsTrayControllerTest.kt @@ -8,6 +8,7 @@ import androidx.navigation.NavController import androidx.navigation.NavDirections import androidx.navigation.NavOptions import io.mockk.MockKAnnotations +import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every import io.mockk.impl.annotations.MockK @@ -19,6 +20,7 @@ import io.mockk.spyk import io.mockk.unmockkStatic import io.mockk.verify import io.mockk.verifyOrder +import mozilla.appservices.places.BookmarkRoot import mozilla.components.browser.state.action.TabListAction import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.state.selector.getNormalOrPrivateTabs @@ -31,6 +33,9 @@ import mozilla.components.browser.state.store.BrowserStore import mozilla.components.browser.storage.sync.Tab import mozilla.components.browser.storage.sync.TabEntry import mozilla.components.concept.base.profiler.Profiler +import mozilla.components.concept.storage.BookmarkNode +import mozilla.components.concept.storage.BookmarkNodeType +import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.feature.accounts.push.CloseTabsUseCases import mozilla.components.feature.tabs.TabsUseCases import mozilla.components.support.test.ext.joinBlocking @@ -65,12 +70,10 @@ import org.mozilla.fenix.collections.show import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.TabCollectionStorage import org.mozilla.fenix.components.appstate.AppAction -import org.mozilla.fenix.components.bookmarks.BookmarksUseCase import org.mozilla.fenix.ext.maxActiveTime import org.mozilla.fenix.ext.potentialInactiveTabs import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.home.HomeFragment -import org.mozilla.fenix.library.bookmarks.BookmarksSharedViewModel import org.mozilla.fenix.utils.Settings import java.util.concurrent.TimeUnit @@ -103,12 +106,10 @@ class DefaultTabsTrayControllerTest { private val appStore: AppStore = mockk(relaxed = true) private val settings: Settings = mockk(relaxed = true) - private val bookmarksUseCase: BookmarksUseCase = mockk(relaxed = true) + private val bookmarksStorage: BookmarksStorage = mockk(relaxed = true) private val closeSyncedTabsUseCases: CloseTabsUseCases = mockk(relaxed = true) private val collectionStorage: TabCollectionStorage = mockk(relaxed = true) - private val bookmarksSharedViewModel: BookmarksSharedViewModel = mockk(relaxed = true) - private val coroutinesTestRule: MainCoroutineRule = MainCoroutineRule() private val testDispatcher = coroutinesTestRule.testDispatcher @@ -1177,18 +1178,20 @@ class DefaultTabsTrayControllerTest { } @Test - fun `GIVEN one tab is selected WHEN the save selected tabs to bookmarks button is clicked THEN report the telemetry and show a snackbar`() = runTestOnMain { + fun `GIVEN one tab selected and no bookmarks previously saved WHEN saving selected tabs to bookmarks THEN save bookmark in root, report telemetry, show snackbar`() = runTestOnMain { var showBookmarkSnackbarInvoked = false + coEvery { bookmarksStorage.getRecentBookmarks(1) } returns listOf() + coEvery { bookmarksStorage.getBookmark(BookmarkRoot.Mobile.id) } returns makeBookmarkFolder(guid = BookmarkRoot.Mobile.id) every { trayStore.state.mode.selectedTabs } returns setOf(createTab(url = "https://mozilla.org")) createController( - showBookmarkSnackbar = { + showBookmarkSnackbar = { _, _ -> showBookmarkSnackbarInvoked = true }, ).handleBookmarkSelectedTabsClicked() - coVerify(exactly = 1) { bookmarksUseCase.addBookmark(any(), any(), any(), any()) } + coVerify(exactly = 1) { bookmarksStorage.addItem(eq(BookmarkRoot.Mobile.id), any(), any(), any()) } assertTrue(showBookmarkSnackbarInvoked) assertNotNull(TabsTray.bookmarkSelectedTabs.testGetValue()) @@ -1197,6 +1200,79 @@ class DefaultTabsTrayControllerTest { assertEquals("1", snapshot.single().extra?.getValue("tab_count")) } + @Test + fun `GIVEN one tab selected and a previously saved bookmark WHEN saving selected tabs to bookmarks THEN save bookmark in last saved folder, report telemetry, show snackbar`() = runTestOnMain { + var showBookmarkSnackbarInvoked = false + + val parentGuid = "parentGuid" + val previousBookmark = makeBookmarkItem(parentGuid = parentGuid) + coEvery { bookmarksStorage.getRecentBookmarks(eq(1), any(), any()) } returns listOf(previousBookmark) + coEvery { bookmarksStorage.getBookmark(parentGuid) } returns makeBookmarkFolder(guid = parentGuid) + every { trayStore.state.mode.selectedTabs } returns setOf(createTab(url = "https://mozilla.org")) + + createController( + showBookmarkSnackbar = { _, _ -> + showBookmarkSnackbarInvoked = true + }, + ).handleBookmarkSelectedTabsClicked() + + coVerify(exactly = 1) { bookmarksStorage.addItem(eq(parentGuid), any(), any(), any()) } + assertTrue(showBookmarkSnackbarInvoked) + + assertNotNull(TabsTray.bookmarkSelectedTabs.testGetValue()) + val snapshot = TabsTray.bookmarkSelectedTabs.testGetValue()!! + assertEquals(1, snapshot.size) + assertEquals("1", snapshot.single().extra?.getValue("tab_count")) + } + + @Test + fun `GIVEN multiple tabs selected and no bookmarks previously saved WHEN saving selected tabs to bookmarks THEN save bookmarks in root, report telemetry, show a snackbar`() = runTestOnMain { + var showBookmarkSnackbarInvoked = false + + coEvery { bookmarksStorage.getRecentBookmarks(1) } returns listOf() + coEvery { bookmarksStorage.getBookmark(BookmarkRoot.Mobile.id) } returns makeBookmarkFolder(guid = BookmarkRoot.Mobile.id) + every { trayStore.state.mode.selectedTabs } returns setOf(createTab(url = "https://mozilla.org"), createTab(url = "https://mozilla2.org")) + + createController( + showBookmarkSnackbar = { _, _ -> + showBookmarkSnackbarInvoked = true + }, + ).handleBookmarkSelectedTabsClicked() + + coVerify(exactly = 2) { bookmarksStorage.addItem(eq(BookmarkRoot.Mobile.id), any(), any(), any()) } + assertTrue(showBookmarkSnackbarInvoked) + + assertNotNull(TabsTray.bookmarkSelectedTabs.testGetValue()) + val snapshot = TabsTray.bookmarkSelectedTabs.testGetValue()!! + assertEquals(1, snapshot.size) + assertEquals("2", snapshot.single().extra?.getValue("tab_count")) + } + + @Test + fun `GIVEN multiple tabs selected and a previously saved bookmark WHEN saving selected tabs to bookmarks THEN save bookmarks in same folder as recent bookmark, report telemetry, show a snackbar`() = runTestOnMain { + var showBookmarkSnackbarInvoked = false + + val parentGuid = "parentGuid" + val previousBookmark = makeBookmarkItem(parentGuid = parentGuid) + coEvery { bookmarksStorage.getRecentBookmarks(eq(1), any(), any()) } returns listOf(previousBookmark) + coEvery { bookmarksStorage.getBookmark(parentGuid) } returns makeBookmarkFolder(guid = parentGuid) + every { trayStore.state.mode.selectedTabs } returns setOf(createTab(url = "https://mozilla.org"), createTab(url = "https://mozilla2.org")) + + createController( + showBookmarkSnackbar = { _, _ -> + showBookmarkSnackbarInvoked = true + }, + ).handleBookmarkSelectedTabsClicked() + + coVerify(exactly = 2) { bookmarksStorage.addItem(eq(parentGuid), any(), any(), any()) } + assertTrue(showBookmarkSnackbarInvoked) + + assertNotNull(TabsTray.bookmarkSelectedTabs.testGetValue()) + val snapshot = TabsTray.bookmarkSelectedTabs.testGetValue()!! + assertEquals(1, snapshot.size) + assertEquals("2", snapshot.single().extra?.getValue("tab_count")) + } + @Test fun `GIVEN active page is not normal tabs WHEN the normal tabs page button is clicked THEN report the metric`() { every { trayStore.state.selectedPage } returns Page.PrivateTabs @@ -1272,7 +1348,7 @@ class DefaultTabsTrayControllerTest { showUndoSnackbarForSyncedTab: (CloseTabsUseCases.UndoableOperation) -> Unit = { _ -> }, showCancelledDownloadWarning: (Int, String?, String?) -> Unit = { _, _, _ -> }, showCollectionSnackbar: (Int, Boolean) -> Unit = { _, _ -> }, - showBookmarkSnackbar: (Int) -> Unit = { _ -> }, + showBookmarkSnackbar: (Int, String?) -> Unit = { _, _ -> }, ): DefaultTabsTrayController { return DefaultTabsTrayController( activity = activity, @@ -1286,7 +1362,7 @@ class DefaultTabsTrayControllerTest { profiler = profiler, navigationInteractor = navigationInteractor, tabsUseCases = tabsUseCases, - bookmarksUseCase = bookmarksUseCase, + bookmarksStorage = bookmarksStorage, closeSyncedTabsUseCases = closeSyncedTabsUseCases, collectionStorage = collectionStorage, ioDispatcher = testDispatcher, @@ -1298,7 +1374,28 @@ class DefaultTabsTrayControllerTest { showCancelledDownloadWarning = showCancelledDownloadWarning, showCollectionSnackbar = showCollectionSnackbar, showBookmarkSnackbar = showBookmarkSnackbar, - bookmarksSharedViewModel = bookmarksSharedViewModel, ) } + + private fun makeBookmarkFolder(guid: String) = BookmarkNode( + type = BookmarkNodeType.FOLDER, + parentGuid = BookmarkRoot.Mobile.id, + guid = guid, + position = 42U, + title = "title", + url = "url", + dateAdded = 0L, + children = null, + ) + + private fun makeBookmarkItem(parentGuid: String) = BookmarkNode( + type = BookmarkNodeType.ITEM, + parentGuid = parentGuid, + guid = "guid", + position = 42U, + title = "title", + url = "url", + dateAdded = 0L, + children = null, + ) } diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt index a8ae568af1a6..ed3e8baa4ac1 100644 --- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt +++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt @@ -62,23 +62,24 @@ class FenixSnackbarKtTest { @Test fun `WHEN bookmarkMessage is called with different parameters THEN correct text will be set`() { + val parentFolderTitle = "title" val mockContext: Context = mockk { - every { getString(R.string.bookmark_saved_snackbar) } - .answers { "test1" } - every { getString(R.string.snackbar_message_bookmarks_saved) } - .answers { "test2" } + every { getString(R.string.bookmark_saved_in_folder_snackbar, eq(parentFolderTitle)) } + .answers { "Bookmark saved in title!" } + every { getString(R.string.snackbar_message_bookmarks_saved_in, eq(parentFolderTitle)) } + .answers { "Bookmarks saved in title!" } } val snackbar: FenixSnackbar = mockk { every { context }.answers { mockContext } } every { snackbar.setText(any()) }.answers { snackbar } - snackbar.bookmarkMessage(1) - snackbar.bookmarkMessage(2) + snackbar.bookmarkMessage(1, parentFolderTitle) + snackbar.bookmarkMessage(2, parentFolderTitle) verifyOrder { - snackbar.setText("test1") - snackbar.setText("test2") + snackbar.setText("Bookmark saved in title!") + snackbar.setText("Bookmarks saved in title!") } }