ADFA-5515: keep the chart on the newest samples across a carousel rebind - #1797
Open
davidschachterADFA wants to merge 4 commits into
Open
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
…as 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
davidschachterADFA
force-pushed
the
feature/ADFA-5515-carousel-resume-viewport
branch
from
September 7, 2026 12:42
5cb5c60 to
53ef2d2
Compare
…515-carousel-resume-viewport
…515-carousel-resume-viewport
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in how the metrics charts place their viewport. The second one, added in the last commit, is the field report ADFA-5515 was filed from; the first is real and was found on the way to it.
Defect 1: the window is placed against a transform that is about to be thrown away
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.
onBindViewHolderattaches the renderer while RecyclerView is still laying that chart out, which putsshowNewestWindowon a view with no plotting area:setVisibleXRangeMaximumclamps its scale against an empty content rect.moveViewToXcannot run at all, so MPAndroidChart queues it as a viewport job.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.
The change, two halves matching the two ways it goes wrong:
hasChartDimens()is false rather than leaving state behind for the layout to corrupt.SafeLineChartgainsmoveViewToXNow, which isMoveViewJob's body run inline, so the scale and the translation are computed against one transform. A size-changing layout 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.
Defect 2: a pan on one page disabled the follow-window on every page after it
attachreleased the outgoing chart's layout listener and nothing else, souserHasZoomed-- the flag that says the viewport is the user's rather than the renderer's -- survived onto the replacement.showNewestWindowreturns early on that flag, and so does every redraw after it. So once a user had panned, every page bound from then on opened on the oldest samples and stayed there: not until the next sample, but permanently. That is the reported symptom, an empty-looking plot reading -9999s, and it is why it shows up on resume and never on a fresh chart -- it needs a pan first.The fix is for
attachto run the wholedetachwhen it is replacing a different chart, rather than picking off one line of its teardown. That also clearsMemoryUsageChartRenderer.pidToDatasetIdx, which maps pids to dataset indexes in the chart that is going away.This is what I could not reproduce on hardware when the first three commits were written -- see below.
Two existing tests were passing because of defect 1
MetricsChartAxisTapTestandMetricsAnnotationSpanTestboth panned withchart.moveViewToX(0f)to simulate a user dragging to the oldest samples. On a Robolectric chart that is never attached to a window,View.postdrops the job in the view's 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, which is the bug. Both now pan for real throughmoveViewToXNow, and pass with or without this change.ChartLayout's doc claimed thedrawinlayOutAndDrawwas what ran the scroll. That was never the mechanism, and it is no longer needed for the window at all; the comment is corrected.Verification
Four new tests in
MetricsChartNewestWindowTest, each proved to fail against the unfixed renderer for the symptom it is named for:lowestVisibleX0.0, expected 139.0 -- no window at all, the whole buffer from its oldest endhighestVisibleX60.0, expected 199.0 -- a 60-sample window sitting on the oldest samples, the shape of the reported -9999s to -9939slowestVisibleX0.0, expected 139.0lowestVisibleX0.0, expected 139.0 -- the oldest samples, which is the reportA fifth test pins that a detached renderer stops following the chart it left; it is a guard on the new layout listener and passes either way by design.
Full
:appunit suite (655 tests) andspotlessCheckgreen. Sibling sweep:moveViewToX,setVisibleXRangeMaximum,fitScreen,centerViewPortandaddViewportJobhave no other production call sites outsideMetricsChartRenderer, andresetZoom'sfitScreen+ re-apply now goes through the same synchronous path. For defect 2, the only other place per-chart state is released isMetricsCarouselController's threedetach()calls on teardown, which were already correct;MemoryUsageChartRendereris the onedetachoverride and it is reached through the same virtual call.Why the device repro failed the first time
The first three commits went up saying the field cause was still open, because I could not reproduce the report on a Pixel 6 Pro (Android 17, arm64) with the sampling rate at 60s -- neither a home/resume round trip nor the ticket's own Run → build → dismiss-the-install-prompt sequence parked the chart, against a build of the unfixed code.
Defect 2 explains that: the repro needs a pan before the resume, and I never panned. A chart that has only ever followed the newest samples rebinds correctly however many times you pause it.
Font scale
No layout or text sizing changes: this touches only where the chart's viewport is placed. Verified at font scale 1.0. Not re-verified at 2.0, since ADFA-5527 (the base of this branch) covers chart text scaling and nothing here affects it.
🤖 Generated with Claude Code
https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j