Repository navigation
RESIZE KEYS K4 follow-up: a row's minimum rounds up, so no dead row key press (review notes A, B, C of #32471) - #32550
Conversation
…ve a middle row's lower edge, and take no dead press one permille above a minimum (red) Follow-ups A, B, C of the re-review of #32471 (SHIP at 76e9d24, comment 6585271874). A: the scrolling test of #32471 could not fail for the part step; [520, 500] must give [500, 500]. B: a three-row case. C: the row minimum in permille is rounded to the nearest, so a row stored one above it can take an Up press that moves no pixel. (cherry picked from commit ecf6522)
…ht, so it fails on a floor rounded to the nearest permille (red) The first version (ecf6522, red step e7a1ebcb) passed on the old code: its few pane stacks never hit a minimum that sits just under a half permille. A copy of RowStackGeometry finds 56 such content heights between 64 and 400 pt at a 857 pt column (for example 64.75 pt: floor 114 rounded, 115 real; stored 115, Up sends 114 and no pixel moves). (cherry picked from commit 3f9caa0)
… placement, so no row key press is dead next to it Review note C of #32471: the floor came from RowStackGeometry.permille(forPixelHeight:), which rounds to the nearest permille, so it could be up to half a permille below the real minimum; a row stored one permille above that floor took an Up press that moved no pixel. The floor now rounds up from (minimum + gap) / (height + gap) * 1000 (times the fill scale). Notes A and B need no code change: the tests pin the behavior #32471 already has. (cherry picked from commit f83ca3b) (cherry picked from commit ff9dbbe)
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
|
|
Review (independent subagent): FIX FIRST at 77abc43 I read the diff Finding (blocking): the floor is exact in continuous permille, not on the pixel gridWhether a press moves the edge depends on Worked example (H 857, gap 0, scale 2, row minimum 100.5 pt, so
Port counts, as (dead presses F+1 -> F, refusals one pixel above the shown minimum), base -> head:
(n ≈ 1250 to 1345 minimum heights per row.) So:
Suggested fix: compute the floor on the pixel grid at the screen's scale. Use the largest permille whose placed height still shows at What I checked and found sound
|
… the row at its minimum (red) Review of #32550 (FIX FIRST at 77abc43, comment 6587348634): the floor rounded up from the exact placement can show one device pixel above the minimum (Up refused while one more pixel could move), and at 1x dead presses remain. ShownResizeRange takes the backing scale (LayoutRootView passes the screen's reconciled scale); the floor does not use it yet here, so the test fails.
…ill shows at the row minimum on the screen's pixel grid Review of #32550 (FIX FIRST at 77abc43): rounded up from the exact placement, the floor could show one device pixel above the minimum (Up refused while one more pixel could move), and at 1x dead presses remained. The floor now replays RowStackGeometry.compute's placement on the screen's backing scale (ShownResizeRange.scale): the largest stored permille whose placed height, rounded to a pixel, is at most the minimum rounded up to one. A few steps from the exact estimate, no layout pass.
…d, so Up reaches the row minimum where both minimums do not fit Green + GUI step 7d3cdbc4 at 15df074 (KeyboardResizeRowsPixelFloorTests): with tall minimums (both rows need about 426 pt of an 857 pt column) the Up press was refused 0.5 to 2.5 pt above the minimum. Cause: stepped() refused every press when low > high, but high only bounds Down (it keeps the lower row at its minimum); Up only grows the lower row. Not a rounding order: the pixel floor is exact.
|
Review (independent subagent): SHIP at 429fea2 I read the diff What I checked
Non-blocking notes (follow-ups, not regressions in the PR's claim)
|
Summary
Follow-ups of the RESIZE KEYS K4 review (#32471, re-review SHIP at 76e9d24, comment 6585271874, notes A, B, C):
RowStackGeometry.permille(forPixelHeight:), which rounds to the nearest permille, so it could be up to half a permille below the real minimum. A row stored one permille above that floor took an Up press (Ctrl-Shift-K / Ctrl-Cmd-Up) that changed the model and moved no pixel: a dead press.ShownResizeRange.rowHeightsnow takes the largest stored permille that still shows at the minimum on the screen's pixel grid (the backing scale comes fromLayoutRootView), and each press direction checks only its own bound.[520, 500]pressed Up stops exactly at[500, 500](the P0 RESIZE KEYS K4: Ctrl-Shift-J/K and Ctrl-Cmd-Down/Up move a row edge one exact step #32471 test could not fail for this).Testing
On the fleet (
scripts/ci/package-test-lane.sh suite Packages/macOS/CmuxNext ...), compile-checked first. Same three commits as on the earlier branch (fix/resize-keys-rows-followup-2), cherry-picked onto feat after #32471 landed:e7a1ebcb854157ef99fd6a3ePASSED: no red (its few pane stacks never hit a minimum just under a half permille). The test was rewritten.0238a2a684d3c6729caf3137FAILED as expected: 54 dead presses. A and B pass on the old code (they pin P0 RESIZE KEYS K4: Ctrl-Shift-J/K and Ctrl-Cmd-Down/Up move a row edge one exact step #32471's behavior).ec668ceba17c41f19174e264PASSED: KeyboardResizeRowsFollowupTests 3/3, KeyboardResizeRowsEdgeCasesTests 5/5, KeyboardResizeRowsTests 2/2, KeyboardResizeShownLimitTests 2/2.Review FIX FIRST at 77abc43 (comment 6587348634): the floor rounded up from the exact placement could show one device pixel above the minimum, and dead presses remained at 1x. Then:
KeyboardResizeRowsPixelFloorTestsat 1x and 2x;ShownResizeRangetakes the backing scale): step1165ac076f27889b2fc1771f(aws-m4pro-4) FAILED as expected (dead presses at 1x, rows stopping above the minimum at 1x and 2x).7d3cdbc426944307db777996(cmux14) FAILED: a real fault: Up was refused 0.5-2.5 pt above a tall row minimum where the two rows' minimums do not both fit (stepped() refused every press when low > high, but high bounds Down only).d1c80cab9116595a31aa0175PASSED; green + GUI proof (display worker gui-session-v1, CMUX_TEST_REQUIRE_GUI=1) at the exact head 429fea2: step307a58a0d37a11f3b30f3c21on cmux-austin-mini-0 PASSED, 18 test cases, none skipped (these suites need no daemon): PixelFloor (1x, 2x), Followup 3/3, RowsEdgeCases 5/5, Rows 2/2, ShownLimit 2/2. Five identical submissions coalesced into this one step; the suites have no timing, so a repeat adds no evidence.Observed / Not run
No capture: geometry rounding only.
Changelog
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes the dead row-resize key press next to a row's minimum so Ctrl-Shift-K / Ctrl-Cmd-Up always moves an edge or is refused at 1x and 2x.
ShownResizeRangenow takes the screen's backing scale and derives the floor as the largest stored permille that still shows at the row's minimum on the pixel grid, replayingRowStackGeometry's placement. This replaces both the nearest-permille rounding (up to half a permille too low, causing dead presses) and the exact-placement rounding up (which could show one pixel above the minimum and still dead-press at 1x). Up presses now stop only at the upper row's minimum, so Up still moves the edge where both rows' minimums cannot fit — the lower row only grows.Written for commit 94e5501. Summary will update on new commits.