From 30c7b5ea5ef5ae270c3e592d3d893d34e9047764 Mon Sep 17 00:00:00 2001 From: Kainalu Hagiwara Date: Tue, 28 Jul 2020 16:57:43 -0700 Subject: [PATCH] For #12831 - Disable SwipeRefreshLayout while swiping a bookmark. --- .../library/bookmarks/BookmarkController.kt | 10 ++ .../bookmarks/BookmarkFragmentInteractor.kt | 8 ++ .../bookmarks/BookmarkFragmentStore.kt | 71 ++++++----- .../library/bookmarks/BookmarkTouchHelper.kt | 9 ++ .../fenix/library/bookmarks/BookmarkView.kt | 13 +- .../bookmarks/BookmarkControllerTest.kt | 18 +++ .../BookmarkFragmentInteractorTest.kt | 18 +++ .../bookmarks/BookmarkFragmentStoreTest.kt | 113 +++++++++++++++--- 8 files changed, 215 insertions(+), 45 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 add69b004..5d203028f 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 @@ -45,6 +45,8 @@ interface BookmarkController { fun handleBookmarkFolderDeletion(nodes: Set) fun handleRequestSync() fun handleBackPressed() + fun handleStartSwipingItem() + fun handleStopSwipingItem() } @Suppress("TooManyFunctions") @@ -169,6 +171,14 @@ class DefaultBookmarkController( } } + override fun handleStartSwipingItem() { + store.dispatch(BookmarkFragmentAction.SwipeRefreshAvailabilityChanged(false)) + } + + override fun handleStopSwipingItem() { + store.dispatch(BookmarkFragmentAction.SwipeRefreshAvailabilityChanged(true)) + } + private fun openInNewTab( searchTermOrURL: String, newTab: Boolean, 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 4f5e757fc..3a1a4f252 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 @@ -120,4 +120,12 @@ class BookmarkFragmentInteractor( override fun onRequestSync() { bookmarksController.handleRequestSync() } + + override fun onStartSwipingItem() { + bookmarksController.handleStartSwipingItem() + } + + override fun onStopSwipingItem() { + bookmarksController.handleStopSwipingItem() + } } 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 d2c2341de..e1c4a8f43 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 @@ -23,12 +23,14 @@ class BookmarkFragmentStore( * @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 + * @property isSwipeToRefreshEnabled true if swipe to refresh should be enabled */ data class BookmarkFragmentState( val tree: BookmarkNode?, val mode: Mode = Mode.Normal(), val guidBackstack: List = emptyList(), - val isLoading: Boolean = true + val isLoading: Boolean = true, + val isSwipeToRefreshEnabled: Boolean = true ) : State { sealed class Mode { open val selectedItems = emptySet() @@ -49,6 +51,7 @@ sealed class BookmarkFragmentAction : Action { object DeselectAll : BookmarkFragmentAction() object StartSync : BookmarkFragmentAction() object FinishSync : BookmarkFragmentAction() + data class SwipeRefreshAvailabilityChanged(val enabled: Boolean) : BookmarkFragmentAction() } /** @@ -71,31 +74,37 @@ private fun bookmarkFragmentStateReducer( } + action.tree.guid val items = state.mode.selectedItems.filter { it in action.tree } + val 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()) + } state.copy( tree = action.tree, - 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()) - }, + mode = mode, guidBackstack = backstack, - isLoading = false + isLoading = false, + isSwipeToRefreshEnabled = mode !is BookmarkFragmentState.Mode.Selecting ) } - is BookmarkFragmentAction.Select -> - state.copy(mode = BookmarkFragmentState.Mode.Selecting(state.mode.selectedItems + action.item)) + is BookmarkFragmentAction.Select -> state.copy( + mode = BookmarkFragmentState.Mode.Selecting(state.mode.selectedItems + action.item), + isSwipeToRefreshEnabled = false + ) is BookmarkFragmentAction.Deselect -> { val items = state.mode.selectedItems - action.item + val mode = if (items.isEmpty()) { + BookmarkFragmentState.Mode.Normal() + } else { + BookmarkFragmentState.Mode.Selecting(items) + } state.copy( - mode = if (items.isEmpty()) { - BookmarkFragmentState.Mode.Normal() - } else { - BookmarkFragmentState.Mode.Selecting(items) - } + mode = mode, + isSwipeToRefreshEnabled = mode !is BookmarkFragmentState.Mode.Selecting ) } is BookmarkFragmentAction.DeselectAll -> @@ -104,18 +113,22 @@ private fun bookmarkFragmentStateReducer( 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) - ) + }, + isSwipeToRefreshEnabled = true ) + is BookmarkFragmentAction.StartSync -> state.copy( + mode = BookmarkFragmentState.Mode.Syncing, + isSwipeToRefreshEnabled = true + ) + is BookmarkFragmentAction.FinishSync -> state.copy( + mode = BookmarkFragmentState.Mode.Normal( + showMenu = shouldShowMenu(state.tree?.guid) + ), + isSwipeToRefreshEnabled = true + ) + is BookmarkFragmentAction.SwipeRefreshAvailabilityChanged -> state.copy( + isSwipeToRefreshEnabled = action.enabled && state.mode !is BookmarkFragmentState.Mode.Selecting + ) } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkTouchHelper.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkTouchHelper.kt index 1cecff4fe..b31b99fa6 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkTouchHelper.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkTouchHelper.kt @@ -116,6 +116,15 @@ class BookmarkTouchCallback( target: RecyclerView.ViewHolder ): Boolean = false + override fun onSelectedChanged(viewHolder: RecyclerView.ViewHolder?, actionState: Int) { + super.onSelectedChanged(viewHolder, actionState) + if (actionState == ItemTouchHelper.ACTION_STATE_SWIPE) { + interactor.onStartSwipingItem() + } else { + interactor.onStopSwipingItem() + } + } + private fun setBounds( background: Drawable, backgroundBounds: Rect, 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 a64f4b6ee..9a0e4644a 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 @@ -98,6 +98,16 @@ interface BookmarkViewInteractor : SelectionInteractor { * */ fun onRequestSync() + + /** + * Handles the start of a swipe on a bookmark. + */ + fun onStartSwipingItem() + + /** + * Handles the end of a swipe on a bookmark. + */ + fun onStopSwipingItem() } class BookmarkView( @@ -152,8 +162,7 @@ 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.isEnabled = state.isSwipeToRefreshEnabled view.swipe_refresh.isRefreshing = state.mode is BookmarkFragmentState.Mode.Syncing } diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt index d55eb3818..2d0f2deb4 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt @@ -337,4 +337,22 @@ class BookmarkControllerTest { navController.popBackStack() } } + + @Test + fun `handleStartSwipingItem disables swipe to refresh`() { + controller.handleStartSwipingItem() + + verify { + bookmarkStore.dispatch(BookmarkFragmentAction.SwipeRefreshAvailabilityChanged(false)) + } + } + + @Test + fun `handleStopSwipingItem attempts to enable swipe to refresh`() { + controller.handleStopSwipingItem() + + verify { + bookmarkStore.dispatch(BookmarkFragmentAction.SwipeRefreshAvailabilityChanged(true)) + } + } } 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 5f0a409cc..d640a9c78 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 @@ -210,4 +210,22 @@ class BookmarkFragmentInteractorTest { bookmarkController.handleRequestSync() } } + + @Test + fun `start swiping an item`() { + interactor.onStartSwipingItem() + + verify { + bookmarkController.handleStartSwipingItem() + } + } + + @Test + fun `stop swiping an item`() { + interactor.onStopSwipingItem() + + verify { + bookmarkController.handleStopSwipingItem() + } + } } 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 f0e6f629c..321558e07 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 @@ -80,32 +80,63 @@ class BookmarkFragmentStoreTest { @Test fun `ensure selected items remain selected after a tree change`() = runBlocking { - val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder))) + val initialState = BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder)), + isLoading = false, + isSwipeToRefreshEnabled = false + ) val store = BookmarkFragmentStore(initialState) store.dispatch(BookmarkFragmentAction.Change(newTree)).join() - assertEquals(store.state.tree, newTree) - assertEquals(store.state.mode, BookmarkFragmentState.Mode.Selecting(setOf(subfolder))) + assertEquals( + store.state, + BookmarkFragmentState( + newTree, + BookmarkFragmentState.Mode.Selecting(setOf(subfolder)), + guidBackstack = listOf(tree.guid), + isLoading = false, + isSwipeToRefreshEnabled = false + ) + ) } @Test - fun `select and deselect bookmarks changes the mode`() = runBlocking { + fun `select and deselect a single bookmark changes the mode and swipe to refresh state`() = runBlocking { val initialState = BookmarkFragmentState(tree) val store = BookmarkFragmentStore(initialState) store.dispatch(BookmarkFragmentAction.Select(childItem)).join() - assertEquals(store.state, BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(childItem)))) + assertEquals( + store.state, + BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Selecting(setOf(childItem)), + isSwipeToRefreshEnabled = false + ) + ) store.dispatch(BookmarkFragmentAction.Deselect(childItem)).join() - assertEquals(store.state, BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Normal())) + assertEquals( + store.state, + BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Normal(), + isSwipeToRefreshEnabled = true + ) + ) } @Test fun `selecting the same item twice does nothing`() = runBlocking { - val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder))) + val initialState = BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder)), + isSwipeToRefreshEnabled = false + ) val store = BookmarkFragmentStore(initialState) store.dispatch(BookmarkFragmentAction.Select(item)).join() @@ -115,7 +146,11 @@ class BookmarkFragmentStoreTest { @Test fun `deselecting an unselected bookmark does nothing`() = runBlocking { - val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(childItem))) + val initialState = BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Selecting(setOf(childItem)), + isSwipeToRefreshEnabled = false + ) val store = BookmarkFragmentStore(initialState) store.dispatch(BookmarkFragmentAction.Deselect(item)).join() @@ -134,14 +169,25 @@ class BookmarkFragmentStoreTest { } @Test - fun `deselect all bookmarks changes the mode`() = runBlocking { - val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(item, childItem))) - val store = BookmarkFragmentStore(initialState) + fun `deselect all bookmarks changes the mode and updates swipe to refresh state`() = + runBlocking { + val initialState = BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Selecting(setOf(item, childItem)), + isSwipeToRefreshEnabled = false + ) + val store = BookmarkFragmentStore(initialState) - store.dispatch(BookmarkFragmentAction.DeselectAll).join() + store.dispatch(BookmarkFragmentAction.DeselectAll).join() - assertEquals(store.state, initialState.copy(mode = BookmarkFragmentState.Mode.Normal())) - } + assertEquals( + store.state, + initialState.copy( + mode = BookmarkFragmentState.Mode.Normal(), + isSwipeToRefreshEnabled = true + ) + ) + } @Test fun `deselect all bookmarks when none are selected`() = runBlocking { @@ -214,6 +260,45 @@ class BookmarkFragmentStoreTest { assertEquals(BookmarkFragmentState.Mode.Syncing, store.state.mode) } + @Test + fun `enabling swipe to refresh in Normal mode works`() = runBlocking { + val initialState = BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Normal(), + isSwipeToRefreshEnabled = false + ) + val store = BookmarkFragmentStore(initialState) + + store.dispatch(BookmarkFragmentAction.SwipeRefreshAvailabilityChanged(true)).join() + assertEquals(true, store.state.isSwipeToRefreshEnabled) + } + + @Test + fun `enabling swipe to refresh in Syncing mode works`() = runBlocking { + val initialState = BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Syncing, + isSwipeToRefreshEnabled = false + ) + val store = BookmarkFragmentStore(initialState) + + store.dispatch(BookmarkFragmentAction.SwipeRefreshAvailabilityChanged(true)).join() + assertEquals(true, store.state.isSwipeToRefreshEnabled) + } + + @Test + fun `enabling swipe to refresh in Selecting mode does not work`() = runBlocking { + val initialState = BookmarkFragmentState( + tree, + BookmarkFragmentState.Mode.Selecting(emptySet()), + isSwipeToRefreshEnabled = false + ) + val store = BookmarkFragmentStore(initialState) + + store.dispatch(BookmarkFragmentAction.SwipeRefreshAvailabilityChanged(true)).join() + assertEquals(false, store.state.isSwipeToRefreshEnabled) + } + 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())