Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1773 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 23 23
Lines 6473 6473
=======================================
Hits 6457 6457
Misses 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
Thanks for making this example. It works quite well, but I did run into one issue. After running enough commands where the prompt should display at the bottom of the screen, it instead is hidden outside the CLI panel. |
|
Thanks for catching this, I can reproduce it. The I moved the border to a wrapping container in a35a055, so the PTY now matches the visible area (26 rows, cursor on row 25 after 40 commands). |
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
@DG-Elom I really love this example! Thank you for adding it and submitting this PR.
Please don't be discouraged by my thorough review. I found a number of small edge cases that can either cause a crash, display something that doesn't make sense, or result in behavior that doesn't seem to match the documentation.
| "[b]cmd2 status[/b]\n" | ||
| f"State: [{color}]{self.state}[/{color}]\n" | ||
| f"Commands run: {self.commands_run}\n" | ||
| f"Last command: {self.last_command or '-'}\n" |
There was a problem hiding this comment.
The command name is put into the StatusPanel's markup string without escaping. Typing [/b] or a[/x] at the prompt crashes the application with a MarkupError traceback and the textual app exits.
Fix the crash in one way or another. Easiest way is probably to wrap the command name in textual.markup.escape. Another alternative would be to catch the MarkupError and handle that case, but that is likely more work.
A different approach would be to only set self.last_commnad inside a cmd2 lifecycle hook that runs post command so it doesn't get set for invalid commands.
| def _on_command_end(self, data: cmd2.plugin.CommandFinalizationData) -> cmd2.plugin.CommandFinalizationData: | ||
| """Report that the command is done, even when it failed.""" | ||
| self._commands_run += 1 | ||
| self._last_duration = time.monotonic() - self._command_started |
There was a problem hiding this comment.
The finalization hook also runs for blank lines, Ctrl-C, comments and parse errors, and precmd never runs for those. So it adds to the command count and computes the duration from a start time that is either old or 0.0.
Pressing Enter on a blank prompt as the first input showed a duration of about 590,000s (the system uptime on my laptop)
Should fix this so either it doesn't update the duration for invalid commands or that it always updates the start time even for unsuccessful commands.
| """Report the initial status before the first prompt.""" | ||
| self._report_status("idle") | ||
|
|
||
| def _on_command_start(self, data: cmd2.plugin.PrecommandData) -> cmd2.plugin.PrecommandData: |
There was a problem hiding this comment.
Commands called from run_script, history -r and py also fire the hooks. The panel flips to idle in the middle of the outer command, one typed command adds 4 to the count, and the last command shown is wrong.
Fix: keep a depth counter, or skip when in_script() or in_pyscript() is True.
| return | ||
| try: | ||
| status = json.loads(event.title.removeprefix(STATUS_PREFIX)) | ||
| except json.JSONDecodeError: |
There was a problem hiding this comment.
Only JSONDecodeError is caught. A program in the pty that sets a title like cmd2-status:[], or one with the wrong value types, crashes the host.
Fix: check isinstance(status, dict) and also catch TypeError and ValueError.
| # The border is on a container because a Terminal sizes its pseudo-terminal from its outer size, so a | ||
| # border on the Terminal itself makes the cmd2 app draw more rows than are visible and hides its prompt | ||
| with Vertical(id="cmd2-pane"): | ||
| yield Terminal(command=[sys.executable, __file__, "--cmd2-only"], id="cmd2") |
There was a problem hiding this comment.
bittty's WindowsPTY doesn't accept a list as the command, so on Windows the pane stays blank. The docstring says Windows works.
Fix: Either pass the executable and the command line separately on win32, or document the example as POSIX-only.
| from typing import Any, ClassVar | ||
|
|
||
| from prompt_toolkit.application import get_app_session | ||
| from textual.app import App, ComposeResult |
There was a problem hiding this comment.
It feels a little off that --cmd2-only needs Textual.
textual is imported at the top of the module, so the "without Textual" mode fails with ModuleNotFoundError unless textual-tty is installed.
Moving the imports into the host-only code path fixes this.
|
|
||
| def on_terminal_process_exited(self, _event: Terminal.ProcessExited) -> None: | ||
| """Exit when the cmd2 application exits, for example due to the quit command.""" | ||
| self.exit() |
There was a problem hiding this comment.
The child's exit code is never passed back, so the host always returns 0.
If the child crashes at startup, its traceback is wiped when the screen is restored.
Recommend passing back the child's exit code.
| "last_command": self._last_command, | ||
| "last_duration": self._last_duration, | ||
| } | ||
| self.set_window_title(STATUS_PREFIX + json.dumps(status)) |
There was a problem hiding this comment.
Running --cmd2-only in a normal terminal (as opposed to say, a tmux terminal) overwrites the tab title with the status JSON and never restores it.
| def do_slow(self, args: list[str]) -> None: | ||
| """Run for a number of seconds (default 3) so the status panel can be seen in the running state.""" | ||
| try: | ||
| seconds = float(args[0]) if args else 3.0 |
There was a problem hiding this comment.
slow -1, slow nan and slow inf print "Working…" and then raise an error from time.sleep.
Recommend validating prior to printing anything using something like:
import math
...
if seconds < 0 or not math.isfinite(seconds):
self.perror(f"Argument must be a finite positive number: {args[0]}"
return| """ | ||
|
|
||
| # Priority bindings are needed because the focused Terminal sends every key press to the cmd2 application | ||
| BINDINGS: ClassVar[list[BindingType]] = [Binding("ctrl+q", "quit", "Quit", priority=True)] |
There was a problem hiding this comment.
Textual already has a priority binding for Ctrl-Q. The override only makes Quit show in the Footer, but the comment implies the binding has to be added.
3 participants
Adds
examples/textual_tty_embed.py, showing how to embed a cmd2 app in a Textual app with textual-tty, with a realtime status panel next to the terminal.--cmd2-only) in a textual-ttyTerminal, so the cmd2 app is a plain prompt-toolkit app that doesn't know it's embedded.set_window_title()from precmd / command finalization hooks. TheTerminal.TitleChangedmessage updates the panel, so no extra IPC is needed. A Textual-driven clock panel updates independently.uv run --with textual-tty examples/textual_tty_embed.py.examples/README.md.Tested headless with Textual's Pilot (textual 8.2.8, textual-tty 0.4.0, Linux): typing commands into the embedded terminal shows cmd2's output, and the status panel goes idle -> running -> idle with the command count updated.
make test,make mypy,make docs-testand the prek hooks pass.Note that textual-tty is still pre-alpha, so the example may need small updates if its API changes. Happy to adjust the approach (e.g. a socket instead of the window title) if you prefer.
Closes #1771