Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Commit

Permalink
For #11404 - Add 'Open all' optionst in bookmarks
Browse files Browse the repository at this point in the history
* Add "Open all in new tabs" options in bookmarks
* Add "Open all in private tabs" options in bookmarks
* Add metrics tracking if the usage of "Open all in..." in bookmarks

Co-authored-by: Pg <pg.developper.fr@gmail.com>
  • Loading branch information
rocketsroger and Taknok committed Sep 23, 2022
1 parent a2011e2 commit 45752c9
Show file tree
Hide file tree
Showing 17 changed files with 567 additions and 68 deletions.
32 changes: 32 additions & 0 deletions app/metrics.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2460,6 +2460,22 @@ bookmarks_management:
metadata:
tags:
- Bookmarks
open_all_in_new_tabs:
type: event
description: |
A user opened a folder of bookmarks at once in new tabs.
bugs:
- https://github.com/mozilla-mobile/fenix/issues/11404
data_reviews:
- TBD
data_sensitivity:
- interaction
notification_emails:
- android-probes@mozilla.com
expires: 120
metadata:
tags:
- Bookmarks
open_in_private_tab:
type: event
description: |
Expand Down Expand Up @@ -2504,6 +2520,22 @@ bookmarks_management:
metadata:
tags:
- Bookmarks
open_all_in_private_tabs:
type: event
description: |
A user opened a folder of bookmarks at once in new private tabs.
bugs:
- https://github.com/mozilla-mobile/fenix/issues/11404
data_reviews:
- TBD
data_sensitivity:
- interaction
notification_emails:
- android-probes@mozilla.com
expires: 120
metadata:
tags:
- Bookmarks
edited:
type: event
description: |
Expand Down
90 changes: 90 additions & 0 deletions app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,96 @@ class BookmarksTest {
}
}

@Test
fun openAllInTabsTest() {
val nbPages = 4
val webPages = List(nbPages) {
TestAssetHelper.getGenericAsset(mockWebServer, it + 1)
}

homeScreen {
}.openThreeDotMenu {
}.openBookmarks {
createFolder("root")
createFolder("sub", "root")
createFolder("empty", "root")
}.closeMenu {
}

browserScreen {
createBookmark(webPages[0].url, "root")
createBookmark(webPages[1].url, "root")
createBookmark(webPages[2].url, "sub")
// out of folder and should not be opened
createBookmark(webPages[3].url)
}.openTabDrawer {
closeTab()
}

browserScreen {
}.openThreeDotMenu {
}.openBookmarks {
}.openThreeDotMenu("root") {
}.clickOpenAllInTabs {
verifyTabTrayIsOpened()
verifyNormalModeSelected()

verifyExistingOpenTabs("Test_Page_1")
verifyExistingOpenTabs("Test_Page_2")
verifyExistingOpenTabs("Test_Page_3")
swipeTabRight("Test_Page_1")
swipeTabRight("Test_Page_2")
swipeTabRight("Test_Page_3")
// no more tabs should be presents and auto close tab tray
verifyTabTrayIsClosed()
}
}

@Test
fun openAllInPrivateTabsTest() {
val nbPages = 4
val webPages = List(nbPages) {
TestAssetHelper.getGenericAsset(mockWebServer, it + 1)
}

homeScreen {
}.openThreeDotMenu {
}.openBookmarks {
createFolder("root")
createFolder("sub", "root")
createFolder("empty", "root")
}.closeMenu {
}

browserScreen {
createBookmark(webPages[0].url, "root")
createBookmark(webPages[1].url, "root")
createBookmark(webPages[2].url, "sub")
// out of folder and should not be opened
createBookmark(webPages[3].url)
}.openTabDrawer {
closeTab()
}

browserScreen {
}.openThreeDotMenu {
}.openBookmarks {
}.openThreeDotMenu("root") {
}.clickOpenAllInPrivateTabs {
verifyTabTrayIsOpened()
verifyPrivateModeSelected()

verifyExistingOpenTabs("Test_Page_1")
verifyExistingOpenTabs("Test_Page_2")
verifyExistingOpenTabs("Test_Page_3")
swipeTabRight("Test_Page_1")
swipeTabRight("Test_Page_2")
swipeTabRight("Test_Page_3")
// no more tabs should be presents and auto close tab tray
verifyTabTrayIsClosed()
}
}

@Test
fun openBookmarkInPrivateTabTest() {
val defaultWebPage = TestAssetHelper.getGenericAsset(mockWebServer, 1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,12 +150,21 @@ class BookmarksRobot {
.click()
}

fun createFolder(name: String) {
fun createFolder(name: String, parent: String? = null) {
clickAddFolderButton()
addNewFolderName(name)
if (!parent.isNullOrBlank()) {
setParentFolder(parent)
}
saveNewFolder()
}

fun setParentFolder(parentName: String) {
clickParentFolderSelector()
selectFolder(parentName)
navigateUp()
}

fun clickAddFolderButton() {
mDevice.waitNotNull(
Until.findObject(By.desc("Add folder")),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,13 +334,21 @@ class BrowserRobot {
menuSaveImage.click()
}

fun createBookmark(url: Uri) {
fun createBookmark(url: Uri, folder: String? = null) {
navigationToolbar {
}.enterURLAndEnterToBrowser(url) {
// needs to wait for the right url to load before saving a bookmark
verifyUrl(url.toString())
}.openThreeDotMenu {
}.bookmarkPage { }
}.bookmarkPage {
// continue only if a folder is defined
}.takeIf { !folder.isNullOrBlank() }?.let {
it.openThreeDotMenu {
}.editBookmarkPage {
setParentFolder(folder!!)
saveEditBookmark()
}
}
}

fun clickLinkMatchingText(expectedText: String) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,20 @@ class ThreeDotMenuBookmarksRobot {
return TabDrawerRobot.Transition()
}

fun clickOpenAllInTabs(interact: TabDrawerRobot.() -> Unit): TabDrawerRobot.Transition {
openAllInTabsButton().click()

TabDrawerRobot().interact()
return TabDrawerRobot.Transition()
}

fun clickOpenAllInPrivateTabs(interact: TabDrawerRobot.() -> Unit): TabDrawerRobot.Transition {
openAllInPrivateTabsButton().click()

TabDrawerRobot().interact()
return TabDrawerRobot.Transition()
}

fun clickDelete(interact: BookmarksRobot.() -> Unit): BookmarksRobot.Transition {
deleteButton().click()

Expand All @@ -71,4 +85,8 @@ private fun openInNewTabButton() = onView(withText("Open in new tab"))

private fun openInPrivateTabButton() = onView(withText("Open in private tab"))

private fun openAllInTabsButton() = onView(withText("Open all in new tabs"))

private fun openAllInPrivateTabsButton() = onView(withText("Open all in private tabs"))

private fun deleteButton() = onView(withText("Delete"))
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,14 @@ class ThreeDotMenuMainRobot {
return BrowserRobot.Transition()
}

fun editBookmarkPage(interact: BookmarksRobot.() -> Unit): BookmarksRobot.Transition {
mDevice.waitNotNull(Until.findObject(By.text("Bookmarks")), waitingTime)
editBookmarkButton().click()

BookmarksRobot().interact()
return BookmarksRobot.Transition()
}

fun openHelp(interact: BrowserRobot.() -> Unit): BrowserRobot.Transition {
mDevice.waitNotNull(Until.findObject(By.text("Help")), waitingTime)
helpButton().click()
Expand Down
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 All @@ -15,6 +16,7 @@ import mozilla.appservices.places.BookmarkRoot
import mozilla.components.concept.engine.EngineSession
import mozilla.components.concept.engine.prompt.ShareData
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.concept.storage.BookmarkNodeType
import mozilla.components.feature.tabs.TabsUseCases
import mozilla.components.service.fxa.sync.SyncReason
import org.mozilla.fenix.BrowserDirection
Expand Down Expand Up @@ -44,6 +46,7 @@ interface BookmarkController {
fun handleCopyUrl(item: BookmarkNode)
fun handleBookmarkSharing(item: BookmarkNode)
fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode)
fun handleBookmarkFolderOpening(folder: BookmarkNode, mode: BrowsingMode)

/**
* Handle bookmark nodes deletion
Expand Down Expand Up @@ -73,8 +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 alertHeavyOpen: (Int, () -> Unit) -> Unit,
private val deleteBookmarkNodes: (Set<BookmarkNode>, BookmarkRemoveType) -> Unit,
private val deleteBookmarkFolder: (Set<BookmarkNode>) -> Unit,
private val showTabTray: () -> Unit,
Expand All @@ -83,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 @@ -105,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 @@ -158,6 +166,59 @@ class DefaultBookmarkController(
showTabTray()
}

private fun extractURLsFromTree(node: BookmarkNode): MutableList<String> {
val urls = mutableListOf<String>()

when (node.type) {
BookmarkNodeType.FOLDER -> {
// if not leaf (empty) folder
if (!node.children.isNullOrEmpty()) {
node.children!!.iterator().forEach {
urls.addAll(extractURLsFromTree(it))
}
}
}
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 {
// 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)
}
}
}

override fun handleBookmarkDeletion(nodes: Set<BookmarkNode>, removeType: BookmarkRemoveType) {
deleteBookmarkNodes(nodes, removeType)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
tabsUseCases = activity?.components?.useCases?.tabsUseCases,
loadBookmarkNode = ::loadBookmarkNode,
showSnackbar = ::showSnackBarWithText,
alertHeavyOpen = ::alertHeavyOpen,
deleteBookmarkNodes = ::deleteMulti,
deleteBookmarkFolder = ::showRemoveFolderDialog,
showTabTray = ::showTabTray,
Expand Down Expand Up @@ -271,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 All @@ -292,6 +293,27 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
}
}

private fun alertHeavyOpen(n: Int, function: () -> (Unit)) {
AlertDialog.Builder(requireContext()).apply {
setTitle(R.string.open_all_warning_title)
setMessage(String.format(context.getString(R.string.open_all_warning_message), n))
setPositiveButton(
R.string.open_all_warning_confirm,
) { dialog, _ ->
function()
dialog.dismiss()
}
setNegativeButton(
R.string.open_all_warning_cancel,
) { dialog: DialogInterface, _ ->
dialog.dismiss()
}
setCancelable(false)
create()
show()
}
}

private fun deleteMulti(
selected: Set<BookmarkNode>,
eventType: BookmarkRemoveType = BookmarkRemoveType.MULTIPLE,
Expand Down
Loading

0 comments on commit 45752c9

Please sign in to comment.