From 00dbafaf7a62c8a3e4bd5faa0c52e91cc45e5bab Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sun, 6 Sep 2026 22:37:20 -0700 Subject: [PATCH 1/2] ADFA-5515: apply the chart's newest window against the transform it was measured on The carousel is unbound on pause and rebound on resume, and a rebind gives the pager a new adapter, so every page is a freshly inflated chart. onBindViewHolder attaches the renderer while RecyclerView is still laying that chart out, which puts showNewestWindow on a view with no plotting area: setVisibleXRangeMaximum clamps its scale against an empty content rect, and moveViewToX cannot run at all, so it is queued as a viewport job. The layout that follows resets the chart's transform to identity. The queued job then converts its x value through *that* transform, and the clamp afterwards restores the scale around a translation computed for a different one -- so the viewport lands neither where it was nor where it was asked to go, and nothing corrects it until the next sample lands a redraw. Two halves, matching the two ways it goes wrong: - Don't place a window before there is a plot to place it in. Bail while hasChartDimens() is false rather than leaving state behind for the layout to corrupt. - Re-apply on every layout, and apply it synchronously. SafeLineChart gains moveViewToXNow, which is MoveViewJob's body run inline, so the scale and the translation are computed against one transform. A layout that changes the chart's size resets the transform, which is precisely when the window has to go back. This also covers the rotation case ADFA-5486 fixed by re-applying on every redraw: the window now returns on the layout itself rather than on the next sample. Two existing tests turned out to be passing because of the defect. Both panned with chart.moveViewToX(0f) to simulate a user dragging to the oldest samples, and on a Robolectric chart that is never attached to a window View.post drops the job in the run queue, which is only flushed on attach -- so the pan moved nothing. They passed only while the chart already happened to be sitting on the oldest samples. Both now pan for real through moveViewToXNow, and pass with or without this change. ChartLayout's doc said the draw in layOutAndDraw was what ran the scroll; that was never the mechanism, and it is no longer needed for the window. Verified: the three new tests fail without the change, each for the symptom it is named for -- bound-before-layout and resize read lowestVisibleX 0.0 against an expected 139.0 (no window at all, the whole buffer from its oldest end), and bound-after-layout reads highestVisibleX 60.0 against an expected 199.0, a window sitting on the oldest samples, which is the shape of the reported -9999s to -9939s. Full :app unit suite and spotlessCheck green. Not confirmed as the cause of the field report. On a Pixel 6 Pro, with the sampling rate set to 60s so a mis-parked viewport would persist for up to a minute, neither a home/resume round trip nor the ticket's own repro -- Run, build, dismiss the install prompt -- parked the chart on this build, with or without the change. The defect fixed here is real and reproducible in isolation; whether it is what ADFA-5515 saw in the field is still open. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/ui/MetricsChartRenderer.kt | 34 ++++- .../com/itsaky/androidide/ui/SafeLineChart.kt | 21 +++ .../com/itsaky/androidide/ui/ChartLayout.kt | 9 +- .../ui/MetricsAnnotationSpanTest.kt | 2 +- .../androidide/ui/MetricsChartAxisTapTest.kt | 2 +- .../ui/MetricsChartNewestWindowTest.kt | 121 ++++++++++++++++++ 6 files changed, 180 insertions(+), 9 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt 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 d06591e9b5..f20ed304ce 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 @@ -155,8 +156,12 @@ 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 [detach] that names it. + this.chart?.removeOnLayoutChangeListener(newestWindowOnLayout) this.chart = chart configure(chart) + chart.addOnLayoutChangeListener(newestWindowOnLayout) rebuild() } @@ -167,6 +172,7 @@ abstract class MetricsChartRenderer( @CallSuper open fun detach() { userHasZoomed = false + chart?.removeOnLayoutChangeListener(newestWindowOnLayout) chart = null } @@ -277,10 +283,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 8362a2565c..b5acd0f684 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/SafeLineChart.kt @@ -125,6 +125,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 8653db671e..e651e41daf 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..7f44cfb2e0 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt @@ -0,0 +1,121 @@ +/* + * 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) + } + + 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 + } +} From 53ef2d25ad18bff6ff55b4ff9f29e12de42fb28b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 04:50:08 -0700 Subject: [PATCH 2/2] ADFA-5515: clear the pan flag when a rebind replaces the chart attach() released only the outgoing chart's layout listener, so userHasZoomed survived onto the replacement. A user who had panned once got every later page bound with the follow-window already disabled -- showNewestWindow returns early on the flag, and so does every redraw after it -- leaving the chart parked in the zeroed head of the buffer for good rather than until the next sample. That is the field symptom this ticket was filed for, and why it reproduces on resume and never on a fresh chart: it needs a pan first. The earlier commits fixed the bind-before-layout ordering, which was a real defect but not this one. Running the whole detach() also clears MemoryUsageChartRenderer's pidToDatasetIdx, which maps pids to dataset indexes in the chart that is going away. The test fails without the fix with lowestVisibleX 0.0 where 139.0 is expected -- the oldest samples, which is the report. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/ui/MetricsChartRenderer.kt | 13 +++++++++-- .../ui/MetricsChartNewestWindowTest.kt | 22 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) 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 f20ed304ce..edb96d9592 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsChartRenderer.kt @@ -157,8 +157,17 @@ 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 [detach] that names it. - this.chart?.removeOnLayoutChangeListener(newestWindowOnLayout) + // 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) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt index 7f44cfb2e0..d9636409b2 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt @@ -111,6 +111,28 @@ class MetricsChartNewestWindowTest { 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