From eeec7d0d66d3c31308cc8de1f8538decf3e093b6 Mon Sep 17 00:00:00 2001 From: Matthew DeVore Date: Wed, 5 Feb 2025 21:17:45 +0000 Subject: [PATCH] Run body of applyTopology for all non-noop drags In onBlockTouchUp, if DisplayTopology.rearrange happened to revert the change made by the drag so that it matched the before-drag layout, the blocks would not be moved, so the block would be in the dragged position but not the normalized position. This will happen when rearrange has a bug or is otherwise optimizing the layout, which is dependent on the implementation of rearrange. The test field mTimesReceivedSameTopology has been replaced with one that represents an observable positive operation: mTimesRefreshedBlocks, the validation of which has been added to some existing tests. Flag: com.android.settings.flags.display_topology_pane_in_display_list Test: move display so that rearrange reverts the change, then exit and re-enter the external display fragment, and verify it matches the state when left Test: DisplayTopologyPreferenceTest Bug: b/394361999 Bug: b/394355269 Change-Id: Ic3028747d283db77f144831352b7687fe2706391 --- .../display/DisplayTopology.kt | 27 +++++++++++-- .../display/DisplayTopologyPreferenceTest.kt | 40 +++++++++++++++++-- 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/com/android/settings/connecteddevice/display/DisplayTopology.kt b/src/com/android/settings/connecteddevice/display/DisplayTopology.kt index c30d98a0c2b..a3c710c59db 100644 --- a/src/com/android/settings/connecteddevice/display/DisplayTopology.kt +++ b/src/com/android/settings/connecteddevice/display/DisplayTopology.kt @@ -334,12 +334,15 @@ class DisplayTopologyPreference(context : Context) * @param displayHeight height of display being dragged in actual (not View) coordinates * @param dragOffsetX difference between event rawX coordinate and X of the display in the pane * @param dragOffsetY difference between event rawY coordinate and Y of the display in the pane + * @param didMove true if we have detected the user intentionally wanted to drag rather than + * just click */ private data class BlockDrag( val stationaryDisps : List>, val display: DisplayBlock, val displayId: Int, val displayWidth: Float, val displayHeight: Float, - val dragOffsetX: Float, val dragOffsetY: Float) + val dragOffsetX: Float, val dragOffsetY: Float, + var didMove: Boolean = false) private var mTopologyInfo : TopologyInfo? = null private var mDrag : BlockDrag? = null @@ -369,7 +372,7 @@ class DisplayTopologyPreference(context : Context) applyTopology(topology) } - @VisibleForTesting var mTimesReceivedSameTopology = 0 + @VisibleForTesting var mTimesRefreshedBlocks = 0 private fun applyTopology(topology: DisplayTopology) { mTopologyHint.text = context.getString(R.string.external_display_topology_hint) @@ -386,7 +389,6 @@ class DisplayTopologyPreference(context : Context) oldBounds.zip(newBounds).all { (old, new) -> old.first == new.first && sameDisplayPosition(old.second, new.second) }) { - mTimesReceivedSameTopology++ return } @@ -438,6 +440,7 @@ class DisplayTopologyPreference(context : Context) } } mPaneContent.removeViews(newBounds.size, recycleableBlocks.size) + mTimesRefreshedBlocks++ mTopologyInfo = TopologyInfo(topology, scaling, newBounds) @@ -481,6 +484,7 @@ class DisplayTopologyPreference(context : Context) val snapRect = clampPosition(drag.stationaryDisps.map { it.second }, dispDragRect) drag.display.place(topology.scaling.displayToPaneCoor(snapRect.left, snapRect.top)) + drag.didMove = true return true } @@ -491,6 +495,15 @@ class DisplayTopologyPreference(context : Context) mPaneContent.requestDisallowInterceptTouchEvent(false) drag.display.setHighlighted(false) + mDrag = null + if (!drag.didMove) { + // If no move event occurred, ignore the drag completely. + // TODO(b/352648432): Responding to a single move event no matter how small may be too + // sensitive. It is easy to slide by a small amount just by force of pressing down the + // mouse button. Keep an eye on this. + return true + } + val newCoor = topology.scaling.paneToDisplayCoor( drag.display.x, drag.display.y) val newTopology = topology.topology.copy() @@ -499,9 +512,15 @@ class DisplayTopologyPreference(context : Context) val arr = hashMapOf(*newPositions.toTypedArray()) newTopology.rearrange(arr) + + // Setting mTopologyInfo to null forces applyTopology to skip the no-op drag check. This is + // necessary because we don't know if newTopology.rearrange has mutated the topology away + // from what the user has dragged into position. + mTopologyInfo = null + applyTopology(newTopology) + injector.displayTopology = newTopology - refreshPane() return true } } diff --git a/tests/robotests/src/com/android/settings/connecteddevice/display/DisplayTopologyPreferenceTest.kt b/tests/robotests/src/com/android/settings/connecteddevice/display/DisplayTopologyPreferenceTest.kt index 5dcb26514ae..8b7f64ccd55 100644 --- a/tests/robotests/src/com/android/settings/connecteddevice/display/DisplayTopologyPreferenceTest.kt +++ b/tests/robotests/src/com/android/settings/connecteddevice/display/DisplayTopologyPreferenceTest.kt @@ -186,6 +186,8 @@ class DisplayTopologyPreferenceTest { fun dragDisplayDownward() { val (leftBlock, _) = setupTwoDisplays() + preference.mTimesRefreshedBlocks = 0 + val downEvent = MotionEventBuilder.newBuilder() .setPointer(0f, 0f) .setAction(MotionEvent.ACTION_DOWN) @@ -208,12 +210,16 @@ class DisplayTopologyPreferenceTest { val child = rootChildren[0] assertThat(child.position).isEqualTo(POSITION_LEFT) assertThat(child.offset).isWithin(1f).of(82f) + + assertThat(preference.mTimesRefreshedBlocks).isEqualTo(1) } @Test fun dragRootDisplayToNewSide() { val (leftBlock, rightBlock) = setupTwoDisplays() + preference.mTimesRefreshedBlocks = 0 + val downEvent = MotionEventBuilder.newBuilder() .setAction(MotionEvent.ACTION_DOWN) .setPointer(0f, 0f) @@ -251,6 +257,32 @@ class DisplayTopologyPreferenceTest { assertThat(paneChildren[0].x) .isWithin(1f) .of(paneChildren[1].x) + + assertThat(preference.mTimesRefreshedBlocks).isEqualTo(1) + } + + @Test + fun noRefreshForUnmovingDrag() { + val (leftBlock, rightBlock) = setupTwoDisplays() + + preference.mTimesRefreshedBlocks = 0 + + val downEvent = MotionEventBuilder.newBuilder() + .setAction(MotionEvent.ACTION_DOWN) + .setPointer(0f, 0f) + .build() + + val upEvent = MotionEventBuilder.newBuilder().setAction(MotionEvent.ACTION_UP).build() + + rightBlock.dispatchTouchEvent(downEvent) + rightBlock.dispatchTouchEvent(upEvent) + + // After drag, the original views should still be present. + val paneChildren = getPaneChildren() + assertThat(paneChildren.indexOf(leftBlock)).isNotEqualTo(-1) + assertThat(paneChildren.indexOf(rightBlock)).isNotEqualTo(-1) + + assertThat(preference.mTimesRefreshedBlocks).isEqualTo(0) } @Test @@ -269,13 +301,15 @@ class DisplayTopologyPreferenceTest { @Test fun applyNewTopologyViaListenerUpdate() { setupTwoDisplays() + + preference.mTimesRefreshedBlocks = 0 val newTopology = injector.topology!!.copy() newTopology.addDisplay(/* displayId= */ 8008, /* width= */ 300f, /* height= */ 320f) injector.topology = newTopology injector.topologyListener!!.accept(newTopology) - assertThat(preference.mTimesReceivedSameTopology).isEqualTo(0) + assertThat(preference.mTimesRefreshedBlocks).isEqualTo(1) val paneChildren = getPaneChildren() assertThat(paneChildren).hasSize(3) @@ -293,11 +327,11 @@ class DisplayTopologyPreferenceTest { injector.topology = twoDisplayTopology(POSITION_TOP, /* offset= */ 12.0f) preparePane() - assertThat(preference.mTimesReceivedSameTopology).isEqualTo(0) + preference.mTimesRefreshedBlocks = 0 injector.topology = twoDisplayTopology(POSITION_TOP, /* offset= */ 12.1f) injector.topologyListener!!.accept(injector.topology!!) - assertThat(preference.mTimesReceivedSameTopology).isEqualTo(1) + assertThat(preference.mTimesRefreshedBlocks).isEqualTo(0) } @Test