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) } }