From 0a5a60fae7a760f276206d47fc9117422ae4cbc0 Mon Sep 17 00:00:00 2001 From: Steven Knipe Date: Thu, 4 Nov 2021 20:54:18 -0700 Subject: [PATCH] Remove multi-tab movement, only allow dragging if tab groups disabled, fix tab positioning during movement I'm forced to suppress LongParameterList to get the settings information where it needs to go though --- .../fenix/tabstray/TabsTrayController.kt | 24 ++++----- .../fenix/tabstray/TabsTrayInteractor.kt | 11 ++-- .../browser/AbstractBrowserTabViewHolder.kt | 8 +-- .../browser/AbstractBrowserTrayList.kt | 52 +++++++++---------- .../tabstray/browser/BrowserTabViewHolder.kt | 13 +++-- .../tabstray/browser/BrowserTabsAdapter.kt | 7 ++- 6 files changed, 58 insertions(+), 57 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt index 9477566b7..3e0c3e410 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayController.kt @@ -60,13 +60,12 @@ interface TabsTrayController { fun handleMultipleTabsDeletion(tabs: Collection) /** - * Moves [tabs] to before/after the target + * Moves [tabId] to replace position of [targetId] * - * @param tabs The tabs to be moved - * @param targetId The id of the tab that the [tabs] will be placed next to - * @param placeAfter Place [tabs] before or after the target + * @param tabId The tabs to be moved + * @param targetId The id of the tab that the [tab] will replace */ - fun handleTabsMove(tabs: Collection, targetId: String?, placeAfter: Boolean) + fun handleTabsMove(tabId: String, targetId: String?) /** * Navigate from TabsTray to Recently Closed section in the History fragment. @@ -182,20 +181,17 @@ class DefaultTabsTrayController( } /** - * Moves [tabs] to before/after the target + * Moves [tabId] to replace position of [targetId] * - * @param tabs The tabs to be moved - * @param targetId The id of the tab that the [tabs] will be placed next to - * @param placeAfter Place [tabs] before or after the target + * @param tabId The tabs to be moved + * @param targetId The id of the tab that the [tab] will replace */ override fun handleTabsMove( - tabs: Collection, + tabId: String, targetId: String?, - placeAfter: Boolean, ) { - val tabIDs = tabs.map { it.id } - if (targetId != null) { - tabsUseCases.moveTabs(tabIDs, targetId, placeAfter) + if (targetId != null && tabId != targetId) { + tabsUseCases.moveTabs(tabId, targetId) } } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt index 9ea0eaf3f..d6db8963d 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayInteractor.kt @@ -36,12 +36,12 @@ interface TabsTrayInteractor { fun onInactiveDebugClicked(tabs: Collection) /** - * Invoked when [tabs] should be moved to before/after [targetId] from a drag-drop operation + * Invoked when [tabId] should be moved replace [targetId]'s position + * due to a drag-drop operation */ fun onTabsMove( - tabs: Collection, + tabId: String, targetId: String?, - placeAfter: Boolean, ) /** @@ -75,11 +75,10 @@ class DefaultTabsTrayInteractor( } override fun onTabsMove( - tabs: Collection, + tabId: String, targetId: String?, - placeAfter: Boolean, ) { - controller.handleTabsMove(tabs, targetId, placeAfter) + controller.handleTabsMove(tabId, targetId) } override fun onInactiveDebugClicked(tabs: Collection) { diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTabViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTabViewHolder.kt index c765bc834..65779e4e0 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTabViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTabViewHolder.kt @@ -31,6 +31,7 @@ import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.increaseTapArea import org.mozilla.fenix.ext.removeAndDisable import org.mozilla.fenix.ext.removeTouchDelegate +import org.mozilla.fenix.utils.Settings import org.mozilla.fenix.ext.showAndEnable import org.mozilla.fenix.ext.toShortUrl import org.mozilla.fenix.selection.SelectionHolder @@ -56,8 +57,9 @@ abstract class AbstractBrowserTabViewHolder( private val selectionHolder: SelectionHolder?, @VisibleForTesting internal val featureName: String, + private val settings: Settings?, private val store: BrowserStore = itemView.context.components.core.store, - private val metrics: MetricController = itemView.context.components.analytics.metrics + private val metrics: MetricController = itemView.context.components.analytics.metrics, ) : TabViewHolder(itemView) { private val faviconView: ImageView? = @@ -222,13 +224,13 @@ abstract class AbstractBrowserTabViewHolder( metrics.track(Event.CollectionTabLongPressed) interactor.select(item) true - } else if (holder.selectedItems.contains(item) && itemView.parent is NormalBrowserTrayList) { + } else if (holder.selectedItems.contains(item) && settings?.searchTermTabGroupsAreEnabled == false) { // Only allow a drag to start if the tab is a non-grouped tab (parent is normal list) // Non-grouped tabs can be selected, but they can't be drop targets // so it's useless to try to move them (and they're auto-sorted anyway) val shadow = View.DragShadowBuilder(itemView) @Suppress("DEPRECATION") - itemView.startDrag(null, shadow, holder.selectedItems, 0) + itemView.startDrag(null, shadow, item, 0) // startDragAndDrop is the non-deprecated version, but requires API 24 true } else { diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTrayList.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTrayList.kt index c25c8b581..c9d52f976 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTrayList.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/AbstractBrowserTrayList.kt @@ -5,6 +5,7 @@ package org.mozilla.fenix.tabstray.browser import android.content.Context +import android.graphics.Rect import android.util.AttributeSet import android.view.DragEvent import androidx.recyclerview.widget.RecyclerView @@ -73,38 +74,35 @@ abstract class AbstractBrowserTrayList @JvmOverloads constructor( } // Find the closest item to the x/y position of the drop. - // This gets all the children of the tab tray, which will not include grouped tabs - // Since those are children of the group which is a child of the tab tray. - // Determine if the drop is before or after based on the current grid/list view setting - private fun getDropPosition(x: Float, y: Float): Pair? { + private fun getDropPosition(x: Float, y: Float): String? { if (childCount < 2) return null // If there's 0 or 1 tabs visible, can't reorder - val isGrid = context.components.settings.gridTabView + if (layoutManager == null) return null + val lm = layoutManager!! var bestDist = Float.MAX_VALUE var bestId: String? = null - var placeAfter = false for (i in 0 until childCount) { - val targetHolder = findViewHolderForAdapterPosition(i) - val proposedTarget = getChildAt(i)!! - val xDiff = x - (proposedTarget.x + proposedTarget.width / 2) - val yDiff = y - (proposedTarget.y + proposedTarget.height / 2) - val dist = abs(xDiff) + abs(yDiff) - if (dist < bestDist && targetHolder is TabViewHolder) { - val targetTabId = targetHolder.tab?.id - bestDist = dist - bestId = targetTabId - val modifier = if (isGrid) xDiff else yDiff - placeAfter = (modifier > 0) + val proposedTarget = getChildAt(i) + val targetHolder = findContainingViewHolder(proposedTarget) + if (targetHolder is TabViewHolder) { + var rect = Rect() // Use layoutManager to get post-animation positioning + lm.getDecoratedBoundsWithMargins(proposedTarget, rect) + val targetX = (rect.left + rect.right) / 2 + val targetY = (rect.top + rect.bottom) / 2 + val xDiff = x - targetX + val yDiff = y - targetY + val dist = abs(xDiff) + abs(yDiff) + if (dist < bestDist) { + bestDist = dist + bestId = targetHolder.tab?.id + } } } - return Pair(bestId, placeAfter) + return bestId } private val dragListen = OnDragListener { _, event -> when (event.action) { DragEvent.ACTION_DRAG_STARTED -> { - // This check is required for the unchecked cast later on - if (event.localState is Collection<*>) { - (event.localState as Collection<*>).all { it is TabSessionState } - } else false + (event.localState is TabSessionState) } DragEvent.ACTION_DRAG_ENTERED -> { true @@ -112,9 +110,8 @@ abstract class AbstractBrowserTrayList @JvmOverloads constructor( DragEvent.ACTION_DRAG_LOCATION -> { val target = getDropPosition(event.x, event.y) if (target != null) { - val (targetId, placeAfter) = target - @Suppress("UNCHECKED_CAST") // Cast is checked on drag start - interactor.onTabsMove(event.localState as Collection, targetId, placeAfter) + val source = (event.localState as TabSessionState) + interactor.onTabsMove(source.id, target) } true } @@ -124,9 +121,8 @@ abstract class AbstractBrowserTrayList @JvmOverloads constructor( DragEvent.ACTION_DROP -> { val target = getDropPosition(event.x, event.y) if (target != null) { - val (targetId, placeAfter) = target - @Suppress("UNCHECKED_CAST") // Cast is checked on drag start - interactor.onTabsMove(event.localState as Collection, targetId, placeAfter) + val source = (event.localState as TabSessionState) + interactor.onTabsMove(source.id, target) } true } diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabViewHolder.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabViewHolder.kt index ca0ae0405..354b79ba1 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabViewHolder.kt @@ -16,6 +16,7 @@ import mozilla.components.concept.base.images.ImageLoader import org.mozilla.fenix.R import org.mozilla.fenix.databinding.TabTrayGridItemBinding import org.mozilla.fenix.ext.increaseTapArea +import org.mozilla.fenix.utils.Settings import kotlin.math.max import org.mozilla.fenix.selection.SelectionHolder import org.mozilla.fenix.tabstray.TabsTrayStore @@ -32,14 +33,16 @@ sealed class BrowserTabViewHolder(itemView: View) : RecyclerView.ViewHolder(item * @param itemView [View] that displays a "tab". * @param featureName [String] representing the name of the feature displaying tabs. Used in telemetry reporting. */ + @Suppress("LongParameterList") class GridViewHolder( imageLoader: ImageLoader, override val browserTrayInteractor: BrowserTrayInteractor, store: TabsTrayStore, selectionHolder: SelectionHolder? = null, itemView: View, - featureName: String - ) : AbstractBrowserTabViewHolder(itemView, imageLoader, store, selectionHolder, featureName) { + featureName: String, + settings: Settings? = null + ) : AbstractBrowserTabViewHolder(itemView, imageLoader, store, selectionHolder, featureName, settings) { private val closeButton: AppCompatImageButton = itemView.findViewById(R.id.mozac_browser_tabstray_close) @@ -86,14 +89,16 @@ sealed class BrowserTabViewHolder(itemView: View) : RecyclerView.ViewHolder(item * @param itemView [View] that displays a "tab". * @param featureName [String] representing the name of the feature displaying tabs. Used in telemetry reporting. */ + @Suppress("LongParameterList") class ListViewHolder( imageLoader: ImageLoader, override val browserTrayInteractor: BrowserTrayInteractor, store: TabsTrayStore, selectionHolder: SelectionHolder? = null, itemView: View, - featureName: String - ) : AbstractBrowserTabViewHolder(itemView, imageLoader, store, selectionHolder, featureName) { + featureName: String, + settings: Settings? = null + ) : AbstractBrowserTabViewHolder(itemView, imageLoader, store, selectionHolder, featureName, settings) { override val thumbnailSize: Int get() = max( itemView.resources.getDimensionPixelSize(R.dimen.tab_tray_list_item_thumbnail_height), diff --git a/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt b/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt index 6b5d57019..2f0fabaa4 100644 --- a/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/tabstray/browser/BrowserTabsAdapter.kt @@ -17,6 +17,7 @@ import org.mozilla.fenix.components.Components import org.mozilla.fenix.databinding.TabTrayGridItemBinding import org.mozilla.fenix.databinding.TabTrayItemBinding import org.mozilla.fenix.ext.components +import org.mozilla.fenix.ext.settings import org.mozilla.fenix.selection.SelectionHolder import org.mozilla.fenix.tabstray.TabsTrayStore @@ -67,9 +68,11 @@ class BrowserTabsAdapter( return when (viewType) { ViewType.GRID.layoutRes -> - BrowserTabViewHolder.GridViewHolder(imageLoader, interactor, store, selectionHolder, view, featureName) + BrowserTabViewHolder.GridViewHolder(imageLoader, interactor, store, + selectionHolder, view, featureName, context.settings()) else -> - BrowserTabViewHolder.ListViewHolder(imageLoader, interactor, store, selectionHolder, view, featureName) + BrowserTabViewHolder.ListViewHolder(imageLoader, interactor, store, + selectionHolder, view, featureName, context.settings()) } }