From 51018be38825a19b4795231eada96484983ef961 Mon Sep 17 00:00:00 2001 From: mcarare Date: Wed, 28 Oct 2020 12:43:47 +0200 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/15413: Use proper url when bookmarking a page. --- .../fenix/browser/BaseBrowserFragment.kt | 14 ++++--- .../toolbar/BrowserToolbarMenuController.kt | 4 +- ...DefaultBrowserToolbarMenuControllerTest.kt | 40 +++++++++++++++++-- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt index a5f7b5fcff..7164ed9597 100644 --- a/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/browser/BaseBrowserFragment.kt @@ -321,7 +321,11 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler, browserAnimator = browserAnimator, customTabSession = customTabSessionId?.let { sessionManager.findSessionById(it) }, openInFenixIntent = openInFenixIntent, - bookmarkTapped = { viewLifecycleOwner.lifecycleScope.launch { bookmarkTapped(it) } }, + bookmarkTapped = { url: String, title: String -> + viewLifecycleOwner.lifecycleScope.launch { + bookmarkTapped(url, title) + } + }, scope = viewLifecycleOwner.lifecycleScope, tabCollectionStorage = requireComponents.core.tabCollectionStorage, topSitesStorage = requireComponents.core.topSitesStorage, @@ -1052,10 +1056,10 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler, } } - private suspend fun bookmarkTapped(session: Session) = withContext(IO) { + private suspend fun bookmarkTapped(sessionUrl: String, sessionTitle: String) = withContext(IO) { val bookmarksStorage = requireComponents.core.bookmarksStorage val existing = - bookmarksStorage.getBookmarksWithUrl(session.url).firstOrNull { it.url == session.url } + bookmarksStorage.getBookmarksWithUrl(sessionUrl).firstOrNull { it.url == sessionUrl } if (existing != null) { // Bookmark exists, go to edit fragment withContext(Main) { @@ -1068,8 +1072,8 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler, // Save bookmark, then go to edit fragment val guid = bookmarksStorage.addItem( BookmarkRoot.Mobile.id, - url = session.url, - title = session.title, + url = sessionUrl, + title = sessionTitle, position = null ) diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarMenuController.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarMenuController.kt index 9c0734c373..90f91763fa 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarMenuController.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarMenuController.kt @@ -65,7 +65,7 @@ class DefaultBrowserToolbarMenuController( private val swipeRefresh: SwipeRefreshLayout, private val customTabSession: Session?, private val openInFenixIntent: Intent, - private val bookmarkTapped: (Session) -> Unit, + private val bookmarkTapped: (String, String) -> Unit, private val scope: CoroutineScope, private val tabCollectionStorage: TabCollectionStorage, private val topSitesStorage: DefaultTopSitesStorage, @@ -273,7 +273,7 @@ class DefaultBrowserToolbarMenuController( } ToolbarMenu.Item.Bookmark -> { sessionManager.selectedSession?.let { - bookmarkTapped(it) + getProperUrl(it)?.let { url -> bookmarkTapped(url, it.title) } } } ToolbarMenu.Item.Bookmarks -> browserAnimator.captureEngineViewAndDrawStatically { diff --git a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarMenuControllerTest.kt b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarMenuControllerTest.kt index 4ed3263176..a5c2d6c314 100644 --- a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarMenuControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarMenuControllerTest.kt @@ -73,7 +73,7 @@ class DefaultBrowserToolbarMenuControllerTest { @RelaxedMockK private lateinit var activity: HomeActivity @RelaxedMockK private lateinit var navController: NavController @RelaxedMockK private lateinit var findInPageLauncher: () -> Unit - @RelaxedMockK private lateinit var bookmarkTapped: (Session) -> Unit + @RelaxedMockK private lateinit var bookmarkTapped: (String, String) -> Unit @RelaxedMockK private lateinit var sessionManager: SessionManager @RelaxedMockK private lateinit var currentSession: Session @RelaxedMockK private lateinit var openInFenixIntent: Intent @@ -224,14 +224,48 @@ class DefaultBrowserToolbarMenuControllerTest { } @Test - fun handleToolbarBookmarkPress() = runBlockingTest { + fun handleToolbarBookmarkPressWithReaderModeInactive() = runBlockingTest { val item = ToolbarMenu.Item.Bookmark + val title = "Mozilla" + val readerUrl = "moz-extension://1234" + val readerTab = createTab( + url = readerUrl, + readerState = ReaderState(active = false, activeUrl = "https://1234.org"), + title = title + ) + browserStore = + BrowserStore(BrowserState(tabs = listOf(readerTab), selectedTabId = readerTab.id)) + every { currentSession.id } returns readerTab.id + every { currentSession.title } returns title + every { currentSession.url } returns "https://mozilla.org" val controller = createController(scope = this) controller.handleToolbarItemInteraction(item) verify { metrics.track(Event.BrowserMenuItemTapped(Event.BrowserMenuItemTapped.Item.BOOKMARK)) } - verify { bookmarkTapped(currentSession) } + verify { bookmarkTapped("https://mozilla.org", title) } + } + + @Test + fun handleToolbarBookmarkPressWithReaderModeActive() = runBlockingTest { + val item = ToolbarMenu.Item.Bookmark + val title = "Mozilla" + val readerUrl = "moz-extension://1234" + val readerTab = createTab( + url = readerUrl, + readerState = ReaderState(active = true, activeUrl = "https://mozilla.org"), + title = title + ) + browserStore = BrowserStore(BrowserState(tabs = listOf(readerTab), selectedTabId = readerTab.id)) + every { currentSession.id } returns readerTab.id + every { currentSession.title } returns title + every { currentSession.url } returns readerUrl + + val controller = createController(scope = this) + controller.handleToolbarItemInteraction(item) + + verify { metrics.track(Event.BrowserMenuItemTapped(Event.BrowserMenuItemTapped.Item.BOOKMARK)) } + verify { bookmarkTapped("https://mozilla.org", title) } } @Test