Exclude separators from the bookmarks UI

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<RecyclerView.ViewHolder>() {
 
-    private var tree: List<BookmarkNode> = listOf()
+    @VisibleForTesting var tree: List<BookmarkNode> = 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<BookmarkNode> = mutableListOf()
         val notFolders: MutableList<BookmarkNode> = mutableListOf()
+        val separators: MutableList<BookmarkNode> = mutableListOf()
         allNodes.forEach {
-            if (it.type == BookmarkNodeType.FOLDER) {
+            when (it.type) {
-                folders.add(it)
+                BookmarkNodeType.SEPARATOR -> separators.add(it)
-            } else {
+                BookmarkNodeType.FOLDER -> folders.add(it)
-                notFolders.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(

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.assertEquals
-import org.junit.Assert.assertFalse
 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(
+        val tree = testFolder("123", "root", listOf(
-            BookmarkNodeType.FOLDER, "123", null, 0, "Mobile", null, listOf(
+            testBookmarkItem("someFolder", "http://mozilla.org"),
-                item,
+            testSeparator("123"),
-                BookmarkNode(BookmarkNodeType.SEPARATOR, "789", "123", 1, null, null, null),
+            testBookmarkItem("123", "https://www.mozilla.org/en-US/firefox/")
-                BookmarkNode(
-                    BookmarkNodeType.ITEM,
-                    "987",
-                    "123",
-                    2,
-                    "Firefox",
-                    "https://www.mozilla.org/en-US/firefox/",
-                    null
-                )
             )
         )
         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)

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<BookmarkNodeWithDepth>(), url.flatNodeList(null))
 
         val root = testFolder("root", null, null)
@@ -99,3 +99,7 @@ internal fun testFolder(guid: String, parentGuid: String?, children: List<Bookma
     null,
     children
 )
+
+internal fun testSeparator(parentGuid: String) = BookmarkNode(
+    BookmarkNodeType.SEPARATOR, "guid#${Math.random() * 1000}", parentGuid, null, null, null, null
+)