DO NOT MERGE: continuous prompt-toolkit application - does not fix the toolbar flicker (findings recorded) - #1748
Closed
tleonhardt wants to merge 33 commits into
Closed
DO NOT MERGE: continuous prompt-toolkit application - does not fix the toolbar flicker (findings recorded)#1748tleonhardt wants to merge 33 commits into
tleonhardt wants to merge 33 commits into
Conversation
… and refreshing during command execution. Prompts, pagers, and shell commands temporarily suspend it while using the terminal. Try the work command examples/getting_started.py for a demonstration. The feature uses two prompt-toolkit displays at different times: the existing PromptSession while waiting for input, and a dedicated CommandToolbar while commands execute. - Command execution stays on the main thread. The command loop starts the toolbar’s UI in a background thread, using the same content callback, styling, and refresh interval. - Output appears above the toolbar. Stable stream wrappers route Python and piped subprocess output through prompt-toolkit’s output proxy. Their identities remain consistent across cmd2 redirection and toolbar suspension. - Input remains coordinated. The toolbar handles terminal position reports, saves typed-ahead keys for the next prompt, and forwards Ctrl-C to the main thread. - Other terminal interfaces get exclusive access. Nested prompts, pagers, and shell commands temporarily suspend the toolbar. Custom commands can do the same with suspend_bottom_toolbar(). - Cleanup restores normal terminal operation. When execution ends, buffered output is flushed, workers are stopped, and the original streams are restored. Most supporting code lives in cmd2/command_toolbar.py, with lifecycle integration in cmd2/cmd2.py. Because get_bottom_toolbar() runs in the UI thread during commands, shared mutable state should be protected with a lock.
1. Pipes to interactive targets (cmd2.py:3366, command_toolbar.py:43) — the pipe process now inherits the real terminal via the new pipe_target() helper, and the toolbar suspends for the life of the pipe (held in RedirectionSavedState.toolbar_suspension, released in _restore_output). Streams with no file descriptor still fall back to PIPE. Verified under a real pty: alias | python3 -c "...isatty()" printed PIPE_ISATTY True; before the fix that command produced no output at all. 2. Ctrl-C not reaching subprocesses (command_toolbar.py:161) — POSIX now uses os.killpg(os.getpgrp(), SIGINT), matching what the terminal driver does. Verified under a pty with a child that reports its own SIGINT: post-fix CHILD_GOT_SIGINT appears and the child exits immediately; pre-fix it never fired and the child ran to completion. Windows keeps interrupt_main() — broadcasting a console control event there would hit unrelated processes on the same console. Ctrl-\ stays inert, which matches the main prompt's existing behavior; documented rather than changed. 3. Failing stop() disabling the toolbar (cmd2.py:2098) — _command_toolbar = None moved into its own finally. 4. Writes racing _pause() (command_toolbar.py) — an RLock shared by the toolbar and its ToolbarStreams serializes writes against the proxy swap. The lock is deliberately released before _thread.join(), since the toolbar thread can itself be blocked writing through a stream. 5. Post-startup failures swallowed — new _app_exited() detaches the streams when the display stops on its own, so output goes to the terminal instead of a dead proxy, and reports the error. Worth noting: the review's stated trigger doesn't hold — a get_bottom_toolbar() that raises on refresh is caught by asyncio's default handler and the thread survives. The fix targets the case where the thread genuinely dies. 6. Final flush cancelled before writing — does not reproduce. Application.run_async already awaits wait_for_cpr_responses() and any in-flight run_in_terminal futures before its loop closes (application.py:770-779). I built the exact scenario — an output with responds_to_cpr=True and a CPR future confirmed pending at pause time — and the trailing text arrived with and without the proposed fix. I reverted my drain implementation, which was adding a run_in_terminal round-trip and up to a second of CPR waiting at every command boundary for nothing. I kept the test as a regression guard and a two-line guard against Application.exit() raising in a loop callback if the app has already stopped. 7. Docs/example mismatch — do_work now uses Annotated[int, Argument(nargs="?")], so work and work 5 both work. Verified in the real app.
Three tests called _read_raw_input() after the toolbar context had exited, so no prompt-toolkit app session was active. patch_stdout() then asked the default app session for its output, which lazily builds a real terminal Output: harmless Vt100_Output on POSIX, but Win32Output on Windows, which raises NoConsoleScreenBufferError without a console screen buffer of the kind GitHub Actions does not provide. Bind the fixture's app session to the test pipe and recording output so every prompt-toolkit call in these tests resolves to the test terminal rather than the ambient one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C7nkx8kKC2mpaMGfLwXe6J
…d an embedded pager Following prompt-toolkit’s [layout composition model](https://python-prompt-toolkit.readthedocs.io/en/3.0.52/pages/full_screen_apps.html#the-layout). - CommandToolbar (cmd2/command_toolbar.py:101) now reuses main_session.app and its actual toolbar container, eliminating the second application and duplicated display configuration. - The new pager (cmd2/pager.py:37) shares that application. The toolbar continues refreshing during scrolling, searching, and resizing. Colors, wrapped lines, and horizontal scrolling are supported. - Enabling the toolbar selects the embedded pager. Set self.use_builtin_pager = False to retain your external pager. An external pager such as less takes control of the terminal; a layout alone cannot reserve space around it. That path still temporarily suspends the toolbar. This simplifies rendering ownership, although the embedded pager adds code. The command UI thread and output proxy remain necessary for synchronous commands. There’s also one guarded dependency on PromptSession’s internal layout structure, verified against versions 3.0.52 and 3.0.53.
Closing the terminal's input ends the display's Application.run() with EOFError, which _resume() swallows because it is an ordinary shutdown rather than a failure the running command should hear about. Nothing exercised that handler, leaving it as the one uncovered line in the new module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C7nkx8kKC2mpaMGfLwXe6J
page() constructed a full Pager -- a TextArea, a SearchToolbar, an ANSI lexer and its key bindings -- and then threw it away whenever the output fit on screen, which is a common case for ppaged(). Only the line measurements were ever used on that path. Move the size check into a module-level output_fits() that measures the text directly, and build the Pager only once the output has to be scrolled. A shared _fragments() helper does the one ANSI parse for both, so the measurement cannot drift from what the view would render. The fast path's size check drops from ~0.15ms to ~0.02ms per call, so it isn't a massive speedup or anything, but still cleaner.
1. Chop-mode paging no longer loses your column — cmd2/pager.py:_scroll() The destination column now comes from the window's current horizontal_scroll when chop is set, instead of target * window_width (always 0 in chop mode) 2. A toolbar that can't start no longer takes the session down - cmd2/cmd2.py _command_toolbar_context() catches startup failure, reports once via perror, sets a new _command_toolbar_disabled flag so later commands don't retry and re-spam and runs the command without a toolbar 3. ppaged() no longer starts a toolbar outside the command loop — it now pages inside a toolbar only when one is already running, and otherwise falls through to the external pager. The documentation has been updated to contain truthful statements in this regard. 4. use_builtin_pager documented as opt-out only — initialization.md, os.md, prompt.md and the ppaged() docstring now state that it cannot turn the embedded pager on where no toolbar is running, since the two share one display.
…g visible hyperlink text, colors, and width calculations When ppaged() receives Rich output containing hyperlinks, Rich emits OSC 8 sequences that prompt-toolkit's ANSI parser does not understand. Their contents become visible pager text: a linked click here renders as 8;id=...;https://example.comclick here8;;. This also corrupts width calculations and searchable text. This change strips unsupported OSC sequences before passing captured output to cmd2's new internal pager.
…toolbar.py Also: - Moved common code used by test_command_toolbar.py and test_pager.py to conftest.py Since the pager is effectively its own component, it feels cleaner to have the tests in their own file.
Cause: rich's Style.render() skips the OSC 8 hyperlink wrapper entirely when legacy_windows=True. Console(force_terminal=True) on Windows auto-detects a legacy console, so the fixture string never contained \x1b]8; — the failure was in the test's setup, not in the pager. Fix (tests/test_pager.py:31): pass legacy_windows=False to the Console, so the fixture emits real hyperlink sequences on every platform.
test_command_toolbar_ui_call_reports_display_failure patched call_soon_threadsafe on the toolbar's live event loop and swallowed every call, not just the one it meant to drop. The patch was still installed while asyncio.run() tore the loop down, and asyncio resolves the default executor's join future through call_soon_threadsafe, so that report was dropped too. On Linux and macOS prompt_toolkit registers stdin with loop.add_reader, no default executor is ever created, and shutdown_default_executor() returns immediately. On Windows prompt_toolkit parks its WaitForMultipleObjects watcher in loop.run_in_executor(), so the executor exists and the loop waits on a future nothing resolves: 300 seconds on Python 3.12 and later, and forever before that, where shutdown_default_executor() takes no timeout. Windows CI went from 34s to 345s on 3.14, and hung for six hours on 3.11. Drop only the request made on the command thread and pass everything else through. The display still dies with the ValueError the test asserts on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ej3Nyn6vGngxYqm9FA1Jw5
…Windows It now has three commands, in the order your colleague should try them: 1. slow — sleeps 5 seconds. Does TOOLBAR IS HERE stay visible the whole time? This is the discriminating test: if the toolbar isn't up during a command, the real bug is that CommandToolbar never starts on Windows, and the pager is just collateral damage. 2. longout — pages 500 lines. Built-in pager ends in q: quit; the external one shows -- More --. 3. diag — dumps the guard state to real stderr (so it survives whatever the pager does to the screen). The single most useful thing to report is the slow result plus the diag block — that alone should help narrow down the underlying root cause.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1748 +/- ##
==========================================
- Coverage 99.64% 99.61% -0.04%
==========================================
Files 23 25 +2
Lines 5973 6542 +569
==========================================
+ Hits 5952 6517 +565
- Misses 21 25 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
tleonhardt
changed the title
Counting frames that omit the toolbar is not a stable property. The rendered byte stream and visible_windows both depend on prompt-toolkit's screen diffing and on how many frames were drawn while input arrived, and the borrowed application can finish early over a pipe input, producing teardown frames that are indistinguishable from a real gap. Assert the run's lifecycle instead: reading a line must not stop or restart the application that draws the toolbar. Pin read_line()'s own echo with a count of cmd2's writes plus an empty input buffer, which catches both a vanished line and a duplicated one without depending on frame timing.
tleonhardt
changed the title
Member
Author
|
This branch and PR represent a failed attempt at preventing flicker in the bottom toolbar by having a single |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
This PR targets
main, so its diff contains the whole persistent bottom toolbar and embedded pager feature — the commits onconsolidated_toolbar— plus 10 further commits that change how the toolbar's prompt-toolkit application is driven. Those 10, from341a1eb5onward, are the delta; everything before isconsolidated_toolbarunchanged.mainhas since been merged in, which brought the right prompt fix (#1749) along with it.What this branch does
Keeps one prompt-toolkit
Applicationrunning for the whole ofcmdloop(), swappinglayout/key_bindingson it instead of stopping and restarting between the prompt and each command — the techniqueCommandToolbar.page()already uses for the pager.That part works:
tests/test_toolbar_flicker.pyasserts that reading a line neither stops nor restarts the application, and reverting the prompt routing makes it fail withthe application was restarted after the prompt.It just does not fix the flicker.
What actually causes the flicker
The original diagnosis was that
PromptSessionexcludes the bottom toolbar from every run's final frame via the~is_doneterm in its filter, so ending a run between the prompt and the command erases the toolbar. That is true, and this branch removes that boundary. It is not what the user sees.A blink is a full-screen erase followed by a repaint. Counting
\x1b[J(erase to end of screen) in the window right after pressing Enter on an empty line, drivingexamples/getting_started.pythrough a real pty — reproducible byte-for-byte across runs:consolidated_toolbarThis branch is worse. The regression is
_echo_accepted: a continuous application never renders theis_doneframe that used to commit the prompt line to scrollback, soread_line()writes the line itself throughStdoutProxy. Every proxy write goes throughrun_in_terminal, which callsrenderer.erase()and repaints. One erase was removed and another added.The deeper point is that the erase and the repaint arrive in the same pty write batch — there is no time gap in our process to shrink. The terminal receives "wipe this region, now redraw it" and sometimes paints the intermediate state. That is why the blink is intermittent and terminal-dependent, and why reducing the number of application runs was never going to fix it. Printing anything above a live prompt-toolkit UI requires that erase; it is inherent to
run_in_terminal.Earlier measurements in this PR that suggested success counted how many times the prompt line was written (2x to 1x). That was real, but it measured the wrong thing.
One further boundary, also insufficient
_run_cmdfinalization_hooksis decorated@command_toolbar.suspend_toolbar— it callstermios.tcsetattr(..., self._initial_termios_settings)to repair a terminal corrupted by binary output, which would clobber prompt-toolkit's raw mode. It costs a full stop/restart of the display on every command, on this branch and onconsolidated_toolbaralike. Removing it takes pause/resume cycles per command to zero and drops erases from 6 to 4 per command, but not to zero, so it does not fix the blink either.The one confirmed win — extracted and merged
Manual testing found a genuine improvement: the right prompt used to be redrawn into the scrollback beside every accepted line; here it stays only on the live prompt line.
PromptSessionrenders the right prompt as aFloatwith no~is_donefilter (shortcuts/prompt.py:157-162), unlike the bottom toolbar container, so it is included in the committed final frame. This branch fixed it only incidentally, because_echo_acceptedwritesprompt + textand never the right prompt — the same code responsible for the erase regression above.That fix needed none of this architecture. It was extracted as a standalone change against
mainand merged in #1749.Known defects in this branch
Prompt-mode Ctrl-C routing is unverified, and its test passes for the wrong reason
test_read_line_ctrl_c_raises_without_stopping_the_appsends"partial\x03"beforeread_line()switches the application into prompt mode. The keys are therefore handled with command-mode semantics, where theinterruptbinding callsos.killpg(os.getpgrp(), SIGINT). Spying on_fail_line, the method the prompt-mode Ctrl-C binding calls:It is never called. The
KeyboardInterruptthat makes the test pass comes from that realSIGINT, not from the binding added in3ed28313. The claim that this branch routes prompt-mode interrupts without ending the run is therefore unverified. Ctrl-D is genuinely exercised (_fail_line calls (ctrl-d): ['EOFError']), because\x04is swallowed bysave_keyand replayed into prompt mode rather than raising a signal.Free-threaded CI failures: a hang, not a crash
The 3.14t jobs on Linux and macOS fail after roughly 40-60 seconds with
The runner has received a shutdown signalfollowed byThe operation was canceled, and no failing test.fail-fastisfalse, so these are not collateral from another job, andmain's 3.14t jobs pass consistently on all three platforms.Reproduced locally on a real free-threaded build (
uv sync --python 3.14t,sys._is_gil_enabled()isFalse): the suite hangs intest_read_line_ctrl_c_raises_without_stopping_the_app, at aThread.join(). Run in isolation it hangs insidepytest.raises, meaningread_line()never returns. The cause is the same as above — under the GIL theSIGINThappens to land while the main thread is insideread_line(), so it surfaces as the expectedKeyboardInterrupt; without the GIL the timing shifts, the signal lands outside that window, andread_line()blocks forever. A hung suite with threads still running is consistent with the runner being killed.Windows 3.14t passes, because the Windows
interruptbinding uses_thread.interrupt_main()rather thanos.killpg.The design gap behind both
In the real
cmdloop, between a command finishing andread_line()entering prompt mode, the application is still in command mode. A Ctrl-C typed in that window sendsSIGINTto the whole process group instead of just aborting the line. This branch widened that window, because prompt mode is now entered by an explicit layout swap rather than by starting a fresh application that already has the prompt's own bindings. Any future attempt at a continuous application has to close this window.Data races
read_line()has two unsynchronized cross-thread accesses, independent of the above:self._lineis written by the command thread (command_toolbar.py:457) and read by the UI thread (:430,:439);session.default_buffer.accept_handleris set by the UI thread insideenter()(:463) and restored by the command thread infinally(:493). The second is the more dangerous — if a queued Enter is processed after the stock handler has been restored, that handler callsapp.exit()and terminates the continuous application.Where the work should go instead
To stop the blink, the toolbar row has to stay out of the erased region — a terminal scroll region (DECSTBM) is how persistent status bars normally avoid exactly this. That is a change at the output layer, not the application layer, and is not what this branch does.