From 52199e367bf49441bf98d7664eeb016b71c9e47a Mon Sep 17 00:00:00 2001 From: person808 Date: Thu, 18 Jun 2020 16:01:05 -0700 Subject: [PATCH] For #2165 - Add swipe to refresh gesture to bookmarks view. --- .../library/bookmarks/BookmarkController.kt | 32 ++++++++---- .../library/bookmarks/BookmarkFragment.kt | 15 ++++++ .../bookmarks/BookmarkFragmentInteractor.kt | 4 ++ .../bookmarks/BookmarkFragmentStore.kt | 52 ++++++++++++++++--- .../fenix/library/bookmarks/BookmarkView.kt | 16 +++++- .../fenix/library/bookmarks/DesktopFolders.kt | 5 +- .../main/res/layout/component_bookmark.xml | 22 +++++--- .../bookmarks/BookmarkControllerTest.kt | 25 +++++++-- .../BookmarkFragmentInteractorTest.kt | 9 ++++ .../bookmarks/BookmarkFragmentStoreTest.kt | 37 +++++++++++++ .../library/bookmarks/DesktopFoldersTest.kt | 19 ------- 11 files changed, 184 insertions(+), 52 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt index 8ac76a6727bb..6575129342b6 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt @@ -12,7 +12,6 @@ import androidx.core.content.getSystemService import androidx.navigation.NavController import androidx.navigation.NavDirections import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.Dispatchers.Main import kotlinx.coroutines.launch import mozilla.components.concept.engine.prompt.ShareData @@ -44,6 +43,7 @@ interface BookmarkController { fun handleOpeningBookmark(item: BookmarkNode, mode: BrowsingMode) fun handleBookmarkDeletion(nodes: Set, eventType: Event) fun handleBookmarkFolderDeletion(node: BookmarkNode) + fun handleRequestSync() fun handleBackPressed() } @@ -55,6 +55,7 @@ class DefaultBookmarkController( private val store: BookmarkFragmentStore, private val sharedViewModel: BookmarksSharedViewModel, private val loadBookmarkNode: suspend (String) -> BookmarkNode?, + private val syncBookmarks: suspend () -> Unit, private val showSnackbar: (String) -> Unit, private val deleteBookmarkNodes: (Set, Event) -> Unit, private val deleteBookmarkFolder: (BookmarkNode) -> Unit, @@ -92,6 +93,10 @@ class DefaultBookmarkController( } override fun handleBookmarkSelected(node: BookmarkNode) { + if (store.state.mode is BookmarkFragmentState.Mode.Syncing) { + return + } + when (node.inRoots()) { true -> showSnackbar(resources.getString(R.string.bookmark_cannot_edit_root)) false -> store.dispatch(BookmarkFragmentAction.Select(node)) @@ -132,16 +137,25 @@ class DefaultBookmarkController( deleteBookmarkFolder(node) } + override fun handleRequestSync() { + scope.launch { + store.dispatch(BookmarkFragmentAction.StartSync) + syncBookmarks.invoke() + store.dispatch(BookmarkFragmentAction.FinishSync) + } + } + override fun handleBackPressed() { invokePendingDeletion.invoke() - val parentGuid = store.state.tree?.parentGuid - if (parentGuid == null) { - navController.popBackStack() - } else { - scope.launch(IO) { - context.bookmarkStorage.getBookmark(parentGuid)?.let { - handleBookmarkExpand(it) - } + scope.launch(Main) { + val parentGuid = store.state.guidBackstack.findLast { guid -> + store.state.tree?.guid != guid && context.bookmarkStorage.getBookmark(guid) != null + } + if (parentGuid == null) { + navController.popBackStack() + } else { + val parent = context.bookmarkStorage.getBookmark(parentGuid)!! + handleBookmarkExpand(parent) } } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt index c9e78b1ca8bc..75ecccbaab47 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt @@ -34,6 +34,7 @@ import mozilla.components.concept.engine.prompt.ShareData import mozilla.components.concept.storage.BookmarkNode import mozilla.components.concept.storage.BookmarkNodeType import mozilla.components.lib.state.ext.consumeFrom +import mozilla.components.service.fxa.sync.SyncReason import mozilla.components.support.base.feature.UserInteractionHandler import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R @@ -89,6 +90,7 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan store = bookmarkStore, sharedViewModel = sharedViewModel, loadBookmarkNode = ::loadBookmarkNode, + syncBookmarks = ::syncBookmarks, showSnackbar = ::showSnackBarWithText, deleteBookmarkNodes = ::deleteMulti, deleteBookmarkFolder = ::showRemoveFolderDialog, @@ -393,4 +395,17 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan } } } + + private suspend fun syncBookmarks() { + invokePendingDeletion() + requireComponents.backgroundServices.accountManager.syncNowAsync(SyncReason.User).await() + // The current bookmark node we are viewing may be made invalid after syncing so we + // check if the current node is valid and if it isn't we find the nearest valid ancestor + // and open it + val validAncestorGuid = bookmarkStore.state.guidBackstack.findLast { guid -> + requireContext().bookmarkStorage.getBookmark(guid) != null + } ?: BookmarkRoot.Mobile.id + val node = requireContext().bookmarkStorage.getBookmark(validAncestorGuid)!! + bookmarkInteractor.open(node) + } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt index 3fbc2c570c42..1a9cc084233e 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt @@ -116,4 +116,8 @@ class BookmarkFragmentInteractor( override fun deselect(item: BookmarkNode) { bookmarksController.handleBookmarkDeselected(item) } + + override fun onRequestSync() { + bookmarksController.handleRequestSync() + } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStore.kt index 8bdcd4617cb9..d445648d4b74 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStore.kt @@ -20,10 +20,14 @@ class BookmarkFragmentStore( * The complete state of the bookmarks tree and multi-selection mode * @property tree The current tree of bookmarks, if one is loaded * @property mode The current bookmark multi-selection mode + * @property guidBackstack A set of guids for bookmark nodes we have visited. Used to traverse back + * up the tree after a sync. + * @property isLoading true if bookmarks are still being loaded from disk */ data class BookmarkFragmentState( val tree: BookmarkNode?, val mode: Mode = Mode.Normal(), + val guidBackstack: Set = linkedSetOf(), val isLoading: Boolean = true ) : State { sealed class Mode { @@ -31,6 +35,7 @@ data class BookmarkFragmentState( data class Normal(val showMenu: Boolean = true) : Mode() data class Selecting(override val selectedItems: Set) : Mode() + object Syncing : Mode() } } @@ -42,6 +47,8 @@ sealed class BookmarkFragmentAction : Action { data class Select(val item: BookmarkNode) : BookmarkFragmentAction() data class Deselect(val item: BookmarkNode) : BookmarkFragmentAction() object DeselectAll : BookmarkFragmentAction() + object StartSync : BookmarkFragmentAction() + object FinishSync : BookmarkFragmentAction() } /** @@ -56,16 +63,26 @@ private fun bookmarkFragmentStateReducer( ): BookmarkFragmentState { return when (action) { is BookmarkFragmentAction.Change -> { + // If we change to a node we have already visited, we pop the backstack until the node + // is the last item. If we haven't visited the node yet, we just add it to the end of the + // backstack + val backstack = state.guidBackstack.takeWhile { guid -> + guid != action.tree.guid + } + action.tree.guid + val items = state.mode.selectedItems.filter { it in action.tree } state.copy( tree = action.tree, - mode = if (BookmarkRoot.Root.id == action.tree.guid) { - BookmarkFragmentState.Mode.Normal(false) - } else if (items.isEmpty()) { - BookmarkFragmentState.Mode.Normal() - } else { - BookmarkFragmentState.Mode.Selecting(items.toSet()) + mode = when { + state.mode is BookmarkFragmentState.Mode.Syncing -> { + BookmarkFragmentState.Mode.Syncing + } + items.isEmpty() -> { + BookmarkFragmentState.Mode.Normal(shouldShowMenu(action.tree.guid)) + } + else -> BookmarkFragmentState.Mode.Selecting(items.toSet()) }, + guidBackstack = backstack.toSet(), isLoading = false ) } @@ -81,11 +98,30 @@ private fun bookmarkFragmentStateReducer( } ) } - BookmarkFragmentAction.DeselectAll -> - state.copy(mode = BookmarkFragmentState.Mode.Normal()) + is BookmarkFragmentAction.DeselectAll -> + state.copy( + mode = if (state.mode is BookmarkFragmentState.Mode.Syncing) { + BookmarkFragmentState.Mode.Syncing + } else { + BookmarkFragmentState.Mode.Normal() + } + ) + is BookmarkFragmentAction.StartSync -> + state.copy( + mode = BookmarkFragmentState.Mode.Syncing + ) + is BookmarkFragmentAction.FinishSync -> + state.copy( + mode = BookmarkFragmentState.Mode.Normal( + showMenu = shouldShowMenu(state.tree?.guid) + ) + ) } } +private fun shouldShowMenu(currentGuid: String?): Boolean = + BookmarkRoot.Root.id != currentGuid + operator fun BookmarkNode.contains(item: BookmarkNode): Boolean { return children?.contains(item) ?: false } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt index c43293925132..2500f374ef8b 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkView.kt @@ -93,6 +93,12 @@ interface BookmarkViewInteractor : SelectionInteractor { * */ fun onBackPressed() + + /** + * Handles user requested sync of bookmarks. + * + */ + fun onRequestSync() } class BookmarkView( @@ -117,13 +123,18 @@ class BookmarkView( view.bookmark_folders_sign_in.setOnClickListener { navController.navigate(NavGraphDirections.actionGlobalTurnOnSync()) } + view.swipe_refresh.setOnRefreshListener { + interactor.onRequestSync() + } } fun update(state: BookmarkFragmentState) { tree = state.tree if (state.mode != mode) { mode = state.mode - interactor.onSelectionModeSwitch(mode) + if (mode is BookmarkFragmentState.Mode.Normal || mode is BookmarkFragmentState.Mode.Selecting) { + interactor.onSelectionModeSwitch(mode) + } } bookmarkAdapter.updateData(state.tree, mode) @@ -149,6 +160,9 @@ class BookmarkView( } } view.bookmarks_progress_bar.isVisible = state.isLoading + view.swipe_refresh.isEnabled = + state.mode is BookmarkFragmentState.Mode.Normal || state.mode is BookmarkFragmentState.Mode.Syncing + view.swipe_refresh.isRefreshing = state.mode is BookmarkFragmentState.Mode.Syncing } override fun onBackPressed(): Boolean { diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt index 24e4ac6b12e0..f53ce4b917b0 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/DesktopFolders.kt @@ -50,7 +50,7 @@ class DesktopFolders( val childrenWithVirtualFolder = listOfNotNull(virtualDesktopFolder()) + node.children.orEmpty() - node.copy(children = childrenWithVirtualFolder, parentGuid = null) + node.copy(children = childrenWithVirtualFolder) } BookmarkRoot.Root.id -> node.copy( @@ -59,8 +59,7 @@ class DesktopFolders( restructureMobileRoots(node.children) } else { restructureDesktopRoots(node.children) - }, - parentGuid = BookmarkRoot.Mobile.id + } ) BookmarkRoot.Menu.id, BookmarkRoot.Toolbar.id, BookmarkRoot.Unfiled.id -> // If we're looking at one of the desktop roots, change their titles to friendly names. diff --git a/app/src/main/res/layout/component_bookmark.xml b/app/src/main/res/layout/component_bookmark.xml index b5bd21b504b3..fe859e744d2f 100644 --- a/app/src/main/res/layout/component_bookmark.xml +++ b/app/src/main/res/layout/component_bookmark.xml @@ -9,15 +9,21 @@ android:layout_width="match_parent" android:layout_height="match_parent"> - + android:layout_height="wrap_content" > + + + BookmarkNode? = mockk(relaxed = true) + private val syncBookmarks: suspend () -> Unit = mockk(relaxed = true) private val showSnackbar: (String) -> Unit = mockk(relaxed = true) private val deleteBookmarkNodes: (Set, Event) -> Unit = mockk(relaxed = true) private val deleteBookmarkFolder: (BookmarkNode) -> Unit = mockk(relaxed = true) @@ -118,6 +120,7 @@ class BookmarkControllerTest { store = bookmarkStore, sharedViewModel = sharedViewModel, loadBookmarkNode = loadBookmarkNode, + syncBookmarks = syncBookmarks, showSnackbar = showSnackbar, deleteBookmarkNodes = deleteBookmarkNodes, deleteBookmarkFolder = deleteBookmarkFolder, @@ -237,10 +240,12 @@ class BookmarkControllerTest { } @Test - fun `handleBookmarkSelected does not dispatch Select action for bookmark roots`() { - controller.handleBookmarkSelected(root) + fun `handleBookmarkSelected does not select in Syncing mode`() { + every { bookmarkStore.state.mode } returns BookmarkFragmentState.Mode.Syncing + + controller.handleBookmarkSelected(item) - verify { bookmarkStore wasNot called } + verify { bookmarkStore.dispatch(BookmarkFragmentAction.Select(item)) wasNot called } } @Test @@ -329,7 +334,19 @@ class BookmarkControllerTest { } @Test - fun `handleBackPressed from root bookmark node should trigger handleBackPressed in NavController`() { + fun `handleRequestSync dispatches actions in the correct order`() { + controller.handleRequestSync() + + coVerifyOrder { + bookmarkStore.dispatch(BookmarkFragmentAction.StartSync) + syncBookmarks.invoke() + bookmarkStore.dispatch(BookmarkFragmentAction.FinishSync) + } + } + + @Test + fun `handleBackPressed with one item in backstack should trigger handleBackPressed in NavController`() { + every { bookmarkStore.state.guidBackstack } returns linkedSetOf(tree.guid) every { bookmarkStore.state.tree } returns tree controller.handleBackPressed() diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt index 03dde953c339..6aba5b365e77 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt @@ -201,4 +201,13 @@ class BookmarkFragmentInteractorTest { bookmarkController.handleBackPressed() } } + + @Test + fun `request a sync`() { + interactor.onRequestSync() + + verify { + bookmarkController.handleRequestSync() + } + } } diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStoreTest.kt index 03d49f141de2..6dbf3c0f446d 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentStoreTest.kt @@ -41,6 +41,30 @@ class BookmarkFragmentStoreTest { assertEquals(store.state.mode, initialState.mode) } + @Test + fun `changing the tree of bookmarks adds the tree to the visited nodes`() = runBlocking { + val initialState = BookmarkFragmentState(null) + val store = BookmarkFragmentStore(initialState) + + store.dispatch(BookmarkFragmentAction.Change(tree)).join() + store.dispatch(BookmarkFragmentAction.Change(subfolder)).join() + + assertEquals(linkedSetOf(tree.guid, subfolder.guid), store.state.guidBackstack) + } + + @Test + fun `changing to a node that is in the backstack removes backstack items after that node`() = runBlocking { + val initialState = BookmarkFragmentState( + null, + guidBackstack = linkedSetOf(tree.guid, subfolder.guid, item.guid) + ) + val store = BookmarkFragmentStore(initialState) + + store.dispatch(BookmarkFragmentAction.Change(tree)).join() + + assertEquals(linkedSetOf(tree.guid), store.state.guidBackstack) + } + @Test fun `change the tree of bookmarks to the same value`() = runBlocking { val initialState = BookmarkFragmentState(tree) @@ -177,6 +201,19 @@ class BookmarkFragmentStoreTest { assertEquals(store.state.mode, BookmarkFragmentState.Mode.Normal(false)) } + @Test + fun `changing the tree or deselecting in Syncing mode should stay in Syncing mode`() = runBlocking { + val initialState = BookmarkFragmentState(tree) + val store = BookmarkFragmentStore(initialState) + + store.dispatch(BookmarkFragmentAction.StartSync).join() + store.dispatch(BookmarkFragmentAction.Change(childItem)) + assertEquals(BookmarkFragmentState.Mode.Syncing, store.state.mode) + + store.dispatch(BookmarkFragmentAction.DeselectAll).join() + assertEquals(BookmarkFragmentState.Mode.Syncing, store.state.mode) + } + private val item = BookmarkNode(BookmarkNodeType.ITEM, "456", "123", 0, "Mozilla", "http://mozilla.org", null) private val separator = BookmarkNode(BookmarkNodeType.SEPARATOR, "789", "123", 1, null, null, null) private val subfolder = BookmarkNode(BookmarkNodeType.FOLDER, "987", "123", 0, "Subfolder", null, listOf()) diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/DesktopFoldersTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/DesktopFoldersTest.kt index d5b240caf570..603048a287f4 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/DesktopFoldersTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/DesktopFoldersTest.kt @@ -5,7 +5,6 @@ package org.mozilla.fenix.library.bookmarks import android.content.Context -import io.mockk.coEvery import io.mockk.every import io.mockk.mockk import io.mockk.spyk @@ -20,7 +19,6 @@ 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.ext.components import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -67,23 +65,6 @@ class DesktopFoldersTest { assertEquals(testContext.getString(R.string.library_desktop_bookmarks_unfiled), desktopFolders.withRootTitle(mockNodeWithTitle("unfiled")).title) } - @Test - fun `withOptionalDesktopFolders makes mobile bookmarks node the root of the tree`() = runBlocking { - coEvery { context.bookmarkStorage.getTree(BookmarkRoot.Root.id, any()) } returns basicNode.copy() - - val node = basicNode.copy(guid = BookmarkRoot.Mobile.id, parentGuid = BookmarkRoot.Root.id) - val desktopFolders = DesktopFolders(context, showMobileRoot = true) - - assertEquals(null, desktopFolders.withOptionalDesktopFolders(node).parentGuid) - } - - @Test - fun `withOptionalDesktopFolders makes the root node a child of mobile bookmarks`() = runBlocking { - val desktopFolders = DesktopFolders(context, showMobileRoot = true) - - assertEquals(BookmarkRoot.Mobile.id, desktopFolders.withOptionalDesktopFolders(basicNode).parentGuid) - } - @Test fun `withOptionalDesktopFolders other node`() = runBlocking { val node = basicNode.copy(guid = "12345")