Repository navigation
fix(tty): make send-string-to-terminal deliver its bytes like GNU - #468
Merged
Merged
Conversation
Issue #453. The builtin validated its arguments and returned nil, dropping the string: every Lisp-side terminal negotiation that runs through this primitive -- kkp.el's Kitty Keyboard Protocol query and CSI > flags u enable sequence, evil-escape's escape hatch -- was writing to a sink, so the terminal never answered. GNU's Fsend_string_to_terminal (src/dispnew.c:6808) writes SBYTES raw to the terminal without alteration: stdout for output_initial, the tty's own stream for termcap terminals, and errors for a window-system device ("Device %d is not a termcap terminal device") and for a suspended one ("Terminal is currently suspended"). Mirror all four paths through a new TerminalHost::write_bytes: TtyTerminalHost writes stdout on the evaluator thread (same ordering as every TtyRif write, popup-menu precedent), SecondaryTtyHost writes that terminal's device, and the initial terminal writes stdout directly.
Issue #453 regression. Boots a GNU/neomacs pair on real PTYs and requires a unique marker on BOTH transports: the bytes are raw PTY output, not screen content, so the assertion polls the transport rather than a rendered grid. Scope note: the issue also asks for a KKP handshake test (negotiation followed by Escape exiting Evil Insert). That needs a harness-side terminal emulator that answers the CSI query plus a kkp.el fixture neither test crate carries; it would test kkp.el and terminal emulation, not this primitive. The raw-bytes pair test pins the layer that was actually broken: primitive to PTY, byte-identical to GNU.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (7)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new PTY regression test can pass even against the unfixed no-op because the pasted expression's minibuffer echo already places the marker bytes on the transport it scans, so it does not guard the targeted bug.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR fixes issue #453 by making the send-string-to-terminal builtin actually write its bytes to the target terminal instead of validating arguments and returning nil. The previous no-op silently dropped every Lisp-side terminal negotiation (notably kkp.el's Kitty Keyboard Protocol query/enable sequences), which is why KKP never activated in -nw and Evil got stuck in Insert state. The change mirrors GNU's Fsend_string_to_terminal dispatch: raw bytes to stdout for the initial terminal, to the tty's own stream for a termcap terminal, and the same errors GNU raises for window-system/suspended/dead terminals.
Changes:
- Decode the string + terminal designator in the builtin, then dispatch to a new
write_bytes_to_terminalkeyed onTerminalOutputMethod(exhaustive match for GNU-parity totality). - Add a defaulted
TerminalHost::write_bytestrait method, implemented byTtyTerminalHost(evaluator-thread stdout write) andSecondaryTtyHost(that terminal's own device). - Add core unit tests (termcap host delivery, initial-stdout, window-system/suspended rejection) and a new PTY pair regression test.
| File | Description |
|---|---|
crates/neovm-core/src/emacs_core/display/dispnew/pure.rs |
Builtin now extracts raw string bytes, resolves the terminal id, and calls the dispatch helper instead of returning nil. |
crates/neovm-core/src/emacs_core/display/terminal/pure.rs |
Adds write_bytes_to_terminal dispatch + default TerminalHost::write_bytes; exposes decode_terminal_id_eval. |
crates/neomacs/src/tty_frontend.rs |
TtyTerminalHost::write_bytes writes raw bytes to stdout (the primary tty stream). |
crates/neomacs/src/secondary_tty.rs |
SecondaryTtyHost/registry/session write raw bytes to the secondary terminal's own device. |
crates/neovm-core/src/emacs_core/display/dispnew/tests/mod.rs |
New core tests for delivery, initial-stdout, and the two error paths. |
crates/neomacs-tui-tests/tests/send_string_to_terminal.rs |
New PTY pair regression asserting the marker reaches the transport on both GNU and neomacs. |
crates/neomacs-tui-tests/tests/tui.rs |
Registers the new test module in the aggregated tui binary. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
+38
| eval_expression_one( | ||
| session, | ||
| &format!( | ||
| "(send-string-to-terminal \"{}\")", | ||
| std::str::from_utf8(MARKER).unwrap() | ||
| ), | ||
| ); | ||
| let deadline = Instant::now() + Duration::from_secs(15); | ||
| loop { | ||
| session.read(Duration::from_millis(100)); | ||
| if session | ||
| .recent_output() | ||
| .windows(MARKER.len()) | ||
| .any(|window| window == MARKER) |
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
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.
Fixes #453.
Root cause
The builtin validated its arguments and returned nil — the string went to a sink. Every Lisp-side terminal negotiation that runs through this primitive was writing into the void:
kkp.el's Kitty Keyboard Protocol support query andCSI > flags uenable sequence never reached the terminal, so the terminal never answered, and escape-hatch flows like evil-escape saw nothing either.GNU semantics (src/dispnew.c:6808)
Fsend_string_to_terminalwrites SBYTES raw, "without alteration":output_initialfwrite+fflushonstdoutTerminalHost::write_byteserror("Device %d is not a termcap terminal device")error("Terminal is currently suspended")wrong-type-argument terminal-live-pThe Lisp-visible decoding stays in
dispnew/pure.rs; the per-terminal dispatch is a newpub(crate) write_bytes_to_terminalkeyed onTerminalOutputMethod— the enum makes every path exhaustive at compile time. Hosts opt in through a defaultedTerminalHost::write_bytestrait method:TtyTerminalHostwrites stdout on the evaluator thread (same ordering as every TtyRif write — the popup-menu precedent),SecondaryTtyHostwrites that terminal's own device.Verification
dispnew/terminalsweep 159/159cargo fmt --all --checkandcargo check --workspace --all-targetscleanScope note on the KKP handshake test
The issue also asks for a negotiation-then-Evil-Escape E2E test. That needs a harness-side terminal emulator that answers the CSI query plus a
kkp.elfixture neither test crate carries — it would test kkp.el and terminal emulation, not this primitive. The raw-bytes pair test pins the layer that was actually broken (primitive → PTY); GNU 31.1 itself has no native KKP, so Lisp-side negotiation remains the parity mechanism.Unrelated finding
While tracing the input path I found primary-terminal
suspend-tty/resume-ttyare dead-ends:TtyInputReaderconsumes the render-command channel and swallowsLifecycleCommand::SuspendTty/ResumeTty. Out of scope here — worth its own issue.