Skip to content

Commit a734941

Browse files
committed
Fix reserved toolbar corruption at the terminal bottom
Make room for the reserved band before installing scroll margins at startup and after a terminal handoff. Index down while full-screen margins are still installed, then move back up before saving the cursor. This preserves output and keeps the prompt cursor out of the toolbar without requiring CPR before the input reader starts. Erase owned toolbar cells before releasing the margins so short shell output cannot leave toolbar fragments in the screen or scrollback. Skip the erase if the geometry changed or cannot be measured, while still resetting the margins. Add pyte as a test-only dependency and exercise actual cursor reports, scrolling, output preservation, and subsequent prompts. Account for pyte's DECRC clamping difference from the reproduced terminal behavior. Retarget partial-paint fault injection so acquisition flush changes do not make it fail the wrong operation. Validation: - 2547 passed, 6 skipped in 3.56s with coverage - 320 repeated parallel regression tests passed - Three mutations killed: startup adjustment, handoff adjustment, erase - make check and make docs-test passed - Isolated tmux checks passed at 12, 24, and 40 rows, including bottom-row startup, repeated overflowing shell commands, 160/160 exact output lines, working subsequent prompts, and full margins restored on quit - User's manual macOS testing confirmed both reported bugs are fixed
1 parent 2621908 commit a734941

8 files changed

Lines changed: 253 additions & 38 deletions

‎cmd2/scroll_region.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@
3333
3434
The caller is responsible for the cursor not being inside the reserved band when the region is
3535
established: this helper cannot discover the cursor's row without a cursor-position report,
36-
which needs input it does not have. Placing the prompt within the usable area belongs to the
37-
layer that owns the terminal.
36+
which needs input it does not have. The physical-terminal owner makes room under the cursor
37+
while full-screen margins are still installed, before saving its position and narrowing the
38+
region.
3839
3940
A region needs at least two usable rows. DECSTBM requires the bottom margin to be greater
4041
than the top, so a degenerate ``ESC [ 1 ; 1 r`` is ignored and the terminal silently keeps

‎cmd2/terminal_display.py‎

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,25 @@ def write_margin_change(self, sequence: str) -> None:
226226
# now rather than whenever something else happens to flush.
227227
self._output.flush()
228228

229+
def make_room_for_region(self, geometry: Geometry) -> None:
230+
"""Keep the cursor and preceding output above the band before narrowing margins.
231+
232+
Full-screen margins must still be installed. Indexing down by the band's height
233+
scrolls only when the cursor is near the bottom. Moving back up by the same amount
234+
then leaves it at its original row, or at the last usable row if scrolling occurred.
235+
IND preserves the column and is independent of the tty's newline translation.
236+
237+
This needs no CPR, so it also works before the application's input reader starts.
238+
The cursor saved by the margin change must be this adjusted position, not the old
239+
physical row which may now belong to the band. Flush the adjustment before the
240+
margin operation, so a failure there cannot strand it in the backend's buffer.
241+
242+
:param geometry: the eligible geometry whose band needs room
243+
"""
244+
rows = geometry.reserved_rows
245+
self._output.write_raw("\x1bD" * rows + f"\x1b[{rows}A")
246+
self._output.flush()
247+
229248
def install_region(self, geometry: Geometry) -> None:
230249
"""Install the scroll region described by ``geometry``.
231250
@@ -234,9 +253,22 @@ def install_region(self, geometry: Geometry) -> None:
234253
"""
235254
self.write_margin_change(scroll_region_sequence(geometry.physical_rows, geometry.reserved_rows))
236255

237-
def release_region(self) -> None:
238-
"""Restore full-screen scroll margins."""
239-
self.write_margin_change(reset_scroll_region_sequence())
256+
def release_region(self, geometry: Geometry | None = None) -> None:
257+
"""Restore full-screen margins, removing owned toolbar cells before they can scroll.
258+
259+
:param geometry: the region being released, if one was successfully installed
260+
"""
261+
sequence = reset_scroll_region_sequence()
262+
if geometry is not None:
263+
# A resize or viewport move can invalidate the owned band before teardown.
264+
# If measurement fails, still reset the margins; do not guess where to erase.
265+
with suppress(Exception):
266+
if self.measure(geometry.generation, geometry.reserved_rows) == geometry:
267+
# ED deliberately reaches the physical bottom here: these are the rows
268+
# being returned, not application output. The save/restore below also
269+
# preserves the prompt's cursor and attributes around the erase.
270+
sequence = f"\x1b[{geometry.usable_rows + 1};1H\x1b[0m\x1b[J" + sequence
271+
self.write_margin_change(sequence)
240272

241273

242274
class TerminalDisplay:
@@ -319,6 +351,7 @@ def acquire(self) -> bool:
319351
geometry = self._measure()
320352
if not geometry.is_eligible:
321353
return False
354+
self._terminal.make_room_for_region(geometry)
322355
self._terminal.install_region(geometry)
323356
except BaseException:
324357
# Give the lease back *first*. Cleanup can fail too -- a terminal that could not
@@ -365,8 +398,8 @@ def _teardown(self) -> None:
365398
self._handoff_active = False
366399
if self._geometry is None:
367400
return
368-
self._geometry = None
369-
self._terminal.release_region()
401+
geometry, self._geometry = self._geometry, None
402+
self._terminal.release_region(geometry)
370403

371404
def reconfigure(self) -> bool:
372405
"""Resample the terminal and re-establish the reservation for the new geometry.
@@ -393,6 +426,10 @@ def reconfigure(self) -> bool:
393426
self._adapter = None
394427
self._terminal.release_region()
395428
return False
429+
if self._geometry is None:
430+
# The guest may have scrolled to the physical bottom, just as the shell
431+
# that launched us may have done before the initial acquisition.
432+
self._terminal.make_room_for_region(geometry)
396433
self._terminal.install_region(geometry)
397434
self._geometry = geometry
398435
if self._adapter is None:
@@ -417,8 +454,8 @@ def release_region_for_handoff(self) -> None:
417454
self._handoff_active = True
418455
if self._geometry is None:
419456
return
420-
self._geometry = None
421-
self._terminal.release_region()
457+
geometry, self._geometry = self._geometry, None
458+
self._terminal.release_region(geometry)
422459

423460
def reacquire_region_after_handoff(self) -> None:
424461
"""Re-establish the reservation after a program hands the terminal back.

‎pyproject.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ dev = [
4343
"mkdocstrings[python]>=1",
4444
"mypy>=2.3.1",
4545
"prek>=0.3.5",
46+
"pyte>=0.8.2",
4647
"pytest>=8.1.1",
4748
"pytest-cov>=5",
4849
"pytest-mock>=3.14.1",
@@ -62,6 +63,7 @@ quality = ["prek>=0.3.5"]
6263
test = [
6364
"codecov>=2.1",
6465
"coverage>=7.11.3",
66+
"pyte>=0.8.2",
6567
"pytest>=8.1.1",
6668
"pytest-cov>=5",
6769
"pytest-mock>=3.14.1",

‎tests/test_reserved_output.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ def test_the_region_is_anchored_at_row_one(self) -> None:
248248
"""The arithmetic above is only sound because of this."""
249249
output, stream = make_output(rows=24)
250250
TerminalDisplay(output).acquire()
251-
assert stream.getvalue() == "\x1b7\x1b[1;23r\x1b8"
251+
assert stream.getvalue() == "\x1bD\x1b[1A\x1b7\x1b[1;23r\x1b8"
252252

253253
def test_a_cursor_in_the_reserved_band_gives_a_nonpositive_height(self) -> None:
254254
"""The R5 hazard, stated in geometry terms: rejecting it belongs to the bridge, but
@@ -361,7 +361,7 @@ def test_entering_the_alternate_screen_restores_full_margins_first(self) -> None
361361
adapter, stream, display = make_reserved(rows=24)
362362
adapter.enter_alternate_screen()
363363
adapter.flush()
364-
assert stream.getvalue().startswith("\x1b7\x1b[r\x1b8"), "margins were left installed"
364+
assert stream.getvalue().startswith("\x1b7\x1b[24;1H\x1b[0m\x1b[J\x1b[r\x1b8"), "margins were left installed"
365365
assert not display.is_reserved
366366

367367
def test_leaving_the_alternate_screen_re_establishes_the_region(self) -> None:

‎tests/test_reserved_terminal.py‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
"""Reservation boundaries interpreted by a terminal, rather than a fixed row-one CPR stub."""
2+
3+
import io
4+
import sys
5+
import threading
6+
from types import SimpleNamespace
7+
8+
import pyte
9+
import pytest
10+
11+
from cmd2.reserved_toolbar import ReservedToolbar
12+
from cmd2.utils import StdSim
13+
14+
from .test_reserved_lifecycle import Harness
15+
16+
17+
class TerminalScreen(pyte.HistoryScreen):
18+
"""Keep DECRC's physical position even when it is outside the scroll margins.
19+
20+
pyte 0.8.2 unconditionally clamps a restored cursor to the margins. iTerm2 and
21+
tmux retain the physical row when origin mode is off; that distinction is the
22+
bug these tests exercise. Let pyte restore against the whole screen in that mode.
23+
"""
24+
25+
def restore_cursor(self) -> None:
26+
margins = self.margins
27+
if self.savepoints and not self.savepoints[-1].origin:
28+
self.margins = None
29+
try:
30+
super().restore_cursor()
31+
finally:
32+
self.margins = margins
33+
34+
35+
class EmulatedTerminal(io.StringIO):
36+
"""Parse output and answer CPR where the cursor actually is when the query arrives."""
37+
38+
def __init__(self, rows, reply) -> None:
39+
super().__init__()
40+
self.screen = TerminalScreen(80, rows, history=1000)
41+
self.reports = []
42+
43+
def respond(data):
44+
self.reports.append(self.screen.cursor.y + 1)
45+
reply(data)
46+
47+
self.screen.write_process_input = respond
48+
self.parser = pyte.Stream(self.screen)
49+
self.buffer = SimpleNamespace(write=lambda data: self.write(data.decode("utf-8")))
50+
51+
def write(self, data):
52+
count = super().write(data)
53+
# The tty's output processing supplies carriage returns to subprocess newlines.
54+
self.parser.feed(data.replace("\n", "\r\n"))
55+
return count
56+
57+
def isatty(self) -> bool:
58+
return True
59+
60+
61+
@pytest.fixture
62+
def terminal_harness(request):
63+
rows = getattr(request, "param", 24)
64+
harness = Harness(rows=rows)
65+
stream = EmulatedTerminal(rows, harness.pipe.send_text)
66+
harness.stream = stream
67+
harness.backend.stdout = stream
68+
harness.app.stdout = stream
69+
try:
70+
yield harness, stream
71+
finally:
72+
harness.close()
73+
74+
75+
def read_prompt(harness, terminal) -> None:
76+
"""Use the real input reader and CPR binding, then accept a prompt with known height."""
77+
ui = harness.app.main_session.app
78+
sent = False
79+
80+
def ready(app):
81+
nonlocal sent
82+
if not sent and app.renderer._min_available_height > 0:
83+
sent = True
84+
harness.pipe.send_text("next\n")
85+
86+
ui.after_render += ready
87+
watchdog = threading.Timer(5, harness.pipe.close)
88+
watchdog.daemon = True
89+
watchdog.start()
90+
try:
91+
assert harness.app._read_raw_input("TEST> ", harness.app.main_session) == "next"
92+
assert sent
93+
assert terminal.reports
94+
assert all(row < terminal.screen.lines for row in terminal.reports)
95+
assert terminal.screen.display[-1].startswith("STATUS")
96+
finally:
97+
watchdog.cancel()
98+
watchdog.join()
99+
ui.after_render -= ready
100+
101+
102+
@pytest.mark.parametrize("row", [1, 22, 23, 24])
103+
@pytest.mark.parametrize("reserved", [1, 2])
104+
def test_acquisition_keeps_existing_output_and_cursor_above_the_band(terminal_harness, row, reserved) -> None:
105+
harness, terminal = terminal_harness
106+
terminal.write(f"\x1b[{row};1Hexisting output")
107+
with ReservedToolbar(harness.app.main_session, lambda: "STATUS", reserved_rows=reserved):
108+
expected_row = min(row, 24 - reserved)
109+
assert terminal.screen.cursor.y + 1 == expected_row
110+
assert terminal.screen.cursor.x == len("existing output")
111+
assert terminal.screen.display[expected_row - 1].startswith("existing output")
112+
assert terminal.screen.display[24 - reserved].startswith("STATUS")
113+
assert terminal.screen.margins is None
114+
115+
116+
@pytest.mark.parametrize("terminal_harness", [12, 24, 40], indirect=True)
117+
def test_a_prompt_started_on_the_bottom_row_gets_a_usable_cursor_report(terminal_harness, capsys) -> None:
118+
harness, terminal = terminal_harness
119+
terminal.write(f"\x1b[{terminal.screen.lines};1H")
120+
with harness.app._reserved_toolbar_context():
121+
# Fail before trying input if startup has left the cursor in the band.
122+
assert terminal.screen.cursor.y < terminal.screen.lines - 1
123+
read_prompt(harness, terminal)
124+
assert "doesn't support cursor position requests" not in capsys.readouterr().err
125+
assert terminal.screen.margins is None
126+
127+
128+
@pytest.mark.parametrize("terminal_harness", [12, 24, 40], indirect=True)
129+
def test_shell_output_scrolling_to_the_bottom_returns_a_working_prompt(terminal_harness, capfd) -> None:
130+
harness, terminal = terminal_harness
131+
# Exercise a real shell subprocess and ProcReader, echoing its captured bytes to
132+
# the same terminal as the toolbar. This supplies a real pipe rather than fileno()
133+
# on the in-memory terminal.
134+
harness.app.stdout = StdSim(terminal, echo=True)
135+
with harness.app._reserved_toolbar_context():
136+
command = f"!\"{sys.executable}\" -S -c \"print('out\\n' * 80, end='')\""
137+
harness.app.onecmd_plus_hooks(command)
138+
assert harness.app.last_result == 0
139+
assert terminal.screen.cursor.y == terminal.screen.lines - 2
140+
assert terminal.screen.display[-3].rstrip() == "out"
141+
assert terminal.screen.display[-1].startswith("STATUS")
142+
# Every emitted line must remain in the viewport or scrollback exactly once.
143+
history = ["".join(line[x].data for x in sorted(line)) for line in terminal.screen.history.top]
144+
assert sum(line.rstrip() == "out" for line in history + terminal.screen.display) == 80
145+
read_prompt(harness, terminal)
146+
assert "doesn't support cursor position requests" not in capfd.readouterr().err
147+
assert terminal.screen.margins is None
148+
149+
150+
def test_release_does_not_erase_rows_from_a_changed_viewport(terminal_harness) -> None:
151+
harness, terminal = terminal_harness
152+
with harness.app._reserved_toolbar_context():
153+
# The reserved row was cropped by a resize; the new bottom row is application
154+
# output. CUP to the old row would clamp there and erase somebody else's text.
155+
terminal.screen.resize(lines=12, columns=80)
156+
harness.size = type(harness.size)(rows=12, columns=80)
157+
terminal.write("\x1b[12;1Hkeep this output")
158+
assert terminal.screen.display[-1].startswith("keep this output")
159+
assert terminal.screen.margins is None
160+
161+
162+
def test_release_still_resets_margins_when_geometry_cannot_be_measured(terminal_harness, monkeypatch) -> None:
163+
harness, terminal = terminal_harness
164+
with harness.app._reserved_toolbar_context():
165+
166+
def unavailable():
167+
raise OSError("terminal size unavailable")
168+
169+
monkeypatch.setattr(harness.backend, "get_size", unavailable)
170+
assert terminal.screen.margins is None

0 commit comments

Comments
 (0)