Repository navigation
fix: kitty and sixel capture, mouse release modifiers - #738
Merged
Merged
Conversation
`decode()` read the count of a sixel repeat (`!<n><sixel>`) as zero when it was `0` or missing, so the sixel was neither painted nor advanced past: `#1;2;100;0;0#1~!0~~` and `#1;2;100;0;0#1~!~~` both decoded 2x6 where xterm draws 3x6. xterm: "If the repeat count is zero or omitted, it is treated as 1" (graphics_sixel.c). Windows Terminal's SixelParser reads the count through VTParameter, where "both 0 and omitted values will default to 1". The count is now at least one. Before, graphics::tests::a_sixel_repeat_of_zero_or_of_no_count_repeats_once failed on the first body with `left: (2, 6) right: (3, 6)`. It passes now, and pins the middle column red, a `!0?` that skips one column unpainted, and a given count (`!1~!2~`, three columns) unchanged. Closes #698 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
In the legacy, UTF-8 (?1005) and urxvt (?1015) encodings `click_with` and `drag` sent a release as a bare button 3, which dropped the chord's shift, alt and ctrl bits. xterm's BtnCode (button.c) starts from `32 + (KeyState << 2)` and adds 3 for a release, so the modifiers stay, and Windows Terminal sets the same three bits on its release code (_windowsButtonToXEncoding). SGR (?1006) already kept them. The release is now `3 + (button & 0b11100)`. This changes bytes an application receives, so it is a Changed entry with a migration sentence. Before, tests/input.rs a_release_keeps_the_modifier_keys_in_every_encoding read these four releases off the wire (hex; press, [motion,] release): ?1000 ctrl+left 1b5b4d302523 1b5b4d232523 want ..33.. ?1015 shift+alt+left "\e[44;5;3M" "\e[35;5;3M" want 47 ?1005 ctrl+shift+left ..34c28524 1b5b4d23c28524 want ..37.. ?1002 shift+left drag ..24.. ..44.. 1b5b4d232322 want ..27.. All four now match xterm. The test is one emit program that sets each mode in turn and reads an exact byte count, so a wrong release shows as wrong hex rather than as a hang. It ran 15 times in a row here. It adds one PTY spawn, so it should go through the stress workflow, which cannot be triggered from this worktree. Closes #701 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The tracker read a kitty escape's keys from `dcs_head`, the first 128 bytes of the string. In a longer control block the value that straddled byte 127 was cut short and every key after it was dropped. kitty parses the keys straight off its escape buffer (parse_graphics_code) and has no such cut. The block now has a buffer of its own, filled from the `G` to the first `;`. It is bounded at 256 KiB, kitty's MAX_ESCAPE_CODE_LENGTH: the length past which kitty drops an escape that is still arriving. A pair that bound cuts in two is dropped whole, so no value is read as its own first digits. The buffer does not depend on the capture bound, which the issue's other suggestion (parse the retained body) would have: with `capture_graphics(0)` no body is kept. The 128-byte head stays for the `G` test and for XTGETTCAP names, unchanged. Before, a_kitty_control_block_is_read_past_its_first_128_bytes failed on a 142-byte block naming the twenty keys of a transmission and its placement: two payloads where there is one, kitty TransmitAndPlace 10000x10000px 1000x1cells ... (moves: true) kitty Transmit ?px at (0, 0) rgba 9 bytes that is `r=1000` read as `r=1`, `C=1` and `m=1` lost, and the second chunk counted as an image. It passes now at capture bounds 4 MiB and 0, and so does the reviewer's shape (nine `z=` pads, then `i=9,...,m=1`). a_kitty_control_block_is_cut_where_kitty_stops_reading pins the bound's edges, and hostile_input_cannot_panic_or_grow_a_buffer_past_its_bound now drives a control block to twice the bound and requires the peak to equal it. Both were seen to fail: `<=` for `<` on the bound trips "kitty control block overran", and leaving out the truncation reads `i=91` as `Some(9)`. SECURITY.md lists the new bound beside the 128-byte head. Closes #700 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Under `Graphics::Kitty`, `image_step` returned early on `payload.id()?` and the payload builder never read `I=`, so a placement that names its image by number (`a=p,I=7`) never moved the cursor, with or without `c=` and `r=`. kitty's `handle_put_command` looks the image up by `g->id` and otherwise by `img_by_client_number`, the newest image with that number, and places it. The protocol: commands that refer to an image by number "will act on only the newest image with that number". The placement now carries `I=` (`Placement::number`), and the log keeps it beside each payload (`Named`, which also holds the #672 `forgets` the log already kept in lockstep). A placement with a nonzero `i=` goes on as before; one with only a number is sized from the newest transmission with that number, or by `c=` and `r=`; one with neither places nothing in kitty ("Put graphics command without image id or number") and moves nothing here. `i=0` is no id, as in kitty's `if (g->id) ... else if (g->image_number)`: before, `a=p,i=0,c=2,r=3` was looked up as id 0 and moved by `c=`/`r=`. Before (with the lookup put back as it was), four tests failed: a_kitty_placement_by_image_number_moves_the_cursor `a=p,I=7,c=2,r=3` left: (0, 0) right: (2, 2) a_kitty_image_number_names_the_newest_image_sent_with_it left: (0, 0) right: (0, 1) a_kitty_placement_that_names_no_image_does_not_move `i=0,c=2,r=3` left: (2, 2) right: (0, 0) The first holds the issue's three rows (by id, by number with `c=`/`r=`, by number sized from `s=30,v=50`: (2,2), (2,2), (2,3), kitty's figures). The second pins which image a number names: the newest, even one that declared no size; not one sent under another number or under an id of the same value; read off an upload's first chunk; and `i=0,I=7` decided by the number. All pass now, as does the rest of the lib suite. Not done: a command naming both `i=` and `I=` is refused by kitty, and termlens still places it. LIMITATIONS already says so, and it is left for its own issue. Closes #696 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The gate that takes an APC for the kitty graphics protocol read `self.dcs_head.first()`, slot 0 of a fixed array, whatever `dcs_head_len` said, and `reset_dcs_scanner` zeroes only the length. An empty APC (`ESC _ ESC \`) after a kitty escape therefore found that escape's `G` still in slot 0 and was taken for a kitty escape. The gate now reads the first byte of `dcs_head[..dcs_head_len]`. Before, an_apc_is_kittys_only_by_its_own_first_byte failed on the empty APC after `a=d`: left: 2 right: 1 [kitty Delete ..., kitty Transmit ?px at (0, 0) rgba 0 bytes] and, with that half taken out, on the empty APC inside an upload, which was taken as the upload's last chunk and left the real last chunk to be counted as an image of its own: [kitty TransmitAndPlace 2x1px ... i=5 rgba 30 bytes in 2 chunks, kitty Transmit ?px at (0, 0) rgba 9 bytes] It passes now. The same test pins an APC whose first byte is `X`, and one that is `XG;AAAA`, after a kitty escape and inside an upload; those overwrote slot 0 and passed before too. Closes #735 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Under `Graphics::Kitty`, a placement whose last row is past the bottom
margin of a scroll region moved the cursor a line feed at a time. The
region scrolled by the overshoot, as in kitty, but the cursor stopped on
the margin. kitty 0.49.2's `screen_handle_graphics_command` (screen.c)
moves the cursor to the last row first, then scrolls the region by
`y - margin_bottom` without moving the cursor, then clamps it with
`screen_ensure_bounds(self, false, in_margins)`: to the region only
under DECOM and when the moved cursor was inside it (`in_margins` is
taken before the wrap to the next row), to the screen otherwise.
The protocol leaves this open ("the exact positioning of the cursor is
undefined, and up to implementations"); LIMITATIONS promises the cursor
"moves past an image as the declared terminal moves it", and the
decision was to follow kitty. After the line feeds, which scroll the
region by exactly kitty's count, a `VPA` puts the cursor on kitty's row
when the line feeds stopped short of it. vt100's `VPA` is an absolute
row clamped to the screen whatever the margins and origin mode say. The
tracker gains `origin_mode()` for the DECOM case.
kitty's quirk is not copied: with the cursor already below the region,
`y > margin_bottom` holds and kitty scrolls the region anyway. termlens
keeps the line feeds there, which scroll nothing and end on kitty's row.
Before (the `VPA` taken out), on 20x24 with `CSI 1;10 r`:
a_kitty_placement_past_a_scroll_region_leaves_the_cursor_where_kitty_does
row 7, `a=T,c=2,r=5`, then X: left: X at (9, 2) right: (11, 2)
a_kitty_placement_that_wraps_past_a_scroll_region_moves_as_kitty_does
on the margin at column 22, `c=2,r=1`: left: (9, 0) right: (10, 0)
The first is the issue's case (cursor (9,3) against kitty's (11,3)). Both
pass now. They also pin the region's scroll (MARK from row 2 to 0, text
below the region unmoved), origin mode with the last row outside the
region, a clamp to the screen's last row, a cursor starting inside the
region without overshoot and above a region that starts at row 4, the
no-scroll case below the region, and no region at all. Under DECOM the
wrap case stays on the margin, as kitty clamps it there; taking that
rule out fails the second test with (10, 0) for (9, 0).
The reviewers' probes (tests/kitty_margin.rs, through a real PTY, the
inside and below-region cases) pass against this branch.
LIMITATIONS says the three things: the protocol leaves it open, termlens
follows kitty, and not kitty's below-region scroll. DESIGN says how the
cursor gets there. A Changed entry, since a test may see the next text
land lower.
Closes #697
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
kitty 0.49.2 answers a transmission (no action, a=t, a=T) or a placement
(a=p) that names an image, i= or I=, through finish_command_response in
graphics.c: an OK once the image's data has loaded (at the last chunk of
an upload, addressed by the first chunk's keys), q=1 suppressing the OK
and q=2 every reply, and an EINVAL refusal for one naming both i= and I=
that only q=2 silences. That OK reports that the image loaded, which
termlens cannot know (it opens no file and decodes only with `decode`),
so, per AGENTS.md, no reply is sent. termlens sent nothing and recorded
nothing, so an application blocked on the reply timed out with no note.
Now the tracker carries such a command out as Query::Unanswerable, its
shape the escape that asked (an upload's first chunk), alongside its
payload (SeqEvent::GraphicsQuery), so the next timeout names it. One that
asked for quiet is not named, nor one without i=/I=, a delete, or an
upload a delete, sixel or RIS dropped. Frames (a=f) and composes (a=c),
which kitty also answers, are left out and LIMITATIONS says so.
An upload cut by a sixel or a RIS is undefined in the protocol ("the
client must finish sending all chunks for a single image before sending
any other graphics related escape codes"); termlens keeps dropping it,
now pinned, and LIMITATIONS says both.
Before: a_kitty_command_kitty_would_answer_is_named_unanswered failed
(left: [], right: ["^[_Gi=7,a=T,f=24,s=1,v=1;"]), and in tests/queries.rs
a_kitty_transmission_waiting_for_its_reply_is_named failed with "timed
out after 400ms while waiting for the screen predicate to hold" and no
note. Both pass now, as does
the_last_chunk_after_a_sixel_or_reset_is_no_part_of_the_upload, which
passed before too (it pins behaviour, not a fix).
a_kitty_query_that_never_reaches_the_loader_leaves_an_upload_alone now
also expects the upload's (i=5, no q=) reply named at its last chunk.
Closes #680
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
# Conflicts: # CHANGELOG.md
# 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.
Seven fixes to kitty and sixel capture and mouse input, one commit each.
Changed (a test can see them; each CHANGELOG entry has a migration sentence):
?1005) and urxvt (?1015) encodings: button3plus+4shift,+8alt,+16ctrl, as xterm'sBtnCodeand Windows Terminal send it. A ctrl+click under?1000was released as0x23where xterm sends0x33. SGR always kept them.docs/LIMITATIONS.mdpromises the cursor moves "as the declared terminal moves it", so termlens now follows kitty 0.49.2: the region scrolls by the overshoot and the cursor is left on the image's last row, below the margin, clamped to the screen (CSI 1;10 r, row 7,a=T,c=2,r=5: row 11, was 9). kitty's quirk of scrolling the region when the cursor starts below it is not followed, and LIMITATIONS says so.i=orI=) withOKunlessq=asks for quiet; that reply reports whether the image loaded, which termlens cannot know, and AGENTS.md's rule is to answer truthfully or name the query in the next timeout. termlens now names such a command in the timeout note, following kitty'sfinish_command_response:a=t,a=T,a=pand no action, an upload named at its last chunk by its first,q=1/q=2keeping it out. Nothing is sent on the wire.a=fanda=c, which kitty also answers, are not covered, and LIMITATIONS says so. An upload cut by a sixel or a RIS, which the protocol forbids the client to send mid-upload, keeps termlens's behaviour, now pinned by a test and documented. This settles both parts of Kitty capture: replies to transmissions with i=, and uploads across a sixel or RIS, differ from kitty #680.Fixed:
!0~,!~) paints nothing #698. A sixel repeat with a count of zero or none (!0~,!~) repeats once, as DEC, xterm and Windows Terminal read it; it painted nothing.m=1sat there ended the upload at once.a=p,I=n) never moves the cursor #696. A placement by image number (a=p,I=n) moves the cursor, byc=/r=or sized from the newest transmission with that number, as the protocol says commands naming a number do.i=0is no id, as in kitty.Gin.Each fix has a test that failed before it. main is merged in; the merged tree passes clippy, the library tests and the graphics, queries and input suites, and the agent's full preflight was 40 ok, 1 skipped (zizmor without pipx), 0 failed.
Closes #680
Closes #696
Closes #697
Closes #698
Closes #700
Closes #701
Closes #735