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 7226d03ad..b46307e15 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,7 +7,6 @@ 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 @@ -76,9 +75,9 @@ class DefaultBookmarkController( private val store: BookmarkFragmentStore, private val sharedViewModel: BookmarksSharedViewModel, private val tabsUseCases: TabsUseCases?, - private val loadBookmarkNode: suspend (String, Boolean) -> BookmarkNode?, + private val loadBookmarkNode: suspend (String) -> BookmarkNode?, private val showSnackbar: (String) -> Unit, - private val alertHeavyOpen: (Int, () -> Unit) -> Unit, + private val onOpenAllInTabsEmpty: () -> Unit, private val deleteBookmarkNodes: (Set, BookmarkRemoveType) -> Unit, private val deleteBookmarkFolder: (Set) -> Unit, private val showTabTray: () -> Unit, @@ -87,10 +86,6 @@ 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)) @@ -113,7 +108,7 @@ class DefaultBookmarkController( override fun handleBookmarkExpand(folder: BookmarkNode) { handleAllBookmarksDeselected() scope.launch { - val node = loadBookmarkNode.invoke(folder.guid, false) ?: return@launch + val node = loadBookmarkNode.invoke(folder.guid) ?: return@launch sharedViewModel.selectedFolder = node store.dispatch(BookmarkFragmentAction.Change(node)) } @@ -166,56 +161,32 @@ class DefaultBookmarkController( showTabTray() } - private fun extractURLsFromTree(node: BookmarkNode): MutableList { - val urls = mutableListOf() + private suspend fun recursiveBookmarkFolderOpening(folder: BookmarkNode, firstLaunch: Boolean = false) { + val node = loadBookmarkNode.invoke(folder.guid) ?: return + if (!node.children.isNullOrEmpty()) { + if (firstLaunch) invokePendingDeletion.invoke() - when (node.type) { - BookmarkNodeType.FOLDER -> { - // if not leaf (empty) folder - if (!node.children.isNullOrEmpty()) { - node.children!!.iterator().forEach { - urls.addAll(extractURLsFromTree(it)) + 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)) } } + BookmarkNodeType.SEPARATOR -> {} + } + }.also { + if (firstLaunch) { + activity.browsingModeManager.mode = BrowsingMode.fromBoolean(mode == BrowsingMode.Private) + showTabTray() } } - BookmarkNodeType.ITEM -> { - node.url?.let { urls.add(it) } - } - BookmarkNodeType.SEPARATOR -> {} - } - - return urls + } else if (firstLaunch) onOpenAllInTabsEmpty.invoke() } override fun handleBookmarkFolderOpening(folder: BookmarkNode, mode: BrowsingMode) { // potentially heavy function with a lot of bookmarks to open => use a coroutine scope.launch { - // 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) - } + recursiveBookmarkFolderOpening(folder, true, mode) } } 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 83799dbcd..48a778f52 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, - alertHeavyOpen = ::alertHeavyOpen, + onOpenAllInTabsEmpty = ::onOpenAllInTabsEmpty, deleteBookmarkNodes = ::deleteMulti, deleteBookmarkFolder = ::showRemoveFolderDialog, showTabTray = ::showTabTray, @@ -272,11 +272,11 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan return bookmarkView.onBackPressed() } - private suspend fun loadBookmarkNode(guid: String, recursive: Boolean = false): BookmarkNode? = withContext(IO) { + private suspend fun loadBookmarkNode(guid: String): BookmarkNode? = withContext(IO) { // Only runs if the fragment is attached same as [runIfFragmentIsAttached] context?.let { requireContext().bookmarkStorage - .getTree(guid, recursive) + .getTree(guid, false) ?.let { desktopFolders.withOptionalDesktopFolders(it) } } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index c6ebd10ef..506489061 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -620,15 +620,6 @@ 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 195c7ec04..535365735 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,11 +17,9 @@ 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 @@ -195,7 +193,7 @@ class BookmarkControllerTest { fun `handleBookmarkExpand should refresh and change the active bookmark node`() = runTestOnMain { var loadBookmarkNodeInvoked = false createController( - loadBookmarkNode = { _: String, _: Boolean -> + loadBookmarkNode = { loadBookmarkNodeInvoked = true tree }, @@ -367,7 +365,7 @@ class BookmarkControllerTest { showTabTray = { showTabTrayInvoked = true }, - loadBookmarkNode = { guid: String, _: Boolean -> + loadBookmarkNode = { fun recurseFind(item: BookmarkNode, guid: String): BookmarkNode? { if (item.guid == guid) { return item @@ -381,7 +379,7 @@ class BookmarkControllerTest { return null } } - recurseFind(tree, guid) + recurseFind(tree, it) }, ).handleBookmarkFolderOpening(tree, BrowsingMode.Normal) @@ -401,7 +399,7 @@ class BookmarkControllerTest { showTabTray = { showTabTrayInvoked = true }, - loadBookmarkNode = { guid: String, _: Boolean -> + loadBookmarkNode = { fun recurseFind(item: BookmarkNode, guid: String): BookmarkNode? { if (item.guid == guid) { return item @@ -415,7 +413,7 @@ class BookmarkControllerTest { return null } } - recurseFind(tree, guid) + recurseFind(tree, it) }, ).handleBookmarkFolderOpening(tree, BrowsingMode.Private) @@ -429,24 +427,18 @@ class BookmarkControllerTest { } @Test - 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 - + fun `handleBookmarkFolderOpening for an empty folder should show toast`() { + var onOpenAllInTabsEmptyInvoked = false createController( - alertHeavyOpen = { _: Int, _: () -> Unit -> alertHeavyOpenInvoked = true }, - loadBookmarkNode = { _: String, _: Boolean -> - tree + onOpenAllInTabsEmpty = { + onOpenAllInTabsEmptyInvoked = true }, - ).handleBookmarkFolderOpening(tree, BrowsingMode.Normal) - - unmockkConstructor(DefaultBookmarkController::class) + loadBookmarkNode = { + subfolder + }, + ).handleBookmarkFolderOpening(subfolder, BrowsingMode.Normal) - assertTrue(alertHeavyOpenInvoked) + assertTrue(onOpenAllInTabsEmptyInvoked) } @Test @@ -517,9 +509,9 @@ class BookmarkControllerTest { @Suppress("LongParameterList") private fun createController( - loadBookmarkNode: suspend (String, Boolean) -> BookmarkNode? = { _, _ -> null }, + loadBookmarkNode: suspend (String) -> BookmarkNode? = { _ -> null }, showSnackbar: (String) -> Unit = { _ -> }, - alertHeavyOpen: (Int, () -> Unit) -> Unit = { _: Int, _: () -> Unit -> }, + onOpenAllInTabsEmpty: () -> Unit = { }, deleteBookmarkNodes: (Set, BookmarkRemoveType) -> Unit = { _, _ -> }, deleteBookmarkFolder: (Set) -> Unit = { _ -> }, showTabTray: () -> Unit = { }, @@ -534,7 +526,7 @@ class BookmarkControllerTest { tabsUseCases = tabsUseCases, loadBookmarkNode = loadBookmarkNode, showSnackbar = showSnackbar, - alertHeavyOpen = alertHeavyOpen, + onOpenAllInTabsEmpty = onOpenAllInTabsEmpty, deleteBookmarkNodes = deleteBookmarkNodes, deleteBookmarkFolder = deleteBookmarkFolder, showTabTray = showTabTray, diff --git a/detekt-baseline.xml b/detekt-baseline.xml index f3ba36a31..7117a6d97 100644 --- a/detekt-baseline.xml +++ b/detekt-baseline.xml @@ -547,12 +547,11 @@ 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$suspend fun updateMenu(itemType: BookmarkNodeType, itemId: String) + UndocumentedPublicFunction:BookmarkItemMenu.kt$BookmarkItemMenu$fun updateMenu(itemType: BookmarkNodeType) 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)