Reserved-row toolbar: Stage 1 primitives, dependency contracts, and Windows inventory - #1751
Draft
tleonhardt wants to merge 10 commits into
Draft
Reserved-row toolbar: Stage 1 primitives, dependency contracts, and Windows inventory#1751tleonhardt wants to merge 10 commits into
tleonhardt wants to merge 10 commits into
Conversation
Groundwork for keeping the bottom toolbar off the scroll path. Not yet wired into toolbar rendering; that is a separate change. A DECSTBM scroll region stops ordinary output from scrolling through the bottom rows, but is not sufficient on its own: ED (ESC [ J), which prompt_toolkit's renderer uses to erase, ignores the scroll margins and clears to the bottom of the display regardless. DL (ESC [ M) is bounded by the margins, and deleting every line from the cursor to the bottom margin produces the same all-blank result, so it substitutes for ED while respecting the reserved rows. DL also needs no knowledge of the cursor's row, which matters because Output does not track one, and it degrades to ED's behaviour when no region is set. The region is anchored at row 1 because a region starting lower orphans the rows above it, which then never scroll and never reach the terminal's scrollback. Verified against real tmux 3.7c: the reserved row survives 120 lines of scrolling and a bounded erase, rows above the cursor are preserved, scrolled lines reach scrollback from the first line, and the reserved row never leaks into history.
The reserved-row toolbar work relies on details of prompt_toolkit that are not public API: the cursor-position-report arithmetic, both destructive erase calls going through the Output interface, and the renderer's diff baseline. A silent change to any of these would break terminal rendering in ways that are hard to attribute, so lock them behaviorally here instead. The Windows cases can only run on Windows, where CI is the only place they get exercised: Windows10_Output is a registered virtual subclass rather than a real one, so capability checks must not rely on inheritance; geometry is delegated to the native backend, so adapting get_size() alone is insufficient; its inner VT output carries a zero-size stub; and legacy Win32Output.erase_down is a separate implementation that a VT sequence replacement never reaches. Verified by mutating prompt_toolkit locally: breaking the CPR formula, removing either erase call, or leaving the diff baseline set each fails these tests.
Temporary diagnostic for qualifying the reserved-row bottom toolbar on Windows. It records what the qualification needs and a checkout cannot otherwise supply: the input and output backend classes actually selected, viewport geometry against the backing console buffer, which object serves each delegated method, the rows-below-cursor value the renderer uses for available height, and the console mode before and after a scroll-region probe. It refuses to run unless stdout and stdin are both terminals. Redirecting or piping selects PlainTextOutput, which would record the wrong backend and silently invalidate the whole inventory. The probe always restores full-screen margins and attributes, so it leaves the terminal as it found it. Intended to be removed once Windows qualification is complete.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## reserved_row_toolbar #1751 +/- ##
========================================================
- Coverage 99.69% 99.66% -0.03%
========================================================
Files 25 26 +1
Lines 6464 6515 +51
========================================================
+ Hits 6444 6493 +49
- Misses 20 22 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Two defects found in review, neither covered by the existing tests. Entering the context saved only whether the output already had an instance-level erase_down, not what it was. Exiting then left the bounded erase installed and lost the caller's implementation, so lines kept being deleted after the reservation ended. Save the previous attribute and put it back. scroll_region_sequence() accepted a reservation leaving one usable row and emitted ESC[1;1r. DECSTBM requires the bottom margin to exceed the top, so terminals ignore that sequence and keep their previous margins: the caller believes it holds a reservation while output still scrolls through the reserved rows and destroys them. Measured on tmux 3.7c, where the marker was overwritten and 22 rows of output spilled through the region. The failure is total rather than degraded, so callers must release the reservation below this floor instead of narrowing it. Reject the case with a message naming the floor. Both fixes are mutation-checked: restoring the reported behavior, or lowering the floor, fails the new tests.
Vt100_Output.write_raw() only appends to the output's own buffer, so neither the region nor its reset reached the terminal on its own. Verified before fixing: after leaving the context the reset was still sitting in the buffer as ['\x1b[r'], on the normal and the exception path alike. That left the margins restricted whenever the body exited without another renderer operation, so later shell output kept scrolling inside the old region. Entry had the same defect from the other direction: the reservation was only queued, so anything written before the next flush could still scroll through the reserved rows. Flush after writing each sequence, so the reservation is in force once __enter__ returns and restoration has actually happened once __exit__ does. Four tests were passing only because they flushed after the context exited, or asserted outside it; they now assert what a caller would actually observe. Removing either flush fails them.
…rase Two defects found in review. Changing the scroll margins moves the cursor. DECSTBM homes it, and so does the reset: measured on tmux 3.7c, a cursor at row 10 column 7 landed at row 1 column 1 after each. Entering the reservation below existing output therefore sent later rendering to the top of the screen, and leaving it did the same to subsequent shell output, overwriting what was already there. Wrap both margin changes in a save/restore pair. This helper cannot place the cursor when it starts inside the reserved band, because discovering the row needs a cursor-position report and it has no input to read one from. That precondition is now documented; placing the prompt within the usable area belongs to the layer that owns the terminal. Renderer.clear(), which Ctrl-L reaches, calls erase_screen() as well as erase_down(). Only the latter was bounded, so an unbounded ED2 still erased the reserved rows. Bound erase_screen too: home inside the region and delete every usable line, which matches ED2's erase-and-home semantics without touching the reserved rows. Verified against a real terminal rather than only through emitted sequences: with the cursor at row 9 column 5, it is unchanged after entry and after exit, sits at the region's home after a clear, and the reserved row survives Ctrl-L with the region fully cleared. All fifteen mutations of this module fail their tests.
Three claims in this module were wrong or overstated. The opening paragraphs said DL leaves "the same all-blank result" as ED and was safe to leave installed. It is not: ED preserves the part of the cursor's line before the cursor, while DL deletes the whole line. Measured on tmux 3.7c, running DL from a nonzero column destroyed committed text to the left of the cursor. The substitution is sound only from column zero, which the three renderer paths reaching erase_down all normalize to first. That is a precondition to enforce, not a property to assume. The bounded full-screen erase was described as reproducing ED2 semantics. ED2 does not move the cursor -- measured, a cursor at row 12 column 33 was unchanged across it -- and Renderer.clear() homes separately afterwards. The bounded form homes first because DL clears downward from the cursor, so covering the usable area means starting at its top. That is a chosen contract, now described as one. No behavior changes.
_call_in_ui() polls the pending future with a 0.1s timeout and, on expiry, re-raises when the future is already done. That branch exists because concurrent.futures.TimeoutError is TimeoutError on Python 3.11+, so a callback raising a timeout of its own cannot be told apart by type from the poll expiring. Re-raising the caught exception conflates the two. When the callback completes in the window between the poll expiring and the future being inspected, the caller is told the call timed out even though it succeeded. Ask the future for its outcome instead: a callback that raised a timeout still propagates it, and one that produced a value now returns it. Found while investigating an intermittent failure of test_command_toolbar_ui_call_propagates_failures, which reproduced once in 60 runs before this change and not once in 120 after. The added regression test drives the interleaving deterministically rather than relying on timing.
Member
Author
|
@kmvanbrunt @bambu Can one of your please follow the instructions towards the bottom of the description under the The purpose of this branch and PR is to do some proof-of-concept testing for a new architectural approach where the bottom row (or 2 or whatever) is truly reserved just for the status bar and is completely separate from the normal app. The concept appears viable for both Linux and Mac, but needs some manual testing validation on Windows. The basic thing to run is: uv run python scripts/windows_toolbar_inventory.pyThen there is a checklist of things to look for. I sent you both an email with a status document explaining a little more. |
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
1 participant
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.
Stage 1 qualification work for the reserved-row bottom toolbar. Draft — this exists to
run the Windows CI matrix and give the Windows tester a known commit to check out. It
changes no runtime behaviour: nothing here is wired into toolbar rendering.
Targets the long-lived
reserved_row_toolbarintegration branch.Background
Pressing Enter makes the bottom toolbar blink, rated objectionable by a user on three
real-terminal runs. The cause is now understood: prompt-toolkit only rewrites rows that
differ from
Renderer._last_screen, andrenderer.erase()discards that baseline, forcinga full repaint. Three paths trigger it per command. The repaint that restores the toolbar
is the same event that erases it, so removing the erases cannot work — proven by positive
control.
The design (specified separately) keeps the toolbar on a DECSTBM-reserved bottom row that
ordinary output never scrolls through, with the renderer's destructive operations bounded to
the region above it.
What's here
cmd2/scroll_region.py+ tests — DECSTBM sequences and a margin-bounded replacement forerase_down.EDignores the scroll margins and would destroy the reserved row;DLisbounded by them and needs no knowledge of the cursor's row, which matters because
Outputdoes not track one. 15 tests, 100% line coverage, five mutations each killing their test.
tests/test_prompt_toolkit_contracts.py— locks the non-public prompt-toolkit detailsthe design depends on, so a dependency upgrade fails loudly here instead of silently
breaking terminal rendering: the CPR available-height arithmetic, both destructive erase
calls going through the
Outputinterface, and the renderer's diff baseline.Four of these run only on Windows, and CI is the only place they can run:
Windows10_Outputis a registered virtual subclass rather than a real one, so capabilitychecks must not rely on inheritance; geometry is delegated natively, so adapting
get_size()alone is insufficient; its inner VT output carries a zero-size stub; and legacyWin32Output.erase_downis a separate implementation a VT replacement never reaches.scripts/windows_toolbar_inventory.py— temporary diagnostic for the Windows tester,included so the tool and the code under test come from the same commit. It refuses to run
unless stdout and stdin are both terminals, because redirection selects
PlainTextOutputand would record the wrong backend. Its probe always restores margins and attributes. To be
removed once Windows qualification is done.
Evidence so far
POSIX primitives were qualified through real tmux 3.7c at 12, 24 and 40 rows — 15 probes
including controls, all agreeing with expectation. Highlights: a region anchored at row 1
preserves scrollback and protects the reserved row; anchoring at row 2 orphans the first
line; a one-usable-row region is not honoured at all;
DLat a nonzero column destroys theline prefix, so column-zero normalization must be enforced rather than assumed.
What this does not establish
exercises the contract tests and the suite, not real terminal behaviour.
Windows checklist for the manual pass
After CI is green, run in each terminal (directly — do not pipe or redirect):
For each: attach the generated JSON, then confirm — every check is phrased so yes is good:
TOOLBARMARKERstayed on the bottom row for the whole scrollprobe line 0001and in orderTOOLBARMARKER(it must not appear) — a no here is a hard failureOn mintty additionally re-run the original embedded-pager scenario (bottom bar on entry,
PgUp/PgDn, arrows, Enter,
/search) and record the launch command verbatim.