Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Commit

Permalink
[fenix] For mozilla-mobile/fenix#11404 - Create alert message when a …
Browse files Browse the repository at this point in the history 
…lot of tabs are to be opened.

Current threshold is 15.
  • Loading branch information
@Taknok @rvandermeulen
Taknok authored and rvandermeulen committed Sep 19, 2022
1 parent 83ed1ab commit 8517fea
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 41 deletions.
69 changes: 49 additions & 20 deletions fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<BookmarkNode>, BookmarkRemoveType) -> Unit,
private val deleteBookmarkFolder: (Set<BookmarkNode>) -> Unit,
private val showTabTray: () -> Unit,
Expand All @@ -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))
Expand All @@ -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))
}
Expand Down Expand Up @@ -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<String> {
val urls = mutableListOf<String>()

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)
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
tabsUseCases = activity?.components?.useCases?.tabsUseCases,
loadBookmarkNode = ::loadBookmarkNode,
showSnackbar = ::showSnackBarWithText,
onOpenAllInTabsEmpty = ::onOpenAllInTabsEmpty,
alertHeavyOpen = ::alertHeavyOpen,
deleteBookmarkNodes = ::deleteMulti,
deleteBookmarkFolder = ::showRemoveFolderDialog,
showTabTray = ::showTabTray,
Expand Down Expand Up @@ -272,11 +272,11 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), 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) }
}
}
Expand Down
9 changes: 9 additions & 0 deletions fenix/app/src/main/res/values/strings.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,15 @@
<!-- Content description (not visible, for screen readers etc.): "Close button for library settings" -->
<string name="content_description_close_button">Close</string>

<!-- Title to show in alert when a lot of tabs are to be opened -->
<string name="open_all_warning_title">Confirm open</string>
<!-- Message to show in alert when a lot of tabs are to be opened -->
<string name="open_all_warning_message">You are about to open %d tabs. This might slow down Firefox. Are you sure you want to continue ?</string>
<!-- Dialog button text for confirming open all tabs -->
<string name="open_all_warning_confirm">Open tabs</string>
<!-- Dialog button text for canceling open all tabs -->
<string name="open_all_warning_cancel">Cancel</string>

<!-- Text to show users they have one site in the history group section of the History fragment.
%d is a placeholder for the number of sites in the group. -->
<string name="history_search_group_site">%d site</string>
Expand Down
42 changes: 25 additions & 17 deletions fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
},
Expand Down Expand Up @@ -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
Expand All @@ -379,7 +381,7 @@ class BookmarkControllerTest {
return null
}
}
recurseFind(tree, it)
recurseFind(tree, guid)
},
).handleBookmarkFolderOpening(tree, BrowsingMode.Normal)

Expand All @@ -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
Expand All @@ -413,7 +415,7 @@ class BookmarkControllerTest {
return null
}
}
recurseFind(tree, it)
recurseFind(tree, guid)
},
).handleBookmarkFolderOpening(tree, BrowsingMode.Private)

Expand All @@ -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<DefaultBookmarkController>() 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
Expand Down Expand Up @@ -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<BookmarkNode>, BookmarkRemoveType) -> Unit = { _, _ -> },
deleteBookmarkFolder: (Set<BookmarkNode>) -> Unit = { _ -> },
showTabTray: () -> Unit = { },
Expand All @@ -526,7 +534,7 @@ class BookmarkControllerTest {
tabsUseCases = tabsUseCases,
loadBookmarkNode = loadBookmarkNode,
showSnackbar = showSnackbar,
onOpenAllInTabsEmpty = onOpenAllInTabsEmpty,
alertHeavyOpen = alertHeavyOpen,
deleteBookmarkNodes = deleteBookmarkNodes,
deleteBookmarkFolder = deleteBookmarkFolder,
showTabTray = showTabTray,
Expand Down
3 changes: 2 additions & 1 deletion fenix/detekt-baseline.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -547,11 +547,12 @@
<ID>UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleBookmarkTapped(item: BookmarkNode)</ID>
<ID>UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleCopyUrl(item: BookmarkNode)</ID>
<ID>UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode)</ID>
<ID>UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleBookmarkFolderOpening(folder: BookmarkNode, mode: BrowsingMode)</ID>
<ID>UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleRequestSync()</ID>
<ID>UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleSearch()</ID>
<ID>UndocumentedPublicFunction:BookmarkController.kt$BookmarkController$fun handleSelectionModeSwitch()</ID>
<ID>UndocumentedPublicFunction:BookmarkFragmentStore.kt$operator fun BookmarkNode.contains(item: BookmarkNode): Boolean</ID>
<ID>UndocumentedPublicFunction:BookmarkItemMenu.kt$BookmarkItemMenu$fun updateMenu(itemType: BookmarkNodeType)</ID>
<ID>UndocumentedPublicFunction:BookmarkItemMenu.kt$BookmarkItemMenu$suspend fun updateMenu(itemType: BookmarkNodeType, itemId: String)</ID>
<ID>UndocumentedPublicFunction:BookmarkNodeViewHolder.kt$BookmarkNodeViewHolder$fun bind( item: BookmarkNode, mode: BookmarkFragmentState.Mode, payload: BookmarkPayload, )</ID>
<ID>UndocumentedPublicFunction:BookmarkSearchController.kt$BookmarkSearchController$fun handleEditingCancelled()</ID>
<ID>UndocumentedPublicFunction:BookmarkSearchController.kt$BookmarkSearchController$fun handleTextChanged(text: String)</ID>
Expand Down

0 comments on commit 8517fea

Please sign in to comment.