From 4740af4a86f1866de4fea1d36d9142870a282b24 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 6 Sep 2026 23:29:06 -0400 Subject: [PATCH 1/8] Add reserved-row scroll region with a margin-bounded erase 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. --- cmd2/scroll_region.py | 110 ++++++++++++++++++++++++++++++++++++ tests/test_scroll_region.py | 92 ++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+) create mode 100644 cmd2/scroll_region.py create mode 100644 tests/test_scroll_region.py diff --git a/cmd2/scroll_region.py b/cmd2/scroll_region.py new file mode 100644 index 000000000..8532b2f1a --- /dev/null +++ b/cmd2/scroll_region.py @@ -0,0 +1,110 @@ +"""Reserve the bottom rows of the terminal from scrolling, with a margin-bounded erase. + +A DECSTBM scroll region keeps ordinary output from scrolling through the bottom rows, so a +bottom toolbar painted there is never consumed by the scroll. On its own that is not enough: +``ED`` (``ESC [ J``), which prompt_toolkit's renderer uses to erase, ignores the scroll +margins and erases 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 leaves the same +all-blank result, so it is a drop-in replacement that respects the reserved rows. + +``DL`` needs no knowledge of the cursor's row, which matters because ``Output`` does not track +one. When no scroll region is set the margins cover the whole screen and the replacement +behaves exactly like ``ED``, so it is safe to leave installed. + +The region must be anchored at row 1. A region starting lower orphans the rows above it: they +never scroll and so never reach the terminal's scrollback. +""" + +from types import TracebackType +from typing import TYPE_CHECKING, Self + +if TYPE_CHECKING: # pragma: no cover + from prompt_toolkit.output import Output + +#: Upper bound on the lines one ``DL`` may delete. Terminals clamp the count to the scroll +#: region, so any value at least as large as the tallest plausible terminal clears to the +#: bottom margin exactly. +_MAX_ROWS = 9999 + + +def scroll_region_sequence(total_rows: int, reserved_rows: int) -> str: + """Build the DECSTBM sequence reserving ``reserved_rows`` rows at the bottom. + + :param total_rows: height of the terminal in rows + :param reserved_rows: number of bottom rows to keep out of the scroll region + :return: the escape sequence setting the scroll region + :raises ValueError: if the reservation would leave no usable rows + """ + if reserved_rows < 1: + raise ValueError(f"reserved_rows must be at least 1, got {reserved_rows}") + usable = total_rows - reserved_rows + if usable < 1: + raise ValueError(f"reserving {reserved_rows} of {total_rows} rows leaves no usable rows") + return f"\x1b[1;{usable}r" + + +def reset_scroll_region_sequence() -> str: + """Build the sequence restoring full-screen scroll margins. + + :return: the escape sequence resetting the scroll region + """ + return "\x1b[r" + + +def bounded_erase_down_sequence(rows: int = _MAX_ROWS) -> str: + """Build a margin-bounded replacement for ``ED``. + + :param rows: maximum number of lines to delete; terminals clamp this to the scroll region + :return: the escape sequence clearing from the cursor to the bottom margin + """ + return f"\x1b[{rows}M" + + +class ReservedBottomRows: + """Context manager reserving bottom rows and bounding the renderer's erase. + + On entry it sets the scroll region and replaces the output's ``erase_down`` with a + margin-bounded equivalent; on exit it restores both, even if the body raises. + """ + + def __init__(self, output: "Output", reserved_rows: int = 1) -> None: + """Initialize the region. + + :param output: the prompt_toolkit output to reserve rows on + :param reserved_rows: number of bottom rows to keep out of the scroll region + """ + self._output = output + self._reserved_rows = reserved_rows + self._total_rows = output.get_size().rows + # Validate eagerly so a bad reservation fails at construction, not on entry. + self._region_sequence = scroll_region_sequence(self._total_rows, reserved_rows) + self._had_own_erase_down = False + + @property + def usable_rows(self) -> int: + """Number of rows available to the application, excluding the reserved rows.""" + return self._total_rows - self._reserved_rows + + def _bounded_erase_down(self) -> None: + """Erase from the cursor to the bottom margin, leaving the reserved rows intact.""" + self._output.write_raw(bounded_erase_down_sequence(self.usable_rows)) + + def __enter__(self) -> Self: + """Set the scroll region and install the bounded erase.""" + self._output.write_raw(self._region_sequence) + # Shadow the bound method with an instance attribute. setattr keeps this legible to + # type checkers, which otherwise reject assigning over a method. + self._had_own_erase_down = "erase_down" in vars(self._output) + setattr(self._output, "erase_down", self._bounded_erase_down) # noqa: B010 + return self + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: TracebackType | None, + ) -> None: + """Restore the original erase and full-screen scroll margins.""" + if not self._had_own_erase_down: + delattr(self._output, "erase_down") + self._output.write_raw(reset_scroll_region_sequence()) diff --git a/tests/test_scroll_region.py b/tests/test_scroll_region.py new file mode 100644 index 000000000..d8e28a264 --- /dev/null +++ b/tests/test_scroll_region.py @@ -0,0 +1,92 @@ +"""Tests for the reserved-bottom-row scroll region and its margin-bounded erase.""" + +import io + +import pytest +from prompt_toolkit.output.vt100 import Vt100_Output + +from cmd2 import scroll_region as sr + + +def make_output(rows: int = 24, cols: int = 80) -> tuple[Vt100_Output, io.StringIO]: + stream = io.StringIO() + return Vt100_Output( + stream, lambda: __import__("prompt_toolkit.data_structures", fromlist=["Size"]).Size(rows, cols) + ), stream + + +class TestSequences: + def test_scroll_region_is_anchored_at_row_one(self) -> None: + """Anchoring at row 1 is required or lines scrolled out never reach scrollback.""" + assert sr.scroll_region_sequence(24, 1) == "\x1b[1;23r" + + def test_scroll_region_honors_multiple_reserved_rows(self) -> None: + assert sr.scroll_region_sequence(24, 3) == "\x1b[1;21r" + + def test_reset_sequence_restores_full_screen_margins(self) -> None: + assert sr.reset_scroll_region_sequence() == "\x1b[r" + + def test_bounded_erase_uses_delete_line_not_erase_display(self) -> None: + """ED ignores the margins; DL is bounded by them, so the pinned row survives.""" + seq = sr.bounded_erase_down_sequence() + assert seq.endswith("M"), f"expected a DL sequence, got {seq!r}" + assert "J" not in seq, "must not use ED, which ignores the scroll margins" + + @pytest.mark.parametrize(("total", "reserved"), [(24, 0), (24, 24), (24, 25), (1, 1), (24, -1)]) + def test_rejects_regions_that_would_leave_no_usable_rows(self, total: int, reserved: int) -> None: + with pytest.raises(ValueError, match=r"reserved_rows must be at least 1|leaves no usable rows"): + sr.scroll_region_sequence(total, reserved) + + +class TestReservedBottomRows: + def test_sets_the_region_on_enter_and_resets_it_on_exit(self) -> None: + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=1): + output.flush() + assert "\x1b[1;23r" in stream.getvalue() + output.flush() + assert stream.getvalue().endswith("\x1b[r") + + def test_erase_down_is_bounded_while_reserved(self) -> None: + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=1): + output.flush() + stream.truncate(0), stream.seek(0) + output.erase_down() + output.flush() + written = stream.getvalue() + assert "\x1b[J" not in written, "unbounded ED would destroy the reserved row" + assert written.endswith("M") + + def test_bounded_erase_clears_the_whole_usable_region(self) -> None: + """A DL count of 1 (or 0, which terminals read as 1) would clear one row, not the region.""" + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=2) as region: + output.flush() # drain the region sequence out of prompt_toolkit's buffer first + stream.truncate(0), stream.seek(0) + output.erase_down() + output.flush() + assert stream.getvalue() == f"\x1b[{region.usable_rows}M" + + def test_original_erase_down_is_restored_on_exit(self) -> None: + output, stream = make_output(rows=24) + original = output.erase_down + with sr.ReservedBottomRows(output, reserved_rows=1): + assert output.erase_down != original + assert output.erase_down == original + stream.truncate(0), stream.seek(0) + output.erase_down() + output.flush() + assert "\x1b[J" in stream.getvalue() + + def test_region_is_reset_even_if_the_body_raises(self) -> None: + output, stream = make_output(rows=24) + with pytest.raises(RuntimeError), sr.ReservedBottomRows(output, reserved_rows=1): + raise RuntimeError("boom") + output.flush() + assert stream.getvalue().endswith("\x1b[r") + + def test_usable_rows_excludes_the_reserved_rows(self) -> None: + output, _ = make_output(rows=24) + region = sr.ReservedBottomRows(output, reserved_rows=2) + assert region.usable_rows == 22 From 56aa0af99f14e67764bd636604f393648f562d2b Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 7 Sep 2026 01:12:54 -0400 Subject: [PATCH 2/8] Add contract tests for the prompt_toolkit internals we depend on 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. --- tests/test_prompt_toolkit_contracts.py | 154 +++++++++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 tests/test_prompt_toolkit_contracts.py diff --git a/tests/test_prompt_toolkit_contracts.py b/tests/test_prompt_toolkit_contracts.py new file mode 100644 index 000000000..3ed7400d6 --- /dev/null +++ b/tests/test_prompt_toolkit_contracts.py @@ -0,0 +1,154 @@ +"""Contract tests for the prompt_toolkit internals the reserved-row design depends on. + +These lock assumptions that are *not* part of prompt_toolkit's public API. They exist so a +dependency upgrade fails loudly here rather than silently breaking terminal rendering, and +so the Windows-only facts are exercised by CI, which is the only place they can run. + +Each test names the design requirement it protects. +""" + +import inspect +import sys + +import pytest +from prompt_toolkit.output import DummyOutput, Output +from prompt_toolkit.renderer import Renderer +from prompt_toolkit.styles import default_ui_style + +WINDOWS_ONLY = pytest.mark.skipif(sys.platform != "win32", reason="Windows backend is importable only on Windows") + + +def make_renderer(output: Output) -> Renderer: + return Renderer(default_ui_style(), output) + + +class TestCursorPositionArithmetic: + """Protects the geometry model: physical CPR rows against a virtual total.""" + + def test_available_height_is_rows_minus_row_plus_one(self) -> None: + """The reserved-row geometry model depends on this exact formula.""" + output = DummyOutput() + renderer = make_renderer(output) + renderer.report_absolute_cursor_row(5) + assert renderer._min_available_height == output.get_size().rows - 5 + 1 + + def test_cursor_in_the_reserved_band_yields_nonpositive_height(self) -> None: + """The R5 hazard: a CPR answered from the reserved row reports no usable height. + + With the region anchored at row 1, a virtual height of U and a physical cursor on + row H > U gives a nonpositive result. On a VT backend -- where there is no native + rows-below query to fall back on -- that leaves height_is_known false, so the + toolbar is not drawn at all, with no error. + """ + physical_rows, reserved = 24, 1 + usable = physical_rows - reserved + + class Vt100LikeOutput(DummyOutput): + """A virtual size, and no native rows-below query -- as on POSIX.""" + + def get_size(self): # type: ignore[no-untyped-def] + size = super().get_size() + return type(size)(rows=usable, columns=size.columns) + + def get_rows_below_cursor_position(self) -> int: + raise NotImplementedError + + renderer = make_renderer(Vt100LikeOutput()) + renderer.report_absolute_cursor_row(physical_rows) # cursor parked in the band + assert renderer._min_available_height <= 0 + assert not renderer.height_is_known + + def test_a_cursor_inside_the_usable_region_keeps_height_known(self) -> None: + """Positive pair for the hazard above: the same setup, cursor in the usable area.""" + physical_rows, reserved = 24, 1 + usable = physical_rows - reserved + + class Vt100LikeOutput(DummyOutput): + def get_size(self): # type: ignore[no-untyped-def] + size = super().get_size() + return type(size)(rows=usable, columns=size.columns) + + def get_rows_below_cursor_position(self) -> int: + raise NotImplementedError + + renderer = make_renderer(Vt100LikeOutput()) + renderer.report_absolute_cursor_row(usable) # last usable row + assert renderer._min_available_height == 1 + assert renderer.height_is_known + + +class TestEraseInterceptionPoints: + """Protects the bounded-erase design: both destructive paths go through Output.""" + + def test_output_interface_exposes_both_erase_operations(self) -> None: + assert callable(getattr(Output, "erase_down", None)) + assert callable(getattr(Output, "erase_screen", None)) + + def test_renderer_erase_goes_through_output_erase_down(self) -> None: + """`renderer.erase()` must remain interceptable at the Output boundary.""" + calls: list[str] = [] + + class RecordingOutput(DummyOutput): + def erase_down(self) -> None: + calls.append("erase_down") + + make_renderer(RecordingOutput()).erase() + assert "erase_down" in calls + + def test_renderer_clear_goes_through_output_erase_screen(self) -> None: + """Ctrl-L must remain interceptable; an unbounded ED2 would wipe the reserved row.""" + calls: list[str] = [] + + class RecordingOutput(DummyOutput): + def erase_screen(self) -> None: + calls.append("erase_screen") + + make_renderer(RecordingOutput()).clear() + assert "erase_screen" in calls + + +class TestDiffBaseline: + """Protects the discarded-frame recovery contract (design section 7.2.1).""" + + def test_renderer_starts_with_no_diff_baseline(self) -> None: + assert make_renderer(DummyOutput())._last_screen is None + + def test_reset_clears_the_diff_baseline(self) -> None: + renderer = make_renderer(DummyOutput()) + renderer._last_screen = object() # type: ignore[assignment] + renderer.reset() + assert renderer._last_screen is None + + +@WINDOWS_ONLY +class TestWindowsBackendContract: + """Windows facts the design relies on. CI is the only place these can run.""" + + def test_windows10_output_is_a_registered_virtual_subclass(self) -> None: + """isinstance succeeds, but it is not in the MRO -- capability checks must not + rely on inheritance.""" + from prompt_toolkit.output.windows10 import Windows10_Output + + assert issubclass(Windows10_Output, Output) + assert Output not in Windows10_Output.__mro__ + + def test_geometry_is_delegated_natively(self) -> None: + """Adapting get_size() alone is insufficient; available height comes from Win32.""" + from prompt_toolkit.output.windows10 import Windows10_Output + + source = inspect.getsource(Windows10_Output.__getattr__) + assert "get_size" in source + assert "get_rows_below_cursor_position" in source + + def test_inner_vt100_output_has_a_zero_size_stub(self) -> None: + """Anything wrapping Windows10_Output must never consult vt100_output.get_size().""" + from prompt_toolkit.output.windows10 import Windows10_Output + + assert "Size(0, 0)" in inspect.getsource(Windows10_Output.__init__) + + def test_legacy_win32_erase_down_is_a_separate_implementation(self) -> None: + """The VT sequence replacement does not reach legacy Win32Output.""" + from prompt_toolkit.output.vt100 import Vt100_Output + from prompt_toolkit.output.win32 import Win32Output + + assert Win32Output.erase_down is not Vt100_Output.erase_down From 3f4b68fe11918f2134bbfc1710dfce7e456878e5 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 7 Sep 2026 09:04:45 -0400 Subject: [PATCH 3/8] Add a Windows terminal inventory script for toolbar qualification 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. --- .gitignore | 3 + scripts/windows_toolbar_inventory.py | 213 +++++++++++++++++++++++++++ 2 files changed, 216 insertions(+) create mode 100755 scripts/windows_toolbar_inventory.py diff --git a/.gitignore b/.gitignore index 36763d4c6..b718ccb5e 100644 --- a/.gitignore +++ b/.gitignore @@ -66,3 +66,6 @@ gha-creds-*.json # Superpowers extension docs/superpowers + +# Artifacts from scripts/windows_toolbar_inventory.py +scripts/windows_toolbar_inventory_*.json diff --git a/scripts/windows_toolbar_inventory.py b/scripts/windows_toolbar_inventory.py new file mode 100755 index 000000000..6d92ff299 --- /dev/null +++ b/scripts/windows_toolbar_inventory.py @@ -0,0 +1,213 @@ +#!/usr/bin/env python3 +"""Windows terminal inventory for the reserved-row bottom-toolbar work. + + uv run python scripts/windows_toolbar_inventory.py + +Run this INSIDE each terminal under test (Windows Terminal, mintty/git-bash, +conhost). It records what the plan requires before manual qualification: backend +classes actually selected, viewport vs backing-buffer geometry, console-mode +restoration, and the checks that can be made without a human looking at the screen. + +It changes no cmd2 code and makes no permanent terminal changes: every escape +sequence it emits is reset before exit. +""" + +from __future__ import annotations + +import contextlib +import json +import os +import platform +import sys +from datetime import datetime, timezone +from typing import TYPE_CHECKING, Any + +import prompt_toolkit +from prompt_toolkit.input.defaults import create_input +from prompt_toolkit.output.defaults import create_output + +if TYPE_CHECKING: # pragma: no cover + from prompt_toolkit.output import Output + + +def section(title: str) -> None: + """Print a section heading. + + :param title: heading text + """ + print(f"\n=== {title} ===") + + +def inventory() -> tuple[dict[str, Any], Output]: + """Collect backend, geometry and console-mode facts for the current terminal. + + :return: the collected facts, and the output object they were collected from + """ + out = create_output(stdout=sys.stdout) + try: + inp = create_input() + in_cls = f"{type(inp).__module__}.{type(inp).__name__}" + except (OSError, ValueError, ImportError) as exc: # pragma: no cover - diagnostic + in_cls = f"" + + size = out.get_size() + data: dict[str, Any] = { + "captured_at": datetime.now(timezone.utc).isoformat(), + "platform": platform.platform(), + "os_release": platform.release(), + "python": sys.version, + "python_executable": sys.executable, + "prompt_toolkit": prompt_toolkit.__version__, + "TERM": os.environ.get("TERM"), + "WT_SESSION": os.environ.get("WT_SESSION"), + "MSYSTEM": os.environ.get("MSYSTEM"), + "TERM_PROGRAM": os.environ.get("TERM_PROGRAM"), + "output_class": f"{type(out).__module__}.{type(out).__name__}", + "input_class": in_cls, + "stdout_isatty": sys.stdout.isatty(), + "stdin_isatty": sys.stdin.isatty(), + "viewport_rows": size.rows, + "viewport_columns": size.columns, + } + + # Windows: the backing buffer is usually taller than the viewport, and the + # delegation whitelist means geometry comes from the native side while + # rendering goes through VT. + try: + info = out.get_win32_screen_buffer_info() # type: ignore[attr-defined] + data["win32_buffer_size"] = {"X": info.dwSize.X, "Y": info.dwSize.Y} + data["win32_window"] = { + "Left": info.srWindow.Left, + "Top": info.srWindow.Top, + "Right": info.srWindow.Right, + "Bottom": info.srWindow.Bottom, + } + data["win32_cursor"] = {"X": info.dwCursorPosition.X, "Y": info.dwCursorPosition.Y} + data["viewport_differs_from_buffer"] = (info.srWindow.Bottom - info.srWindow.Top + 1) != info.dwSize.Y + except (AttributeError, OSError, NotImplementedError) as exc: + data["win32_screen_buffer_info"] = f"" + + try: + data["rows_below_cursor_position"] = out.get_rows_below_cursor_position() + except (AttributeError, OSError, NotImplementedError) as exc: + data["rows_below_cursor_position"] = f"" + + # Which object actually serves erase_down / get_size on this backend? + for name in ("erase_down", "erase_screen", "get_size", "get_rows_below_cursor_position", "flush"): + try: + bound = getattr(out, name) + owner = getattr(bound, "__self__", None) + data[f"delegate::{name}"] = ( + f"{type(owner).__module__}.{type(owner).__name__}" if owner is not None else "" + ) + except (AttributeError, OSError, NotImplementedError) as exc: + data[f"delegate::{name}"] = f"" + + data["console_mode_before"] = _console_mode() + return data, out + + +def _console_mode() -> str: + """Read the Win32 console output mode, if this is a Windows console. + + :return: a human-readable description of the mode, or why it is unavailable + """ + try: + from ctypes import byref, windll # type: ignore[attr-defined] + from ctypes.wintypes import DWORD, HANDLE # type: ignore[attr-defined] + + handle = HANDLE(windll.kernel32.GetStdHandle(-11)) + mode = DWORD() + if not windll.kernel32.GetConsoleMode(handle, byref(mode)): + return "" + value = mode.value + return ( + f"0x{value:04X} " + f"(VIRTUAL_TERMINAL_PROCESSING={'on' if value & 0x0004 else 'off'}, " + f"WRAP_AT_EOL={'on' if value & 0x0002 else 'off'})" + ) + except (ImportError, AttributeError, OSError) as exc: + return f"" + + +def probe_decstbm(out: Output, rows: int) -> dict[str, Any]: + """Set and reset a reserved-row region; report whether modes survive it. + + Deliberately conservative: it prints a marker, establishes the region, writes + enough lines to scroll, then resets. The human confirms what they saw. + + :param out: the output to emit through + :param rows: the terminal's physical height + :return: what was emitted, and the console mode afterwards + """ + results: dict[str, Any] = {} + mark = "TOOLBARMARKER" + try: + out.write_raw(f"\x1b[{rows};1H{mark}") + out.write_raw(f"\x1b[1;{rows - 1}r") + out.write_raw("\x1b[1;1H") + for i in range(1, rows * 3): + out.write_raw(f"probe line {i:04d}\r\n") + out.flush() + results["region_emitted"] = f"\\x1b[1;{rows - 1}r" + results["lines_written"] = rows * 3 - 1 + finally: + out.write_raw("\x1b[r") # always restore full-screen margins + out.write_raw("\x1b[0m") + out.flush() + results["console_mode_after"] = _console_mode() + return results + + +def main() -> int: + """Run the inventory and the visual probe. + + :return: process exit status + """ + # Piping or redirecting selects PlainTextOutput, which would record the wrong + # backend and silently invalidate the whole inventory. + if not sys.stdout.isatty() or not sys.stdin.isatty(): + print("REFUSING TO RUN: stdout/stdin is not a terminal.") + print("Run this directly in the terminal under test -- do not pipe or redirect it,") + print("or the recorded backend classes will be wrong.") + return 2 + + data, out = inventory() + section("Environment inventory") + for k, v in data.items(): + print(f" {k:38s} {v}") + + rows = data["viewport_rows"] + if rows < 4: + print("\n viewport too small for the probe; resize and rerun") + return 2 + + section("DECSTBM probe (visual confirmation required)") + print(" About to reserve the bottom row, scroll past it, then reset.") + print(" WATCH THE BOTTOM ROW. Press Enter when ready.") + with contextlib.suppress(EOFError): + input() + data["decstbm_probe"] = probe_decstbm(out, rows) + + section("Report") + print(" console mode before :", data["console_mode_before"]) + print(" console mode after :", data["decstbm_probe"]["console_mode_after"]) + print(" modes match :", data["console_mode_before"] == data["decstbm_probe"]["console_mode_after"]) + print("\n Answer in the result template:") + print(" 1. Did TOOLBARMARKER stay on the bottom row for the whole scroll?") + print(" 2. Is the scrollback complete (probe line 0001 upward) and in order?") + print(" 3. Does TOOLBARMARKER appear anywhere in the scrollback? (it must not)") + print(" 4. After this program exits, does the shell prompt behave normally?") + + # Write beside this script rather than into whatever directory the tester + # happened to be in, so the artifact is easy to find and collect. + path = os.path.join(os.path.dirname(os.path.abspath(__file__)), f"windows_toolbar_inventory_{platform.node()}.json") + with open(path, "w") as fh: + json.dump(data, fh, indent=2, default=str) + print(f"\n machine-readable record written to: {path}") + print(" Attach that file to the result template.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) From 396f08da6a7b480f9ad6df52635c408746566a45 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 7 Sep 2026 10:04:29 -0400 Subject: [PATCH 4/8] Fix erase restoration and reject one-row scroll regions 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. --- cmd2/scroll_region.py | 35 +++++++++++++++++++++++++++-------- tests/test_scroll_region.py | 30 +++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/cmd2/scroll_region.py b/cmd2/scroll_region.py index 8532b2f1a..ac2185525 100644 --- a/cmd2/scroll_region.py +++ b/cmd2/scroll_region.py @@ -13,14 +13,25 @@ The region must be anchored at row 1. A region starting lower orphans the rows above it: they never scroll and so never reach the terminal's scrollback. + +A region needs at least two usable rows. DECSTBM requires the bottom margin to be greater +than the top, so a degenerate ``ESC [ 1 ; 1 r`` is ignored and the terminal silently keeps +its previous margins -- measured on tmux 3.7c, where output then scrolled through the +reserved row and destroyed it. The failure is total rather than degraded, so callers must +release the reservation below this floor rather than narrow it. """ from types import TracebackType from typing import TYPE_CHECKING, Self if TYPE_CHECKING: # pragma: no cover + from collections.abc import Callable + from prompt_toolkit.output import Output +#: Smallest region a terminal will honour. ``ESC [ 1 ; 1 r`` is ignored outright. +MIN_USABLE_ROWS = 2 + #: Upper bound on the lines one ``DL`` may delete. Terminals clamp the count to the scroll #: region, so any value at least as large as the tallest plausible terminal clears to the #: bottom margin exactly. @@ -33,13 +44,16 @@ def scroll_region_sequence(total_rows: int, reserved_rows: int) -> str: :param total_rows: height of the terminal in rows :param reserved_rows: number of bottom rows to keep out of the scroll region :return: the escape sequence setting the scroll region - :raises ValueError: if the reservation would leave no usable rows + :raises ValueError: if the reservation would leave fewer than two usable rows """ if reserved_rows < 1: raise ValueError(f"reserved_rows must be at least 1, got {reserved_rows}") usable = total_rows - reserved_rows - if usable < 1: - raise ValueError(f"reserving {reserved_rows} of {total_rows} rows leaves no usable rows") + if usable < MIN_USABLE_ROWS: + raise ValueError( + f"reserving {reserved_rows} of {total_rows} rows leaves {usable} usable row(s); " + f"a scroll region needs at least {MIN_USABLE_ROWS}" + ) return f"\x1b[1;{usable}r" @@ -78,7 +92,7 @@ def __init__(self, output: "Output", reserved_rows: int = 1) -> None: self._total_rows = output.get_size().rows # Validate eagerly so a bad reservation fails at construction, not on entry. self._region_sequence = scroll_region_sequence(self._total_rows, reserved_rows) - self._had_own_erase_down = False + self._previous_erase_down: Callable[[], None] | None = None @property def usable_rows(self) -> int: @@ -93,8 +107,10 @@ def __enter__(self) -> Self: """Set the scroll region and install the bounded erase.""" self._output.write_raw(self._region_sequence) # Shadow the bound method with an instance attribute. setattr keeps this legible to - # type checkers, which otherwise reject assigning over a method. - self._had_own_erase_down = "erase_down" in vars(self._output) + # type checkers, which otherwise reject assigning over a method. Capture any + # override already installed by a caller so exit can put it back rather than + # leaving ours in place. + self._previous_erase_down = vars(self._output).get("erase_down") setattr(self._output, "erase_down", self._bounded_erase_down) # noqa: B010 return self @@ -105,6 +121,9 @@ def __exit__( traceback: TracebackType | None, ) -> None: """Restore the original erase and full-screen scroll margins.""" - if not self._had_own_erase_down: - delattr(self._output, "erase_down") + if self._previous_erase_down is None: + delattr(self._output, "erase_down") # fall back to the class implementation + else: + setattr(self._output, "erase_down", self._previous_erase_down) # noqa: B010 + self._previous_erase_down = None self._output.write_raw(reset_scroll_region_sequence()) diff --git a/tests/test_scroll_region.py b/tests/test_scroll_region.py index d8e28a264..9f6fa5839 100644 --- a/tests/test_scroll_region.py +++ b/tests/test_scroll_region.py @@ -23,6 +23,10 @@ def test_scroll_region_is_anchored_at_row_one(self) -> None: def test_scroll_region_honors_multiple_reserved_rows(self) -> None: assert sr.scroll_region_sequence(24, 3) == "\x1b[1;21r" + def test_accepts_the_two_usable_row_floor(self) -> None: + """Two usable rows is the smallest region terminals actually honour.""" + assert sr.scroll_region_sequence(3, 1) == "\x1b[1;2r" + def test_reset_sequence_restores_full_screen_margins(self) -> None: assert sr.reset_scroll_region_sequence() == "\x1b[r" @@ -32,9 +36,12 @@ def test_bounded_erase_uses_delete_line_not_erase_display(self) -> None: assert seq.endswith("M"), f"expected a DL sequence, got {seq!r}" assert "J" not in seq, "must not use ED, which ignores the scroll margins" - @pytest.mark.parametrize(("total", "reserved"), [(24, 0), (24, 24), (24, 25), (1, 1), (24, -1)]) - def test_rejects_regions_that_would_leave_no_usable_rows(self, total: int, reserved: int) -> None: - with pytest.raises(ValueError, match=r"reserved_rows must be at least 1|leaves no usable rows"): + @pytest.mark.parametrize( + ("total", "reserved"), + [(24, 0), (24, 24), (24, 25), (1, 1), (24, -1), (24, 23), (2, 1), (3, 2)], + ) + def test_rejects_regions_that_would_leave_too_few_usable_rows(self, total: int, reserved: int) -> None: + with pytest.raises(ValueError, match=r"reserved_rows must be at least 1|needs at least"): sr.scroll_region_sequence(total, reserved) @@ -79,6 +86,23 @@ def test_original_erase_down_is_restored_on_exit(self) -> None: output.flush() assert "\x1b[J" in stream.getvalue() + def test_restores_a_pre_existing_instance_level_erase_down(self) -> None: + """A caller's own override must survive the reservation, not be replaced by ours.""" + output, _stream = make_output(rows=24) + calls: list[str] = [] + + def caller_override() -> None: + calls.append("caller") + + output.erase_down = caller_override # type: ignore[method-assign] + + with sr.ReservedBottomRows(output, reserved_rows=1): + pass + + assert output.erase_down is caller_override + output.erase_down() + assert calls == ["caller"], "the caller's override must be the one that runs" + def test_region_is_reset_even_if_the_body_raises(self) -> None: output, stream = make_output(rows=24) with pytest.raises(RuntimeError), sr.ReservedBottomRows(output, reserved_rows=1): From 6c7d2b9fba0aa15aef931382086f497a7050051a Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 7 Sep 2026 10:30:50 -0400 Subject: [PATCH 5/8] Flush the scroll region and its reset to the terminal 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. --- cmd2/scroll_region.py | 8 ++++++++ tests/test_scroll_region.py | 25 ++++++++++++++++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/cmd2/scroll_region.py b/cmd2/scroll_region.py index ac2185525..e9dcb47bd 100644 --- a/cmd2/scroll_region.py +++ b/cmd2/scroll_region.py @@ -106,6 +106,10 @@ def _bounded_erase_down(self) -> None: def __enter__(self) -> Self: """Set the scroll region and install the bounded erase.""" self._output.write_raw(self._region_sequence) + # write_raw only appends to the output's own buffer, so without this the + # reservation is merely queued and anything written before the next flush + # still scrolls through the reserved rows. + self._output.flush() # Shadow the bound method with an instance attribute. setattr keeps this legible to # type checkers, which otherwise reject assigning over a method. Capture any # override already installed by a caller so exit can put it back rather than @@ -127,3 +131,7 @@ def __exit__( setattr(self._output, "erase_down", self._previous_erase_down) # noqa: B010 self._previous_erase_down = None self._output.write_raw(reset_scroll_region_sequence()) + # Restoration has to reach the terminal here. A body that exits without another + # renderer operation -- an exception, or application shutdown -- would otherwise + # leave the margins restricted and later shell output scrolling inside them. + self._output.flush() diff --git a/tests/test_scroll_region.py b/tests/test_scroll_region.py index 9f6fa5839..6f6e5434c 100644 --- a/tests/test_scroll_region.py +++ b/tests/test_scroll_region.py @@ -49,11 +49,26 @@ class TestReservedBottomRows: def test_sets_the_region_on_enter_and_resets_it_on_exit(self) -> None: output, stream = make_output(rows=24) with sr.ReservedBottomRows(output, reserved_rows=1): - output.flush() assert "\x1b[1;23r" in stream.getvalue() - output.flush() assert stream.getvalue().endswith("\x1b[r") + def test_the_region_reaches_the_terminal_on_entry(self) -> None: + """Callers may rely on the reservation being in force once __enter__ returns.""" + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=1): + # No flush of our own: prompt_toolkit buffers write_raw, so an unflushed + # region sequence would leave the reservation merely promised. + assert "\x1b[1;23r" in stream.getvalue() + assert not output._buffer + + def test_the_reset_reaches_the_terminal_without_a_further_flush(self) -> None: + """A body that exits without another renderer operation must still be restored.""" + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=1): + output.flush() + assert stream.getvalue().endswith("\x1b[r"), "margins were left restricted" + assert not output._buffer, "the reset is still sitting in prompt_toolkit's buffer" + def test_erase_down_is_bounded_while_reserved(self) -> None: output, stream = make_output(rows=24) with sr.ReservedBottomRows(output, reserved_rows=1): @@ -69,11 +84,11 @@ def test_bounded_erase_clears_the_whole_usable_region(self) -> None: """A DL count of 1 (or 0, which terminals read as 1) would clear one row, not the region.""" output, stream = make_output(rows=24) with sr.ReservedBottomRows(output, reserved_rows=2) as region: - output.flush() # drain the region sequence out of prompt_toolkit's buffer first stream.truncate(0), stream.seek(0) output.erase_down() output.flush() - assert stream.getvalue() == f"\x1b[{region.usable_rows}M" + # Assert inside the region: on exit the reset is written and flushed too. + assert stream.getvalue() == f"\x1b[{region.usable_rows}M" def test_original_erase_down_is_restored_on_exit(self) -> None: output, stream = make_output(rows=24) @@ -107,8 +122,8 @@ def test_region_is_reset_even_if_the_body_raises(self) -> None: output, stream = make_output(rows=24) with pytest.raises(RuntimeError), sr.ReservedBottomRows(output, reserved_rows=1): raise RuntimeError("boom") - output.flush() assert stream.getvalue().endswith("\x1b[r") + assert not output._buffer def test_usable_rows_excludes_the_reserved_rows(self) -> None: output, _ = make_output(rows=24) From 78a7df427fa11938a1c7ac6993cbb49433cc6f23 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 7 Sep 2026 10:57:20 -0400 Subject: [PATCH 6/8] Preserve the cursor across margin changes and bound the full-screen erase 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. --- cmd2/scroll_region.py | 88 ++++++++++++++++++++++++++++++------- tests/test_scroll_region.py | 64 +++++++++++++++++++++++++-- 2 files changed, 132 insertions(+), 20 deletions(-) diff --git a/cmd2/scroll_region.py b/cmd2/scroll_region.py index e9dcb47bd..9781d2259 100644 --- a/cmd2/scroll_region.py +++ b/cmd2/scroll_region.py @@ -14,6 +14,16 @@ The region must be anchored at row 1. A region starting lower orphans the rows above it: they never scroll and so never reach the terminal's scrollback. +Changing the margins moves the cursor. DECSTBM homes it, and so does the reset -- measured on +tmux 3.7c, where a cursor at row 10 landed at row 1 after each. Every margin change is +therefore wrapped in a save/restore pair (``ESC 7`` / ``ESC 8``), or entering the region below +existing output would send later rendering to the top of the screen and overwrite it. + +The caller is responsible for the cursor not being inside the reserved band when the region is +established: this helper cannot discover the cursor's row without a cursor-position report, +which needs input it does not have. Placing the prompt within the usable area belongs to the +layer that owns the terminal. + A region needs at least two usable rows. DECSTBM requires the bottom margin to be greater than the top, so a degenerate ``ESC [ 1 ; 1 r`` is ignored and the terminal silently keeps its previous margins -- measured on tmux 3.7c, where output then scrolled through the @@ -65,6 +75,35 @@ def reset_scroll_region_sequence() -> str: return "\x1b[r" +def cursor_save_sequence() -> str: + """Build the sequence saving the cursor position. + + :return: the DECSC escape sequence + """ + return "\x1b7" + + +def cursor_restore_sequence() -> str: + """Build the sequence restoring a saved cursor position. + + :return: the DECRC escape sequence + """ + return "\x1b8" + + +def bounded_erase_screen_sequence(rows: int) -> str: + """Build a margin-bounded replacement for ``ED2``. + + ``ED2`` erases the whole display and homes the cursor. Within a region the equivalent is + to home inside the region and delete every usable line, which leaves the reserved rows + untouched. + + :param rows: number of usable rows to clear + :return: the escape sequence clearing the usable region and homing within it + """ + return f"\x1b[1;1H{bounded_erase_down_sequence(rows)}" + + def bounded_erase_down_sequence(rows: int = _MAX_ROWS) -> str: """Build a margin-bounded replacement for ``ED``. @@ -92,7 +131,7 @@ def __init__(self, output: "Output", reserved_rows: int = 1) -> None: self._total_rows = output.get_size().rows # Validate eagerly so a bad reservation fails at construction, not on entry. self._region_sequence = scroll_region_sequence(self._total_rows, reserved_rows) - self._previous_erase_down: Callable[[], None] | None = None + self._previous: dict[str, Callable[[], None] | None] = {} @property def usable_rows(self) -> int: @@ -103,19 +142,34 @@ def _bounded_erase_down(self) -> None: """Erase from the cursor to the bottom margin, leaving the reserved rows intact.""" self._output.write_raw(bounded_erase_down_sequence(self.usable_rows)) + def _bounded_erase_screen(self) -> None: + """Erase the usable region and home within it, leaving the reserved rows intact.""" + self._output.write_raw(bounded_erase_screen_sequence(self.usable_rows)) + + def _write_preserving_cursor(self, sequence: str) -> None: + """Emit a margin change without moving the cursor. + + :param sequence: the margin sequence to emit + """ + self._output.write_raw(f"{cursor_save_sequence()}{sequence}{cursor_restore_sequence()}") + # write_raw only appends to the output's own buffer, so this has to reach the + # terminal here rather than waiting for whatever flushes next. + self._output.flush() + def __enter__(self) -> Self: """Set the scroll region and install the bounded erase.""" - self._output.write_raw(self._region_sequence) - # write_raw only appends to the output's own buffer, so without this the - # reservation is merely queued and anything written before the next flush - # still scrolls through the reserved rows. - self._output.flush() - # Shadow the bound method with an instance attribute. setattr keeps this legible to + self._write_preserving_cursor(self._region_sequence) + # Shadow the bound methods with instance attributes. setattr keeps this legible to # type checkers, which otherwise reject assigning over a method. Capture any # override already installed by a caller so exit can put it back rather than - # leaving ours in place. - self._previous_erase_down = vars(self._output).get("erase_down") - setattr(self._output, "erase_down", self._bounded_erase_down) # noqa: B010 + # leaving ours in place. Both destructive paths need bounding: the renderer's + # erase() reaches erase_down, and its clear() -- Ctrl-L -- reaches erase_screen. + for name, bounded in ( + ("erase_down", self._bounded_erase_down), + ("erase_screen", self._bounded_erase_screen), + ): + self._previous[name] = vars(self._output).get(name) + setattr(self._output, name, bounded) return self def __exit__( @@ -125,13 +179,13 @@ def __exit__( traceback: TracebackType | None, ) -> None: """Restore the original erase and full-screen scroll margins.""" - if self._previous_erase_down is None: - delattr(self._output, "erase_down") # fall back to the class implementation - else: - setattr(self._output, "erase_down", self._previous_erase_down) # noqa: B010 - self._previous_erase_down = None - self._output.write_raw(reset_scroll_region_sequence()) + for name, previous in self._previous.items(): + if previous is None: + delattr(self._output, name) # fall back to the class implementation + else: + setattr(self._output, name, previous) + self._previous.clear() # Restoration has to reach the terminal here. A body that exits without another # renderer operation -- an exception, or application shutdown -- would otherwise # leave the margins restricted and later shell output scrolling inside them. - self._output.flush() + self._write_preserving_cursor(reset_scroll_region_sequence()) diff --git a/tests/test_scroll_region.py b/tests/test_scroll_region.py index 6f6e5434c..c76c3ca4e 100644 --- a/tests/test_scroll_region.py +++ b/tests/test_scroll_region.py @@ -30,6 +30,12 @@ def test_accepts_the_two_usable_row_floor(self) -> None: def test_reset_sequence_restores_full_screen_margins(self) -> None: assert sr.reset_scroll_region_sequence() == "\x1b[r" + def test_bounded_erase_screen_clears_the_region_and_homes_within_it(self) -> None: + """ED2 erases the whole display; the bounded form must stop at the margin.""" + seq = sr.bounded_erase_screen_sequence(23) + assert seq == "\x1b[1;1H\x1b[23M" + assert "\x1b[2J" not in seq, "ED2 would erase the reserved rows" + def test_bounded_erase_uses_delete_line_not_erase_display(self) -> None: """ED ignores the margins; DL is bounded by them, so the pinned row survives.""" seq = sr.bounded_erase_down_sequence() @@ -45,12 +51,28 @@ def test_rejects_regions_that_would_leave_too_few_usable_rows(self, total: int, sr.scroll_region_sequence(total, reserved) +class TestCursorPreservation: + """DECSTBM homes the cursor, so every margin change must save and restore it.""" + + def test_entry_wraps_the_margin_change_in_save_and_restore(self) -> None: + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=1): + assert stream.getvalue() == "\x1b7\x1b[1;23r\x1b8" + + def test_exit_wraps_the_margin_reset_in_save_and_restore(self) -> None: + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=1): + stream.truncate(0), stream.seek(0) + assert stream.getvalue() == "\x1b7\x1b[r\x1b8" + + class TestReservedBottomRows: def test_sets_the_region_on_enter_and_resets_it_on_exit(self) -> None: output, stream = make_output(rows=24) with sr.ReservedBottomRows(output, reserved_rows=1): assert "\x1b[1;23r" in stream.getvalue() - assert stream.getvalue().endswith("\x1b[r") + # the reset is wrapped in save/restore, so it is not the final bytes + assert stream.getvalue().endswith("\x1b7\x1b[r\x1b8") def test_the_region_reaches_the_terminal_on_entry(self) -> None: """Callers may rely on the reservation being in force once __enter__ returns.""" @@ -66,7 +88,7 @@ def test_the_reset_reaches_the_terminal_without_a_further_flush(self) -> None: output, stream = make_output(rows=24) with sr.ReservedBottomRows(output, reserved_rows=1): output.flush() - assert stream.getvalue().endswith("\x1b[r"), "margins were left restricted" + assert stream.getvalue().endswith("\x1b7\x1b[r\x1b8"), "margins were left restricted" assert not output._buffer, "the reset is still sitting in prompt_toolkit's buffer" def test_erase_down_is_bounded_while_reserved(self) -> None: @@ -90,6 +112,42 @@ def test_bounded_erase_clears_the_whole_usable_region(self) -> None: # Assert inside the region: on exit the reset is written and flushed too. assert stream.getvalue() == f"\x1b[{region.usable_rows}M" + def test_erase_screen_is_bounded_while_reserved(self) -> None: + """Ctrl-L reaches erase_screen; an unbounded ED2 would wipe the reserved row.""" + output, stream = make_output(rows=24) + with sr.ReservedBottomRows(output, reserved_rows=1) as region: + stream.truncate(0), stream.seek(0) + output.erase_screen() + output.flush() + assert "\x1b[2J" not in stream.getvalue() + assert stream.getvalue() == f"\x1b[1;1H\x1b[{region.usable_rows}M" + + def test_original_erase_screen_is_restored_on_exit(self) -> None: + output, stream = make_output(rows=24) + original = output.erase_screen + with sr.ReservedBottomRows(output, reserved_rows=1): + assert output.erase_screen != original + assert output.erase_screen == original + stream.truncate(0), stream.seek(0) + output.erase_screen() + output.flush() + assert "\x1b[2J" in stream.getvalue() + + def test_restores_a_pre_existing_instance_level_erase_screen(self) -> None: + """Same contract as erase_down: a caller's override must survive.""" + output, _stream = make_output(rows=24) + calls: list[str] = [] + + def caller_override() -> None: + calls.append("caller") + + output.erase_screen = caller_override # type: ignore[method-assign] + with sr.ReservedBottomRows(output, reserved_rows=1): + pass + assert output.erase_screen is caller_override + output.erase_screen() + assert calls == ["caller"] + def test_original_erase_down_is_restored_on_exit(self) -> None: output, stream = make_output(rows=24) original = output.erase_down @@ -122,7 +180,7 @@ def test_region_is_reset_even_if_the_body_raises(self) -> None: output, stream = make_output(rows=24) with pytest.raises(RuntimeError), sr.ReservedBottomRows(output, reserved_rows=1): raise RuntimeError("boom") - assert stream.getvalue().endswith("\x1b[r") + assert stream.getvalue().endswith("\x1b7\x1b[r\x1b8") assert not output._buffer def test_usable_rows_excludes_the_reserved_rows(self) -> None: From 68bc2a814098a5cedf7cce116b7e776a4a14472a Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 7 Sep 2026 11:09:24 -0400 Subject: [PATCH 7/8] Correct the erase documentation to match measured behavior 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. --- cmd2/scroll_region.py | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/cmd2/scroll_region.py b/cmd2/scroll_region.py index 9781d2259..06c52170d 100644 --- a/cmd2/scroll_region.py +++ b/cmd2/scroll_region.py @@ -4,12 +4,18 @@ bottom toolbar painted there is never consumed by the scroll. On its own that is not enough: ``ED`` (``ESC [ J``), which prompt_toolkit's renderer uses to erase, ignores the scroll margins and erases 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 leaves the same -all-blank result, so it is a drop-in replacement that respects the reserved rows. +by the margins, so deleting every line from the cursor to the bottom margin clears the usable +area while leaving the reserved rows intact. + +**The substitution is only valid from column zero.** ``ED`` preserves the part of the cursor's +line before the cursor; ``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 three renderer +paths that reach ``erase_down`` all move to column zero first, which is what makes the +replacement sound for them -- it is a precondition to enforce, not a property to assume, and +the replacement must not be installed unconditionally outside an active reservation. ``DL`` needs no knowledge of the cursor's row, which matters because ``Output`` does not track -one. When no scroll region is set the margins cover the whole screen and the replacement -behaves exactly like ``ED``, so it is safe to leave installed. +one. The region must be anchored at row 1. A region starting lower orphans the rows above it: they never scroll and so never reach the terminal's scrollback. @@ -94,9 +100,11 @@ def cursor_restore_sequence() -> str: def bounded_erase_screen_sequence(rows: int) -> str: """Build a margin-bounded replacement for ``ED2``. - ``ED2`` erases the whole display and homes the cursor. Within a region the equivalent is - to home inside the region and delete every usable line, which leaves the reserved rows - untouched. + This is a deliberate contract rather than a reproduction of ``ED2``. ``ED2`` erases the + display without moving the cursor -- ``Renderer.clear()`` homes it separately afterwards. + The bounded form homes inside the region first because ``DL`` clears downward from the + cursor, so reaching the whole usable area requires starting at its top. Callers therefore + get an erase-and-home, which is what ``Renderer.clear()`` produces anyway. :param rows: number of usable rows to clear :return: the escape sequence clearing the usable region and homing within it @@ -143,7 +151,10 @@ def _bounded_erase_down(self) -> None: self._output.write_raw(bounded_erase_down_sequence(self.usable_rows)) def _bounded_erase_screen(self) -> None: - """Erase the usable region and home within it, leaving the reserved rows intact.""" + """Erase the usable region and home within it, leaving the reserved rows intact. + + Homing is part of this contract; see :func:`bounded_erase_screen_sequence`. + """ self._output.write_raw(bounded_erase_screen_sequence(self.usable_rows)) def _write_preserving_cursor(self, sequence: str) -> None: From 84bc19e8e66fc668cc91f612caa2afdde40122a5 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 7 Sep 2026 11:49:40 -0400 Subject: [PATCH 8/8] Fix a race that reported a timeout for a successful UI call _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. --- cmd2/command_toolbar.py | 7 ++++++- tests/test_command_toolbar.py | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/cmd2/command_toolbar.py b/cmd2/command_toolbar.py index da8c1c881..96faacb31 100644 --- a/cmd2/command_toolbar.py +++ b/cmd2/command_toolbar.py @@ -376,7 +376,12 @@ def call() -> None: value = result.result(timeout=0.1) except FutureTimeoutError: if result.done(): - raise + # The callback finished while this poll was expiring, or raised a + # TimeoutError of its own -- indistinguishable here, because + # concurrent.futures.TimeoutError is TimeoutError on Python 3.11+. + # Ask the future for its outcome rather than re-raising this poll's + # timeout, which would report a failure for a call that succeeded. + return result.result() self._check_running() else: return value diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index 963e27695..1134eff4c 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -3,7 +3,8 @@ import sys import threading import time -from concurrent.futures import ThreadPoolExecutor +from concurrent.futures import Future, ThreadPoolExecutor +from concurrent.futures import TimeoutError as FutureTimeoutError from types import SimpleNamespace from unittest import mock @@ -431,6 +432,36 @@ def fail(exception: BaseException) -> None: assert toolbar._call_in_ui(lambda: time.sleep(0.2) or "finished") == "finished" +def test_command_toolbar_ui_call_returns_a_result_that_lands_during_the_poll(toolbar_app, monkeypatch) -> None: + """A callback finishing while the poll expires must return its value, not a timeout. + + `concurrent.futures.TimeoutError` is `TimeoutError` on Python 3.11+, so the poll + expiring and the callback raising a timeout of its own are indistinguishable by type. + Re-raising the caught exception once the future is done therefore reports a timeout for + a call that actually succeeded. + """ + app, _, _ = toolbar_app + + class RacyFuture(Future): + """Completes, and only then reports the poll as having expired.""" + + def __init__(self) -> None: + super().__init__() + self._polled = False + + def result(self, timeout=None): # type: ignore[no-untyped-def] + if timeout is not None and not self._polled: + self._polled = True + super().result(timeout=5) # let the callback finish first + raise FutureTimeoutError # then act as though the poll had expired + return super().result(timeout) + + monkeypatch.setattr(command_toolbar, "Future", RacyFuture) + with app._command_toolbar_context(): + toolbar = app._command_toolbar + assert toolbar._call_in_ui(lambda: "finished") == "finished" + + def test_command_toolbar_ui_call_after_display_stopped(toolbar_app) -> None: app, pipe, _ = toolbar_app with app._command_toolbar_context():