For #3203: Updates edit bookmarks to have user friendly names (#3372)

Co-authored-by: Colin Lee <mncolinlee@gmail.com>
Co-authored-by: Sawyer Blatz <sdblatz@gmail.com>"

app/src/main/java/org/mozilla/fenix/ext/BookmarkNode.kt

@@ -0,0 +1,129 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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.ext
+
+import android.content.Context
+import mozilla.appservices.places.BookmarkRoot
+import mozilla.components.browser.storage.sync.PlacesBookmarksStorage
+import mozilla.components.concept.storage.BookmarkNode
+import org.mozilla.fenix.R
+
+var rootTitles: Map<String, String> = emptyMap()
+
+@SuppressWarnings("ReturnCount")
+suspend fun BookmarkNode?.withOptionalDesktopFolders(
+    context: Context?
+): BookmarkNode? {
+    // No-op if node is missing.
+    if (this == null) {
+        return null
+    }
+
+    // If we're in the mobile root and logged in, add-in a synthetic "Desktop Bookmarks" folder.
+    if (this.guid == BookmarkRoot.Mobile.id &&
+        context?.components?.backgroundServices?.accountManager?.authenticatedAccount() != null
+    ) {
+        // We're going to make a copy of the mobile node, and add-in a synthetic child folder to the top of the
+        // children's list that contains all of the desktop roots.
+        val childrenWithVirtualFolder: MutableList<BookmarkNode> = mutableListOf()
+        virtualDesktopFolder(context)?.let { childrenWithVirtualFolder.add(it) }
+
+        this.children?.let { children ->
+            childrenWithVirtualFolder.addAll(children)
+        }
+
+        return BookmarkNode(
+            type = this.type,
+            guid = this.guid,
+            parentGuid = this.parentGuid,
+            position = this.position,
+            title = this.title,
+            url = this.url,
+            children = childrenWithVirtualFolder
+        )
+
+        // If we're looking at the root, that means we're in the "Desktop Bookmarks" folder.
+        // Rename its child roots and remove the mobile root.
+    } else if (this.guid == BookmarkRoot.Root.id) {
+        return BookmarkNode(
+            type = this.type,
+            guid = this.guid,
+            parentGuid = this.parentGuid,
+            position = this.position,
+            title = rootTitles[this.title],
+            url = this.url,
+            children = processDesktopRoots(this.children)
+        )
+        // If we're looking at one of the desktop roots, change their titles to friendly names.
+    } else if (this.guid in listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id)) {
+        return BookmarkNode(
+            type = this.type,
+            guid = this.guid,
+            parentGuid = this.parentGuid,
+            position = this.position,
+            title = rootTitles[this.title],
+            url = this.url,
+            children = this.children
+        )
+    }
+
+    // Otherwise, just return the node as-is.
+    return this
+}
+
+private suspend fun virtualDesktopFolder(context: Context?): BookmarkNode? {
+    val rootNode = context?.bookmarkStorage()?.getTree(BookmarkRoot.Root.id, false) ?: return null
+    return BookmarkNode(
+        type = rootNode.type,
+        guid = rootNode.guid,
+        parentGuid = rootNode.parentGuid,
+        position = rootNode.position,
+        title = rootTitles[rootNode.title],
+        url = rootNode.url,
+        children = rootNode.children
+    )
+}
+
+/**
+ * Removes 'mobile' root (to avoid a cyclical bookmarks tree in the UI) and renames other roots to friendly titles.
+ */
+private fun processDesktopRoots(roots: List<BookmarkNode>?): List<BookmarkNode>? {
+    if (roots == null) {
+        return null
+    }
+
+    return roots.filter { rootTitles.containsKey(it.title) }.map {
+        BookmarkNode(
+            type = it.type,
+            guid = it.guid,
+            parentGuid = it.parentGuid,
+            position = it.position,
+            title = rootTitles[it.title],
+            url = it.url,
+            children = it.children
+        )
+    }
+}
+
+fun Context?.bookmarkStorage(): PlacesBookmarksStorage? {
+    return this?.components?.core?.bookmarksStorage
+}
+
+fun setRootTitles(context: Context) {
+    rootTitles = mapOf(
+        "root" to context.getString(R.string.library_desktop_bookmarks_root),
+        "menu" to context.getString(R.string.library_desktop_bookmarks_menu),
+        "toolbar" to context.getString(R.string.library_desktop_bookmarks_toolbar),
+        "unfiled" to context.getString(R.string.library_desktop_bookmarks_unfiled)
+    )
+}
+
+operator fun BookmarkNode?.minus(child: String): BookmarkNode {
+    return this!!.copy(children = this.children?.filter { it.guid != child })
+}
+
+operator fun BookmarkNode?.minus(children: Set<BookmarkNode>): BookmarkNode {
+    return this!!.copy(children = this.children?.filter { it !in children })
+}

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt

@@ -30,7 +30,6 @@ import kotlinx.coroutines.Dispatchers.Main
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.launch
 import mozilla.appservices.places.BookmarkRoot
-import mozilla.components.browser.storage.sync.PlacesBookmarksStorage
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
 import mozilla.components.concept.sync.AccountObserver
@@ -45,9 +44,13 @@ import org.mozilla.fenix.R
 import org.mozilla.fenix.components.FenixSnackbar
 import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.components.metrics.MetricController
+import org.mozilla.fenix.ext.bookmarkStorage
 import org.mozilla.fenix.ext.components
+import org.mozilla.fenix.ext.minus
 import org.mozilla.fenix.ext.nav
+import org.mozilla.fenix.ext.setRootTitles
 import org.mozilla.fenix.ext.urlToTrimmedHost
+import org.mozilla.fenix.ext.withOptionalDesktopFolders
 import org.mozilla.fenix.mvi.ActionBusFactory
 import org.mozilla.fenix.mvi.getAutoDisposeObservable
 import org.mozilla.fenix.mvi.getManagedEmitter
@@ -74,9 +77,6 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve
     override val coroutineContext: CoroutineContext
         get() = Main + job
 
-    // Map of internal "root titles" to user friendly labels.
-    private lateinit var rootTitles: Map<String, String>
-
     override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? {
         val view = inflater.inflate(R.layout.fragment_bookmark, container, false)
         bookmarkComponent = BookmarkComponent(
@@ -104,13 +104,7 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve
     // Fill out our title map once we have context.
     override fun onAttach(context: Context) {
         super.onAttach(context)
-
-        rootTitles = mapOf(
-            "root" to context.getString(R.string.library_desktop_bookmarks_root),
-            "menu" to context.getString(R.string.library_desktop_bookmarks_menu),
-            "toolbar" to context.getString(R.string.library_desktop_bookmarks_toolbar),
-            "unfiled" to context.getString(R.string.library_desktop_bookmarks_unfiled)
-        )
+        setRootTitles(context)
     }
 
     override fun onCreate(savedInstanceState: Bundle?) {
@@ -133,7 +127,8 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve
 
     private fun loadInitialBookmarkFolder(currentGuid: String): Job {
         return launch(IO) {
-            currentRoot = bookmarkStorage()?.getTree(currentGuid).withOptionalDesktopFolders() as BookmarkNode
+            currentRoot =
+                context?.bookmarkStorage()?.getTree(currentGuid).withOptionalDesktopFolders(context) as BookmarkNode
 
             launch(Main) {
                 getManagedEmitter<BookmarkChange>().onNext(BookmarkChange.Change(currentRoot!!))
@@ -267,11 +262,13 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve
 
                             allowUndo(
                                 view!!,
-                                getString(R.string.bookmark_deletion_snackbar_message,
-                                    it.item.url?.urlToTrimmedHost(context)),
+                                getString(
+                                    R.string.bookmark_deletion_snackbar_message,
+                                    it.item.url?.urlToTrimmedHost(context)
+                                ),
                                 getString(R.string.bookmark_undo_deletion), { refreshBookmarks() }
                             ) {
-                                bookmarkStorage()?.deleteNode(it.item.guid)
+                                context.bookmarkStorage()?.deleteNode(it.item.guid)
                                 metrics()?.track(Event.RemoveBookmark)
                                 refreshBookmarks()
                             }
@@ -385,12 +382,12 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve
 
     private suspend fun deleteSelectedBookmarks(selected: Set<BookmarkNode> = getSelectedBookmarks()) {
         selected.forEach {
-            bookmarkStorage()?.deleteNode(it.guid)
+            context?.bookmarkStorage()?.deleteNode(it.guid)
         }
     }
 
     private suspend fun refreshBookmarks() {
-        bookmarkStorage()?.getTree(currentRoot!!.guid, false).withOptionalDesktopFolders()
+        context?.bookmarkStorage()?.getTree(currentRoot!!.guid, false).withOptionalDesktopFolders(context)
             ?.let { node ->
                 getManagedEmitter<BookmarkChange>().onNext(BookmarkChange.Change(node))
             }
@@ -402,111 +399,7 @@ class BookmarkFragment : Fragment(), CoroutineScope, BackHandler, AccountObserve
         }
     }
 
-    @SuppressWarnings("ReturnCount")
-    private suspend fun BookmarkNode?.withOptionalDesktopFolders(): BookmarkNode? {
-        // No-op if node is missing.
-        if (this == null) {
-            return null
-        }
-
-        // If we're in the mobile root and logged in, add-in a synthetic "Desktop Bookmarks" folder.
-        if (this.guid == BookmarkRoot.Mobile.id &&
-            activity?.components?.backgroundServices?.accountManager?.authenticatedAccount() != null) {
-            // We're going to make a copy of the mobile node, and add-in a synthetic child folder to the top of the
-            // children's list that contains all of the desktop roots.
-            val childrenWithVirtualFolder: MutableList<BookmarkNode> = mutableListOf()
-            virtualDesktopFolder()?.let { childrenWithVirtualFolder.add(it) }
-
-            this.children?.let { children ->
-                childrenWithVirtualFolder.addAll(children)
-            }
-
-            return BookmarkNode(
-                type = this.type,
-                guid = this.guid,
-                parentGuid = this.parentGuid,
-                position = this.position,
-                title = this.title,
-                url = this.url,
-                children = childrenWithVirtualFolder
-            )
-
-            // If we're looking at the root, that means we're in the "Desktop Bookmarks" folder.
-            // Rename its child roots and remove the mobile root.
-        } else if (this.guid == BookmarkRoot.Root.id) {
-            return BookmarkNode(
-                type = this.type,
-                guid = this.guid,
-                parentGuid = this.parentGuid,
-                position = this.position,
-                title = rootTitles[this.title],
-                url = this.url,
-                children = processDesktopRoots(this.children)
-            )
-            // If we're looking at one of the desktop roots, change their titles to friendly names.
-        } else if (this.guid in listOf(BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id)) {
-            return BookmarkNode(
-                type = this.type,
-                guid = this.guid,
-                parentGuid = this.parentGuid,
-                position = this.position,
-                title = rootTitles[this.title],
-                url = this.url,
-                children = this.children
-            )
-        }
-
-        // Otherwise, just return the node as-is.
-        return this
-    }
-
-    private suspend fun virtualDesktopFolder(): BookmarkNode? {
-        val rootNode = bookmarkStorage()?.getTree(BookmarkRoot.Root.id, false) ?: return null
-        return BookmarkNode(
-            type = rootNode.type,
-            guid = rootNode.guid,
-            parentGuid = rootNode.parentGuid,
-            position = rootNode.position,
-            title = rootTitles[rootNode.title],
-            url = rootNode.url,
-            children = rootNode.children
-        )
-    }
-
-    /**
-     * Removes 'mobile' root (to avoid a cyclical bookmarks tree in the UI) and renames other roots to friendly titles.
-     */
-    private fun processDesktopRoots(roots: List<BookmarkNode>?): List<BookmarkNode>? {
-        if (roots == null) {
-            return null
-        }
-
-        return roots.filter { rootTitles.containsKey(it.title) }.map {
-            BookmarkNode(
-                type = it.type,
-                guid = it.guid,
-                parentGuid = it.parentGuid,
-                position = it.position,
-                title = rootTitles[it.title],
-                url = it.url,
-                children = it.children
-            )
-        }
-    }
-
     private fun metrics(): MetricController? {
         return context?.components?.analytics?.metrics
     }
-
-    private fun bookmarkStorage(): PlacesBookmarksStorage? {
-        return context?.components?.core?.bookmarksStorage
-    }
-}
-
-operator fun BookmarkNode?.minus(child: String): BookmarkNode {
-    return this!!.copy(children = this.children?.filter { it.guid != child })
-}
-
-operator fun BookmarkNode?.minus(children: Set<BookmarkNode>): BookmarkNode {
-    return this!!.copy(children = this.children?.filter { it !in children })
 }

app/src/main/java/org/mozilla/fenix/library/bookmarks/selectfolder/SelectBookmarkFolderAdapter.kt

@@ -7,6 +7,7 @@ package org.mozilla.fenix.library.bookmarks.selectfolder
 import android.view.LayoutInflater
 import android.view.View
 import android.view.ViewGroup
+import androidx.constraintlayout.widget.ConstraintSet
 import androidx.core.content.ContextCompat
 import androidx.recyclerview.widget.RecyclerView
 import kotlinx.android.extensions.LayoutContainer
@@ -79,6 +80,7 @@ class SelectBookmarkFolderAdapter(private val sharedViewModel: BookmarksSharedVi
         init {
             bookmark_favicon.visibility = View.VISIBLE
             bookmark_title.visibility = View.VISIBLE
+            bookmark_url.visibility = View.GONE
             bookmark_separator.visibility = View.GONE
             bookmark_layout.isClickable = true
         }
@@ -91,6 +93,14 @@ class SelectBookmarkFolderAdapter(private val sharedViewModel: BookmarksSharedVi
                     R.attr.neutral.getColorIntFromAttr(containerView!!.context)
                 }
 
+            // Center the bookmark title since we don't have a url
+            val constraintSet = ConstraintSet()
+            constraintSet.clone(bookmark_layout)
+            constraintSet.connect(
+                bookmark_title.id, ConstraintSet.BOTTOM, ConstraintSet.PARENT_ID, ConstraintSet.BOTTOM
+            )
+            constraintSet.applyTo(bookmark_layout)
+
             val backgroundTintList = ContextCompat.getColorStateList(containerView.context, backgroundTint)
             bookmark_favicon.backgroundTintList = backgroundTintList
             val res = if (selected) R.drawable.mozac_ic_check else R.drawable.ic_folder_icon

app/src/main/java/org/mozilla/fenix/library/bookmarks/selectfolder/SelectBookmarkFolderFragment.kt

@@ -4,6 +4,7 @@
 
 package org.mozilla.fenix.library.bookmarks.selectfolder
 
+import android.content.Context
 import android.graphics.PorterDuff
 import android.graphics.PorterDuffColorFilter
 import android.os.Bundle
@@ -19,7 +20,6 @@ import androidx.lifecycle.ViewModelProviders
 import kotlinx.android.synthetic.main.fragment_select_bookmark_folder.*
 import kotlinx.android.synthetic.main.fragment_select_bookmark_folder.view.*
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Dispatchers.IO
 import kotlinx.coroutines.Dispatchers.Main
 import kotlinx.coroutines.Job
@@ -36,6 +36,8 @@ import org.mozilla.fenix.R
 import org.mozilla.fenix.ext.getColorFromAttr
 import org.mozilla.fenix.ext.nav
 import org.mozilla.fenix.ext.requireComponents
+import org.mozilla.fenix.ext.setRootTitles
+import org.mozilla.fenix.ext.withOptionalDesktopFolders
 import org.mozilla.fenix.library.bookmarks.BookmarksSharedViewModel
 import org.mozilla.fenix.library.bookmarks.SignInAction
 import org.mozilla.fenix.library.bookmarks.SignInChange
@@ -58,7 +60,13 @@ class SelectBookmarkFolderFragment : Fragment(), CoroutineScope, AccountObserver
     private lateinit var signInComponent: SignInComponent
 
     override val coroutineContext: CoroutineContext
-        get() = Dispatchers.Main + job
+        get() = Main + job
+
+    // Fill out our title map once we have context.
+    override fun onAttach(context: Context) {
+        super.onAttach(context)
+        setRootTitles(context)
+    }
 
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
@@ -109,7 +117,8 @@ class SelectBookmarkFolderFragment : Fragment(), CoroutineScope, AccountObserver
         checkIfSignedIn()
 
         launch(IO) {
-            bookmarkNode = requireComponents.core.bookmarksStorage.getTree(folderGuid!!, true)
+            bookmarkNode =
+                requireComponents.core.bookmarksStorage.getTree(folderGuid!!, true).withOptionalDesktopFolders(context)
             launch(Main) {
                 (activity as HomeActivity).title = bookmarkNode?.title ?: getString(R.string.library_bookmarks)
                 val adapter = SelectBookmarkFolderAdapter(sharedViewModel)

app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkViewModelTest.kt

@@ -14,6 +14,7 @@ import org.junit.Before
 import org.junit.Test
 import org.mozilla.fenix.TestUtils
 import org.mozilla.fenix.TestUtils.bus
+import org.mozilla.fenix.ext.minus
 import org.mozilla.fenix.mvi.getManagedEmitter
 
 class BookmarkViewModelTest {