From c75e331a30d7c651090e648a7c69221d699af5ad Mon Sep 17 00:00:00 2001 From: Pg Date: Thu, 7 Apr 2022 18:39:07 +0200 Subject: [PATCH] For #11404 - Create alert message when a lot of tabs are to be opened. Current threshold is 15. --- .../library/bookmarks/BookmarkController.kt | 69 +++++++++++++------ .../library/bookmarks/BookmarkFragment.kt | 6 +- app/src/main/res/values/strings.xml | 9 +++ .../bookmarks/BookmarkControllerTest.kt | 42 ++++++----- detekt-baseline.xml | 3 +- 5 files changed, 88 insertions(+), 41 deletions(-) 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 b46307e15..7226d03ad 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 @@ -7,6 +7,7 @@ package org.mozilla.fenix.library.bookmarks import android.content.ClipData import android.content.ClipboardManager import android.content.res.Resources +import androidx.annotation.VisibleForTesting import androidx.navigation.NavController import androidx.navigation.NavDirections import kotlinx.coroutines.CoroutineScope @@ -75,9 +76,9 @@ class DefaultBookmarkController( private val store: BookmarkFragmentStore, private val sharedViewModel: BookmarksSharedViewModel, private val tabsUseCases: TabsUseCases?, - private val loadBookmarkNode: suspend (String) -> BookmarkNode?, + private val loadBookmarkNode: suspend (String, Boolean) -> BookmarkNode?, private val showSnackbar: (String) -> Unit, - private val onOpenAllInTabsEmpty: () -> Unit, + private val alertHeavyOpen: (Int, () -> Unit) -> Unit, private val deleteBookmarkNodes: (Set, BookmarkRemoveType) -> Unit, private val deleteBookmarkFolder: (Set) -> Unit, private val showTabTray: () -> Unit, @@ -86,6 +87,10 @@ class DefaultBookmarkController( private val resources: Resources = activity.resources + @Suppress("MagicNumber") + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + internal val maxOpenBeforeWarn = 15 + override fun handleBookmarkChanged(item: BookmarkNode) { sharedViewModel.selectedFolder = item store.dispatch(BookmarkFragmentAction.Change(item)) @@ -108,7 +113,7 @@ class DefaultBookmarkController( override fun handleBookmarkExpand(folder: BookmarkNode) { handleAllBookmarksDeselected() scope.launch { - val node = loadBookmarkNode.invoke(folder.guid) ?: return@launch + val node = loadBookmarkNode.invoke(folder.guid, false) ?: return@launch sharedViewModel.selectedFolder = node store.dispatch(BookmarkFragmentAction.Change(node)) } @@ -161,32 +166,56 @@ class DefaultBookmarkController( showTabTray() } - private suspend fun recursiveBookmarkFolderOpening(folder: BookmarkNode, firstLaunch: Boolean = false) { - val node = loadBookmarkNode.invoke(folder.guid) ?: return - if (!node.children.isNullOrEmpty()) { - if (firstLaunch) invokePendingDeletion.invoke() + private fun extractURLsFromTree(node: BookmarkNode): MutableList { + val urls = mutableListOf() - node.children!!.iterator().forEach { - when (it.type) { - BookmarkNodeType.FOLDER -> recursiveBookmarkFolderOpening(it, mode = mode) - BookmarkNodeType.ITEM -> { - it.url?.let { url -> tabsUseCases?.addTab?.invoke(url, private = (mode == BrowsingMode.Private)) } + when (node.type) { + BookmarkNodeType.FOLDER -> { + // if not leaf (empty) folder + if (!node.children.isNullOrEmpty()) { + node.children!!.iterator().forEach { + urls.addAll(extractURLsFromTree(it)) } - BookmarkNodeType.SEPARATOR -> {} - } - }.also { - if (firstLaunch) { - activity.browsingModeManager.mode = BrowsingMode.fromBoolean(mode == BrowsingMode.Private) - showTabTray() } } - } else if (firstLaunch) onOpenAllInTabsEmpty.invoke() + BookmarkNodeType.ITEM -> { + node.url?.let { urls.add(it) } + } + BookmarkNodeType.SEPARATOR -> {} + } + + return urls } override fun handleBookmarkFolderOpening(folder: BookmarkNode, mode: BrowsingMode) { // potentially heavy function with a lot of bookmarks to open => use a coroutine scope.launch { - recursiveBookmarkFolderOpening(folder, true, mode) + // if more than maxOpenBeforeWarn (15) elements + val tree = loadBookmarkNode.invoke(folder.guid, true) ?: return@launch + val urls = extractURLsFromTree(tree) + + val openAll = { load: Boolean -> + for (url in urls) { + tabsUseCases?.addTab?.invoke( + url, + private = (mode == BrowsingMode.Private), + startLoading = load, + ) + } + activity.browsingModeManager.mode = + BrowsingMode.fromBoolean(mode == BrowsingMode.Private) + showTabTray() + } + + // warn user if more than maxOpenBeforeWarn (15) + if (urls.size >= maxOpenBeforeWarn) { + alertHeavyOpen(urls.size) { + // do not load bookmarks when more than maxOpenBeforeWarn (15) + openAll(false) + } + } else { + openAll(true) + } } } 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 48a778f52..83799dbcd 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 @@ -101,7 +101,7 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan tabsUseCases = activity?.components?.useCases?.tabsUseCases, loadBookmarkNode = ::loadBookmarkNode, showSnackbar = ::showSnackBarWithText, - onOpenAllInTabsEmpty = ::onOpenAllInTabsEmpty, + alertHeavyOpen = ::alertHeavyOpen, deleteBookmarkNodes = ::deleteMulti, deleteBookmarkFolder = ::showRemoveFolderDialog, showTabTray = ::showTabTray, @@ -272,11 +272,11 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan return bookmarkView.onBackPressed() } - private suspend fun loadBookmarkNode(guid: String): BookmarkNode? = withContext(IO) { + private suspend fun loadBookmarkNode(guid: String, recursive: Boolean = false): BookmarkNode? = withContext(IO) { // Only runs if the fragment is attached same as [runIfFragmentIsAttached] context?.let { requireContext().bookmarkStorage - .getTree(guid, false) + .getTree(guid, recursive) ?.let { desktopFolders.withOptionalDesktopFolders(it) } } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 229221db9..f9bdfdede 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -624,6 +624,15 @@ Close + + Confirm open + + You are about to open %d tabs. This might slow down Firefox. Are you sure you want to continue ? + + Open tabs + + Cancel + %d site 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 535365735..195c7ec04 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 @@ -17,9 +17,11 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk +import io.mockk.mockkConstructor import io.mockk.runs import io.mockk.slot import io.mockk.spyk +import io.mockk.unmockkConstructor import io.mockk.verify import io.mockk.verifyOrder import mozilla.appservices.places.BookmarkRoot @@ -193,7 +195,7 @@ class BookmarkControllerTest { fun `handleBookmarkExpand should refresh and change the active bookmark node`() = runTestOnMain { var loadBookmarkNodeInvoked = false createController( - loadBookmarkNode = { + loadBookmarkNode = { _: String, _: Boolean -> loadBookmarkNodeInvoked = true tree }, @@ -365,7 +367,7 @@ class BookmarkControllerTest { showTabTray = { showTabTrayInvoked = true }, - loadBookmarkNode = { + loadBookmarkNode = { guid: String, _: Boolean -> fun recurseFind(item: BookmarkNode, guid: String): BookmarkNode? { if (item.guid == guid) { return item @@ -379,7 +381,7 @@ class BookmarkControllerTest { return null } } - recurseFind(tree, it) + recurseFind(tree, guid) }, ).handleBookmarkFolderOpening(tree, BrowsingMode.Normal) @@ -399,7 +401,7 @@ class BookmarkControllerTest { showTabTray = { showTabTrayInvoked = true }, - loadBookmarkNode = { + loadBookmarkNode = { guid: String, _: Boolean -> fun recurseFind(item: BookmarkNode, guid: String): BookmarkNode? { if (item.guid == guid) { return item @@ -413,7 +415,7 @@ class BookmarkControllerTest { return null } } - recurseFind(tree, it) + recurseFind(tree, guid) }, ).handleBookmarkFolderOpening(tree, BrowsingMode.Private) @@ -427,18 +429,24 @@ class BookmarkControllerTest { } @Test - fun `handleBookmarkFolderOpening for an empty folder should show toast`() { - var onOpenAllInTabsEmptyInvoked = false + fun `handleBookmarkFolderOpening for more than maxOpenBeforeWarn items should show alert`() { + var alertHeavyOpenInvoked = false + + mockkConstructor(DefaultBookmarkController::class) + every { + anyConstructed() getProperty "maxOpenBeforeWarn" + } propertyType Int::class returns 1 + createController( - onOpenAllInTabsEmpty = { - onOpenAllInTabsEmptyInvoked = true - }, - loadBookmarkNode = { - subfolder + alertHeavyOpen = { _: Int, _: () -> Unit -> alertHeavyOpenInvoked = true }, + loadBookmarkNode = { _: String, _: Boolean -> + tree }, - ).handleBookmarkFolderOpening(subfolder, BrowsingMode.Normal) + ).handleBookmarkFolderOpening(tree, BrowsingMode.Normal) + + unmockkConstructor(DefaultBookmarkController::class) - assertTrue(onOpenAllInTabsEmptyInvoked) + assertTrue(alertHeavyOpenInvoked) } @Test @@ -509,9 +517,9 @@ class BookmarkControllerTest { @Suppress("LongParameterList") private fun createController( - loadBookmarkNode: suspend (String) -> BookmarkNode? = { _ -> null }, + loadBookmarkNode: suspend (String, Boolean) -> BookmarkNode? = { _, _ -> null }, showSnackbar: (String) -> Unit = { _ -> }, - onOpenAllInTabsEmpty: () -> Unit = { }, + alertHeavyOpen: (Int, () -> Unit) -> Unit = { _: Int, _: () -> Unit -> }, deleteBookmarkNodes: (Set, BookmarkRemoveType) -> Unit = { _, _ -> }, deleteBookmarkFolder: (Set) -> Unit = { _ -> }, showTabTray: () -> Unit = { }, @@ -526,7 +534,7 @@ class BookmarkControllerTest { tabsUseCases = tabsUseCases, loadBookmarkNode = loadBookmarkNode, showSnackbar = showSnackbar, - onOpenAllInTabsEmpty = onOpenAllInTabsEmpty, + alertHeavyOpen = alertHeavyOpen, deleteBookmarkNodes = deleteBookmarkNodes, deleteBookmarkFolder = deleteBookmarkFolder, showTabTray = showTabTray, diff --git a/detekt-baseline.xml b/detekt-baseline.xml index 7117a6d97..f3ba36a31 100644 --- a/detekt-baseline.xml +++ b/detekt-baseline.xml @@ -547,11 +547,12 @@ UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleBookmarkTapped(item: BookmarkNode) UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleCopyUrl(item: BookmarkNode) UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode) + UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleBookmarkFolderOpening(folder: BookmarkNode, mode: BrowsingMode) UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleRequestSync() UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleSearch() UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleSelectionModeSwitch() UndocumentedPublicFunction:BookmarkFragmentStore.kt$operator fun BookmarkNode.contains(item: BookmarkNode): Boolean - UndocumentedPublicFunction:BookmarkItemMenu.kt$BookmarkItemMenu$fun updateMenu(itemType: BookmarkNodeType) + UndocumentedPublicFunction:BookmarkItemMenu.kt$BookmarkItemMenu$suspend fun updateMenu(itemType: BookmarkNodeType, itemId: String) UndocumentedPublicFunction:BookmarkNodeViewHolder.kt$BookmarkNodeViewHolder$fun bind( item: BookmarkNode, mode: BookmarkFragmentState.Mode, payload: BookmarkPayload, ) UndocumentedPublicFunction:BookmarkSearchController.kt$BookmarkSearchController$fun handleEditingCancelled() UndocumentedPublicFunction:BookmarkSearchController.kt$BookmarkSearchController$fun handleTextChanged(text: String)