From 3ce9bf93bc4118f262b501f4c0c0cd1674802468 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Tue, 2 Feb 2021 16:22:20 -0800 Subject: [PATCH] Exclude separators from the bookmarks UI --- .../library/bookmarks/BookmarkAdapter.kt | 16 ++-- .../library/bookmarks/BookmarkAdapterTest.kt | 96 +++++++++++++------ .../fenix/library/bookmarks/UtilsKtTest.kt | 8 +- 3 files changed, 84 insertions(+), 36 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt index f6f6a368de..e85ded68a1 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapter.kt @@ -21,23 +21,25 @@ import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkSeparatorViewHold class BookmarkAdapter(private val emptyView: View, private val interactor: BookmarkViewInteractor) : RecyclerView.Adapter() { - private var tree: List = listOf() + @VisibleForTesting var tree: List = listOf() private var mode: BookmarkFragmentState.Mode = BookmarkFragmentState.Mode.Normal() private var isFirstRun = true fun updateData(tree: BookmarkNode?, mode: BookmarkFragmentState.Mode) { - // Display folders above all other bookmarks. val allNodes = tree?.children.orEmpty() val folders: MutableList = mutableListOf() val notFolders: MutableList = mutableListOf() + val separators: MutableList = mutableListOf() allNodes.forEach { - if (it.type == BookmarkNodeType.FOLDER) { - folders.add(it) - } else { - notFolders.add(it) + when (it.type) { + BookmarkNodeType.SEPARATOR -> separators.add(it) + BookmarkNodeType.FOLDER -> folders.add(it) + else -> notFolders.add(it) } } - val newTree = folders + notFolders + // Display folders above all other bookmarks. Exclude separators. + // For separator removal, see discussion in https://github.com/mozilla-mobile/fenix/issues/15214 + val newTree = folders + notFolders - separators val diffUtil = DiffUtil.calculateDiff( BookmarkDiffUtil( diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapterTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapterTest.kt index b80e584a7f..bc04b618b5 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapterTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkAdapterTest.kt @@ -8,29 +8,20 @@ import io.mockk.mockk import io.mockk.spyk import io.mockk.verifyOrder import mozilla.components.concept.storage.BookmarkNode -import mozilla.components.concept.storage.BookmarkNodeType -import org.junit.Assert.assertFalse +import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue +import org.junit.Assert.assertFalse import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.helpers.FenixRobolectricTestRunner +import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkNodeViewHolder @RunWith(FenixRobolectricTestRunner::class) internal class BookmarkAdapterTest { private lateinit var bookmarkAdapter: BookmarkAdapter - private val item = BookmarkNode( - BookmarkNodeType.ITEM, - "456", - "123", - 0, - "Mozilla", - "http://mozilla.org", - null - ) - @Before fun setup() { bookmarkAdapter = spyk( @@ -40,33 +31,83 @@ internal class BookmarkAdapterTest { @Test fun `update adapter from tree of bookmark nodes, null tree returns empty list`() { - val tree = BookmarkNode( - BookmarkNodeType.FOLDER, "123", null, 0, "Mobile", null, listOf( - item, - BookmarkNode(BookmarkNodeType.SEPARATOR, "789", "123", 1, null, null, null), - BookmarkNode( - BookmarkNodeType.ITEM, - "987", - "123", - 2, - "Firefox", - "https://www.mozilla.org/en-US/firefox/", - null - ) + val tree = testFolder("123", "root", listOf( + testBookmarkItem("someFolder", "http://mozilla.org"), + testSeparator("123"), + testBookmarkItem("123", "https://www.mozilla.org/en-US/firefox/") ) ) bookmarkAdapter.updateData(tree, BookmarkFragmentState.Mode.Normal()) bookmarkAdapter.updateData(null, BookmarkFragmentState.Mode.Normal()) verifyOrder { bookmarkAdapter.updateData(tree, BookmarkFragmentState.Mode.Normal()) - bookmarkAdapter.notifyItemRangeInserted(0, 3) + bookmarkAdapter.notifyItemRangeInserted(0, 2) bookmarkAdapter.updateData(null, BookmarkFragmentState.Mode.Normal()) - bookmarkAdapter.notifyItemRangeRemoved(0, 3) + bookmarkAdapter.notifyItemRangeRemoved(0, 2) } } + @Test + fun `update adapter from tree of bookmark nodes, separators are excluded`() { + val sep1 = testSeparator("123") + val sep2 = testSeparator("123") + val item1 = testBookmarkItem("123", "http://mozilla.org") + val item2 = testBookmarkItem("123", "https://www.mozilla.org/en-US/firefox/") + val folder = testFolder("123", "root", title = "Mobile", children = listOf(item1, sep1, item2, sep2)) + bookmarkAdapter.updateData(folder, BookmarkFragmentState.Mode.Normal()) + verifyOrder { + bookmarkAdapter.updateData(folder, BookmarkFragmentState.Mode.Normal()) + bookmarkAdapter.notifyItemRangeInserted(0, 2) + } + + assertEquals(2, bookmarkAdapter.itemCount) + assertEquals(listOf(item1, item2), bookmarkAdapter.tree) + } + + @Test + fun `update adapter from tree of bookmark nodes, folders are moved to the top`() { + val sep1 = testSeparator("123") + val item1 = testBookmarkItem("123", "http://mozilla.org") + val item2 = testBookmarkItem("123", "https://www.mozilla.org/en-US/firefox/") + val item3 = testBookmarkItem("123", "https://www.mozilla.org/en-US/firefox/2") + val item4 = testBookmarkItem("125", "https://www.mozilla.org/en-US/firefox/3") + val folder2 = testFolder("124", "123", title = "Mobile 2", children = emptyList()) + val folder3 = testFolder("125", "123", title = "Mobile 3", children = listOf(item4)) + val folder4 = testFolder("126", "123", title = "Mobile 3", children = emptyList()) + val folder = testFolder("123", "root", title = "Mobile", children = listOf( + folder4, item1, sep1, item2, folder2, folder3, item3 + )) + bookmarkAdapter.updateData(folder, BookmarkFragmentState.Mode.Normal()) + verifyOrder { + bookmarkAdapter.updateData(folder, BookmarkFragmentState.Mode.Normal()) + bookmarkAdapter.notifyItemRangeInserted(0, 6) + } + + assertEquals(6, bookmarkAdapter.itemCount) + assertEquals(listOf(folder4, folder2, folder3, item1, item2, item3), bookmarkAdapter.tree) + } + + @Test + fun `get item view type for different types of nodes`() { + val sep1 = testSeparator("123") + val item1 = testBookmarkItem("123", "https://www.mozilla.org/en-US/firefox/") + val folder1 = testFolder("124", "123", title = "Mobile 2", children = emptyList()) + bookmarkAdapter.updateData( + testFolder("123", "root", listOf(sep1, item1, folder1)), + BookmarkFragmentState.Mode.Normal() + ) + + assertEquals(2, bookmarkAdapter.itemCount) + // item1 + assertEquals(BookmarkNodeViewHolder.LAYOUT_ID, bookmarkAdapter.getItemViewType(0)) + // folder1 + assertEquals(BookmarkNodeViewHolder.LAYOUT_ID, bookmarkAdapter.getItemViewType(1)) + // sep is dropped during update + } + @Test fun `items are the same if they have the same guids`() { + val item = testBookmarkItem("someFolder", "http://mozilla.org") assertTrue(createSingleItemDiffUtil(item, item).areItemsTheSame(0, 0)) assertTrue( createSingleItemDiffUtil( @@ -84,6 +125,7 @@ internal class BookmarkAdapterTest { @Test fun `equal items have same contents unless their selected state changes`() { + val item = testBookmarkItem("someFolder", "http://mozilla.org") assertTrue(createSingleItemDiffUtil(item, item).areContentsTheSame(0, 0)) assertFalse( createSingleItemDiffUtil(item, item.copy(position = 1)).areContentsTheSame(0, 0) diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/UtilsKtTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/UtilsKtTest.kt index df0a2ec149..3ea5539dc3 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/UtilsKtTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/UtilsKtTest.kt @@ -16,7 +16,7 @@ import org.mozilla.fenix.helpers.FenixRobolectricTestRunner class UtilsKtTest { @Test fun `friendly root titles`() { - val url = testBookmarkItem("folder","http://mozilla.org", "Mozilla") + val url = testBookmarkItem("folder", "http://mozilla.org", "Mozilla") assertEquals("Mozilla", friendlyRootTitle(testContext, url)) val folder = testFolder("456", "folder", null, "Folder") @@ -50,7 +50,7 @@ class UtilsKtTest { @Test fun `flatNodeList various cases`() { val url = testBookmarkItem("folder", "http://mozilla.org") - val url2 = testBookmarkItem( "folder2", "http://mozilla.org") + val url2 = testBookmarkItem("folder2", "http://mozilla.org") assertEquals(emptyList(), url.flatNodeList(null)) val root = testFolder("root", null, null) @@ -99,3 +99,7 @@ internal fun testFolder(guid: String, parentGuid: String?, children: List