Skip to content

Commit

Permalink
For mozilla-mobile#11404 - Add 'Open all' options 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 Oct 11, 2022
1 parent 8ed3bf7 commit 6976d1e
Show file tree
Hide file tree
Showing 18 changed files with 519 additions and 29 deletions.
32 changes: 32 additions & 0 deletions app/metrics.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2476,6 +2476,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:
- https://github.com/mozilla-mobile/fenix/pull/27138
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 @@ -2520,6 +2536,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:
- https://github.com/mozilla-mobile/fenix/pull/27138
data_sensitivity:
- interaction
notification_emails:
- android-probes@mozilla.com
expires: 120
metadata:
tags:
- Bookmarks
edited:
type: event
description: |
Expand Down
77 changes: 77 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 @@ -260,6 +260,83 @@ class BookmarksTest {
}
}

@Test
fun openAllInTabsTest() {
val webPages = listOf(
TestAssetHelper.getGenericAsset(mockWebServer, 1),
TestAssetHelper.getGenericAsset(mockWebServer, 2),
TestAssetHelper.getGenericAsset(mockWebServer, 3),
TestAssetHelper.getGenericAsset(mockWebServer, 4),
)

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

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

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

verifyExistingOpenTabs("Test_Page_2", "Test_Page_3", "Test_Page_4")
verifyNoExistingOpenTabs("Test_Page_1")
}
}

@Test
fun openAllInPrivateTabsTest() {
val webPages = listOf(
TestAssetHelper.getGenericAsset(mockWebServer, 1),
TestAssetHelper.getGenericAsset(mockWebServer, 2),
)

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

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

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

verifyExistingOpenTabs("Test_Page_1", "Test_Page_2")
}
}

@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 @@ -33,6 +33,7 @@ import androidx.test.uiautomator.Until
import androidx.test.uiautomator.Until.findObject
import com.google.android.material.bottomsheet.BottomSheetBehavior
import junit.framework.AssertionFailedError
import junit.framework.TestCase.assertFalse
import junit.framework.TestCase.assertTrue
import org.hamcrest.CoreMatchers.allOf
import org.hamcrest.CoreMatchers.anyOf
Expand Down Expand Up @@ -78,6 +79,7 @@ class TabDrawerRobot {
assertSyncedTabsButtonIsSelected(isSelected)

fun verifyExistingOpenTabs(vararg titles: String) = assertExistingOpenTabs(*titles)
fun verifyNoExistingOpenTabs(vararg titles: String) = assertNoExistingOpenTabs(*titles)
fun verifyCloseTabsButton(title: String) = assertCloseTabsButton(title)

fun verifyExistingTabList() = assertExistingTabList()
Expand Down Expand Up @@ -490,6 +492,14 @@ private fun assertExistingOpenTabs(vararg tabTitles: String) {
}
}

private fun assertNoExistingOpenTabs(vararg tabTitles: String) {
for (title in tabTitles) {
assertFalse(
tabItem(title).waitForExists(waitingTimeLong),
)
}
}

private fun assertExistingTabList() {
mDevice.findObject(
UiSelector().resourceId("$packageName:id/tabsTray"),
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 All @@ -27,6 +29,9 @@ import org.mozilla.fenix.ext.nav
import org.mozilla.fenix.ext.navigateSafe
import org.mozilla.fenix.utils.Settings

@VisibleForTesting
internal const val WARN_OPEN_ALL_SIZE = 15

/**
* [BookmarkFragment] controller.
* Delegated by View Interactors, handles container business logic and operates changes on it.
Expand All @@ -44,6 +49,7 @@ interface BookmarkController {
fun handleCopyUrl(item: BookmarkNode)
fun handleBookmarkSharing(item: BookmarkNode)
fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode)
fun handleOpeningFolderBookmarks(folder: BookmarkNode, mode: BrowsingMode)

/**
* Handle bookmark nodes deletion
Expand Down Expand Up @@ -73,11 +79,12 @@ 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 deleteBookmarkNodes: (Set<BookmarkNode>, BookmarkRemoveType) -> Unit,
private val deleteBookmarkFolder: (Set<BookmarkNode>) -> Unit,
private val showTabTray: () -> Unit,
private val warnLargeOpenAll: (Int, () -> Unit) -> Unit,
private val settings: Settings,
) : BookmarkController {

Expand Down Expand Up @@ -105,7 +112,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 +165,57 @@ class DefaultBookmarkController(
showTabTray()
}

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

when (node.type) {
BookmarkNodeType.FOLDER -> {
// if not leaf (empty) folder
node.children?.forEach {
urls.addAll(extractURLsFromTree(it))
}
}
BookmarkNodeType.ITEM -> {
node.url?.let { urls.add(it) }
}
BookmarkNodeType.SEPARATOR -> {}
}

return urls
}

override fun handleOpeningFolderBookmarks(folder: BookmarkNode, mode: BrowsingMode) {
// potentially heavy function with a lot of bookmarks to open => use a coroutine
scope.launch {
// if more than maximum 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 maximum elements
if (urls.size >= WARN_OPEN_ALL_SIZE) {
warnLargeOpenAll(urls.size) {
// do not load bookmarks when more than maximum elements
openAll(false)
}
} else {
openAll(true)
}
}
}

override fun handleBookmarkDeletion(nodes: Set<BookmarkNode>, removeType: BookmarkRemoveType) {
deleteBookmarkNodes(nodes, removeType)
}
Expand Down
Loading

0 comments on commit 6976d1e

Please sign in to comment.