From e69f3dfbbbbf2bf745882365f1fe14187e3d4736 Mon Sep 17 00:00:00 2001 From: Roger Yang Date: Tue, 6 Jul 2021 13:02:52 -0400 Subject: [PATCH] Closes #19090: Show snackbar on adding to bookmarks from tabs tray --- .../java/org/mozilla/fenix/ui/SmokeTest.kt | 5 +-- .../fenix/tabstray/NavigationInteractor.kt | 9 ++--- .../fenix/tabstray/TabsTrayFragment.kt | 39 ++++++++++++++----- .../fenix/tabstray/ext/FenixSnackbar.kt | 17 +++++++- .../tabstray/NavigationInteractorTest.kt | 18 ++------- .../fenix/tabstray/ext/FenixSnackbarKtTest.kt | 30 ++++++++++++-- 6 files changed, 80 insertions(+), 38 deletions(-) diff --git a/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt b/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt index 14fe0bdbeb..6b210b0817 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/ui/SmokeTest.kt @@ -879,7 +879,6 @@ class SmokeTest { } } - @Ignore("Disabling until re-implemented by #19090") @Test fun createFirstCollectionTest() { val firstWebPage = TestAssetHelper.getGenericAsset(mockWebServer, 1) @@ -895,7 +894,7 @@ class SmokeTest { }.goToHomescreen { }.clickSaveTabsToCollectionButton { longClickTab(firstWebPage.title) - longClickTab(secondWebPage.title) + selectTab(secondWebPage.title) }.clickSaveCollection { typeCollectionNameAndSave(collectionName) } @@ -963,7 +962,6 @@ class SmokeTest { } } - @Ignore("Disabling until re-implemented by #19090") @Test fun shareCollectionTest() { val webPage = TestAssetHelper.getGenericAsset(mockWebServer, 1) @@ -985,7 +983,6 @@ class SmokeTest { } } - @Ignore("Disabling until re-implemented by #19090") @Test fun deleteCollectionTest() { val webPage = TestAssetHelper.getGenericAsset(mockWebServer, 1) diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt b/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt index 6400e4b9b2..834fb1d169 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/NavigationInteractor.kt @@ -104,6 +104,7 @@ class DefaultNavigationInteractor( isNewCollection: Boolean, collectionToSelect: Long? ) -> Unit, + private val showBookmarkSnackbar: (tabSize: Int) -> Unit, private val accountManager: FxaAccountManager, private val ioDispatcher: CoroutineContext ) : NavigationInteractor { @@ -171,12 +172,12 @@ class DefaultNavigationInteractor( override fun onSaveToCollections(tabs: Collection) { metrics.track(Event.TabsTraySaveToCollectionPressed) + tabsTrayStore.dispatch(TabsTrayAction.ExitSelectMode) CollectionsDialog( storage = collectionStorage, sessionList = browserStore.getTabSessionState(tabs), onPositiveButtonClick = { id, isNewCollection -> - tabsTrayStore.dispatch(TabsTrayAction.ExitSelectMode) // If collection is null, a new one was created. val event = if (isNewCollection) { @@ -190,9 +191,7 @@ class DefaultNavigationInteractor( metrics.track(event) }, - onNegativeButtonClick = { - tabsTrayStore.dispatch(TabsTrayAction.ExitSelectMode) - } + onNegativeButtonClick = {} ).show(context) } @@ -206,7 +205,7 @@ class DefaultNavigationInteractor( } } - // TODO show successful snackbar here (regardless of operation success). + showBookmarkSnackbar(tabs.size) } override fun onSyncedTabClicked(tab: SyncTab) { diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt index 1bff4d5d5d..155c93a1bb 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt @@ -28,6 +28,7 @@ import kotlinx.android.synthetic.main.fragment_tab_tray_dialog.* import kotlinx.android.synthetic.main.tabs_tray_tab_counter2.* import kotlinx.android.synthetic.main.tabstray_multiselect_items.* import kotlinx.coroutines.Dispatchers +import mozilla.appservices.places.BookmarkRoot import mozilla.components.browser.state.selector.normalTabs import mozilla.components.browser.state.selector.privateTabs import mozilla.components.browser.state.store.BrowserStore @@ -49,8 +50,9 @@ import org.mozilla.fenix.tabstray.browser.SelectionBannerBinding import org.mozilla.fenix.tabstray.browser.SelectionBannerBinding.VisibilityModifier import org.mozilla.fenix.tabstray.browser.SelectionHandleBinding import org.mozilla.fenix.tabstray.ext.anchorWithAction +import org.mozilla.fenix.tabstray.ext.bookmarkMessage +import org.mozilla.fenix.tabstray.ext.collectionMessage import org.mozilla.fenix.tabstray.ext.make -import org.mozilla.fenix.tabstray.ext.message import org.mozilla.fenix.tabstray.ext.orDefault import org.mozilla.fenix.tabstray.ext.showWithTheme import org.mozilla.fenix.utils.allowUndo @@ -133,6 +135,7 @@ class TabsTrayFragment : AppCompatDialogFragment() { bookmarksUseCase = requireComponents.useCases.bookmarksUseCases, collectionStorage = requireComponents.core.tabCollectionStorage, showCollectionSnackbar = ::showCollectionSnackbar, + showBookmarkSnackbar = ::showBookmarkSnackbar, accountManager = requireComponents.backgroundServices.accountManager, ioDispatcher = Dispatchers.IO ) @@ -410,16 +413,10 @@ class TabsTrayFragment : AppCompatDialogFragment() { isNewCollection: Boolean = false, collectionToSelect: Long? ) { - val anchor = if (requireComponents.settings.accessibilityServicesEnabled) { - null - } else { - new_tab_button - } - FenixSnackbar .make(requireView()) - .message(tabSize, isNewCollection) - .anchorWithAction(anchor) { + .collectionMessage(tabSize, isNewCollection) + .anchorWithAction(getSnackbarAnchor()) { findNavController().navigate( TabsTrayFragmentDirections.actionGlobalHome( focusOnAddressBar = false, @@ -430,6 +427,30 @@ class TabsTrayFragment : AppCompatDialogFragment() { }.show() } + @VisibleForTesting + internal fun showBookmarkSnackbar( + tabSize: Int + ) { + FenixSnackbar + .make(requireView()) + .bookmarkMessage(tabSize) + .anchorWithAction(getSnackbarAnchor()) { + findNavController().navigate( + TabsTrayFragmentDirections.actionGlobalBookmarkFragment(BookmarkRoot.Mobile.id) + ) + dismissTabsTray() + } + .show() + } + + private fun getSnackbarAnchor(): View? { + return if (requireComponents.settings.accessibilityServicesEnabled) { + null + } else { + new_tab_button + } + } + companion object { // Minimum number of list items for which to show the tabs tray as expanded. const val EXPAND_AT_LIST_SIZE = 4 diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt b/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt index bde56166fd..9689f68f91 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/ext/FenixSnackbar.kt @@ -9,7 +9,7 @@ import org.mozilla.fenix.R import org.mozilla.fenix.components.FenixSnackbar import org.mozilla.fenix.tabstray.TabsTrayFragment.Companion.ELEVATION -internal fun FenixSnackbar.message( +internal fun FenixSnackbar.collectionMessage( tabSize: Int, isNewCollection: Boolean = false ): FenixSnackbar { @@ -28,6 +28,21 @@ internal fun FenixSnackbar.message( return this } +internal fun FenixSnackbar.bookmarkMessage( + tabSize: Int +): FenixSnackbar { + val stringRes = when { + tabSize > 1 -> { + R.string.snackbar_message_bookmarks_saved + } + else -> { + R.string.bookmark_saved_snackbar + } + } + setText(context.getString(stringRes)) + return this +} + internal inline fun FenixSnackbar.anchorWithAction( anchor: View?, crossinline action: () -> Unit diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt index 74c0117ccb..b14fb0b8ff 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/NavigationInteractorTest.kt @@ -54,6 +54,7 @@ class NavigationInteractorTest { private val context: Context = mockk(relaxed = true) private val collectionStorage: TabCollectionStorage = mockk(relaxed = true) private val showCollectionSnackbar: (Int, Boolean, Long?) -> Unit = mockk(relaxed = true) + private val showBookmarkSnackbar: (Int) -> Unit = mockk(relaxed = true) private val accountManager: FxaAccountManager = mockk(relaxed = true) private val activity: HomeActivity = mockk(relaxed = true) @@ -78,6 +79,7 @@ class NavigationInteractorTest { tabsTrayStore, collectionStorage, showCollectionSnackbar, + showBookmarkSnackbar, accountManager, testDispatcher ) @@ -234,23 +236,9 @@ class NavigationInteractorTest { @Test fun `onBookmarkTabs calls navigation on DefaultNavigationInteractor`() = runBlockingTest { - navigationInteractor = DefaultNavigationInteractor( - context, - activity, - store, - navController, - metrics, - dismissTabTray, - dismissTabTrayAndNavigateHome, - bookmarksUseCase, - tabsTrayStore, - collectionStorage, - showCollectionSnackbar, - accountManager, - coroutineContext - ) navigationInteractor.onSaveToBookmarks(listOf(createTrayTab())) coVerify(exactly = 1) { bookmarksUseCase.addBookmark(any(), any(), any()) } + coVerify(exactly = 1) { showBookmarkSnackbar(1) } } @Test diff --git a/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt b/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt index a91abd7c76..c46f3c2f23 100644 --- a/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt +++ b/app/src/test/java/org/mozilla/fenix/tabstray/ext/FenixSnackbarKtTest.kt @@ -13,7 +13,7 @@ import org.mozilla.fenix.tabstray.TabsTrayFragment.Companion.ELEVATION class FenixSnackbarKtTest { @Test - fun `WHEN message is called with different parameters THEN correct text will be set`() { + fun `WHEN collectionMessage is called with different parameters THEN correct text will be set`() { val mockContext: Context = mockk { every { getString(R.string.create_collection_tabs_saved_new_collection) } .answers { "test1" } @@ -27,9 +27,9 @@ class FenixSnackbarKtTest { } every { snackbar.setText(any()) }.answers { snackbar } - snackbar.message(1, true) - snackbar.message(2, false) - snackbar.message(1, false) + snackbar.collectionMessage(1, true) + snackbar.collectionMessage(2, false) + snackbar.collectionMessage(1, false) verifyOrder { snackbar.setText("test1") @@ -38,6 +38,28 @@ class FenixSnackbarKtTest { } } + @Test + fun `WHEN bookmarkMessage is called with different parameters THEN correct text will be set`() { + val mockContext: Context = mockk { + every { getString(R.string.bookmark_saved_snackbar) } + .answers { "test1" } + every { getString(R.string.snackbar_message_bookmarks_saved) } + .answers { "test2" } + } + val snackbar: FenixSnackbar = mockk { + every { context }.answers { mockContext } + } + every { snackbar.setText(any()) }.answers { snackbar } + + snackbar.bookmarkMessage(1) + snackbar.bookmarkMessage(2) + + verifyOrder { + snackbar.setText("test1") + snackbar.setText("test2") + } + } + @Test fun `WHEN anchorWithAction is called THEN correct text will be set`() { val mockContext: Context = mockk {