Harden the pager's exit and the fallback under an open pager

Seven review findings on the pager-layout fix, in one change.

The pager's teardown could skip its restore: if the display died between
the liveness check and the scheduled exit callback, or the exit raised
before restoring, page() left the full-screen flag and editing mode as the
pager's, and the next main prompt rendered in the alternate screen. The
restore now runs through one nested restore(), on the display's loop when
the exit runs and on the command thread when it does not, so it always
runs exactly once.

Falling back while the pager was open reset the renderer, which quits the
alternate screen and flashes the command output through, and swapped the
layout under the pager. While the pager is on screen the fallback now only
switches routing and redraws, so the native toolbar appears on the
pager's bottom row; the layout and renderer reset wait for the pager's
exit, which applies the display's current layout.

One accessor now applies the display's layout to the application, so the
resume, the fallback and the pager's exit all pick up whichever layout the
display has, and the pager no longer saves a copy to restore. The pager's
teardown and two other sites use the class's own thread_is_alive.

Tests: the mid-pager fallback test now asserts the toolbar is visible and
the pager still up while it is open, and that the alternate screen is
never quit on the wire; a new test fails the pager's exit and checks the
display is restored; pager tests share one driving helper, which closes a
pager that ignores its quit key so a regression fails instead of hanging.

Validation: 2626 passed, 6 skipped with coverage, twice; the mutations
that drop the unconditional restore and that reset under the pager each
fail their test; harness acceptance and dynamic gates PASS at 12, 24 and
40 rows, 23/23 observer controls. make check, make test and make docs-test
passed.

Author: tleonhardt

CHANGELOG.md

@@ -10,7 +10,8 @@
     - Falling back from reserved rendering during a command restores the native toolbar layout and
       stdout proxy, so a recovered toolbar remains visible during the command, including when the
       fallback happens while the command has handed the terminal to another program or while the
-      built-in pager is open.
+      built-in pager is open. A fallback during paging no longer flashes the command output through
+      the pager, and the pager's exit restores the display even if that exit fails.
     - Resuming the reserved command display after a terminal handoff preserves the cursor column of
       unfinished guest output, so later command output continues the same line.
     - A reserved toolbar started in a terminal below the minimum height now activates when the

cmd2/command_toolbar.py

@@ -341,7 +341,9 @@ def _resume(self) -> None:
         # a line the last command left in progress. Its final frame is a suppressed no-op
         # instead, which leaves that output alone.
         erase_when_done = self._reserved_bridge() is None
-        for name, value in (("layout", self._layout), ("key_bindings", self._bindings), ("erase_when_done", erase_when_done)):
+        stack.callback(setattr, self.app, "layout", self.app.layout)
+        self._apply_display_layout()
+        for name, value in (("key_bindings", self._bindings), ("erase_when_done", erase_when_done)):
             stack.callback(setattr, self.app, name, getattr(self.app, name))
             setattr(self.app, name, value)
         self.app.after_render += self._display_started
@@ -417,23 +419,34 @@ def _reservation_stopped(self) -> None:
         bridge behind it, and the native toolbar would never appear for the rest of the
         command. Routing and the redraw of a display that *is* running need its loop.
         """
-        previous_layout = self._layout
         self._layout = self._legacy_layout()
         if self.app.loop is not None and self.app.is_running:
-            self.app.loop.call_soon_threadsafe(self._restore_legacy_display, previous_layout)
+            self.app.loop.call_soon_threadsafe(self._restore_legacy_display)
 
-    def _restore_legacy_display(self, previous_layout: Layout) -> None:
-        """Switch a running display over to legacy routing and layout, on its own loop.
+    def _apply_display_layout(self) -> None:
+        """Put the display's own layout on the application.
 
-        :param previous_layout: the reserved layout the display was started with, replaced on
-            the application only if it is still the one in use
+        The one place that assignment is made from ``_layout``. The display's layout can
+        change while a command runs -- the reservation being abandoned switches it to the
+        legacy one -- and everything that hands the application back to the display, whether
+        a resume or the pager's exit, comes through here and so picks up whichever it is now.
         """
+        self.app.layout = self._layout
+
+    def _restore_legacy_display(self) -> None:
+        """Switch a running display over to legacy routing and layout, on its own loop."""
         if self._pausing or not self.app.is_running or self.app.is_done:
             return
         self._install_legacy_proxy()
-        if self.app.layout is previous_layout:
-            self.app.layout = self._layout
         self.app.erase_when_done = True
+        if self.app.full_screen:
+            # The pager is on screen. Its exit applies the display's layout and resets the
+            # renderer; doing either here would quit the alternate screen under it and flash
+            # the command output through. Routing is switched now, and a redraw shows the
+            # native toolbar on the pager's bottom row; the rest waits for the pager's exit.
+            self.app.invalidate()
+            return
+        self._apply_display_layout()
         self.app.renderer.reset()
         self.app.renderer.request_absolute_cursor_position()
         self.app.invalidate()
@@ -550,7 +563,7 @@ def _pause(self) -> None:
                     # Bounded, so a render callback blocked inside the display cannot hold the
                     # thread that is tearing it down.
                     self._thread.join(timeout=_SHUTDOWN_TIMEOUT)
-                    if self._thread.is_alive():
+                    if self.thread_is_alive:
                         self._abandon_stuck_display()
                 self._finish_pause()
         finally:
@@ -669,7 +682,7 @@ def call() -> None:
                 return value
 
     def _check_running(self) -> None:
-        if self._thread is None or not self._thread.is_alive():
+        if not self.thread_is_alive:
             if self._error is not None:
                 raise self._error
             raise EOFError
@@ -691,12 +704,22 @@ def page(self, text: str, *, chop: bool) -> None:
             filter=Condition(lambda: suspend_to_background_supported() and to_filter(self.cmd.main_session.enable_suspend)()),
         )(self._suspend_binding)
         layout = Layout(HSplit([pager.container, self.toolbar]), focused_element=pager.text)
-        # The layout is deliberately not saved here. The display's layout can change while the
-        # pager is open -- a reservation abandoned mid-page switches it to the legacy one -- and
-        # restoring the layout saved on entry would put the obsolete reserved layout back,
-        # leaving no toolbar. Pager exit reads the display's current layout instead.
         previous = (self.app.key_bindings, self.app.editing_mode, self.app.full_screen)
         entered = False
+        restored = False
+
+        def restore() -> None:
+            """Give the application back to the display, whichever thread is doing it.
+
+            Plain attribute assignments, so this is safe from the display's loop and from the
+            command thread alike. The layout is not restored from a saved copy: the display's
+            layout can change while the pager is open, and the accessor applies the current one.
+            """
+            nonlocal restored
+            restored = True
+            self._apply_display_layout()
+            self.app.key_bindings, self.app.editing_mode, self.app.full_screen = previous
+            self.app.renderer.full_screen = self.app.full_screen
 
         def enter() -> None:
             nonlocal entered
@@ -718,9 +741,7 @@ def leave() -> None:
                 return
             entered = False
             self.app.renderer.erase()
-            self.app.layout = self._layout
-            self.app.key_bindings, self.app.editing_mode, self.app.full_screen = previous
-            self.app.renderer.full_screen = self.app.full_screen
+            restore()
             self.app.renderer.request_absolute_cursor_position()
             # Back to ordinary command output, whose frames are suppressed again so the toolbar
             # stays put. Command finalization and the next prompt lift this in turn.
@@ -740,13 +761,16 @@ def close() -> None:
             while not pager.closed.wait(0.1):
                 self._check_running()
         finally:
-            if self._thread is not None and self._thread.is_alive():
-                self._call_in_ui(leave)
-            else:
-                # The application's shutdown already reset the renderer.
-                self.app.layout = self._layout
-                self.app.key_bindings, self.app.editing_mode, self.app.full_screen = previous
-                self.app.renderer.full_screen = self.app.full_screen
+            try:
+                if self.thread_is_alive:
+                    self._call_in_ui(leave)
+            finally:
+                # Restored here if the exit never ran: the display had already shut down (its
+                # shutdown reset the renderer itself), or the display died between the check
+                # and the callback, or the exit raised before restoring. Left as the pager's,
+                # the full-screen flag and editing mode would carry into the next prompt.
+                if not restored:
+                    restore()
 
     @contextlib.contextmanager
     def suspend(self) -> Iterator[None]:

tests/test_command_toolbar.py

@@ -1173,6 +1173,6 @@ def test_restoring_the_legacy_display_on_a_stopped_display_changes_nothing(toolb
         display = app._command_toolbar
         assert display is not None
     layout = app.main_session.app.layout
-    display._restore_legacy_display(display._layout)
+    display._restore_legacy_display()
     assert display._proxy is None
     assert app.main_session.app.layout is layout

tests/test_reserved_terminal.py

@@ -7,12 +7,15 @@
 import time
 from concurrent.futures import ThreadPoolExecutor
 from types import SimpleNamespace
+from typing import Any
+from unittest import mock
 
 import pyte
 import pytest
 from prompt_toolkit.application import run_in_terminal
 from prompt_toolkit.data_structures import Size
 
+from cmd2 import command_toolbar
 from cmd2.reserved_toolbar import ReservedToolbar
 from cmd2.utils import StdSim
 
@@ -560,6 +563,50 @@ def test_a_carriage_return_progress_line_ends_on_its_final_value(self, terminal_
             assert terminal.screen.display[-1].startswith("STATUS")
 
 
+PAGER_BODY = "\n".join(f"row {index:03d}" for index in range(200))
+
+
+def run_pager(harness, terminal, while_open=None) -> bool:
+    """Page a body taller than the screen on the command display, then quit the pager.
+
+    ``while_open`` runs on the driving thread once the pager has painted its first screen.
+    The quit key is sent either way, so a pager that never draws fails the caller's
+    assertion instead of hanging the blocking ``page()`` call forever.
+
+    :return: whether the pager drew its first screen
+    """
+    display = harness.app._command_toolbar
+    shown = threading.Event()
+    created: list[Any] = []
+    real_pager = command_toolbar.Pager
+
+    def make_pager(*args: Any, **kwargs: Any) -> Any:
+        pager = real_pager(*args, **kwargs)
+        created.append(pager)
+        return pager
+
+    def drive() -> None:
+        try:
+            if wait_for(lambda: terminal.screen.display[0].startswith("row 000")):
+                shown.set()
+                if while_open is not None:
+                    while_open()
+        finally:
+            harness.pipe.send_text("q")
+            # A pager that no longer answers its quit key -- its layout swapped out from
+            # under it, say -- would leave page() blocked forever. Close it by hand so the
+            # test fails on the assertion instead of hanging.
+            if created and not wait_for(created[0].closed.is_set, timeout=3):
+                created[0].closed.set()
+                raise AssertionError("the pager did not close on its quit key")
+
+    with mock.patch.object(command_toolbar, "Pager", make_pager), ThreadPoolExecutor() as executor:
+        future = executor.submit(drive)
+        display.page(PAGER_BODY, chop=False)
+        future.result(timeout=5)
+    return shown.is_set()
+
+
 class TestPager:
     """The built-in pager renders a full screen of its own, so its frames must not be
     suppressed the way an ordinary command's empty frames are."""
@@ -621,21 +668,22 @@ def test_abandoning_reservation_while_the_pager_is_open_keeps_the_fallback_layou
         with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context():
             reserved = harness.app.reserved_toolbar
             display = harness.app._command_toolbar
-            body = "\n".join(f"row {index:03d}" for index in range(200))
-
-            def drive() -> None:
-                try:
-                    if wait_for(lambda: terminal.screen.display[0].startswith("row 000")):
-                        display.app.loop.call_soon_threadsafe(reserved.stop)
-                        wait_for(lambda: reserved.bridge is None)
-                finally:
-                    harness.pipe.send_text("q")
-
-            with ThreadPoolExecutor() as executor:
-                future = executor.submit(drive)
-                display.page(body, chop=False)
-                future.result(timeout=5)
-
+            seen: dict[str, bool] = {}
+
+            def stop_mid_page() -> None:
+                emitted_before = len(terminal.getvalue())
+                display.app.loop.call_soon_threadsafe(reserved.stop)
+                assert wait_for(lambda: reserved.bridge is None)
+                # The fallback must not drop out of the pager: its screen stays up, and the
+                # native toolbar is visible on its bottom row while it is open. The emulator
+                # does not model the alternate screen, so the flash a renderer reset would
+                # cause is checked on the wire: the sequence that quits it is never sent.
+                seen["toolbar"] = wait_for(lambda: terminal.screen.display[-1].startswith("STATUS"))
+                seen["pager"] = terminal.screen.display[0].startswith("row 000")
+                seen["stayed_in_pager"] = "\x1b[?1049l" not in terminal.getvalue()[emitted_before:]
+
+            assert run_pager(harness, terminal, while_open=stop_mid_page)
+            assert seen == {"toolbar": True, "pager": True, "stayed_in_pager": True}
             assert reserved.bridge is None
             assert len(display.app.layout.container.children) == 3
             harness.app.main_session.bottom_toolbar = "RECOVERED"
@@ -645,30 +693,52 @@ def drive() -> None:
 
     def test_the_pager_draws_its_content_over_the_reserved_toolbar(self, terminal_harness) -> None:
         harness, terminal = terminal_harness
+        with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context():
+            assert run_pager(harness, terminal), "the pager never drew its content"
+            # The toolbar is suppressed again for ordinary output once the pager has closed.
+            assert harness.app.reserved_toolbar.bridge._render_suppressed is True
+        assert terminal.screen.margins is None
+
+    def test_pager_teardown_restores_the_display_even_if_leaving_raises(self, terminal_harness, monkeypatch) -> None:
+        """If the display cannot run the pager's exit on its own loop -- here the exit's erase
+        raises -- page() must still put the display back itself. Left as the pager's, the
+        full-screen flag and editing mode would carry into the next main prompt."""
+        harness, terminal = terminal_harness
+        created: list[Any] = []
+        real_pager = command_toolbar.Pager
+
+        def make_pager(*args: Any, **kwargs: Any) -> Any:
+            pager = real_pager(*args, **kwargs)
+            created.append(pager)
+            return pager
+
+        monkeypatch.setattr(command_toolbar, "Pager", make_pager)
         with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context():
             display = harness.app._command_toolbar
-            body = "\n".join(f"row {index:03d}" for index in range(200))
-            shown = threading.Event()
+            bindings = display.app.key_bindings
+            editing_mode = display.app.editing_mode
+
+            def erase_fails() -> None:
+                raise ValueError("erase failed")
 
             def drive() -> None:
-                # Wait until the pager has painted its first screen, then quit it. Quit either
-                # way, so a pager that never draws fails the assertion instead of hanging the
-                # blocking page() call forever.
-                try:
-                    if wait_for(lambda: terminal.screen.display[0].startswith("row 000")):
-                        shown.set()
-                finally:
-                    harness.pipe.send_text("q")
+                assert wait_for(lambda: terminal.screen.display[0].startswith("row 000"))
+                # The exit's first act is an erase; make it raise, then end the pager without
+                # its quit key so the exit runs from page()'s own teardown.
+                monkeypatch.setattr(display.app.renderer, "erase", erase_fails)
+                created[0].closed.set()
 
             with ThreadPoolExecutor() as executor:
                 future = executor.submit(drive)
-                display.page(body, chop=False)
+                with pytest.raises(ValueError, match="erase failed"):
+                    display.page(PAGER_BODY, chop=False)
                 future.result(timeout=5)
 
-            assert shown.is_set(), "the pager never drew its content"
-            # The toolbar is suppressed again for ordinary output once the pager has closed.
-            assert harness.app.reserved_toolbar.bridge._render_suppressed is True
-        assert terminal.screen.margins is None
+            assert display.app.full_screen is False
+            assert display.app.renderer.full_screen is False
+            assert display.app.layout is display._layout
+            assert display.app.key_bindings is display._bindings or display.app.key_bindings is bindings
+            assert display.app.editing_mode is editing_mode
 
     def test_output_that_fits_is_printed_without_a_pager(self, terminal_harness) -> None:
         harness, terminal = terminal_harness