Repository navigation
fix(emu): IL/DL margin, taller resize, alt-screen resize, SGR sub-params, CSI bound - #740
Merged
Merged
Conversation
`CSI 9:1 m` struck the cell through, and `5:1`, `6:1` and `8:0` set blink and conceal, while `unsupported()` listed the same sequence as dropped. xterm reads sub-parameters only on SGR 38 and 48 (charproc.c, CASE_SGR: any other group that has one is skipped), and vt100 matches no such group, so only the attribute shadow acted on them: `rewrite_sgr` read a group's first value as if the group were plain. `25:1`, `28:0`, `29:1` and `0:1` cleared a carrier the same way. The shadow now carries nothing for a group that contains `:`. The unsupported record declines an SGR only when every parameter is a single value the shadow recovers; it flattened sub-parameters, so `^[[5:6m`, `^[[9:9:9m` and `^[[5;8:8m` were applied to the cell and not listed. Before, on 4x10 (a unit probe over Vt100Emulator): ESC[5:1m X blink=true unsupported=["^[[5:1m"] ESC[8:0m X conceal=true unsupported=["^[[8:0m"] ESC[9:1m X strike=true unsupported=["^[[9:1m"] ESC[5:6m X blink=true unsupported=[] ESC[9m ESC[29:1m X strike=false ESC[9m ESC[0:1m X strike=false After: each leaves the style as it was, and each is listed. Tests: an_sgr_sub_parameter_carries_no_attribute_and_is_on_record (vt100.rs), a_group_with_a_sub_parameter_carries_nothing (shadow.rs) and an_sgr_sub_parameter_is_recorded_whatever_its_values (unhandled.rs). All three fail with the two changes reverted and pass with them. Closes #688 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Both rewriters in shadow.rs (ColorNormalizer and AttrShadow) held every byte of a CSI in `pending` until its final byte, to see whether it was an SGR, so a CSI that no final byte ended grew the test process with the child's output: about three bytes of memory for each byte written (#730 measured +48 MiB for a 16 MiB run on 0.11.8). A rewriter now holds at most CSI_HOLD_MAX (256) bytes of one CSI. At the bound a CSI that is not plain digits and separators passes through as it came, as the issue proposed. A plain one is not passed through: it may be an SGR, and an SGR the shadow receives unrewritten sets its carriers (a long `CSI 0...03 m` would read as concealed). Such a CSI is held from there on as vte holds it: each value saturated at 65535, nothing after the 32nd, written short (`ESC [`, each stored value and its separator), at most 193 bytes. Both rewriters then work on the values the parser will see, so a long SGR draws what the same SGR written short draws. When the rest of the sequence goes to the parser unrewritten (a C0 control inside it), a full short form keeps its trailing separator, so the parser ignores the digits that follow, as it would have. The memory test runs in a child of the test binary under `ulimit -d` 48 MiB (helper `under_data_cap`, now shared with the #694 OSC test): shadow.rs from de7ee37: child aborts, "memory allocation of 33554432 bytes failed" this commit: passes a_long_csi_left_to_the_parser_leaves_it_as_full_as_it_was failed on the first version of this change (the BEL case turned the 32nd value `3` into `30`) and passes now. Tests: an_unterminated_csi_leaves_memory_bounded, an_sgr_too_long_to_hold_reads_as_the_same_sgr_written_short and a_csi_too_long_to_hold_is_carried_out_in_both_grids (vt100.rs); an_unterminated_csi_is_held_only_as_far_as_the_bound (reads of 1, 7 and 8192 bytes, pending never past the bound), the_short_form_never_outgrows_the_bound, a_long_csi_keeps_only_the_32_values_vte_stores, a_plain_csi_past_the_hold_bound_is_rewritten_from_vtes_values, a_csi_past_the_hold_bound_that_is_not_plain_passes_through and a_csi_as_long_as_the_hold_bound_is_held_as_written (shadow.rs). SECURITY.md lists the bound. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Insert Line and Delete Line inside the scroll region kept the cursor's column and its pending wrap. xterm's InsertLine and DeleteLine (util.c) return early outside the margins and otherwise set_cur_col to the left margin and ResetWrap; Windows Terminal's _InsertDeleteLineHelper does the same. vt100 0.18.1 only rotates the rows (Grid::insert_lines and delete_lines, which check the row against its own region). The tracker now reports a plain `CSI L` or `CSI M` (SeqEvent::LinesMoved, including a parameter with a sub-parameter, which vt100 dispatches too) and the emulator, once the sequence is fed, feeds a CR to both parsers when the cursor row is inside the region of the screen on display. CR sets the column to 0 and clears vt100's pending wrap. To know the region the tracker's ScreenFrame, which held the top margin per screen since #733, now holds the bottom one too, by vt100's rules: a DECSTBM that is no region, a resize and `1049` clear it; `47` keeps the alternate grid's. Before, on 4x10 after `abcdefghij\r\nklmnopqrst` (unit test over Vt100Emulator; the CR feed disabled): CSI 2;5H CSI L X X at (1,4) CSI 1;5H CSI M X X at (0,4) abcdefghij CSI L X "\nXbcdefghij" (fresh screen; X wrapped onto the a) After: X at (1,0), X at (0,0), and "X\nabcdefghij", as in xterm. Tests: insert_and_delete_line_move_the_cursor_to_the_left_margin (the three rows above plus DL after a pending wrap, `CSI 0;7 L`, `CSI 2:1 M`; whole and byte by byte) and insert_and_delete_line_outside_the_region_leave_the_cursor (above and below the region, on both margins, per screen through 47 and 1049, after a resize). Both fail with the CR feed disabled, and the second fails when the CR is fed unconditionally. The seeded margin sweep (the_report_origin_follows_vt100s_margins_and_origin_mode) now also checks every row against vt100's own region, read under origin mode, and fails when the bottom margin is ignored. Changed, not Fixed: the cursor column and text written after an IL or DL are visible in a screen, so a consumer's test could have matched the old column. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
A resize that made the screen taller by more rows than the history held added blank rows above the text and moved the cursor down with them; with `scrollback(0)` it always did. vt100 0.18.1's Grid::set_size pulls back min(diff, unbounded_len) rows, where unbounded_len counts every row that ever scrolled off, takes the ones its bounded history still has and makes the rest blank. xterm's ScreenResize (screen.c) moves the screen down by Min(nrow - oldrow, savedlines), the rows it can pull back, and adds the rest below; AdjustSavedCursor moves the saved cursor the same. resize_as_xterm (vt100.rs) now resizes both parsers, the grid and the attribute shadow, in two steps when the history holds fewer rows than the screen grows by: first by the rows it holds, which vt100 pulls back whole and leaves the history empty, then, after clear_scrollback (whose only effect on an empty history is to zero unbounded_len), by the rest, which vt100 adds at the bottom. The cursor and saved cursor follow the rows actually pulled. The history length after the resize is what it was before this change, so the copy's capture logic is untouched. The alternate screen has no history in vt100 and already grew at the bottom. The main grid behind it is resized too, out of sight, so it is brought on display for the resize with `CSI ? 47 l`, which in vt100 toggles the grid and nothing else, and hidden again with `CSI ? 47 h`. That is done only when the tracker is between sequences; inside one, a read split open, the main grid is resized as vt100 resizes it, since injected bytes would cut the sequence. Before (10x3, `a\r\nb\r\nc\r\nd\r\ne`, resize to 10x6; the reviewer's PTY probe and the unit test agree): scrollback(0): "\n\nc\nd\ne\n" cursor (4,1) scrollback(1): "\nb\nc\nd\ne\n" cursor (4,1) scrollback(1000): "a\nb\nc\nd\ne\n" cursor (4,1) After: scrollback(0): "c\nd\ne\n\n\n" cursor (2,1) scrollback(1): "b\nc\nd\ne\n\n" cursor (3,1) scrollback(1000): unchanged Tests: a_taller_screen_moves_down_only_by_the_rows_its_history_holds (scrollback 0, 1, 2 and 1000, styled history on and off: text, history, cursor, blink carried by the shadow, the saved cursor still on its row, history carrying on after), a_taller_screen_moves_the_main_screen_as_xterm_does_behind_the_alternate (1049 and 47) and a_taller_resize_inside_a_sequence_leaves_the_sequence_whole (OSC, CSI, ESC and DCS left open across the resize). The first two fail with resize_as_xterm reduced to vt100's set_size; the third fails when grids are switched inside a sequence. The seeded sweep a_history_at_its_bound_is_the_terminal_s_own_through_any_scroll_or_resize compares the grid with a raw vt100 parser; its reference now resizes through resize_as_xterm, since only where the new rows go differs, and the history comparison it exists for is unchanged. The split-read tests pass unchanged. Changed, not Fixed: the screen text and cursor row after a taller resize are visible, and a consumer's test could have matched them. The Terminal::resize rustdoc and DESIGN §2 say where the new rows go. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
While the alternate screen is up, vt100 resizes the main grid too, out of sight, and every accessor reads the grid on display, so termlens settled the main screen's history only once it was back (#576, #682): a row the resize pushed was read at the width the screen had by then, and a row pulled back and pushed again, cut to a narrower screen, kept the text it was first copied with. A row vt100's bounded history dropped meanwhile also stayed in the copy. Vt100Emulator::set_size now puts the main grid on display for the resize when the tracker is between sequences: `CSI ? 47 l` to both parsers (in vt100 it clears MODE_ALTERNATE_SCREEN and nothing else, whether 47 or 1049 put the alternate screen up), then the same steps as with the main screen up (copy the rows a shorter screen pushes off the grid, resize, capture), then `CSI ? 47 h` (sets the mode, and the alternate grid is already allocated). Bytes would cut a sequence a read left open, so inside one the resize is settled on the way back, as before; docs/LIMITATIONS.md lists that case. resize_as_xterm (#684) no longer switches grids itself. Reproduced with the seeded sweep in vt100.rs, its skip after a resize behind the alternate screen turned into a report: on the branch before this commit, cases 314, 411 and 433 (of 600) ended with the copy's rows wider than vt100's own, e.g. case 411 ["AAAAAAAA"] against ["AAAAAA"] after 4x6 -> 2x12 with 1049 up. Six were counted after #729; the #684 resize changes which cases diverge. Now the sweep compares the copy with vt100's history exactly after every step, the skip is gone, and all 600 cases pass. Tests: rows_a_resize_pushes_behind_the_alternate_screen_keep_their_own_width (styled history: a row pushed at 4 columns and the screen widened to 8 before the exit kept 8 cells, now 4; a row pulled back, cut to 4 and pushed again; both against the same resizes with the main screen up). a_resize_while_the_alternate_screen_is_up_keeps_the_rows_still_in_history (#671) expected the copy to keep `a`, which the terminal's history had dropped at its bound while hidden; it now expects `b\nc`, vt100's own history and what the same resizes give with the main screen up, and still checks the copy carries on after. With the switch disabled, both of these, the sweep and #684's alternate-screen test fail. Changed, not Fixed: scrollback_text() and history cells after a resize behind a full-screen application differ, and a consumer's test could have matched the old ones. DESIGN §2 describes the switch. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
# Conflicts: # CHANGELOG.md # SECURITY.md # crates/termlens/src/emu/seq.rs
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
# Conflicts: # CHANGELOG.md
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.
Five emulator fixes, one commit each. Each has a test that failed before it; the commit bodies carry the before and after.
Changed (a test can see them; each CHANGELOG entry has a migration sentence):
CSI LandCSI Mleave the cursor column and a pending wrap where they were #686. Insert Line and Delete Line inside the scroll region move the cursor to the left margin and cancel a pending wrap, as xterm'sInsertLine/DeleteLineand Windows Terminal do. Outside the region the cursor stays.ScreenResizedoes, and adds the rest below. Withscrollback(0)a taller screen used to open blank rows above the text.CSI ? 47 l/hwhile the tracker is between sequences. Inside an open sequence it is settled on the way back, as before, and LIMITATIONS says so. The seeded sweep now compares the copy with vt100's history after every step, with no skip, 600/600.unsupported()lists them as dropped #688. An SGR parameter with a sub-parameter (9:1,5:1,8:0,25:1…) no longer sets or clears an attribute; xterm reads sub-parameters only on 38 and 48.unsupported()also lists the sequences it missed.Fixed:
main is merged in (SECURITY.md and
seq.rsconflicted; both sides kept). The merged tree passes fmt, clippy andcargo test --workspace --all-features.Closes #678
Closes #684
Closes #686
Closes #688
Closes #730