author | mcarare <48995920+mcarare@users.noreply.github.com> |
Mon, 19 May 2025 09:48:31 +0000 (8 weeks ago) | |
changeset 787735 | 59c36992320d5551fc6c56ef31676d66b876e08a |
parent 787734 | 1452a3f37b97d281146ee51cb9eebc544fac8dd1 |
child 787736 | 806d5c9f9237d2cfac2a4e11f514be1aad549c0c |
push id | 231773 |
push user | mcarare@mozilla.com |
push date | Mon, 19 May 2025 09:51:37 +0000 (8 weeks ago) |
treeherder | autoland@59c36992320d [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
git commit | 7f21993b1054179a5d046c545794f79263a21f2d |
reviewers | android-reviewers, pollymce |
bugs | 1965832 |
milestone | 140.0a1 |
first release with | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
last release without | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
--- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt @@ -6,22 +6,23 @@ package org.mozilla.fenix.library.bookma import android.content.Context import androidx.annotation.VisibleForTesting import mozilla.components.browser.menu2.BrowserMenuController import mozilla.components.concept.menu.MenuController import mozilla.components.concept.menu.candidate.TextMenuCandidate import mozilla.components.concept.menu.candidate.TextStyle import mozilla.components.concept.storage.BookmarkNodeType +import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.support.ktx.android.content.getColorFromAttr import org.mozilla.fenix.R -import org.mozilla.fenix.ext.bookmarkStorage class BookmarkItemMenu( private val context: Context, + private val bookmarkStorage: BookmarksStorage, var onItemTapped: ((Item) -> Unit)? = null, ) { enum class Item { Edit, Copy, Share, OpenInNewTab, @@ -29,92 +30,92 @@ class BookmarkItemMenu( OpenAllInNewTabs, OpenAllInPrivateTabs, Delete, } val menuController: MenuController by lazy { BrowserMenuController() } @VisibleForTesting - @SuppressWarnings("LongMethod") internal suspend fun menuItems(itemType: BookmarkNodeType, itemId: String): List<TextMenuCandidate> { - val hasAtLeastOneChild = !context.bookmarkStorage.getTree(itemId, false)?.children.isNullOrEmpty() + val editMenuOption = + TextMenuCandidate(text = context.getString(R.string.bookmark_menu_edit_button)) { + onItemTapped?.invoke(Item.Edit) + } - return listOfNotNull( - if (itemType != BookmarkNodeType.SEPARATOR) { - TextMenuCandidate( - text = context.getString(R.string.bookmark_menu_edit_button), - ) { - onItemTapped?.invoke(Item.Edit) - } - } else { - null - }, - if (itemType == BookmarkNodeType.ITEM) { - TextMenuCandidate( - text = context.getString(R.string.bookmark_menu_copy_button), - ) { - onItemTapped?.invoke(Item.Copy) - } - } else { - null - }, - if (itemType == BookmarkNodeType.ITEM) { - TextMenuCandidate( - text = context.getString(R.string.bookmark_menu_share_button), - ) { - onItemTapped?.invoke(Item.Share) - } - } else { - null - }, - if (itemType == BookmarkNodeType.ITEM) { - TextMenuCandidate( - text = context.getString(R.string.bookmark_menu_open_in_new_tab_button), - ) { - onItemTapped?.invoke(Item.OpenInNewTab) - } - } else { - null - }, - if (itemType == BookmarkNodeType.ITEM) { - TextMenuCandidate( - text = context.getString(R.string.bookmark_menu_open_in_private_tab_button), - ) { - onItemTapped?.invoke(Item.OpenInPrivateTab) - } - } else { - null - }, - if (hasAtLeastOneChild && itemType == BookmarkNodeType.FOLDER) { - TextMenuCandidate( - text = context.getString(R.string.bookmark_menu_open_all_in_tabs_button), - ) { - onItemTapped?.invoke(Item.OpenAllInNewTabs) - } - } else { - null - }, - if (hasAtLeastOneChild && itemType == BookmarkNodeType.FOLDER) { - TextMenuCandidate( - text = context.getString(R.string.bookmark_menu_open_all_in_private_tabs_button), - ) { - onItemTapped?.invoke(Item.OpenAllInPrivateTabs) - } - } else { - null - }, + val copyMenuOption = + TextMenuCandidate(text = context.getString(R.string.bookmark_menu_copy_button)) { + onItemTapped?.invoke(Item.Copy) + } + + val deleteMenuOption = TextMenuCandidate( text = context.getString(R.string.bookmark_menu_delete_button), textStyle = TextStyle(color = context.getColorFromAttr(R.attr.textCritical)), ) { onItemTapped?.invoke(Item.Delete) - }, - ) + } + + val shareMenuOption = + TextMenuCandidate(text = context.getString(R.string.bookmark_menu_share_button)) { + onItemTapped?.invoke(Item.Share) + } + + val openInNewTabMenuOption = + TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_in_new_tab_button)) { + onItemTapped?.invoke(Item.OpenInNewTab) + } + + val openInPrivateTabMenuOption = + TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_in_private_tab_button)) { + onItemTapped?.invoke(Item.OpenInPrivateTab) + } + + val openAllInNewTabsMenuOption = + TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_all_in_tabs_button)) { + onItemTapped?.invoke(Item.OpenAllInNewTabs) + } + + val openAllInPrivateTabsMenuOption = + TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_all_in_private_tabs_button)) { + onItemTapped?.invoke(Item.OpenAllInPrivateTabs) + } + + return buildList { + if (itemType != BookmarkNodeType.SEPARATOR) { + add(editMenuOption) + } + if (itemType == BookmarkNodeType.ITEM) { + add(copyMenuOption) + add(shareMenuOption) + add(openInNewTabMenuOption) + add(openInPrivateTabMenuOption) + } + + if (itemType == BookmarkNodeType.FOLDER && checkAtLeastOneChild(itemId)) { + add(openAllInNewTabsMenuOption) + add(openAllInPrivateTabsMenuOption) + } + + add(deleteMenuOption) + } } /** * Update the menu items for the type of bookmark. */ suspend fun updateMenu(itemType: BookmarkNodeType, itemId: String) { menuController.submitList(menuItems(itemType, itemId)) } + + /** + * Checks if a bookmark item has at least one child. + * + * @param itemId The ID of the bookmark item to check. + * @return `true` if the item has at least one child, `false` otherwise. + */ + @VisibleForTesting + internal suspend fun checkAtLeastOneChild(itemId: String): Boolean = + bookmarkStorage.getTree( + itemId, + false, + )?.children?.isNotEmpty() == true }
--- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt +++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt @@ -7,16 +7,17 @@ package org.mozilla.fenix.library.bookma import androidx.core.view.isVisible import androidx.recyclerview.widget.RecyclerView import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import mozilla.components.concept.storage.BookmarkNode import mozilla.components.concept.storage.BookmarkNodeType import org.mozilla.fenix.R +import org.mozilla.fenix.ext.bookmarkStorage import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.hideAndDisable import org.mozilla.fenix.ext.loadIntoView import org.mozilla.fenix.ext.removeAndDisable import org.mozilla.fenix.ext.showAndEnable import org.mozilla.fenix.library.LibrarySiteItemView import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState import org.mozilla.fenix.library.bookmarks.BookmarkItemMenu @@ -27,17 +28,20 @@ import org.mozilla.fenix.library.bookmar /** * Base class for bookmark node view holders. */ class BookmarkNodeViewHolder( private val containerView: LibrarySiteItemView, private val interactor: BookmarkViewInteractor, ) : RecyclerView.ViewHolder(containerView) { - private val menu = BookmarkItemMenu(containerView.context) + private val menu = BookmarkItemMenu( + containerView.context, + containerView.context.bookmarkStorage, + ) /** * Binds the view holder to the item */ fun bind( item: BookmarkNode, mode: BookmarkFragmentState.Mode, payload: BookmarkPayload,
--- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt +++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt @@ -2,93 +2,90 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ package org.mozilla.fenix.library.bookmarks import android.content.Context import androidx.appcompat.view.ContextThemeWrapper import io.mockk.coEvery -import io.mockk.every +import io.mockk.coVerify import io.mockk.mockk -import io.mockk.mockkStatic -import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.runTest import mozilla.components.concept.menu.candidate.TextMenuCandidate import mozilla.components.concept.menu.candidate.TextStyle +import mozilla.components.concept.storage.BookmarkNode import mozilla.components.concept.storage.BookmarkNodeType +import mozilla.components.concept.storage.BookmarksStorage import mozilla.components.support.ktx.android.content.getColorFromAttr import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.R -import org.mozilla.fenix.ext.bookmarkStorage import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.library.bookmarks.BookmarkItemMenu.Item @RunWith(FenixRobolectricTestRunner::class) class BookmarkItemMenuTest { private lateinit var context: Context private lateinit var menu: BookmarkItemMenu + private lateinit var bookmarksStorage: BookmarksStorage + private var lastItemTapped: Item? = null + private val item = BookmarkNode(BookmarkNodeType.ITEM, "456", "123", 0u, "Mozilla", "http://mozilla.org", 0, 0, null) + private val emptyFolder = BookmarkNode(BookmarkNodeType.FOLDER, "987", "123", 0u, "Subfolder", null, 0, 0, emptyList()) + private val notEmptyFolder = BookmarkNode(BookmarkNodeType.FOLDER, "987", "123", 0u, "Subfolder", null, 0, 0, listOf(item)) @Before fun setup() { context = ContextThemeWrapper(testContext, R.style.NormalTheme) - menu = BookmarkItemMenu(context) { - lastItemTapped = it - } + bookmarksStorage = mockk() + menu = BookmarkItemMenu(context, bookmarksStorage) { lastItemTapped = it } } @Test - fun `delete item has special styling`() = runBlocking { - var deleteItem: TextMenuCandidate? = null - mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") { - every { any<Context>().bookmarkStorage } returns mockk(relaxed = true) + fun `delete item has special styling`() = runTest { + val deleteItem = menu.menuItems(BookmarkNodeType.SEPARATOR, "").last() - deleteItem = menu.menuItems(BookmarkNodeType.SEPARATOR, "").last() - } assertNotNull(deleteItem) - assertEquals("Delete", deleteItem!!.text) + assertEquals("Delete", deleteItem.text) assertEquals( TextStyle(color = context.getColorFromAttr(R.attr.textCritical)), - deleteItem!!.textStyle, + deleteItem.textStyle, ) - deleteItem!!.onClick() + deleteItem.onClick() assertEquals(Item.Delete, lastItemTapped) } @Test - fun `edit item appears for folders`() = runBlocking { + fun `edit item appears for folders`() = runTest { // empty folder - var emptyFolderItems: List<TextMenuCandidate>? = null - mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") { - every { any<Context>().bookmarkStorage } returns mockk(relaxed = true) + coEvery { bookmarksStorage.getTree(any()) } returns emptyFolder - emptyFolderItems = menu.menuItems(BookmarkNodeType.FOLDER, "") - } + var emptyFolderItems = menu.menuItems(BookmarkNodeType.FOLDER, "") + assertNotNull(emptyFolderItems) - assertEquals(2, emptyFolderItems!!.size) + assertEquals(2, emptyFolderItems.size) - // not empty + // not empty folder + coEvery { bookmarksStorage.getTree(any()) } returns notEmptyFolder var folderItems: List<TextMenuCandidate>? = null - mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") { - coEvery { any<Context>().bookmarkStorage.getTree("")?.children } returns listOf(mockk(relaxed = true)) - folderItems = menu.menuItems(BookmarkNodeType.FOLDER, "") - } + folderItems = menu.menuItems(BookmarkNodeType.FOLDER, "") + assertNotNull(folderItems) - assertEquals(4, folderItems!!.size) + assertEquals(4, folderItems.size) - val (edit, openAll, openAllPrivate, delete) = folderItems!! + val (edit, openAll, openAllPrivate, delete) = folderItems assertEquals("Edit", edit.text) assertEquals("Open all in new tabs", openAll.text) assertEquals("Open all in private tabs", openAllPrivate.text) assertEquals("Delete", delete.text) edit.onClick() assertEquals(Item.Edit, lastItemTapped) @@ -99,26 +96,22 @@ class BookmarkItemMenuTest { openAllPrivate.onClick() assertEquals(Item.OpenAllInPrivateTabs, lastItemTapped) delete.onClick() assertEquals(Item.Delete, lastItemTapped) } @Test - fun `all item appears for sites`() = runBlocking { - var siteItems: List<TextMenuCandidate>? = null - mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") { - every { any<Context>().bookmarkStorage } returns mockk(relaxed = true) + fun `all item appears for sites`() = runTest { + val siteItems = menu.menuItems(BookmarkNodeType.ITEM, "") - siteItems = menu.menuItems(BookmarkNodeType.ITEM, "") - } assertNotNull(siteItems) - assertEquals(6, siteItems!!.size) - val (edit, copy, share, openInNewTab, openInPrivateTab, delete) = siteItems!! + assertEquals(6, siteItems.size) + val (edit, copy, share, openInNewTab, openInPrivateTab, delete) = siteItems assertEquals("Edit", edit.text) assertEquals("Copy", copy.text) assertEquals("Share", share.text) assertEquals("Open in new tab", openInNewTab.text) assertEquals("Open in private tab", openInPrivateTab.text) assertEquals("Delete", delete.text) @@ -136,12 +129,26 @@ class BookmarkItemMenuTest { openInPrivateTab.onClick() assertEquals(Item.OpenInPrivateTab, lastItemTapped) delete.onClick() assertEquals(Item.Delete, lastItemTapped) } + @Test + fun `checkAtLeastOneChild is called only for FOLDER type`() = runTest { + coEvery { bookmarksStorage.getTree(any()) } returns emptyFolder + + menu.menuItems(BookmarkNodeType.SEPARATOR, "") + coVerify(exactly = 0) { bookmarksStorage.getTree(any()) } + + menu.menuItems(BookmarkNodeType.ITEM, "") + coVerify(exactly = 0) { bookmarksStorage.getTree(any()) } + + menu.menuItems(BookmarkNodeType.FOLDER, "") + coVerify { bookmarksStorage.getTree(any()) } + } + private operator fun <T> List<T>.component6(): T { return get(5) } }