refactor: extract terminal emulator behind a backend trait - #78
Open
aymanbagabas wants to merge 6 commits into
Open
refactor: extract terminal emulator behind a backend trait#78aymanbagabas wants to merge 6 commits into
aymanbagabas wants to merge 6 commits into
Conversation
The grid was produced directly by an alacritty-specific `Emu` struct, so supporting another emulator meant editing every consumer. Split the seam: - `terminal/cell.rs` holds the neutral grid vocabulary (`EmuCell`, `Color`, `rows_to_strings`) with no emulator dependency - `terminal/emu.rs` defines the `Emulator` trait, the whole contract between the PTY and the grid - `terminal/alacritty.rs` (moved from `emu.rs`) implements it Command/exit/cwd tracking moves out of the emulator into `TermState`. It is derived from the raw PTY byte stream and never touched the alacritty grid, so keeping it outside the trait means every backend reports identical shell integration by construction instead of reimplementing it. `integration.rs` is unchanged. Consumers already spoke `EmuCell`/`Color` rather than alacritty types, so this is import churn for them. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Swapping the emulator behind the `Emulator` trait silently changes what `expect`, `snapshot`, and the SVG renderer see, and the grid had no tests at all. Add a macro-generated suite that every backend opts into with one line, so a second backend is verified against the same contract as the first. Covers grid shape, blank-vs-space cells, SGR attributes, palette/RGB/default colors, wide-char spacers, cursor position and clamping, resize, scrollback retention and ordering, alternate screen, erase, PTY write-back for DSR, and escape sequences split across reads. Each case is its own test, so a failure names the part of the contract that broke. 17 cases pass against the alacritty backend. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Review found several cases asserted far less than their names claimed, so a materially broken backend could pass the suite that exists to gate backends: - cursor clamping accepted any in-bounds value, including (0, 0); now pins the exact bottom-right cell - DSR accepted any CSI prefix; now pins the exact cursor position report - erase and blank cells checked rendered text only, so a backend that dropped colors or attributes passed; now compares whole cells against the default - resize checked only the reported size, never that content survived - full_rows compared lengths, not contents, and never pinned that history is followed by exactly the viewport, which `grid(full = true)` relies on Also covers areas that were untested: split UTF-8 across reads (the reader uses a fixed 8 KiB buffer, so real output splits codepoints), autowrap, tabs, bare CR overwrite, backspace, erase-to-end-of-line, and the scrollback limit, which no case exercised despite the daemon requesting 5000 rows. Verified by mutation: a backend that reports the cursor at the origin, renders blank cells as spaces, or drops scrollback now fails 8 cases; before it passed all of them. Tabs pin only cursor advance and landing column. Alacritty stores a literal tab in the skipped cells and other emulators store blanks, so asserting the rendered text there would fail a correct backend. The macro no longer emits `use` statements into the caller's module; a second backend whose test module imports Color or Emulator itself would have hit E0252. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
aymanbagabas
marked this pull request as draft
July 29, 2026 20:30
The grid vocabulary flattened away detail that the emulator had already parsed, and each loss surfaced somewhere downstream: - `Color::Default` was indistinguishable from ANSI 0, so `expect --fg 0` matched a default-colored cell and claimed it was black when the theme paints it light gray. Colors are now `Option<Color>`, and a default cell matches no expected value at all. - Palette slots 0-15 are themeable and 16-255 are fixed, but both landed in `Color::Idx`, leaving every consumer to re-derive the split. They are now `Color::Named` and `Color::Idx`, divided by numeric range rather than by how the escape sequence spelled it, so backends that carry only a palette index agree with backends that carry a name. - Five distinct underline styles collapsed into one bool, and the underline's own color was dropped entirely. `Option<Underline>` keeps both. - A blank cell and the second column of a double-width character were both the empty string, and text extraction substituted a space for each. That invented a column: `你a` extracted as `你 a`, shifting every subsequent column and breaking text matching. Blanks now hold a real space and the empty string means continuation, which extraction skips. Attributes move into a `bitflags` set, which adds blink and turns the per-cell style comparison in the render and monitor loops into a single integer compare. `ch` becomes a `CompactString`, inline for anything up to 24 bytes, so the grid no longer heap-allocates per cell and can hold the combining marks the backend was discarding. The JSON wire format is unchanged: named and indexed colors both serialize to their palette index and underline stays a boolean, so the JS and Python bindings keep working. The one visible difference is that a continuation cell now reports `""` instead of a space, which the binding types document. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The `cells` response emitted four of the seven boolean attributes the neutral model carries. `dim`, `invisible` and `strike` were parsed, stored and then dropped at the wire, and `blink` had nowhere to go at all. A client should be written against the vocabulary, not against whichever backend happens to be compiled in, so all seven are reported now. `blink` is always false from alacritty, which parses SGR 5/6/25 and discards them, but ghostty (`Style.Flags.blink`) and xterm.js (`FgFlags.BLINK`) both track it and will fill it in without any client change. Underline style and color reach the wire for the same reason, as `underline_style` and `underline_color` alongside the existing boolean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The rationale on `NamedColor` claimed xterm.js carries only palette indices. It does not: `CM_P16` and `CM_P256` are distinct color modes, so xterm.js preserves `SGR 31` vs `SGR 38;5;1` exactly as alacritty does. The reason to drop that distinction is ghostty, whose `Style.Color` has a single `.palette` variant and cannot reproduce it, plus the fact that nothing here consumes it: bold-to-bright keys off the index, not off how the color was spelled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
aymanbagabas
marked this pull request as ready for review
July 29, 2026 21:09
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.
Puts the terminal emulator behind an
Emulatortrait so additional backends (libghostty, xterm.js) can be added without touching the daemon, render, or assert layers, then makes the neutral grid vocabulary lossless enough for those backends to agree on.The seam
Emu→AlacrittyEmu, now the firstimpl Emulator; neutral grid types moved toterminal::cellCommandTrackerlifted out of the emulator intoTermState— OSC 133/633/7 parsing is backend-neutral, so every backend gets identical shell integration by construction rather than reimplementing itemulator_conformance_tests!, backend-agnostic tests any new backend has to passThe cell model
The vocabulary flattened away detail the emulator had already parsed, and each loss surfaced downstream:
Color::Defaultwas indistinguishable from ANSI 0, soexpect --fg 0matched a default-colored cell and reported it as black when the theme paints it light gray. Colors areOption<Color>; a default cell matches nothing."", and text extraction substituted a space for each — inventing a column, so你aextracted as你 aand shifted every column after it. Blanks hold a real space,""means continuation, extraction skips it.Option<Underline>keeps both.Color::Idx. NowColor::NamedandColor::Idx, split by numeric range rather than by how the escape sequence spelled it, so a backend that only carries a palette index agrees with one that carries a name.Attributes move into a
bitflagsset, which adds blink and reduces the per-cell style comparison in the render and monitor loops to one integer compare.chbecomes aCompactString, inline up to 24 bytes, so the grid stops heap-allocating per cell and can hold combining marks the backend was discarding.Wire format
Colors are unchanged: named and indexed both serialize to their palette index.
cellswas reporting four of seven attributes, sodim,invisible,strikeandblinkare added along withunderline_styleandunderline_color.blinkis always false from alacritty, which parses SGR 5/6/25 and discards them, but ghostty and xterm.js both track it. A continuation cell now reports""rather than a space; the binding types document it.