ADFA-5527: Follow the system font scale in the metrics charts, up to a ceiling - #1796
Open
davidschachterADFA wants to merge 2 commits into
Open
Conversation
MPAndroidChart sizes its text in dp, so nothing the charts drew responded to the system font scale: a user who asked for larger text got it everywhere in the IDE except inside these plots, where the text is already the smallest on the screen. Measured before this change, an axis label went 19px to 20px between font scales 1.0 and 2.0 while an ordinary TextView beside it went 36px to 54px. Followed only to 1.5, because a plot is dense by nature and the strip is a fixed 248dp. Option 1 of the four written up on the ticket. Growing the text is not enough on its own, and the first version of this change proved it: the label count stayed put, so the memory page's nine value labels went from 29px apart to 6px. Bigger text, crowded axis -- worse, not better. The count now falls as the text grows, from six at scale 1 to four at the ceiling, as a hint rather than a command so that granularity still has the last word. That matters on the temperature axis, which is pinned to whole degrees. The annotation rows scale too. Eight rows sized for scale-1 text would have overlapped exactly when the labels grew, which is what the staggering exists to prevent. Verified on device, not by eye. At font scale 2.0 the memory page shows five value labels instead of nine, with the smallest gap between them at 28px against 31px at scale 1.0 -- the same readability with visibly larger text. On the temperature and power page, five labels per axis, whole degrees and whole watts, no repeats, smallest gaps 97px and 28px. Not in the metrics carousel stack, deliberately. It touches the shared renderer and the annotation geometry, and five reviewed PRs are waiting on approval; adding it there would invalidate all of them to fix something that predates them. Branched off ADFA-5509 because MetricsChartRenderer does not exist on stage yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz
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.
The test that justifies the 1.5 ceiling compared eight rows against a hand-picked 150dp, with a comment admitting the number was a guess. A guess pins nothing: no layout change could move it, so the test could not tell a shorter strip or a taller title row from a safe one. It now measures the whole way down -- the strip at the dimen the layout uses, the pager after a real measure and layout at 360dp wide, and the plot as the content rect a real chart page reports once a renderer has put its legend and axis on it. Nothing is allowed for by hand. Raising MAX_TEXT_SCALE to 2.0 now fails it, 192dp of rows against a 169dp plot, which is the claim the test makes about why the ceiling is where it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
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.
What
The metrics charts now follow the system font scale, up to a ceiling of 1.5.
MPAndroidChart sizes its text in dp, not sp, so nothing the charts drew responded to the font scale at all — a user who asked for larger text got it everywhere in the IDE except inside these plots, where the text is already the smallest on the screen. Measured before this change, on the same text in two screenshots normalised to one scale:
-54s), drawn by the chartTextViewbeside themWhy a ceiling, and not the full scale
This is option 1 of the four written up on ADFA-5527. A plot is dense by nature and the strip is a fixed
editor_mem_usage_view_heightof 248dp. At the platform's full 2.0 the axis labels collide with each other and the eight staggered annotation rows overlap, so honouring the scale literally would make the chart less readable rather than more. 1.5 gives most of the benefit and keeps the plot legible at the extreme.Growing the text was not enough on its own
The first version of this change scaled the text and left the label count alone. The result was worse, not better: the memory page's nine value labels went from 29 px apart to 6 px — bigger text, crowded axis. The count now falls as the text grows, from six at scale 1 to four at the ceiling, and is applied as a hint rather than a command so that
granularitystill has the last word. That matters on the temperature axis, which ADFA-5499 pinned to whole degrees.The annotation rows scale too. Eight rows sized for scale-1 text would have overlapped exactly when the labels grew, which is what the staggering in ADFA-5509 exists to prevent.
Verified on device, not by eye
Pixel 6 Pro, measured from screenshots rather than judged:
Same readability, visibly larger text. The interaction I was most worried about — the label-count hint fighting the whole-degree granularity on the temperature axis — did not materialise: granularity wins, as the code comment claims.
Six unit tests pin the policy. Three of them fail against the unfixed code; the other three are guards (no change at scale 1, no shrinking below scale 1, eight rows still fit at the ceiling). The annotation-row test asserts the
yOffsetof a drawn marker rather than calling the helper, because an earlier version called the helper directly and passed even with the renderer still using the unscaled constant.Scope
Not part of the metrics carousel stack, deliberately. It touches the shared renderer and the annotation geometry, and five reviewed PRs (#1784, #1785, #1790, #1791, #1792) are waiting on approval — adding it there would invalidate all of them to fix something that predates them. The charts have behaved this way since the memory chart was first added; the carousel work made it reach three pages instead of one.
Branched off
feature/ADFA-5509-build-annotationsrather thanstage, becauseMetricsChartRendererdoes not exist onstageyet. This PR is blocked by that whole stack landing.Review fix: the test that justifies the ceiling now measures
eight annotation rows still fit the plot at the ceilingcompared eight rows against a hand-pickedPLOT_HEIGHT_DP = 150f, with a comment admitting the number was a guess. A guess pins nothing — no layout change could move it, so the test could not tell a shorter strip or a taller title row from a safe one, which is the whole risk a font-scale ticket carries.It measures the whole way down now, with nothing allowed for by hand:
editor_mem_usage_view_height, read from resources;item_metrics_chartpage reports after a real renderer has put its legend and axis on it.Raising
MAX_TEXT_SCALEto 2.0 now fails it — 192dp of rows against a 169dp plot — which is the claim the test makes about why the ceiling is 1.5. It passed at 2.0 before.🤖 Generated with Claude Code
https://claude.ai/code/session_01QeW3M24yD6HNhEnbuNW7Hz