diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt index 2398dd2f50..a5b1d3f669 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -22,6 +22,7 @@ import android.graphics.Bitmap import android.os.SystemClock import android.util.TypedValue import android.view.MotionEvent +import android.view.View import androidx.annotation.CallSuper import androidx.annotation.UiThread import androidx.annotation.VisibleForTesting @@ -186,8 +187,21 @@ abstract class MetricsChartRenderer( */ @UiThread fun attach(chart: SafeLineChart) { + // A rebind can attach the replacement before the view it replaced is recycled, so the + // outgoing chart is let go of here rather than waiting for a [detachIfAttached] that, by + // then, no longer names it. + // + // The whole teardown, not just the listener. [detach] also clears [userHasZoomed], and + // releasing only the listener leaked it onto the replacement: a user who had panned once + // got a chart whose follow-window was disabled for good, because showNewestWindow returns + // early on the flag and every later redraw takes the same early return. That is the + // oldest-samples symptom this ticket was filed for -- reachable only after a pan, which is + // why the resume paths reproduce it and a fresh chart never does. Subclasses clear their own + // per-chart state through the same override. + this.chart?.let { outgoing -> if (outgoing !== chart) detach() } this.chart = chart configure(chart) + chart.addOnLayoutChangeListener(newestWindowOnLayout) rebuild() } @@ -198,6 +212,7 @@ abstract class MetricsChartRenderer( @CallSuper open fun detach() { userHasZoomed = false + chart?.removeOnLayoutChangeListener(newestWindowOnLayout) chart = null } @@ -308,10 +323,36 @@ abstract class MetricsChartRenderer( return } + // Before the first layout there is no plot area to place a window in, and applying one + // anyway is worse than waiting: the scale is clamped against an empty content rect, and the + // layout that follows resets the chart's transform. [newestWindowOnLayout] re-applies it as + // soon as there is something to apply it to (ADFA-5515). + if (!chart.viewPortHandler.hasChartDimens()) { + return + } + chart.setVisibleXRangeMaximum(VISIBLE_SAMPLES.toFloat()) - chart.moveViewToX(newestIndex - VISIBLE_SAMPLES.toFloat() + 1f) + // Not moveViewToX: its scroll is deferred to a later frame and would be converted through + // a different transform from the scale just set here (ADFA-5515). + chart.moveViewToXNow(newestIndex - VISIBLE_SAMPLES.toFloat() + 1f) } + /** + * Re-applies the newest window whenever the chart is laid out. + * + * A layout that changes the chart's size resets its transform, which drops the window and shows + * the whole buffer from its oldest end. Nothing put it back until the next sample landed a + * redraw, so every rebind -- and the carousel is rebound on every resume -- opened on an empty + * plot for a second or more (ADFA-5515). + */ + private val newestWindowOnLayout = + View.OnLayoutChangeListener { view, _, _, _, _, _, _, _, _ -> + val chart = view as? SafeLineChart ?: return@OnLayoutChangeListener + if (chart === this.chart) { + showNewestWindow(chart) + } + } + /** * The sample indices currently on screen, for a series of [sampleCount] samples. * diff --git a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt index 600ad00666..030dd3954f 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt @@ -130,6 +130,27 @@ class SafeLineChart : LineChart { } } + /** + * Scrolls the plot so that [xValue] is its leftmost value, now rather than on a later frame. + * + * What [moveViewToX] does, minus the deferral. That one queues the scroll as a viewport job + * which MPAndroidChart hands to `View.post`, and by the time it runs the transform it converts + * its x value through is no longer the one the caller set it up against -- a layout in between + * resets the transform to identity, and the clamp afterwards restores the scale around a + * translation computed for a different one. The viewport then lands neither where it was nor + * where it was asked to go (ADFA-5515). + * + * On a chart that is not attached to a window the job is worse than late: `View.post` drops it + * in the view's run queue, which is only flushed on attach, so it never runs at all. + */ + fun moveViewToXNow(xValue: Float) { + // The left axis, as moveViewToX itself uses; only the x component is read back. + val transformer = getTransformer(YAxis.AxisDependency.LEFT) ?: return + val target = floatArrayOf(xValue, 0f) + transformer.pointValuesToPixel(target) + viewPortHandler.centerViewPort(target, this) + } + override fun onDraw(canvas: Canvas) { try { super.onDraw(canvas) diff --git a/app/src/test/java/com/itsaky/androidide/ui/ChartLayout.kt b/app/src/test/java/com/itsaky/androidide/ui/ChartLayout.kt index 4bc1d534c8..3c76eb0eee 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/ChartLayout.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/ChartLayout.kt @@ -29,12 +29,9 @@ const val CHART_HEIGHT = 400 /** * Lays this chart out and draws it once, which is what every assertion about its viewport needs. * - * Two separate reasons, both easy to leave out and neither of which fails loudly. Without the - * layout the plot area has no extent, so every coordinate lands on its edge and a hit test cannot - * tell inside from outside. Without the draw the scroll to the newest samples has not run -- - * MPAndroidChart queues `moveViewToX` as a job that only executes during a draw pass -- so the - * chart still reports the *oldest* samples as visible and a window assertion reads the wrong end - * of the buffer. + * Without the layout the plot area has no extent, so every coordinate lands on its edge, a hit test + * cannot tell inside from outside, and the renderer has nothing to place its viewport in. The draw + * is what renders the axes and annotations that assertions about them read back. * * Four test classes had grown their own copy of this, with the comment explaining it in three of * them and the draw missing from one. diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt index 16c953db28..23e24a84d1 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt @@ -84,7 +84,7 @@ class MetricsAnnotationSpanTest { // Pan back to where that marker lives, and record that the user drove the viewport. chart.setVisibleXRangeMaximum(VISIBLE_WINDOW.toFloat()) - chart.moveViewToX(0f) + chart.moveViewToXNow(0f) draw(chart) val event = MotionEvent.obtain(0L, 0L, MotionEvent.ACTION_MOVE, 10f, 10f, 0) chart.onChartGestureListener.onChartTranslate(event, -50f, 0f) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt index 6cfdfa6ed6..c9713c73f9 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt @@ -84,7 +84,7 @@ class MetricsChartAxisTapTest { // Zoom first: an unzoomed chart shows everything, so there is nothing a pan could move. chart.setVisibleXRangeMaximum(VISIBLE_WINDOW.toFloat()) - chart.moveViewToX(0f) + chart.moveViewToXNow(0f) drawOnce(chart) assertThat(chart.lowestVisibleX).isLessThan(10f) assertThat(chart.highestVisibleX).isLessThan(SAMPLES / 2f) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt new file mode 100644 index 0000000000..d9636409b2 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt @@ -0,0 +1,143 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.ui + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.NetworkUsageWatcher +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Where the chart's viewport ends up when a page is bound before it has been laid out. + * + * That is the order the carousel always binds in -- `onBindViewHolder` attaches the renderer while + * RecyclerView is still laying the page out -- and the editor rebinds the whole carousel on every + * resume. Returning from the APK install prompt therefore left the chart parked in the zeroed head + * of the buffer, reading -9999s and drawing nothing, until the next sample landed a redraw a second + * or more later: an empty plot at the moment the user has just run a build and is looking straight + * at it (ADFA-5515). + */ +@RunWith(RobolectricTestRunner::class) +class MetricsChartNewestWindowTest { + private val context = ApplicationProvider.getApplicationContext() + + private fun renderer() = + NetworkUsageChartRenderer( + usageProvider = { + NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + }, + ) + + /** + * The window the chart is meant to settle on: the newest [MetricsChartRenderer.VISIBLE_SAMPLES] + * of them, ending on the newest sample. + * + * The window is asked to start one sample further right than this and is clamped back, since it + * cannot extend past the end of the data -- so the newest sample sits exactly on the right edge. + */ + private fun assertShowsNewestSamples(chart: SafeLineChart) { + val newest = (SAMPLES - 1).toFloat() + assertThat(chart.highestVisibleX).isWithin(TOLERANCE).of(newest) + assertThat(chart.lowestVisibleX) + .isWithin(TOLERANCE) + .of(newest - MetricsChartRenderer.VISIBLE_SAMPLES) + } + + @Test + fun `a page bound before it is laid out still opens on the newest samples`() { + val chart = SafeLineChart(context) + + // The carousel's order: attach first, lay out second. + renderer().attach(chart) + chart.layOutAndDraw() + + assertShowsNewestSamples(chart) + } + + @Test + fun `a page bound after it is laid out opens on the newest samples`() { + val chart = SafeLineChart(context) + chart.layOutAndDraw() + + renderer().attach(chart) + + assertShowsNewestSamples(chart) + } + + @Test + fun `a resize puts the window back without waiting for a sample`() { + val chart = SafeLineChart(context) + renderer().attach(chart) + chart.layOutAndDraw() + + // A size change resets the chart's transform, dropping the window. Only a redraw used to + // restore it, which is why a rotation showed samples from half an hour ago until the next + // tick. + chart.layOutAndDraw(width = CHART_WIDTH, height = CHART_HEIGHT + 40) + + assertShowsNewestSamples(chart) + } + + @Test + fun `a detached renderer stops following the chart it left`() { + val chart = SafeLineChart(context) + val renderer = renderer() + renderer.attach(chart) + chart.layOutAndDraw() + renderer.detach() + + // A resize drops the window, and nothing should put it back: the page is on its way to + // another renderer, and a stale listener would fight whichever one binds next. + chart.layOutAndDraw(width = CHART_WIDTH, height = CHART_HEIGHT + 40) + + assertThat(chart.lowestVisibleX).isWithin(TOLERANCE).of(0f) + } + + @Test + fun `a rebind onto a new page forgets a pan on the old one`() { + val renderer = renderer() + val first = SafeLineChart(context) + renderer.attach(first) + first.layOutAndDraw() + + // The user pans. From here the viewport on *this* chart is theirs, not the renderer's. + checkNotNull(first.onChartGestureListener).onChartTranslate(null, -20f, 0f) + + // A resume rebinds the carousel, which attaches the replacement page before the outgoing + // one is recycled -- so the detach naming the old chart arrives afterwards and finds a + // different one bound. The pan belonged to the page the user left; the fresh page must + // still open on the newest samples. + val second = SafeLineChart(context) + renderer.attach(second) + renderer.detachIfAttached(first) + second.layOutAndDraw() + + assertShowsNewestSamples(second) + } + + private companion object { + /** Longer than the visible window, so there is a wrong end of the buffer to park in. */ + const val SAMPLES = 200 + + /** The viewport is computed in pixels and read back as a value, so it lands near-exactly. */ + const val TOLERANCE = 0.01f + } +}