From a23e3cb77469d8adb68978e695476775db75264c Mon Sep 17 00:00:00 2001 From: Matthew DeVore Date: Tue, 28 Jan 2025 18:53:08 +0000 Subject: [PATCH] Simplify pane sizing and scaling algorithm Simplify the scaling parameters and algorithm and reduce vertical padding. Originally a maxBlockRatio was specified at .12, which would have caused a block length to typically be 2560 * 0.12 = 307. However, this fails to account for smaller displays and high-DPI displays which have smaller dip dimensions. So instead choose a maximum edge length of 256dp, which I have found to give good results on a laptop device and a high DPI tablet, and which does not assume typical sizing for configured displays, and still accomplishes the purpose of limiting scrolling and dragging size. Using the aspect ratio of the display bounds in setting the height frequently caused a very large amount of padding, and limiting based on the max block height only reduced it slightly. Now we set the vertical height based on the minEdgeLength. This length is considered the smallest size that can be comfortably interacted with, and we use it to give ballpark ideal padding size. Note that the unit test cases have increased pane height but in practice (real-world pane width and constraints) this change seems to decrease the pane height in general. Flag: com.android.settings.flags.display_topology_pane_in_display_list Bug: b/352650922 Test: atest DisplayTopologyPreferenceTest.kt Test: atest TopologyScaleTest.kt Test: compare appearance on mid-dpi and high-dpi screens with a single 1080p external display, or with two external displays of smallish logical size Change-Id: Id189892c88a1e833c1f54b0e5447a15f92e3310f --- .../display/DisplayTopology.kt | 58 +++++------ .../display/DisplayTopologyPreferenceTest.kt | 12 +-- .../display/TopologyScaleTest.kt | 98 ++++++++++++------- 3 files changed, 95 insertions(+), 73 deletions(-) diff --git a/src/com/android/settings/connecteddevice/display/DisplayTopology.kt b/src/com/android/settings/connecteddevice/display/DisplayTopology.kt index 32638710b78..50ad5d12c71 100644 --- a/src/com/android/settings/connecteddevice/display/DisplayTopology.kt +++ b/src/com/android/settings/connecteddevice/display/DisplayTopology.kt @@ -67,10 +67,10 @@ fun Float.atLeast(n: Number): Float = max(this, n.toFloat()) * pane coordinates is necessary when rendering the original topology. Conversion in the other * direction, to display coordinates, is necessary for resolve a drag position to display space. * - * The topology pane coordinates are integral and represent the relative position from the upper- - * left corner of the pane. It uses a scale optimized for showing all displays with minimal or no - * scrolling. The display coordinates are floating point and the origin can be in any position. In - * practice the origin will be the upper-left coordinate of the primary display. + * The topology pane coordinates are physical pixels and represent the relative position from the + * upper-left corner of the pane. It uses a scale optimized for showing all displays with minimal + * or no scrolling. The display coordinates are floating point and the origin can be in any + * position. In practice the origin will be the upper-left coordinate of the primary display. * * @param paneWidth width of the pane in view coordinates * @param minPaneHeight smallest allowed height of the pane in view coordinates. This will not @@ -79,14 +79,13 @@ fun Float.atLeast(n: Number): Float = max(this, n.toFloat()) * @param minEdgeLength the smallest length permitted of a display block. This should be set based * on accessibility requirements, but also accounting for padding that appears * around each button. - * @param maxBlockRatio the highest allowed ratio of block size to display size. For instance, a - * value of 0.05 means the block will at most be 1/20 the size of the display - * it represents. This limit may be breached to account for minEdgeLength, - * which is considered an a11y requirement. + * @param maxEdgeLength the longest width or height permitted of a display block. This will limit + * the amount of dragging and scrolling the user will need to do to set the + * arrangement. * @param displaysPos the absolute topology coordinates for each display in the topology. */ class TopologyScale( - paneWidth: Int, minPaneHeight: Float, minEdgeLength: Float, maxBlockRatio: Float, + paneWidth: Int, minPaneHeight: Float, minEdgeLength: Float, maxEdgeLength: Float, displaysPos: Collection) { /** Scale of block sizes to real-world display sizes. Should be less than 1. */ val blockRatio: Float @@ -104,40 +103,33 @@ class TopologyScale( val displayBounds = RectF( Float.MAX_VALUE, Float.MAX_VALUE, Float.MIN_VALUE, Float.MIN_VALUE) var smallestDisplayDim = Float.MAX_VALUE - var biggestDisplayHeight = Float.MIN_VALUE + var biggestDisplayDim = Float.MIN_VALUE // displayBounds is the smallest rect encompassing all displays, in display space. // smallestDisplayDim is the size of the smallest display edge, in display space. for (pos in displaysPos) { displayBounds.union(pos) smallestDisplayDim = minOf(smallestDisplayDim, pos.height(), pos.width()) - biggestDisplayHeight = max(biggestDisplayHeight, pos.height()) + biggestDisplayDim = maxOf(biggestDisplayDim, pos.height(), pos.width()) } - // Set height according to the width and the aspect ratio of the display bounds limited by - // maxBlockRatio. It prevents blocks from being too large, which would make dragging and - // dropping awkward. - blockRatio = maxBlockRatio - .atMost(paneWidth * 0.6 / displayBounds.width()) - // If the `ratio` is set too low because one of the displays will have an edge less - // than minEdgeLength(dp) long, increase it such that the smallest edge is that - // long. + // Initialize blockRatio such that there is 20% padding on left and right sides of the + // display bounds. + blockRatio = (paneWidth * 0.6 / displayBounds.width()).toFloat() + // If the `ratio` is set too high because one of the displays will have an edge + // greater than maxEdgeLength(px) long, decrease it such that the largest edge is + // that long. + .atMost(maxEdgeLength / biggestDisplayDim) + // Also do the opposite of the above, this latter step taking precedence for a11y + // requirements. .atLeast(minEdgeLength / smallestDisplayDim) - // Essentially, we just set the pane height based on the pre-determined pane width and the - // aspect ratio of the display bounds. - paneHeight = (paneWidth.toFloat() / displayBounds.width() * displayBounds.height()) - // We may need to increase it slightly to achieve 20% padding above and below the - // display bounds - this is where the 0.6 comes from. - .atLeast(displayBounds.height() * blockRatio / 0.6) - - // It is easy for the aspect ratio to result in an excessively tall pane, since the - // width is pre-determined and may be considerably wider than necessary. So we + paneHeight = minPaneHeight + // A tall pane is likely to result in more scrolling. So we // prevent the height from growing too large here, by limiting vertical padding to - // the size of the tallest display. This improves results for very tall display - // bounds. - .atMost(blockRatio * (displayBounds.height() + biggestDisplayHeight * 2f)) - .atLeast(minPaneHeight) + // 1.5x of the minEdgeLength on each side. This keeps a comfortable amount of + // padding without it resulting in too much deadspace. + .atLeast(blockRatio * displayBounds.height() + minEdgeLength * 3f) // Set originPaneXY (the location of 0,0 in display space in the pane's coordinate system) // such that the display bounds rect is centered in the pane. @@ -414,7 +406,7 @@ class DisplayTopologyPreference(context : Context) val scaling = TopologyScale( mPaneContent.width, minPaneHeight = mTopologyInfo?.scaling?.paneHeight ?: 0f, minEdgeLength = DisplayTopology.dpToPx(60f, dpi), - maxBlockRatio = DisplayTopology.dpToPx(0.12f, dpi), + maxEdgeLength = DisplayTopology.dpToPx(256f, dpi), newBounds.map { it.second }.toList()) mPaneHolder.layoutParams.let { val newHeight = scaling.paneHeight.toInt() 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 76d647f5daa..5dcb26514ae 100644 --- a/tests/robotests/src/com/android/settings/connecteddevice/display/DisplayTopologyPreferenceTest.kt +++ b/tests/robotests/src/com/android/settings/connecteddevice/display/DisplayTopologyPreferenceTest.kt @@ -334,7 +334,7 @@ class DisplayTopologyPreferenceTest { fun updatedTopologyCancelsDragIfNonTrivialChange() { val (leftBlock, _) = setupTwoDisplays(POSITION_LEFT, /* childOffset= */ 42f) - assertThat(leftBlock.y).isWithin(0.01f).of(142.17f) + assertThat(leftBlock.y).isWithin(0.05f).of(143.76f) leftBlock.dispatchTouchEvent(MotionEventBuilder.newBuilder() .setAction(MotionEvent.ACTION_DOWN) @@ -344,30 +344,30 @@ class DisplayTopologyPreferenceTest { .setAction(MotionEvent.ACTION_MOVE) .setPointer(0f, 30f) .build()) - assertThat(leftBlock.y).isWithin(0.01f).of(172.17f) + assertThat(leftBlock.y).isWithin(0.05f).of(173.76f) // Offset is only different by 0.5 dp, so the drag will not cancel. injector.topology = twoDisplayTopology(POSITION_LEFT, /* childOffset= */ 41.5f) injector.topologyListener!!.accept(injector.topology!!) - assertThat(leftBlock.y).isWithin(0.01f).of(172.17f) + assertThat(leftBlock.y).isWithin(0.05f).of(173.76f) // Move block farther downward. leftBlock.dispatchTouchEvent(MotionEventBuilder.newBuilder() .setAction(MotionEvent.ACTION_MOVE) .setPointer(0f, 50f) .build()) - assertThat(leftBlock.y).isWithin(0.01f).of(192.17f) + assertThat(leftBlock.y).isWithin(0.05f).of(193.76f) injector.topology = twoDisplayTopology(POSITION_LEFT, /* childOffset= */ 20f) injector.topologyListener!!.accept(injector.topology!!) - assertThat(leftBlock.y).isWithin(0.01f).of(125.67f) + assertThat(leftBlock.y).isWithin(0.05f).of(115.60f) // Another move in the opposite direction should not move the left block. leftBlock.dispatchTouchEvent(MotionEventBuilder.newBuilder() .setAction(MotionEvent.ACTION_MOVE) .setPointer(0f, -20f) .build()) - assertThat(leftBlock.y).isWithin(0.01f).of(125.67f) + assertThat(leftBlock.y).isWithin(0.05f).of(115.60f) } @Test diff --git a/tests/robotests/src/com/android/settings/connecteddevice/display/TopologyScaleTest.kt b/tests/robotests/src/com/android/settings/connecteddevice/display/TopologyScaleTest.kt index 53468578cff..e88939bb6dd 100644 --- a/tests/robotests/src/com/android/settings/connecteddevice/display/TopologyScaleTest.kt +++ b/tests/robotests/src/com/android/settings/connecteddevice/display/TopologyScaleTest.kt @@ -19,14 +19,25 @@ package com.android.settings.connecteddevice.display import android.graphics.Point import android.graphics.PointF import android.graphics.RectF +import kotlin.math.abs import org.junit.Assert.assertEquals import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner -fun assertPointF(x: Float, y: Float, delta: Float, actual: PointF) { - assertEquals(x, actual.x, delta) - assertEquals(y, actual.y, delta) +// Assert as many points as possible in a single assert since a tweak can change every assertion +// in a small way and we don't want to re-compile and re-test for *every* individual float. +fun assertPointF(comparisons: List>, delta: Float) { + val errors = StringBuilder() + comparisons.forEachIndexed {i, (a, b) -> + if (abs(b.x - a.x) > delta) { + errors.append("x value at index $i - ${a.x} != ${b.x}\n") + } + if (abs(b.y - a.y) > delta) { + errors.append("y value at index $i - ${a.y} != ${b.y}\n") + } + } + assertEquals("", errors.toString()) } @RunWith(RobolectricTestRunner::class) @@ -35,28 +46,32 @@ class TopologyScaleTest { fun oneDisplay4to3Aspect() { val scale = TopologyScale( /* paneWidth= */ 640, minPaneHeight = 0f, - minEdgeLength = 48f, maxBlockRatio = 0.05f, + minEdgeLength = 48f, maxEdgeLength = 64f, listOf(RectF(0f, 0f, 640f, 480f))) - // blockRatio is higher than 0.05 in order to make the smallest display edge (480 dp) 48dp + // blockRatio is is set in order to make the smallest display edge (480 dp) 48dp // in the pane. assertEquals( - "{TopologyScale blockRatio=0.100000 originPaneXY=288.0,48.0 paneHeight=144.0}", + "{TopologyScale blockRatio=0.100000 originPaneXY=288.0,72.0 paneHeight=192.0}", "" + scale) - assertPointF(352f, 96f, 0.001f, scale.displayToPaneCoor(640f, 480f)) - assertPointF(320f, 72f, 0.001f, scale.displayToPaneCoor(320f, 240f)) - assertPointF(640f, 480f, 0.001f, scale.paneToDisplayCoor(352f, 96f)) + assertPointF( + listOf( + PointF(352f, 120f) to scale.displayToPaneCoor(640f, 480f), + PointF(320f, 96f) to scale.displayToPaneCoor(320f, 240f), + PointF(640f, 240f) to scale.paneToDisplayCoor(352f, 96f), + ), + 0.001f) // Same as original scale but made taller with minPaneHeight. // The paneHeight and origin coordinates are changed but the block ratio is the same. val taller = TopologyScale( /* paneWidth= */ 640, minPaneHeight = 155.0f, - minEdgeLength = 48f, maxBlockRatio = 0.05f, + minEdgeLength = 48f, maxEdgeLength = 64f, listOf(RectF(0f, 0f, 640f, 480f))) assertEquals( - "{TopologyScale blockRatio=0.100000 originPaneXY=288.0,53.5 paneHeight=155.0}", + "{TopologyScale blockRatio=0.100000 originPaneXY=288.0,72.0 paneHeight=192.0}", "" + taller) } @@ -64,41 +79,47 @@ class TopologyScaleTest { fun twoUnalignedDisplays() { val scale = TopologyScale( /* paneWidth= */ 300, minPaneHeight = 0f, - minEdgeLength = 48f, maxBlockRatio = 0.05f, + minEdgeLength = 48f, maxEdgeLength = 96f, listOf(RectF(0f, 0f, 1920f, 1200f), RectF(1920f, -300f, 3840f, 900f))) assertEquals( - "{TopologyScale blockRatio=0.046875 originPaneXY=60.0,37.5 paneHeight=117.2}", + "{TopologyScale blockRatio=0.046875 originPaneXY=60.0,86.1 paneHeight=214.3}", "" + scale) - assertPointF(78.75f, 56.25f, 0.001f, scale.displayToPaneCoor(400f, 400f)) - assertPointF(41.25f, 37.5f, 0.001f, scale.displayToPaneCoor(-400f, 0f)) - assertPointF(-384f, 96f, 0.001f, scale.paneToDisplayCoor(42f, 42f)) + assertPointF( + listOf( + PointF(78.75f, 104.8125f) to scale.displayToPaneCoor(400f, 400f), + PointF(41.25f, 86.0625f) to scale.displayToPaneCoor(-400f, 0f), + PointF(-384f, -940f) to scale.paneToDisplayCoor(42f, 42f), + ), 0.001f) } @Test fun twoDisplaysBlockRatioBumpedForGarSizeMinimumHorizontal() { val scale = TopologyScale( /* paneWidth= */ 192, minPaneHeight = 0f, - minEdgeLength = 48f, maxBlockRatio = 0.05f, + minEdgeLength = 48f, maxEdgeLength = 64f, listOf(RectF(0f, 0f, 240f, 320f), RectF(-240f, -320f, 0f, 0f))) // blockRatio is higher than 0.05 in order to make the smallest display edge (240 dp) 48dp // in the pane. assertEquals( - "{TopologyScale blockRatio=0.200000 originPaneXY=96.0,128.0 paneHeight=256.0}", + "{TopologyScale blockRatio=0.200000 originPaneXY=96.0,136.0 paneHeight=272.0}", "" + scale) - assertPointF(192f, 256f, 0.001f, scale.displayToPaneCoor(480f, 640f)) - assertPointF(96f, 64f, 0.001f, scale.displayToPaneCoor(0f, -320f)) - assertPointF(220f, -430f, 0.001f, scale.paneToDisplayCoor(140f, 42f)) + assertPointF( + listOf( + PointF(192f, 264f) to scale.displayToPaneCoor(480f, 640f), + PointF(96f, 72f) to scale.displayToPaneCoor(0f, -320f), + PointF(220f, -470f) to scale.paneToDisplayCoor(140f, 42f), + ), 0.001f) } @Test - fun paneVerticalPaddingLimitedByTallestDisplay() { + fun paneVerticalPaddingSetByMinEdgeLength() { val scale = TopologyScale( /* paneWidth= */ 300, minPaneHeight = 0f, - minEdgeLength = 48f, maxBlockRatio = 0.05f, + minEdgeLength = 48f, maxEdgeLength = 80f, listOf( RectF(0f, 0f, 640f, 480f), RectF(0f, 480f, 640f, 960f), @@ -108,26 +129,32 @@ class TopologyScaleTest { RectF(0f, 2400f, 640f, 2880f))) assertEquals( - "{TopologyScale blockRatio=0.100000 originPaneXY=118.0,48.0 paneHeight=384.0}", + "{TopologyScale blockRatio=0.125000 originPaneXY=110.0,72.0 paneHeight=504.0}", "" + scale) - assertPointF(150f, 48f, 0.001f, scale.displayToPaneCoor(320f, 0f)) - assertPointF(-180f, 2880f, 0.001f, scale.paneToDisplayCoor(100f, 336f)) + assertPointF( + listOf( + PointF(150f, 72f) to scale.displayToPaneCoor(320f, 0f), + PointF(-80f, 2112f) to scale.paneToDisplayCoor(100f, 336f), + ), 0.001f) } @Test fun limitedByCustomMaxBlockRatio() { val scale = TopologyScale( /* paneWidth= */ 300, minPaneHeight = 0f, - minEdgeLength = 24f, maxBlockRatio = 0.12f, + minEdgeLength = 24f, maxEdgeLength = 77f, listOf( RectF(0f, 0f, 640f, 480f), RectF(0f, 480f, 640f, 960f))) assertEquals( - "{TopologyScale blockRatio=0.120000 originPaneXY=111.6,57.6 paneHeight=230.4}", + "{TopologyScale blockRatio=0.120312 originPaneXY=111.5,36.0 paneHeight=187.5}", "" + scale) - assertPointF(150f, 57.6f, 0.001f, scale.displayToPaneCoor(320f, 0f)) - assertPointF(-96.6667f, 2320f, 0.001f, scale.paneToDisplayCoor(100f, 336f)) + assertPointF( + listOf( + PointF(150f, 36.0f) to scale.displayToPaneCoor(320f, 0f), + PointF(-95.58442f, 2493.5066f) to scale.paneToDisplayCoor(100f, 336f), + ), 0.001f) } @Test @@ -135,15 +162,18 @@ class TopologyScaleTest { // minBlockEdgeLength/minDisplayEdgeLength = 80/480 = 1/6, so the block ratio will be 1/6 val scale = TopologyScale( /* paneWidth= */ 300, minPaneHeight = 0f, - minEdgeLength = 80f, maxBlockRatio = 0.05f, + minEdgeLength = 80f, maxEdgeLength = 100f, listOf( RectF(0f, 0f, 640f, 480f), RectF(0f, 480f, 640f, 960f))) assertEquals( - "{TopologyScale blockRatio=0.166667 originPaneXY=96.7,80.0 paneHeight=320.0}", + "{TopologyScale blockRatio=0.166667 originPaneXY=96.7,120.0 paneHeight=400.0}", "" + scale) - assertPointF(150f, 80f, 0.001f, scale.displayToPaneCoor(320f, 0f)) - assertPointF(20f, 1536f, 0.001f, scale.paneToDisplayCoor(100f, 336f)) + assertPointF( + listOf( + PointF(150f, 120f) to scale.displayToPaneCoor(320f, 0f), + PointF(20f, 1296f) to scale.paneToDisplayCoor(100f, 336f), + ), 0.001f) } }