For #15362: Remove swipe to delete for bookmarks (#16646)

Removed now obsolete feature flag and tests.
Removed obsolete swipe refresh state from BookmarkFragmentState
Also adapted tests and remove obsolete ones.
upstream-sync
Philipp Klein 4 years ago committed by GitHub
parent 9420febe59
commit 2be3fd05f2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -26,11 +26,6 @@ object FeatureFlags {
*/
const val externalDownloadManager = true
/**
* Enables swipe to delete in bookmarks
*/
val bookmarkSwipeToDelete = Config.channel.isNightlyOrDebug
/**
* Enables ETP cookie purging
*/

@ -45,8 +45,6 @@ interface BookmarkController {
fun handleBookmarkFolderDeletion(nodes: Set<BookmarkNode>)
fun handleRequestSync()
fun handleBackPressed()
fun handleStartSwipingItem()
fun handleStopSwipingItem()
}
@Suppress("TooManyFunctions")
@ -171,14 +169,6 @@ 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,

@ -120,12 +120,4 @@ class BookmarkFragmentInteractor(
override fun onRequestSync() {
bookmarksController.handleRequestSync()
}
override fun onStartSwipingItem() {
bookmarksController.handleStartSwipingItem()
}
override fun onStopSwipingItem() {
bookmarksController.handleStopSwipingItem()
}
}

@ -24,14 +24,12 @@ 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<String> = emptyList(),
val isLoading: Boolean = true,
val isSwipeToRefreshEnabled: Boolean = true
val isLoading: Boolean = true
) : State {
sealed class Mode : SelectionHolder<BookmarkNode> {
override val selectedItems = emptySet<BookmarkNode>()
@ -52,7 +50,6 @@ sealed class BookmarkFragmentAction : Action {
object DeselectAll : BookmarkFragmentAction()
object StartSync : BookmarkFragmentAction()
object FinishSync : BookmarkFragmentAction()
data class SwipeRefreshAvailabilityChanged(val enabled: Boolean) : BookmarkFragmentAction()
}
/**
@ -88,13 +85,11 @@ private fun bookmarkFragmentStateReducer(
tree = action.tree,
mode = mode,
guidBackstack = backstack,
isLoading = false,
isSwipeToRefreshEnabled = mode !is BookmarkFragmentState.Mode.Selecting
isLoading = false
)
}
is BookmarkFragmentAction.Select -> state.copy(
mode = BookmarkFragmentState.Mode.Selecting(state.mode.selectedItems + action.item),
isSwipeToRefreshEnabled = false
mode = BookmarkFragmentState.Mode.Selecting(state.mode.selectedItems + action.item)
)
is BookmarkFragmentAction.Deselect -> {
val items = state.mode.selectedItems - action.item
@ -104,8 +99,7 @@ private fun bookmarkFragmentStateReducer(
BookmarkFragmentState.Mode.Selecting(items)
}
state.copy(
mode = mode,
isSwipeToRefreshEnabled = mode !is BookmarkFragmentState.Mode.Selecting
mode = mode
)
}
is BookmarkFragmentAction.DeselectAll ->
@ -114,21 +108,15 @@ private fun bookmarkFragmentStateReducer(
BookmarkFragmentState.Mode.Syncing
} else {
BookmarkFragmentState.Mode.Normal()
},
isSwipeToRefreshEnabled = true
}
)
is BookmarkFragmentAction.StartSync -> state.copy(
mode = BookmarkFragmentState.Mode.Syncing,
isSwipeToRefreshEnabled = true
mode = BookmarkFragmentState.Mode.Syncing
)
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
)
)
}
}

@ -1,146 +0,0 @@
/* 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.library.bookmarks
import android.graphics.Canvas
import android.graphics.Rect
import android.graphics.drawable.Drawable
import androidx.appcompat.content.res.AppCompatResources
import androidx.recyclerview.widget.ItemTouchHelper
import androidx.recyclerview.widget.RecyclerView
import mozilla.components.concept.storage.BookmarkNodeType
import mozilla.components.support.ktx.android.content.getColorFromAttr
import mozilla.components.support.ktx.android.content.getDrawableWithTint
import mozilla.components.support.ktx.android.util.dpToPx
import org.mozilla.fenix.R
import org.mozilla.fenix.home.sessioncontrol.SwipeToDeleteCallback
import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkNodeViewHolder
import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkSeparatorViewHolder
class BookmarkTouchHelper(interactor: BookmarkViewInteractor) :
ItemTouchHelper(BookmarkTouchCallback(interactor))
class BookmarkTouchCallback(
private val interactor: BookmarkViewInteractor
) : ItemTouchHelper.SimpleCallback(0, ItemTouchHelper.LEFT or ItemTouchHelper.RIGHT) {
override fun getSwipeDirs(
recyclerView: RecyclerView,
viewHolder: RecyclerView.ViewHolder
): Int {
// Swiping separators is currently not supported.
if (viewHolder is BookmarkSeparatorViewHolder) {
return 0
}
val item = (viewHolder as BookmarkNodeViewHolder).item
return if (item?.inRoots() == true) {
0
} else {
super.getSwipeDirs(recyclerView, viewHolder)
}
}
/**
* Delete the bookmark when swiped.
*/
override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {
val item = (viewHolder as BookmarkNodeViewHolder).item
item?.let {
interactor.onDelete(setOf(it))
// We need to notify the adapter of a change if we swipe a folder to prevent
// visual bugs when cancelling deletion of a folder
if (item.type == BookmarkNodeType.FOLDER) {
viewHolder.bindingAdapter?.notifyItemChanged(viewHolder.bindingAdapterPosition)
}
}
}
override fun onChildDraw(
c: Canvas,
recyclerView: RecyclerView,
viewHolder: RecyclerView.ViewHolder,
dX: Float,
dY: Float,
actionState: Int,
isCurrentlyActive: Boolean
) {
super.onChildDraw(c, recyclerView, viewHolder, dX, dY, actionState, isCurrentlyActive)
val icon = recyclerView.context.getDrawableWithTint(
R.drawable.ic_delete,
recyclerView.context.getColorFromAttr(R.attr.destructive)
)!!
val background = AppCompatResources.getDrawable(
recyclerView.context,
R.drawable.swipe_delete_background
)!!
val margin =
SwipeToDeleteCallback.MARGIN.dpToPx(recyclerView.resources.displayMetrics)
val cellHeight = viewHolder.itemView.bottom - viewHolder.itemView.top
val iconTop = viewHolder.itemView.top + (cellHeight - icon.intrinsicHeight) / 2
val iconBottom = iconTop + icon.intrinsicHeight
when {
dX > 0 -> { // Swiping to the right
val backgroundBounds = Rect(
viewHolder.itemView.left, viewHolder.itemView.top,
(viewHolder.itemView.left + dX).toInt() + SwipeToDeleteCallback.BACKGROUND_CORNER_OFFSET,
viewHolder.itemView.bottom
)
val iconLeft = viewHolder.itemView.left + margin
val iconRight = viewHolder.itemView.left + margin + icon.intrinsicWidth
val iconBounds = Rect(iconLeft, iconTop, iconRight, iconBottom)
setBounds(background, backgroundBounds, icon, iconBounds)
draw(background, icon, c)
}
dX < 0 -> { // Swiping to the left
val backgroundBounds = Rect(
(viewHolder.itemView.right + dX).toInt() - SwipeToDeleteCallback.BACKGROUND_CORNER_OFFSET,
viewHolder.itemView.top, viewHolder.itemView.right, viewHolder.itemView.bottom
)
val iconLeft = viewHolder.itemView.right - margin - icon.intrinsicWidth
val iconRight = viewHolder.itemView.right - margin
val iconBounds = Rect(iconLeft, iconTop, iconRight, iconBottom)
setBounds(background, backgroundBounds, icon, iconBounds)
draw(background, icon, c)
}
else -> { // View not swiped
val bounds = Rect(0, 0, 0, 0)
setBounds(background, bounds, icon, bounds)
}
}
}
override fun onMove(
recyclerView: RecyclerView,
viewHolder: RecyclerView.ViewHolder,
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,
icon: Drawable,
iconBounds: Rect
) {
background.bounds = backgroundBounds
icon.bounds = iconBounds
}
private fun draw(background: Drawable, icon: Drawable, c: Canvas) {
background.draw(c)
icon.draw(c)
}
}

@ -13,7 +13,6 @@ import kotlinx.android.synthetic.main.component_bookmark.view.*
import mozilla.appservices.places.BookmarkRoot
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.support.base.feature.UserInteractionHandler
import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.NavGraphDirections
import org.mozilla.fenix.R
import org.mozilla.fenix.library.LibraryPageView
@ -99,16 +98,6 @@ interface BookmarkViewInteractor : SelectionInteractor<BookmarkNode> {
*
*/
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(
@ -135,10 +124,6 @@ class BookmarkView(
view.swipe_refresh.setOnRefreshListener {
interactor.onRequestSync()
}
if (FeatureFlags.bookmarkSwipeToDelete) {
BookmarkTouchHelper(interactor).attachToRecyclerView(view.bookmark_list)
}
}
fun update(state: BookmarkFragmentState) {
@ -166,7 +151,8 @@ class BookmarkView(
}
}
view.bookmarks_progress_bar.isVisible = state.isLoading
view.swipe_refresh.isEnabled = state.isSwipeToRefreshEnabled
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
}

@ -337,22 +337,4 @@ 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))
}
}
}

@ -210,22 +210,4 @@ 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()
}
}
}

@ -80,63 +80,32 @@ class BookmarkFragmentStoreTest {
@Test
fun `ensure selected items remain selected after a tree change`() = runBlocking {
val initialState = BookmarkFragmentState(
tree,
BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder)),
isLoading = false,
isSwipeToRefreshEnabled = false
)
val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder)))
val store = BookmarkFragmentStore(initialState)
store.dispatch(BookmarkFragmentAction.Change(newTree)).join()
assertEquals(
store.state,
BookmarkFragmentState(
newTree,
BookmarkFragmentState.Mode.Selecting(setOf(subfolder)),
guidBackstack = listOf(tree.guid),
isLoading = false,
isSwipeToRefreshEnabled = false
)
)
assertEquals(store.state.tree, newTree)
assertEquals(store.state.mode, BookmarkFragmentState.Mode.Selecting(setOf(subfolder)))
}
@Test
fun `select and deselect a single bookmark changes the mode and swipe to refresh state`() = runBlocking {
fun `select and deselect a single bookmark changes the mode`() = 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)),
isSwipeToRefreshEnabled = false
)
)
assertEquals(store.state, BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(childItem))))
store.dispatch(BookmarkFragmentAction.Deselect(childItem)).join()
assertEquals(
store.state,
BookmarkFragmentState(
tree,
BookmarkFragmentState.Mode.Normal(),
isSwipeToRefreshEnabled = true
)
)
assertEquals(store.state, BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Normal()))
}
@Test
fun `selecting the same item twice does nothing`() = runBlocking {
val initialState = BookmarkFragmentState(
tree,
BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder)),
isSwipeToRefreshEnabled = false
)
val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(item, subfolder)))
val store = BookmarkFragmentStore(initialState)
store.dispatch(BookmarkFragmentAction.Select(item)).join()
@ -146,11 +115,7 @@ class BookmarkFragmentStoreTest {
@Test
fun `deselecting an unselected bookmark does nothing`() = runBlocking {
val initialState = BookmarkFragmentState(
tree,
BookmarkFragmentState.Mode.Selecting(setOf(childItem)),
isSwipeToRefreshEnabled = false
)
val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(childItem)))
val store = BookmarkFragmentStore(initialState)
store.dispatch(BookmarkFragmentAction.Deselect(item)).join()
@ -169,25 +134,14 @@ class BookmarkFragmentStoreTest {
}
@Test
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()
assertEquals(
store.state,
initialState.copy(
mode = BookmarkFragmentState.Mode.Normal(),
isSwipeToRefreshEnabled = true
)
)
}
fun `deselect all bookmarks changes the mode`() = runBlocking {
val initialState = BookmarkFragmentState(tree, BookmarkFragmentState.Mode.Selecting(setOf(item, childItem)))
val store = BookmarkFragmentStore(initialState)
store.dispatch(BookmarkFragmentAction.DeselectAll).join()
assertEquals(store.state, initialState.copy(mode = BookmarkFragmentState.Mode.Normal()))
}
@Test
fun `deselect all bookmarks when none are selected`() = runBlocking {
@ -260,45 +214,6 @@ 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())

@ -1,59 +0,0 @@
/* 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.library.bookmarks
import androidx.recyclerview.widget.ItemTouchHelper
import androidx.recyclerview.widget.RecyclerView
import io.mockk.MockKAnnotations
import io.mockk.every
import io.mockk.impl.annotations.RelaxedMockK
import io.mockk.mockk
import io.mockk.verify
import mozilla.components.concept.storage.BookmarkNode
import mozilla.components.concept.storage.BookmarkNodeType
import org.junit.Before
import org.junit.Test
import org.mozilla.fenix.library.bookmarks.viewholders.BookmarkNodeViewHolder
class BookmarkTouchHelperTest {
@RelaxedMockK private lateinit var interactor: BookmarkViewInteractor
@RelaxedMockK private lateinit var viewHolder: BookmarkNodeViewHolder
@RelaxedMockK private lateinit var item: BookmarkNode
private lateinit var touchCallback: BookmarkTouchCallback
@Before
fun setup() {
MockKAnnotations.init(this)
touchCallback = BookmarkTouchCallback(interactor)
every { viewHolder.item } returns item
}
@Test
fun `swiping an item calls onDelete`() {
touchCallback.onSwiped(viewHolder, ItemTouchHelper.LEFT)
verify {
interactor.onDelete(setOf(item))
}
}
@Test
fun `swiping a folder calls onDelete and notifies the adapter of the change`() {
val adapter: RecyclerView.Adapter<BookmarkNodeViewHolder> = mockk(relaxed = true)
every { item.type } returns BookmarkNodeType.FOLDER
every { viewHolder.bindingAdapter } returns adapter
every { viewHolder.bindingAdapterPosition } returns 0
touchCallback.onSwiped(viewHolder, ItemTouchHelper.LEFT)
verify {
interactor.onDelete(setOf(item))
adapter.notifyItemChanged(0)
}
}
}
Loading…
Cancel
Save