commit 868e65dcf9ea4be097be1dde80fcb952886031a6 parent 6e1be6241390b4b4e3524f607afd300fd8309705 Author: Matthew Tighe <matthewdtighe@gmail.com> Date: Tue, 6 Jan 2026 20:59:34 +0000 Bug 1988315 - convert select screen to use expandable chevrons and load bookmarks lazily r=android-reviewers,boek,android-l10n-reviewers,flod Differential Revision: https://phabricator.services.mozilla.com/D276746 Diffstat:
11 files changed, 692 insertions(+), 195 deletions(-)
diff --git a/mobile/android/fenix/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt b/mobile/android/fenix/app/src/androidTest/java/org/mozilla/fenix/ui/BookmarksTest.kt @@ -64,6 +64,7 @@ class BookmarksTest : TestSetup() { }.openThreeDotMenu("Test_Page_1") { }.clickEdit { clickParentFolderSelector() + expandSelectableFolder("Bookmarks") selectFolder(bookmarkFolderName) navigateUp() saveEditBookmark() @@ -72,6 +73,7 @@ class BookmarksTest : TestSetup() { }.openThreeDotMenu("My Folder 2") { }.clickEdit { clickParentFolderSelector() + expandSelectableFolder("Bookmarks") selectFolder(bookmarkFolderName) navigateUp() saveEditBookmark() diff --git a/mobile/android/fenix/app/src/androidTest/java/org/mozilla/fenix/ui/robots/BookmarksRobot.kt b/mobile/android/fenix/app/src/androidTest/java/org/mozilla/fenix/ui/robots/BookmarksRobot.kt @@ -209,6 +209,18 @@ class BookmarksRobot(private val composeTestRule: ComposeTestRule) { Log.i(TAG, "clickParentFolderSelector: Clicked folder selector") } + fun expandSelectableFolder(title: String) { + Log.i(TAG, "expandSelectableFolder: Trying to click expand select folder selector") + composeTestRule.expandBookmarkFolderSelector(title).performClick() + Log.i(TAG, "expandSelectableFolder: Clicked expand select folder selector") + } + + fun closeSelectableFolder(title: String) { + Log.i(TAG, "closeSelectableFolder: Trying to click close select folder selector") + composeTestRule.expandBookmarkFolderSelector(title).performClick() + Log.i(TAG, "closeSelectableFolder: Clicked close select folder selector") + } + fun selectFolder(title: String) { Log.i(TAG, "selectFolder: Trying to click folder with title: $title") composeTestRule.onNodeWithText(title).performClick() @@ -315,6 +327,12 @@ private fun ComposeTestRule.bookmarkNameEditBox() = private fun ComposeTestRule.bookmarkFolderSelector() = onNodeWithText("Bookmarks") +private fun ComposeTestRule.expandBookmarkFolderSelector(title: String) = + onNodeWithContentDescription(getStringResource(R.string.bookmark_select_folder_expand_folder_content_description, title)) + +private fun ComposeTestRule.closeBookmarkFolderSelector(title: String) = + onNodeWithContentDescription(getStringResource(R.string.bookmark_select_folder_close_folder_content_description, title)) + private fun ComposeTestRule.bookmarkURLEditBox() = onNodeWithTag(EDIT_BOOKMARK_ITEM_URL_TEXT_FIELD) diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksAction.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksAction.kt @@ -118,6 +118,8 @@ internal sealed class SelectFolderAction : BookmarksAction { data object ViewAppeared : SelectFolderAction() data class FoldersLoaded(val folders: List<SelectFolderItem>) : SelectFolderAction() data class FilteredFoldersLoaded(val folders: List<SelectFolderItem>) : SelectFolderAction() + data class ExpandedFolderLoaded(val folder: SelectFolderItem) : SelectFolderAction() + data class ChevronClicked(val folder: SelectFolderItem) : SelectFolderAction() data class ItemClicked(val folder: SelectFolderItem) : SelectFolderAction() data object SearchClicked : SelectFolderAction() data object SearchDismissed : SelectFolderAction() diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksMiddleware.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksMiddleware.kt @@ -52,7 +52,7 @@ private const val WARN_OPEN_ALL_SIZE = 15 * feature goes out of scope. * @param ioDispatcher Coroutine dispatcher for IO operations. */ -@Suppress("LongParameterList") +@Suppress("LongParameterList", "LargeClass") internal class BookmarksMiddleware( private val bookmarksStorage: BookmarksStorage, private val clipboardManager: ClipboardManager?, @@ -163,9 +163,10 @@ internal class BookmarksMiddleware( scope.launch(ioDispatcher) { val newFolderTitle = preReductionState.bookmarksAddFolderState.folderBeingAddedTitle + val parentGuid = preReductionState.bookmarksAddFolderState.parent.guid if (newFolderTitle.isNotEmpty()) { val guid = bookmarksStorage.addFolder( - parentGuid = preReductionState.bookmarksAddFolderState.parent.guid, + parentGuid = parentGuid, title = newFolderTitle, ).getOrElse { reportResultGlobally(BookmarksGlobalResultReport.AddFolderFailed) @@ -181,12 +182,30 @@ internal class BookmarksMiddleware( store.dispatch(AddFolderAction.FolderCreated(folder)) + // if we are in the middle of moving items, we consider the end of the + // add folder workflow to be terminal, and finish moving the items + // into the newly created folder + preReductionState.createMovePairs()?.forEach { + val result = bookmarksStorage.updateNode( + it.first, + it.second.copy(parentGuid = guid), + ) + if (result.isFailure) { + reportResultGlobally(BookmarksGlobalResultReport.SelectFolderFailed) + } + } + withContext(Dispatchers.Main) { - if (preReductionState.bookmarksSelectFolderState != null) { + if (preReductionState.bookmarksEditBookmarkState != null) { getNavController().popBackStack( BookmarksDestinations.EDIT_BOOKMARK, inclusive = false, ) + } else if (preReductionState.bookmarksSelectFolderState != null) { + getNavController().popBackStack( + BookmarksDestinations.LIST, + false, + ) } else { getNavController().popBackStack() } @@ -303,8 +322,16 @@ internal class BookmarksMiddleware( -> { getNavController().navigate(BookmarksDestinations.SELECT_FOLDER) } - - SelectFolderAction.ViewAppeared -> store.tryDispatchLoadFolders() + SelectFolderAction.ViewAppeared -> { + if (preReductionState.bookmarksSelectFolderState?.folders.isNullOrEmpty()) { + store.tryDispatchLoadSelectableFolders() + } + } + is SelectFolderAction.ChevronClicked -> { + if (action.folder.expansionState is SelectFolderExpansionState.Closed) { + store.tryDispatchAdditionalSelectableFolders(action.folder) + } + } is BookmarksListMenuAction -> action.handleSideEffects(store, preReductionState) SnackbarAction.Dismissed -> when (preReductionState.bookmarksSnackbarState) { is BookmarksSnackbarState.UndoDeletion -> scope.launch { @@ -375,7 +402,7 @@ internal class BookmarksMiddleware( } } is SelectFolderAction.SortMenu -> scope.launch { - store.tryDispatchLoadFolders() + store.tryDispatchLoadSelectableFolders() saveBookmarkSortOrder(store.state.sortOrder) } is SelectFolderAction.SearchQueryUpdated -> { @@ -412,6 +439,7 @@ internal class BookmarksMiddleware( is AddFolderAction.TitleChanged, is SelectFolderAction.FoldersLoaded, is SelectFolderAction.FilteredFoldersLoaded, + is SelectFolderAction.ExpandedFolderLoaded, is SelectFolderAction.ItemClicked, EditFolderAction.DeleteClicked, is ReceivedSyncSignInUpdate, @@ -420,38 +448,71 @@ internal class BookmarksMiddleware( } } - private fun Store<BookmarksState, BookmarksAction>.tryDispatchLoadFolders() = + private fun Store<BookmarksState, BookmarksAction>.tryDispatchLoadSelectableFolders() = scope.launch { - val folders = if (bookmarksStorage.hasDesktopBookmarks()) { - bookmarksStorage.getTree(BookmarkRoot.Root.id, recursive = true).getOrNull()?.let { rootNode -> - val excludingMobile = - rootNode.children?.filterNot { it.guid == BookmarkRoot.Mobile.id } - val desktopRoot = rootNode.copy(children = excludingMobile) - rootNode.children?.find { it.guid == BookmarkRoot.Mobile.id }?.let { - val newChildren = listOf(desktopRoot) + it.children.orEmpty() - it.copy(children = newChildren) - }?.let { - collectFolders( - node = it, - comparator = state.sortOrder.comparator, - shouldCollect = { node -> !state.isGuidBeingMoved(node.guid) }, - ) + Result.runCatching { + if (!bookmarksStorage.hasDesktopBookmarks()) { + listOf( + loadAsSelectableFolder(guid = BookmarkRoot.Mobile.id, indentation = 0, false)!!, + ) + } else { + val rootNode = bookmarksStorage.getTree(BookmarkRoot.Root.id).getOrNull()!! + val (mobileRootNodes, desktopRootNodes) = + rootNode.children!!.partition { it.guid == BookmarkRoot.Mobile.id } + // there should only be one of these + val mobileNode = mobileRootNodes.first() + + // we want to order these a specific way on mobile + (listOf(mobileNode, rootNode) + desktopRootNodes).mapNotNull { item -> + loadAsSelectableFolder(guid = item.guid, indentation = 0, false) } } - } else { - bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = true).getOrNull() - ?.let { - collectFolders( - node = it, - comparator = state.sortOrder.comparator, - shouldCollect = { node -> !state.isGuidBeingMoved(node.guid) }, - ) - } + }.onSuccess { folders -> + dispatch(SelectFolderAction.FoldersLoaded(folders)) } + } - folders?.also { dispatch(SelectFolderAction.FoldersLoaded(it)) } + private fun Store<BookmarksState, BookmarksAction>.tryDispatchAdditionalSelectableFolders( + folder: SelectFolderItem, + ) = scope.launch { + loadAsSelectableFolder(folder.guid, folder.indentation, true)?.let { + dispatch(SelectFolderAction.ExpandedFolderLoaded(it)) + } } + /** + * Load a guid and optionally its immediate children as select folder items. + */ + private suspend fun loadAsSelectableFolder( + guid: String, + indentation: Int, + shouldOpen: Boolean, + ): SelectFolderItem? = Result.runCatching { + val loadedNode = bookmarksStorage.getTree(guid).getOrNull()!! + if (loadedNode.type != BookmarkNodeType.FOLDER) return null + SelectFolderItem( + indentation = indentation, + folder = BookmarkItem.Folder( + title = resolveFolderTitle(loadedNode), + guid = loadedNode.guid, + position = loadedNode.position, + ), + expansionState = when { + // when we are expanding folders, we need to find all their children that could also be selected + shouldOpen -> SelectFolderExpansionState.Open( + children = loadedNode.children.orEmpty().mapNotNull { node -> + loadAsSelectableFolder(node.guid, indentation + 1, false) + }, + ) + // only mark folders as expandable if they have children that could potentially be selected + (loadedNode.children?.any { it.type == BookmarkNodeType.FOLDER } == true) -> { + SelectFolderExpansionState.Closed + } + else -> SelectFolderExpansionState.None + }, + ) + }.getOrNull() + private fun Store<BookmarksState, BookmarksAction>.tryDispatchLoadFor(guid: String) = scope.launch { bookmarksStorage.getTree(guid).getOrNull()?.let { rootNode -> @@ -568,36 +629,6 @@ internal class BookmarksMiddleware( return urls } - private suspend fun collectFolders( - node: BookmarkNode, - comparator: Comparator<BookmarkItem>, - indentation: Int = 0, - shouldCollect: (BookmarkNode) -> Boolean = { _ -> true }, - folders: MutableList<SelectFolderItem> = mutableListOf(), - ): List<SelectFolderItem> { - if (node.type == BookmarkNodeType.FOLDER && shouldCollect(node)) { - folders.add( - SelectFolderItem( - indentation = indentation, - folder = BookmarkItem.Folder( - guid = node.guid, - title = resolveFolderTitle(node), - position = node.position, - ), - ), - ) - - val sortedChildren = node.childItems().folders().sortedWith(comparator) - sortedChildren.forEach { child -> - val childNode = node.children!!.first { it.guid == child.guid } - val children = collectFolders(childNode, comparator, indentation + 1, shouldCollect) - folders.addAll(children) - } - } - - return folders - } - @Suppress("LongMethod") private fun BookmarksListMenuAction.handleSideEffects( store: Store<BookmarksState, BookmarksAction>, diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksReducer.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksReducer.kt @@ -94,6 +94,25 @@ private fun BookmarksState.handleOpenTabsConfirmationDialogAction( } } +private fun List<SelectFolderItem>.updateItemInTree( + guidToUpdate: String, + transform: (SelectFolderItem) -> SelectFolderItem, +): List<SelectFolderItem> = + map { + if (it.guid == guidToUpdate) { + transform(it) + } else if (it.expansionState is SelectFolderExpansionState.Open) { + it.copy( + expansionState = SelectFolderExpansionState.Open( + children = + it.expansionState.children.updateItemInTree(guidToUpdate, transform), + ), + ) + } else { + it + } + } + private fun BookmarksState.handleSelectFolderAction(action: SelectFolderAction): BookmarksState { return when (action) { is SelectFolderAction.SearchQueryUpdated -> copy( @@ -138,6 +157,26 @@ private fun BookmarksState.handleSelectFolderAction(action: SelectFolderAction): ) is SelectFolderAction.SortMenu -> this.handleSortMenuAction(action) + is SelectFolderAction.ChevronClicked -> if (action.folder.expansionState is SelectFolderExpansionState.Open) { + copy( + bookmarksSelectFolderState = bookmarksSelectFolderState?.copy( + folders = bookmarksSelectFolderState.folders.updateItemInTree( + guidToUpdate = action.folder.guid, + transform = { it.copy(expansionState = SelectFolderExpansionState.Closed) }, + ), + ) ?: this.bookmarksSelectFolderState, + ) + } else { + // we wait for additional items to load when we are expanding a folder + this + } + + is SelectFolderAction.ExpandedFolderLoaded -> copy( + bookmarksSelectFolderState = bookmarksSelectFolderState?.copy( + folders = bookmarksSelectFolderState.folders.updateItemInTree(action.folder.guid, { action.folder }), + ) ?: bookmarksSelectFolderState, + ) + SelectFolderAction.ViewAppeared -> this } } @@ -193,7 +232,6 @@ private fun BookmarksState.handleAddFolderAction(action: AddFolderAction): Bookm folder = action.folder, ), ) - is AddFolderAction.TitleChanged -> this.copy( bookmarksAddFolderState = bookmarksAddFolderState?.copy( folderBeingAddedTitle = action.updatedText, @@ -346,6 +384,13 @@ private fun BookmarksState.respondToBackClick(): BookmarksState = when { bookmarksAddFolderState != null && bookmarksEditBookmarkState != null -> { copy(bookmarksAddFolderState = null) } + bookmarksAddFolderState != null && bookmarksMultiselectMoveState != null -> { + copy( + bookmarksAddFolderState = null, + bookmarksMultiselectMoveState = null, + bookmarksSelectFolderState = null, + ) + } else -> copy( bookmarksMultiselectMoveState = null, bookmarksSelectFolderState = null, diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksScreen.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksScreen.kt @@ -125,6 +125,7 @@ import mozilla.components.lib.state.ext.observeAsComposableState import mozilla.components.lib.state.ext.observeAsState import mozilla.components.support.ktx.android.view.hideKeyboard import mozilla.telemetry.glean.private.NoExtras +import org.mozilla.fenix.Config import org.mozilla.fenix.GleanMetrics.BookmarksManagement import org.mozilla.fenix.R import org.mozilla.fenix.bookmarks.BookmarksTestTag.BOOKMARK_TOOLBAR @@ -998,13 +999,15 @@ private fun SelectFolderScreen( horizontalAlignment = Alignment.CenterHorizontally, ) { items( - items = state?.visibleFolders ?: listOf(), + key = { item -> item.folder.guid }, + items = state?.visibleFolders.orEmpty().flattenToList(), ) { folder -> FolderListItem( folder = folder, isSelected = folder.guid == state?.selectedGuid, showPadding = state?.isSearching ?: true, onClick = { store.dispatch(SelectFolderAction.ItemClicked(folder)) }, + onChevronClick = { store.dispatch(SelectFolderAction.ChevronClicked(folder)) }, ) } @@ -1080,6 +1083,7 @@ private fun FolderListItem( isSelected: Boolean, showPadding: Boolean = true, onClick: () -> Unit, + onChevronClick: () -> Unit, ) { if (folder.isDesktopRoot) { Box( @@ -1097,19 +1101,49 @@ private fun FolderListItem( } } } else { - Box(modifier = Modifier.padding(start = if (!showPadding) folder.startPadding else 0.dp)) { - SelectableIconListItem( - label = folder.title, - isSelected = isSelected, - beforeIconPainter = painterResource(iconsR.drawable.mozac_ic_folder_24), - modifier = Modifier - .width(FirefoxTheme.layout.size.containerMaxWidth) - .toggleable( - value = isSelected, - role = Role.RadioButton, - onValueChange = { onClick() }, - ), - ) + Box( + modifier = Modifier + .padding(start = if (!showPadding) folder.startPadding else 0.dp) + .width(FirefoxTheme.layout.size.containerMaxWidth), + ) { + Row(verticalAlignment = Alignment.CenterVertically) { + when (folder.expansionState) { + is SelectFolderExpansionState.None -> Spacer(modifier = Modifier.size(width = 48.dp, height = 0.dp)) + is SelectFolderExpansionState.Open -> { + IconButton(onClick = onChevronClick) { + Icon( + painter = painterResource(iconsR.drawable.mozac_ic_chevron_down_24), + contentDescription = stringResource( + R.string.bookmark_select_folder_close_folder_content_description, + folder.title, + ), + ) + } + } + is SelectFolderExpansionState.Closed -> { + IconButton(onClick = onChevronClick) { + Icon( + painter = painterResource(iconsR.drawable.mozac_ic_chevron_right_24), + contentDescription = stringResource( + R.string.bookmark_select_folder_expand_folder_content_description, + folder.title, + ), + ) + } + } + } + SelectableIconListItem( + label = folder.title, + isSelected = isSelected, + beforeIconPainter = painterResource(iconsR.drawable.mozac_ic_folder_24), + modifier = Modifier + .toggleable( + value = isSelected, + role = Role.RadioButton, + onValueChange = { onClick() }, + ), + ) + } } } } @@ -1165,15 +1199,20 @@ private fun SelectFolderTopBar(store: BookmarksStore) { SelectFolderSortOverflowMenu(store = store) } - IconButton(onClick = { - store.dispatch(SelectFolderAction.SearchClicked) - }) { - Icon( - painter = painterResource(iconsR.drawable.mozac_ic_search_24), - contentDescription = stringResource( - R.string.select_bookmark_search_button_content_description, - ), - ) + // TODO https://bugzilla.mozilla.org/show_bug.cgi?id=2006505 + if (Config.channel.isDebug) { + IconButton( + onClick = { + store.dispatch(SelectFolderAction.SearchClicked) + }, + ) { + Icon( + painter = painterResource(iconsR.drawable.mozac_ic_search_24), + contentDescription = stringResource( + R.string.select_bookmark_search_button_content_description, + ), + ) + } } if (onNewFolderClick != null) { @@ -2159,50 +2198,73 @@ private fun SelectFolderPreview() { SelectFolderItem( indentation = PREVIEW_INDENTATION_0, folder = BookmarkItem.Folder("Bookmarks", "guid0", null), + expansionState = SelectFolderExpansionState.Closed, ), SelectFolderItem( - indentation = PREVIEW_INDENTATION_2, + indentation = PREVIEW_INDENTATION_0, folder = BookmarkItem.Folder("Bookmarks Menu", BookmarkRoot.Menu.id, null), + expansionState = SelectFolderExpansionState.None, ), SelectFolderItem( - indentation = PREVIEW_INDENTATION_2, + indentation = PREVIEW_INDENTATION_0, folder = BookmarkItem.Folder("Bookmarks Toolbar", BookmarkRoot.Toolbar.id, position = null), + expansionState = SelectFolderExpansionState.None, ), SelectFolderItem( indentation = PREVIEW_INDENTATION_1, folder = BookmarkItem.Folder("Desktop Bookmarks", BookmarkRoot.Root.id, position = null), - ), - SelectFolderItem( - indentation = PREVIEW_INDENTATION_2, - folder = BookmarkItem.Folder("Bookmarks Unfiled", BookmarkRoot.Unfiled.id, position = null), - ), - SelectFolderItem( - indentation = PREVIEW_INDENTATION_1, - folder = BookmarkItem.Folder("Nested One", "guid0", position = null), - ), - SelectFolderItem( - indentation = PREVIEW_INDENTATION_2, - folder = BookmarkItem.Folder("Nested Two", "guid0", position = null), - ), - SelectFolderItem( - indentation = PREVIEW_INDENTATION_2, - folder = BookmarkItem.Folder("Nested Two", "guid0", position = null), - ), - SelectFolderItem( - indentation = PREVIEW_INDENTATION_1, - folder = BookmarkItem.Folder("Nested One", "guid0", position = null), - ), - SelectFolderItem( - indentation = PREVIEW_INDENTATION_2, - folder = BookmarkItem.Folder("Nested Two", "guid1", position = null), - ), - SelectFolderItem( - indentation = PREVIEW_INDENTATION_3, - folder = BookmarkItem.Folder("Nested Three", "guid0", position = null), + expansionState = SelectFolderExpansionState.None, ), SelectFolderItem( indentation = PREVIEW_INDENTATION_0, - folder = BookmarkItem.Folder("Nested 0", "guid0", position = null), + folder = BookmarkItem.Folder("Bookmarks Unfiled", BookmarkRoot.Unfiled.id, position = null), + expansionState = SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + indentation = PREVIEW_INDENTATION_1, + folder = BookmarkItem.Folder("Nested One", "guid0", position = null), + expansionState = SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + indentation = PREVIEW_INDENTATION_2, + folder = BookmarkItem.Folder("Nested Two", "guid0", position = null), + expansionState = SelectFolderExpansionState.None, + ), + SelectFolderItem( + indentation = PREVIEW_INDENTATION_2, + folder = BookmarkItem.Folder("Nested Two", "guid0", position = null), + expansionState = SelectFolderExpansionState.None, + ), + ), + ), + ), + SelectFolderItem( + indentation = PREVIEW_INDENTATION_1, + folder = BookmarkItem.Folder("Nested One", "guid0", position = null), + expansionState = SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + indentation = PREVIEW_INDENTATION_2, + folder = BookmarkItem.Folder("Nested Two", "guid1", position = null), + expansionState = SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + indentation = PREVIEW_INDENTATION_3, + folder = BookmarkItem.Folder( + title = "Nested Three", + guid = "guid0", + position = null, + ), + expansionState = SelectFolderExpansionState.None, + ), + ), + ), + ), + ), + ), + ), + ), + ), ), ), ), diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksState.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksState.kt @@ -227,9 +227,16 @@ internal data class BookmarksEditFolderState( val folder: BookmarkItem.Folder, ) +internal sealed class SelectFolderExpansionState { + data object None : SelectFolderExpansionState() + data object Closed : SelectFolderExpansionState() + data class Open(val children: List<SelectFolderItem>) : SelectFolderExpansionState() +} + internal data class SelectFolderItem( val indentation: Int, val folder: BookmarkItem.Folder, + val expansionState: SelectFolderExpansionState, ) { val guid: String get() = folder.guid @@ -244,6 +251,18 @@ internal data class SelectFolderItem( get() = (16 * indentation).dp } +internal fun List<SelectFolderItem>.flattenToList(): List<SelectFolderItem> = + if (isEmpty()) { + emptyList() + } else { + map { + listOf(it) + ( + (it.expansionState as? SelectFolderExpansionState.Open) + ?.children?.flattenToList() ?: listOf() + ) + }.flatten() + } + /** * State representing the select folder subscreen. * diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksTelemetryMiddleware.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/bookmarks/BookmarksTelemetryMiddleware.kt @@ -63,8 +63,10 @@ internal class BookmarksTelemetryMiddleware : Middleware<BookmarksState, Bookmar is FolderLongClicked, is SelectFolderAction.FoldersLoaded, is SelectFolderAction.FilteredFoldersLoaded, + is SelectFolderAction.ExpandedFolderLoaded, Init, is SelectFolderAction.ItemClicked, + is SelectFolderAction.ChevronClicked, AddFolderAction.ParentFolderClicked, SignIntoSyncClicked, is AddFolderAction.FolderCreated, diff --git a/mobile/android/fenix/app/src/main/res/values/strings.xml b/mobile/android/fenix/app/src/main/res/values/strings.xml @@ -1835,6 +1835,10 @@ <string name="select_bookmark_search_button_content_description">Search folders</string> <!-- Content description for the overflow menu for a bookmark item. %s is a folder name or bookmark title. --> <string name="bookmark_item_menu_button_content_description">Item Menu for %s</string> + <!-- Content description for the chevron that will open a folder on the select folder screen and display its children. %s is the title of the folder that will be opened --> + <string name="bookmark_select_folder_expand_folder_content_description">Expand folder: %s</string> + <!-- Content description for the chevron that will close a folder on the select folder screen so that its children are no longer visible. %s is the title of the folder that will be closed --> + <string name="bookmark_select_folder_close_folder_content_description">Close folder: %s</string> <!-- Content description for the "close" button in bookmarks screen. --> <string name="bookmark_close_button_content_description">Close bookmarks</string> <!-- Title for the bookmark list empty state--> diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/bookmarks/BookmarksMiddlewareTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/bookmarks/BookmarksMiddlewareTest.kt @@ -480,9 +480,8 @@ class BookmarksMiddlewareTest { } @Test - fun `GIVEN current screen is add folder and previous screen is select folder WHEN back is clicked THEN navigate back to the edit bookmark screen`() = runTestOnMain { + fun `GIVEN current screen is add folder and previous screen is select folder WHEN back is clicked THEN skip selection screen straight back to the edit bookmark screen`() = runTestOnMain { `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(0u) - `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = true)).thenReturn(Result.success(generateBookmarkTree())) `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = false)).thenReturn(Result.success(generateBookmarkTree())) `when`(bookmarksStorage.addFolder(BookmarkRoot.Mobile.id, "i'm a new folder")).thenReturn(Result.success("new-guid")) val middleware = buildMiddleware() @@ -492,14 +491,14 @@ class BookmarksMiddlewareTest { val newFolderTitle = "i'm a new folder" store.dispatch(BookmarksListMenuAction.Bookmark.EditClicked(bookmark)) - store.dispatch(AddFolderAction.ParentFolderClicked) + store.dispatch(EditBookmarkAction.FolderClicked) store.dispatch(SelectFolderAction.ViewAppeared) store.dispatch(AddFolderClicked) store.dispatch(AddFolderAction.TitleChanged(newFolderTitle)) store.dispatch(BackClicked) assertNull(store.state.bookmarksSelectFolderState) - verify(bookmarksStorage, times(1)).getTree(BookmarkRoot.Mobile.id, recursive = true) + verify(bookmarksStorage, times(3)).getTree(BookmarkRoot.Mobile.id, recursive = false) verify(navController, times(1)).popBackStack(BookmarksDestinations.EDIT_BOOKMARK, inclusive = false) } @@ -610,7 +609,7 @@ class BookmarksMiddlewareTest { store.dispatch(EditFolderAction.TitleChanged(newFolderTitle)) store.dispatch(EditFolderAction.ParentFolderClicked) store.dispatch(SelectFolderAction.ViewAppeared) - store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem))) + store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem, SelectFolderExpansionState.None))) store.dispatch(BackClicked) store.dispatch(BackClicked) @@ -623,7 +622,7 @@ class BookmarksMiddlewareTest { url = null, ), ) - verify(bookmarksStorage, times(2)).getTree(BookmarkRoot.Mobile.id) + verify(bookmarksStorage, times(3)).getTree(BookmarkRoot.Mobile.id) assertNull(store.state.bookmarksEditFolderState) } @@ -702,7 +701,7 @@ class BookmarksMiddlewareTest { store.dispatch(EditBookmarkClicked(bookmark = bookmark)) store.dispatch(EditBookmarkAction.TitleChanged(title = newBookmarkTitle)) store.dispatch(EditBookmarkAction.FolderClicked) - store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem))) + store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem, SelectFolderExpansionState.None))) store.dispatch(BackClicked) assertNotNull(store.state.bookmarksEditBookmarkState) @@ -815,9 +814,9 @@ class BookmarksMiddlewareTest { } @Test - fun `GIVEN bookmarks in storage and not signed into sync WHEN select folder sub screen view is loaded THEN load folders into sub screen state`() = runTestOnMain { + fun `GIVEN bookmarks in storage and not signed into sync WHEN select folder sub screen view is loaded THEN only mobile root is loaded onto screen`() = runTestOnMain { `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(0u) - `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = true)).thenReturn(Result.success(generateBookmarkTree())) + `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = false)).thenReturn(Result.success(generateBookmarkTree())) val middleware = buildMiddleware() `when`(bookmarksStorage.countBookmarksInTrees(listOf(any()))).thenReturn(0u) val store = middleware.makeStore( @@ -828,7 +827,43 @@ class BookmarksMiddlewareTest { store.dispatch(SelectFolderAction.ViewAppeared) - assertEquals(6, store.state.bookmarksSelectFolderState?.folders?.count()) + assertEquals(1, store.state.bookmarksSelectFolderState?.folders?.count()) + } + + @Test + fun `GIVEN bookmarks in storage and not signed into sync WHEN expanding mobile root on select folder screen THEN mobile root children are shown as well`() = runTestOnMain { + val tree = generateBookmarkTree() + tree.children?.forEach { + if (it.type == BookmarkNodeType.FOLDER) { + `when`(bookmarksStorage.getTree(it.guid, false)).thenReturn(Result.success(generateBookmarkFolder(it.guid, "title", BookmarkRoot.Mobile.id, 0u))) + } + } + `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(0u) + `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, false)).thenReturn(Result.success(tree)) + val middleware = buildMiddleware() + `when`(bookmarksStorage.countBookmarksInTrees(listOf(any()))).thenReturn(0u) + val store = middleware.makeStore( + initialState = BookmarksState.default.copy( + bookmarksSelectFolderState = BookmarksSelectFolderState(outerSelectionGuid = "selection guid"), + ), + ) + + store.dispatch(SelectFolderAction.ViewAppeared) + store.dispatch( + SelectFolderAction.ChevronClicked( + SelectFolderItem( + indentation = 0, + folder = BookmarkItem.Folder( + guid = BookmarkRoot.Mobile.id, + title = "", + position = 0u, + ), + expansionState = SelectFolderExpansionState.Closed, + ), + ), + ) + + assertEquals(6, store.state.bookmarksSelectFolderState?.folders?.flattenToList()?.count()) } @Test @@ -904,7 +939,7 @@ class BookmarksMiddlewareTest { @Test fun `GIVEN bookmarks in storage and not signed into sync but have pre-existing desktop bookmarks saved WHEN select folder sub screen view is loaded THEN load folders, including desktop folders into sub screen state`() = runTestOnMain { `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(1u) - `when`(bookmarksStorage.getTree(BookmarkRoot.Root.id, recursive = true)).thenReturn(Result.success(generateDesktopRootTree())) + mockDesktopFoldersForSelectScreen() val middleware = buildMiddleware() `when`(bookmarksStorage.countBookmarksInTrees(listOf(any()))).thenReturn(1u) val store = middleware.makeStore( @@ -915,15 +950,47 @@ class BookmarksMiddlewareTest { store.dispatch(SelectFolderAction.ViewAppeared) - assertEquals(10, store.state.bookmarksSelectFolderState?.folders?.count()) + assertEquals(4, store.state.bookmarksSelectFolderState?.folders?.count()) } @Test fun `GIVEN bookmarks in storage and has desktop bookmarks WHEN select folder sub screen view is loaded THEN load folders, including desktop folders into sub screen state`() = runTestOnMain { `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(1u) - `when`(bookmarksStorage.getTree(BookmarkRoot.Root.id, recursive = true)).thenReturn(Result.success(generateDesktopRootTree())) + `when`(bookmarksStorage.getTree(BookmarkRoot.Root.id, recursive = false)).thenReturn(Result.success(generateDesktopRootTree())) + `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = false)).thenReturn(Result.success(generateBookmarkTree())) + `when`(bookmarksStorage.getTree(BookmarkRoot.Menu.id, recursive = false)).thenReturn(Result.success(generateBookmarkFolder(BookmarkRoot.Menu.id, "Menu", BookmarkRoot.Root.id, position = 0u))) + `when`(bookmarksStorage.getTree(BookmarkRoot.Toolbar.id, recursive = false)).thenReturn(Result.success(generateBookmarkFolder(BookmarkRoot.Toolbar.id, "Toolbar", BookmarkRoot.Root.id, position = 1u))) + `when`(bookmarksStorage.getTree(BookmarkRoot.Unfiled.id, recursive = false)).thenReturn(Result.success(generateBookmarkFolder(BookmarkRoot.Unfiled.id, "Unfiled", BookmarkRoot.Root.id, position = 2u))) + + val middleware = buildMiddleware() + val store = middleware.makeStore( + initialState = BookmarksState.default.copy( + isSignedIntoSync = true, + bookmarksSelectFolderState = BookmarksSelectFolderState(outerSelectionGuid = "selection guid"), + ), + ) + + store.dispatch(SelectFolderAction.ViewAppeared) + + assertEquals(5, store.state.bookmarksSelectFolderState?.folders?.count()) + } + + @Test + fun `GIVEN bookmarks in storage and has desktop bookmarks WHEN desktop folder is expanded THEN then load its children`() = runTestOnMain { + val childFolderGuid = "child folder" + val childFolder = generateBookmarkFolder(childFolderGuid, "title", BookmarkRoot.Toolbar.id, 5u) + `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(1u) + mockDesktopFoldersForSelectScreen() + `when`(bookmarksStorage.getTree(BookmarkRoot.Toolbar.id, recursive = false)) + .thenReturn( + Result.success( + generateBookmarkFolder(BookmarkRoot.Toolbar.id, "Toolbar", BookmarkRoot.Root.id, position = 1u) + .let { it.copy(children = it.children!! + listOf(childFolder)) }, + ), + ) + `when`(bookmarksStorage.getTree("child folder", recursive = false)).thenReturn(Result.success(childFolder)) + val middleware = buildMiddleware() - `when`(bookmarksStorage.countBookmarksInTrees(listOf(any()))).thenReturn(1u) val store = middleware.makeStore( initialState = BookmarksState.default.copy( isSignedIntoSync = true, @@ -932,8 +999,22 @@ class BookmarksMiddlewareTest { ) store.dispatch(SelectFolderAction.ViewAppeared) + store.dispatch( + SelectFolderAction.ChevronClicked( + SelectFolderItem( + indentation = 0, + folder = BookmarkItem.Folder( + guid = BookmarkRoot.Toolbar.id, + title = "", + position = 0u, + ), + expansionState = SelectFolderExpansionState.Closed, + ), + ), + ) - assertEquals(10, store.state.bookmarksSelectFolderState?.folders?.count()) + assertEquals(5, store.state.bookmarksSelectFolderState?.folders?.count()) + assertEquals(6, store.state.bookmarksSelectFolderState?.folders?.flattenToList()?.count()) } @Test @@ -992,18 +1073,59 @@ class BookmarksMiddlewareTest { @Test fun `GIVEN current screen select folder WHEN the search query is updated THEN FilteredFoldersLoaded gets dispatched with the filtered folders`() = runTestOnMain { - val bookmarksFolder = SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null)) val folders = listOf( - bookmarksFolder, - SelectFolderItem(1, BookmarkItem.Folder("Nested One", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid0", null)), - SelectFolderItem(1, BookmarkItem.Folder("Nested One", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid1", null)), - SelectFolderItem(3, BookmarkItem.Folder("Nested Three", "guid0", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + SelectFolderItem( + 0, + BookmarkItem.Folder("Bookmarks", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 1, + BookmarkItem.Folder("Nested One", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid0", null), + SelectFolderExpansionState.None, + ), + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid0", null), + SelectFolderExpansionState.None, + ), + ), + ), + ), + SelectFolderItem( + 1, + BookmarkItem.Folder("Nested One", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid1", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 3, + BookmarkItem.Folder("Nested Three", "guid0", null), + SelectFolderExpansionState.None, + ), + ), + ), + ), + ), + ), + ), + ), + ), + ), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), SelectFolderExpansionState.None), ) + val bookmarksFolder = folders.first() + val middleware = buildMiddleware() val store = middleware.makeStore( initialState = BookmarksState.default.copy( @@ -1606,7 +1728,7 @@ class BookmarksMiddlewareTest { store.dispatch(AddFolderAction.TitleChanged(newFolderTitle)) store.dispatch(AddFolderAction.ParentFolderClicked) store.dispatch(SelectFolderAction.ViewAppeared) - store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem))) + store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem, SelectFolderExpansionState.None))) store.dispatch(BackClicked) assertNull(store.state.bookmarksSelectFolderState) @@ -1634,7 +1756,7 @@ class BookmarksMiddlewareTest { store.dispatch(BookmarksListMenuAction.Folder.EditClicked(folderItem)) store.dispatch(EditFolderAction.ParentFolderClicked) store.dispatch(SelectFolderAction.ViewAppeared) - store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem))) + store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, newParentItem, SelectFolderExpansionState.None))) store.dispatch(BackClicked) assertNull(store.state.bookmarksSelectFolderState) @@ -1656,13 +1778,13 @@ class BookmarksMiddlewareTest { @Test fun `GIVEN editing a bookmark WHEN selecting a new parent THEN user can successfully add a new folder`() = runTestOnMain { val tree = generateBookmarkTree() + val newFolderGuid = "newFolderGuid" `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(0u) `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id)).thenReturn(Result.success(tree)) - `when`(bookmarksStorage.addFolder("folder guid 4", "newFolder")).thenReturn(Result.success("new-guid")) + `when`(bookmarksStorage.addFolder("folder guid 4", "newFolder")).thenReturn(Result.success(newFolderGuid)) val bookmark = tree.children?.first { it.type == BookmarkNodeType.ITEM }!! val bookmarkItem = BookmarkItem.Bookmark(title = bookmark.title!!, guid = bookmark.guid, url = bookmark.url!!, previewImageUrl = bookmark.url!!, position = bookmark.position) val newFolderTitle = "newFolder" - val newFolderGuid = "newFolderGuid" val parentForNewFolder = tree.children?.last { it.type == BookmarkNodeType.FOLDER }!! val parentForNewFolderItem = BookmarkItem.Folder(title = parentForNewFolder.title!!, guid = parentForNewFolder.guid, position = parentForNewFolder.position) @@ -1671,12 +1793,12 @@ class BookmarksMiddlewareTest { val store = middleware.makeStore() store.dispatch(BookmarksListMenuAction.Bookmark.EditClicked(bookmarkItem)) - store.dispatch(EditFolderAction.ParentFolderClicked) + store.dispatch(EditBookmarkAction.FolderClicked) store.dispatch(SelectFolderAction.ViewAppeared) store.dispatch(AddFolderClicked) store.dispatch(AddFolderAction.TitleChanged(newFolderTitle)) store.dispatch(AddFolderAction.ParentFolderClicked) - store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, parentForNewFolderItem))) + store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, parentForNewFolderItem, SelectFolderExpansionState.None))) store.dispatch(BackClicked) assertNotNull(store.state.bookmarksSelectFolderState) @@ -1688,21 +1810,6 @@ class BookmarksMiddlewareTest { assertNull(store.state.bookmarksAddFolderState) verify(bookmarksStorage).addFolder(parentGuid = parentForNewFolder.guid, title = newFolderTitle) - // replace the previous parent for the new folder in the tree with the updated version - val newFolder = generateBookmarkFolder(guid = newFolderGuid, title = newFolderTitle, parentForNewFolder.guid, (parentForNewFolder.children!!.size + 1).toUInt()) - val updatedParentForNewFolder = parentForNewFolder.copy( - children = listOf(newFolder), - ) - val updatedTree = tree.copy( - children = tree.children!!.mapNotNull { it.takeIf { it.guid != parentForNewFolder.guid } } + updatedParentForNewFolder, - ) - `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = true)).thenReturn(Result.success(updatedTree)) - store.dispatch(SelectFolderAction.ViewAppeared) - - val selectFolderItem = store.state.bookmarksSelectFolderState?.folders?.find { it.guid == newFolderGuid }!! - store.dispatch(SelectFolderAction.ItemClicked(selectFolderItem)) - store.dispatch(BackClicked) - assertNull(store.state.bookmarksSelectFolderState) assertEquals(newFolderGuid, store.state.bookmarksEditBookmarkState?.folder?.guid) assertEquals(newFolderTitle, store.state.bookmarksEditBookmarkState?.folder?.title) @@ -1848,6 +1955,91 @@ class BookmarksMiddlewareTest { assertEquals(BookmarksSnackbarState.SelectFolderFailed, store.state.bookmarksSnackbarState) } + @Test + fun `while moving items and adding a new folder, returning from the add folder screen results in the move items being moved to the new folder`() = runTestOnMain { + val tree = generateBookmarkTree() + `when`(bookmarksStorage.countBookmarksInTrees(listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id))).thenReturn(0u) + `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id)).thenReturn(Result.success(tree)) + + val middleware = buildMiddleware() + val guidsToMove = listOf("item guid 1", "item guid 2") + val store = middleware.makeStore( + initialState = BookmarksState.default.copy( + bookmarksMultiselectMoveState = MultiselectMoveState( + guidsToMove = guidsToMove, + destination = "folder guid 1", + ), + bookmarksSelectFolderState = BookmarksSelectFolderState( + outerSelectionGuid = "folder guid 1", + ), + ), + ) + + store.dispatch(AddFolderClicked) + store.dispatch(AddFolderAction.ParentFolderClicked) + store.dispatch(SelectFolderAction.ViewAppeared) + + val parentNodeForNewFolder = tree.children!!.first { it.type == BookmarkNodeType.FOLDER } + val parentForNewFolder = parentNodeForNewFolder.let { + BookmarkItem.Folder( + title = it.title!!, + guid = it.guid, + position = it.position, + ) + } + val newFolder = BookmarkNode( + type = BookmarkNodeType.FOLDER, + guid = "new folder guid", + title = "new folder title", + parentGuid = parentForNewFolder.guid, + position = 10u, + url = "url", + dateAdded = 0, + lastModified = 0, + children = null, + ) + val mobileRootSelectableItem = SelectFolderItem( + indentation = 0, + folder = BookmarkItem.Folder( + title = tree.title!!, + guid = tree.guid, + position = tree.position!!, + ), + expansionState = SelectFolderExpansionState.Closed, + ) + `when`(bookmarksStorage.addFolder(parentForNewFolder.guid, newFolder.title!!)).thenReturn(Result.success(newFolder.guid)) + `when`(bookmarksStorage.getBookmark(newFolder.guid)).thenReturn(Result.success(newFolder)) + + `when`(bookmarksStorage.getTree(parentForNewFolder.guid)).thenReturn(Result.success(parentNodeForNewFolder)) + store.dispatch(SelectFolderAction.ChevronClicked(mobileRootSelectableItem)) + assertTrue((store.state.bookmarksSelectFolderState?.folders?.first()?.expansionState as? SelectFolderExpansionState.Open)?.children?.any { it.guid == parentForNewFolder.guid } == true) + + store.dispatch(SelectFolderAction.ItemClicked(SelectFolderItem(0, parentForNewFolder, SelectFolderExpansionState.None))) + assertNotNull(store.state.bookmarksSelectFolderState?.innerSelectionGuid) + + store.dispatch(BackClicked) + assertNull(store.state.bookmarksSelectFolderState?.innerSelectionGuid) + assertEquals(parentForNewFolder, store.state.bookmarksAddFolderState?.parent) + + store.dispatch(AddFolderAction.TitleChanged(newFolder.title!!)) + store.dispatch(BackClicked) + assertNull(store.state.bookmarksAddFolderState) + assertNull(store.state.bookmarksSelectFolderState) + assertNull(store.state.bookmarksMultiselectMoveState) + guidsToMove.forEachIndexed { idx, guid -> + verify(bookmarksStorage).updateNode( + guid, + BookmarkInfo( + newFolder.guid, + null, + "item title ${idx + 1}", + "item url ${idx + 1}", + ), + ) + } + verify(navController).popBackStack(BookmarksDestinations.LIST, false) + } + private fun buildMiddleware( useNewSearchUX: Boolean = false, openBookmarksInNewTab: Boolean = false, @@ -1959,4 +2151,11 @@ class BookmarksMiddlewareTest { `when`(backStackEntry.destination).thenReturn(destination) `when`(previousBackStackEntry).thenReturn(backStackEntry) } + + private suspend fun mockDesktopFoldersForSelectScreen() { + `when`(bookmarksStorage.getTree(BookmarkRoot.Root.id, recursive = false)).thenReturn(Result.success(generateDesktopRootTree())) + `when`(bookmarksStorage.getTree(BookmarkRoot.Mobile.id, recursive = false)).thenReturn(Result.success(generateBookmarkTree())) + `when`(bookmarksStorage.getTree(BookmarkRoot.Menu.id, recursive = false)).thenReturn(Result.success(generateBookmarkFolder(BookmarkRoot.Menu.id, "Menu", BookmarkRoot.Root.id, position = 0u))) + `when`(bookmarksStorage.getTree(BookmarkRoot.Unfiled.id, recursive = false)).thenReturn(Result.success(generateBookmarkFolder(BookmarkRoot.Unfiled.id, "Unfiled", BookmarkRoot.Root.id, position = 2u))) + } } diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/bookmarks/BookmarksReducerTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/bookmarks/BookmarksReducerTest.kt @@ -353,7 +353,7 @@ class BookmarksReducerTest { } @Test - fun `GIVEN the add folder screen has been reached from the select folder screen WHEN back clicked THEN only inner selection is removed`() { + fun `GIVEN the add folder screen has been reached from the select folder screen WHEN back clicked from selecting a parent for the new folder THEN only inner selection is removed`() { val state = BookmarksState.default.copy( bookmarksEditBookmarkState = BookmarksEditBookmarkState( bookmark = BookmarkItem.Bookmark("url", "title", "url", "guid", position = null), @@ -380,6 +380,32 @@ class BookmarksReducerTest { } @Test + fun `GIVEN the add folder screen has been reached from the select folder screen while moving bookmarks WHEN back clicked from add folder screen THEN exit fully out of the selection state`() { + val state = BookmarksState.default.copy( + bookmarksAddFolderState = BookmarksAddFolderState( + parent = BookmarkItem.Folder( + guid = BookmarkRoot.Mobile.id, + title = "Bookmarks", + position = null, + ), + folderBeingAddedTitle = "test", + ), + bookmarksSelectFolderState = BookmarksSelectFolderState( + outerSelectionGuid = "outerGuid", + ), + bookmarksMultiselectMoveState = MultiselectMoveState( + listOf("guid1", "guid2"), + "guid to move into", + ), + ) + + val result = bookmarksReducer(state, BackClicked) + + assertNull(result.bookmarksSelectFolderState) + assertNull(result.bookmarksMultiselectMoveState) + } + + @Test fun `WHEN the title of a folder is changed on the edit folder screen THEN that is reflected in state`() { val state = BookmarksState.default.copy( bookmarksEditFolderState = BookmarksEditFolderState( @@ -568,6 +594,7 @@ class BookmarksReducerTest { guid = "", position = null, ), + SelectFolderExpansionState.None, ) val folder2 = SelectFolderItem( @@ -578,6 +605,7 @@ class BookmarksReducerTest { guid = "", position = null, ), + SelectFolderExpansionState.None, ) val state = BookmarksState.default.copy( bookmarksSelectFolderState = BookmarksSelectFolderState( @@ -600,14 +628,54 @@ class BookmarksReducerTest { ) val folders = listOf( - SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null)), - SelectFolderItem(1, BookmarkItem.Folder("Nested One", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid0", null)), - SelectFolderItem(1, BookmarkItem.Folder("Nested One", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid1", null)), - SelectFolderItem(3, BookmarkItem.Folder("Nested Three", "guid0", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + SelectFolderItem( + 0, + BookmarkItem.Folder("Bookmarks", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 1, + BookmarkItem.Folder("Nested One", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid0", null), + SelectFolderExpansionState.None, + ), + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid0", null), + SelectFolderExpansionState.None, + ), + ), + ), + ), + SelectFolderItem( + 1, + BookmarkItem.Folder("Nested One", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid1", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 3, + BookmarkItem.Folder("Nested Three", "guid0", null), + SelectFolderExpansionState.None, + ), + ), + ), + ), + ), + ), + ), + ), + ), + ), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), SelectFolderExpansionState.None), ) val result = bookmarksReducer(state, SelectFolderAction.FoldersLoaded(folders)) @@ -627,18 +695,59 @@ class BookmarksReducerTest { @Test fun `GIVEN we are on the select folder screen and search is active WHEN filtered folders are loaded THEN filtered folders gets updated`() { - val bookmarksFolder = SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null)) val folders = listOf( - bookmarksFolder, - SelectFolderItem(1, BookmarkItem.Folder("Nested One", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid0", null)), - SelectFolderItem(1, BookmarkItem.Folder("Nested One", "guid0", null)), - SelectFolderItem(2, BookmarkItem.Folder("Nested Two", "guid1", null)), - SelectFolderItem(3, BookmarkItem.Folder("Nested Three", "guid0", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + SelectFolderItem( + 0, + BookmarkItem.Folder("Bookmarks", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 1, + BookmarkItem.Folder("Nested One", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid0", null), + SelectFolderExpansionState.None, + ), + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid0", null), + SelectFolderExpansionState.None, + ), + ), + ), + ), + SelectFolderItem( + 1, + BookmarkItem.Folder("Nested One", "guid0", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 2, + BookmarkItem.Folder("Nested Two", "guid1", null), + SelectFolderExpansionState.Open( + listOf( + SelectFolderItem( + 3, + BookmarkItem.Folder("Nested Three", "guid0", null), + SelectFolderExpansionState.None, + ), + ), + ), + ), + ), + ), + ), + ), + ), + ), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), SelectFolderExpansionState.None), ) + val bookmarksFolder = folders.first() + val state = BookmarksState.default.copy( bookmarksSelectFolderState = BookmarksSelectFolderState( @@ -675,8 +784,12 @@ class BookmarksReducerTest { bookmarksSelectFolderState = BookmarksSelectFolderState( outerSelectionGuid = "guid0", folders = listOf( - SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + SelectFolderItem( + 0, + BookmarkItem.Folder("Bookmarks", "guid0", null), + SelectFolderExpansionState.None, + ), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), SelectFolderExpansionState.None), ), ), ) @@ -684,7 +797,7 @@ class BookmarksReducerTest { val result = bookmarksReducer( state = state, action = SelectFolderAction.ItemClicked( - folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), SelectFolderExpansionState.None), ), ) @@ -713,8 +826,8 @@ class BookmarksReducerTest { bookmarksSelectFolderState = BookmarksSelectFolderState( outerSelectionGuid = "guid0", folders = listOf( - SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null), expansionState = SelectFolderExpansionState.Closed), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), expansionState = SelectFolderExpansionState.Closed), ), ), ) @@ -722,7 +835,7 @@ class BookmarksReducerTest { val result = bookmarksReducer( state = state, action = SelectFolderAction.ItemClicked( - folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), expansionState = SelectFolderExpansionState.Closed), ), ) @@ -747,14 +860,14 @@ class BookmarksReducerTest { @Test fun `GIVEN a bookmark WHEN a edit is made THEN the edited state is persisted`() { val bookmarkItem = generateBookmark() - val folderItem = SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null)) + val folderItem = SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid0", null), SelectFolderExpansionState.None) var state = BookmarksState.default.copy( bookmarkItems = listOf(bookmarkItem), bookmarksSelectFolderState = BookmarksSelectFolderState( outerSelectionGuid = "guid0", folders = listOf( folderItem, - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null)), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid0", null), SelectFolderExpansionState.None), ), ), ) @@ -787,10 +900,10 @@ class BookmarksReducerTest { bookmarksSelectFolderState = BookmarksSelectFolderState( outerSelectionGuid = "folder 1", folders = listOf( - SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid 0", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid 1", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 1", "folder 1", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 2", "folder 2", null)), + SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "guid 0", null), SelectFolderExpansionState.None), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "guid 1", null), SelectFolderExpansionState.None), + SelectFolderItem(0, BookmarkItem.Folder("Nested 1", "folder 1", null), SelectFolderExpansionState.None), + SelectFolderItem(0, BookmarkItem.Folder("Nested 2", "folder 2", null), SelectFolderExpansionState.None), ), ), ) @@ -798,7 +911,7 @@ class BookmarksReducerTest { val result = bookmarksReducer( state = state, action = SelectFolderAction.ItemClicked( - folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 2", "folder 2", null)), + folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 2", "folder 2", null), SelectFolderExpansionState.None), ), ) @@ -829,8 +942,8 @@ class BookmarksReducerTest { outerSelectionGuid = "0", innerSelectionGuid = "0", folders = listOf( - SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "0", null)), - SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "1", null)), + SelectFolderItem(0, BookmarkItem.Folder("Bookmarks", "0", null), SelectFolderExpansionState.None), + SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "1", null), SelectFolderExpansionState.None), ), ), ) @@ -838,7 +951,7 @@ class BookmarksReducerTest { val result = bookmarksReducer( state = state, action = SelectFolderAction.ItemClicked( - folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "1", null)), + folder = SelectFolderItem(0, BookmarkItem.Folder("Nested 0", "1", null), SelectFolderExpansionState.None), ), )