Skip to main content

max / shop

kittygfx: add Place / FrameAppend / FrameCompose commands + require payload for a=T Extends kittygfx::Command with the three protocol commands kittybench's synthetic walker was silently dropping (a=p place in cursor / U=1 / virtual flavors, a=f frame append, a=c frame compose) and adds a Control.unicode_placeholder field for U=1. Rejects header-only a=T / a=t / a=f (no `;` separator), which the walker was over-accepting. Refactors accept_transmit -> accept_chunked returning (Control, Vec<u8>) so the caller wraps in the right Command variant based on the *stored* first-chunk action; a continuation chunk parses with the default action 'T', so branching on the incoming chunk's action mis-routed reassembled frames as Transmit. Adds seven kittygfx unit tests. Binary side gets a catch-all match arm — Place / Frame* are parsed but not yet rendered (follow-up in GO tasks 509fd8cb multi-image, e61fb2a1 animation). Coverage on kittybench's synthetic corpus: all 18 fixtures now 100%, up from 3-106% on the six shapes the task called out.
Co-Authored-By
Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-24 19:18 UTC
Signed with PGP, not checked
Commit: e9861de466a58aced2adf407baa8aea2c66cf3c1
Parent: 6752cce
2 files changed, +161 insertions, -24 deletions
M crates/kitty-graphics/src/lib.rs +155 -24
@@ -7,13 +7,14 @@
7 7 //!
8 8 //! [Kitty terminal graphics protocol]: https://sw.kovidgoyal.net/kitty/graphics-protocol/
9 9 //!
10 - //! MVP scope: `a=T` transmit+display, `a=t` transmit, `a=d` delete, formats
11 - //! `f=24` (RGB), `f=32` (RGBA), `f=100` (PNG), medium `t=d` (base64 inline),
12 - //! chunked payloads (`m=1`/`m=0`), placement in cells (`c=`, `r=`), don't-
13 - //! move-cursor (`C=1`).
10 + //! MVP scope: `a=T` transmit+display, `a=t` transmit, `a=p` place, `a=d`
11 + //! delete, `a=f` frame append, `a=c` frame compose, formats `f=24` (RGB),
12 + //! `f=32` (RGBA), `f=100` (PNG), medium `t=d` (base64 inline), chunked
13 + //! payloads (`m=1`/`m=0`), placement in cells (`c=`, `r=`), don't-move-
14 + //! cursor (`C=1`), Unicode-placeholder flag (`U=1`).
14 15 //!
15 - //! Not covered yet: file/temp/shm media, animations, virtual placements
16 - //! (Unicode placeholders), placement IDs, z-order, query.
16 + //! Not covered yet: file/temp/shm media, `a=a` animation control, `a=q`
17 + //! query, placement IDs, z-order, delete sub-selectors.
17 18 //!
18 19 //! Reference read: rio's `rio-backend/src/ansi/kitty_graphics_protocol.rs`
19 20 //! (MIT) — architecture consulted, no direct code copied.
@@ -63,6 +64,10 @@
63 64 pub cell_rows: Option<u32>,
64 65 /// Don't move cursor after placement (`C=1`).
65 66 pub no_cursor_move: bool,
67 + /// Unicode-placeholder placement flag (`U=1`) — image is meant to be
68 + /// positioned via placeholder characters in the text stream, not at the
69 + /// cursor.
70 + pub unicode_placeholder: bool,
66 71 /// `q=` quiet mode — 0 = verbose, 1 = suppress OK, 2 = suppress errors.
67 72 pub quiet: u8,
68 73 /// Set when the chunk carried `m=1` (more chunks follow).
@@ -81,9 +86,21 @@
81 86 control: Control,
82 87 payload: Vec<u8>,
83 88 },
89 + /// Place an already-transmitted image at the cursor or via a Unicode
90 + /// placeholder. Carries no payload — placement is metadata-only.
91 + Place { control: Control },
84 92 /// Delete images. MVP only supports "delete all placements" (`a=d,d=A`) —
85 93 /// the sub-selector variants are stored but not filtered.
86 94 Delete { control: Control },
95 + /// Append a frame to an animated image (`a=f`). Payload carries the
96 + /// frame's pixel data in the same encoding as a Transmit.
97 + FrameAppend {
98 + control: Control,
99 + payload: Vec<u8>,
100 + },
101 + /// Compose an already-transmitted frame from other frames (`a=c`).
102 + /// Carries no payload — composition is metadata-only.
103 + FrameCompose { control: Control },
87 104 }
88 105
89 106 /// Parser for kitty-graphics APC payloads. Feed one APC body at a time via
@@ -116,7 +133,7 @@
116 133
117 134 /// Consume one APC body (the bytes between `\e_G` and the ST/BEL
118 135 /// terminator). Bodies have the form `<control>;<base64-payload>` or
119 - /// `<control>` alone (delete/query with no payload).
136 + /// `<control>` alone (place/delete/compose with no payload).
120 137 pub fn feed(&mut self, body: &[u8]) -> Option<Command> {
121 138 // The protocol always uses `G` as the introducer. Callers may or may
122 139 // not have stripped it; tolerate both.
@@ -124,14 +141,35 @@
124 141
125 142 let (control_bytes, payload_b64) = split_once_byte(body, b';');
126 143 let control = parse_control(control_bytes)?;
127 - let payload = if payload_b64.is_empty() {
128 - Vec::new()
129 - } else {
130 - B64.decode(payload_b64).ok()?
144 +
145 + // Actions that carry image data require the `;` separator, even if
146 + // the payload after it is empty (chunk continuation). Reject header-
147 + // only forms of these — a strict, conformant parser wouldn't accept
148 + // an `a=T` with no payload marker.
149 + let needs_payload = matches!(control.action, 'T' | 't' | 'f');
150 + if needs_payload && payload_b64.is_none() {
151 + return None;
152 + }
153 +
154 + let payload = match payload_b64 {
155 + Some(b) if !b.is_empty() => B64.decode(b).ok()?,
156 + _ => Vec::new(),
131 157 };
132 158
133 159 match control.action {
134 - 'T' | 't' | 'a' => self.accept_transmit(control, payload),
160 + // Payload-carrying actions share the chunk-reassembly path. A
161 + // continuation chunk parses with the default action ('T'), so
162 + // route the completed command using the *stored* first-chunk
163 + // action, not the incoming chunk's action.
164 + 'T' | 't' | 'f' => {
165 + let (control, payload) = self.accept_chunked(control, payload)?;
166 + Some(match control.action {
167 + 'f' => Command::FrameAppend { control, payload },
168 + _ => Command::Transmit { control, payload },
169 + })
170 + }
171 + 'p' => Some(Command::Place { control }),
172 + 'c' => Some(Command::FrameCompose { control }),
135 173 'd' => Some(Command::Delete { control }),
136 174 _ => {
137 175 tracing::trace!("kitty-graphics: unhandled action {}", control.action);
@@ -140,7 +178,13 @@
140 178 }
141 179 }
142 180
143 - fn accept_transmit(&mut self, control: Control, payload: Vec<u8>) -> Option<Command> {
181 + /// Reassemble chunked payload for actions that carry image data.
182 + /// Returns `Some((control, payload))` when a transmission completes.
183 + fn accept_chunked(
184 + &mut self,
185 + control: Control,
186 + payload: Vec<u8>,
187 + ) -> Option<(Control, Vec<u8>)> {
144 188 let key = if let Some(id) = control.id {
145 189 PartialKey::ById(id)
146 190 } else if let Some(n) = control.number {
@@ -151,7 +195,7 @@
151 195
152 196 if !control.more_chunks && !self.partial.contains_key(&key) {
153 197 // Single-shot — no reassembly needed.
154 - return Some(Command::Transmit { control, payload });
198 + return Some((control, payload));
155 199 }
156 200
157 201 let entry = self.partial.entry(key).or_insert_with(|| Partial {
@@ -166,15 +210,18 @@
166 210 None
167 211 } else {
168 212 let Partial { control, payload } = self.partial.remove(&key)?;
169 - Some(Command::Transmit { control, payload })
213 + Some((control, payload))
170 214 }
171 215 }
172 216 }
173 217
174 - fn split_once_byte(bytes: &[u8], sep: u8) -> (&[u8], &[u8]) {
218 + /// Split at the first `sep`. Returns `(before, None)` when no `sep` byte is
219 + /// present — the caller uses that to distinguish "empty payload after `;`"
220 + /// from "no `;` at all".
221 + fn split_once_byte(bytes: &[u8], sep: u8) -> (&[u8], Option<&[u8]>) {
175 222 match bytes.iter().position(|&b| b == sep) {
176 - Some(i) => (&bytes[..i], &bytes[i + 1..]),
177 - None => (bytes, &[]),
223 + Some(i) => (&bytes[..i], Some(&bytes[i + 1..])),
224 + None => (bytes, None),
178 225 }
179 226 }
180 227
@@ -213,6 +260,7 @@
213 260 "c" => c.cell_cols = v.parse().ok(),
214 261 "r" => c.cell_rows = v.parse().ok(),
215 262 "C" => c.no_cursor_move = v == "1",
263 + "U" => c.unicode_placeholder = v == "1",
216 264 "q" => c.quiet = v.parse().unwrap_or(0),
217 265 "m" => match v {
218 266 "0" => c.last_chunk = true,
@@ -268,17 +316,28 @@
268 316 // ---- control-field parsing ---------------------------------------
269 317
270 318 #[test]
271 - fn parses_default_action_and_missing_semicolon() {
272 - // No `;` means control-only, empty payload; default action is 'T'.
319 + fn header_only_transmit_is_rejected() {
320 + // No `;` separator means no payload marker at all. A conformant
321 + // parser rejects `a=T` in this shape; earlier versions over-accepted
322 + // it as a valid header-only command.
323 + let mut p = Parser::new();
324 + assert!(p.feed(b"Gf=32,s=1,v=1").is_none());
325 + assert!(p.feed(b"Ga=T,f=32,s=1,v=1").is_none());
326 + assert!(p.feed(b"Ga=t,f=32,s=1,v=1").is_none());
327 + assert!(p.feed(b"Ga=f,f=32,s=1,v=1").is_none());
328 + }
329 +
330 + #[test]
331 + fn empty_payload_after_semicolon_is_accepted() {
332 + // The `;` separator is present but payload is empty — this is a
333 + // valid chunk-continuation shape.
273 334 let mut p = Parser::new();
274 335 let (control, payload) = transmit(
275 - p.feed(b"Gf=32,s=1,v=1")
276 - .expect("no chunk pending — must dispatch"),
336 + p.feed(b"Gf=32,s=1,v=1;")
337 + .expect("empty-payload transmit with `;` must dispatch"),
277 338 );
278 339 assert_eq!(control.action, 'T');
279 340 assert_eq!(control.format, Some(Format::Rgba));
280 - assert_eq!(control.width_px, Some(1));
281 - assert_eq!(control.height_px, Some(1));
282 341 assert!(payload.is_empty());
283 342 }
284 343
@@ -432,4 +491,76 @@
432 491 let cmd = p.feed(b"Ga=d,d=A").expect("must dispatch");
433 492 assert!(matches!(cmd, Command::Delete { .. }));
434 493 }
494 +
495 + // ---- place / frame-append / frame-compose ------------------------
496 +
497 + #[test]
498 + fn place_at_cursor_returns_place_command() {
499 + let mut p = Parser::new();
500 + let cmd = p
501 + .feed(b"Ga=p,i=1,c=8,r=4")
502 + .expect("place must dispatch header-only");
503 + let Command::Place { control } = cmd else {
504 + panic!("expected Place, got {cmd:?}");
505 + };
506 + assert_eq!(control.action, 'p');
507 + assert_eq!(control.id, Some(1));
508 + assert_eq!(control.cell_cols, Some(8));
509 + assert_eq!(control.cell_rows, Some(4));
510 + assert!(!control.unicode_placeholder);
511 + }
512 +
513 + #[test]
514 + fn place_unicode_placeholder_flag_is_parsed() {
515 + let mut p = Parser::new();
516 + let cmd = p
517 + .feed(b"Ga=p,U=1,i=2,q=2")
518 + .expect("virtual placement must dispatch");
519 + let Command::Place { control } = cmd else {
520 + panic!("expected Place, got {cmd:?}");
521 + };
522 + assert!(control.unicode_placeholder);
523 + assert_eq!(control.quiet, 2);
524 + assert_eq!(control.id, Some(2));
525 + }
526 +
527 + #[test]
528 + fn frame_append_dispatches_with_payload() {
529 + let mut p = Parser::new();
530 + let body = format!("Ga=f,f=32,s=1,v=1,i=5;{}", b64(b"FRAME"));
531 + let cmd = p.feed(body.as_bytes()).expect("frame append must dispatch");
532 + let Command::FrameAppend { control, payload } = cmd else {
533 + panic!("expected FrameAppend, got {cmd:?}");
534 + };
535 + assert_eq!(control.action, 'f');
536 + assert_eq!(control.id, Some(5));
537 + assert_eq!(payload, b"FRAME");
538 + }
539 +
540 + #[test]
541 + fn frame_append_reassembles_chunks() {
542 + let mut p = Parser::new();
543 + let a = format!("Ga=f,f=32,i=9,m=1;{}", b64(b"HEAD"));
544 + let b = format!("Gi=9,m=0;{}", b64(b"TAIL"));
545 + assert!(p.feed(a.as_bytes()).is_none());
546 + let cmd = p.feed(b.as_bytes()).expect("must dispatch on last chunk");
547 + let Command::FrameAppend { control, payload } = cmd else {
548 + panic!("expected FrameAppend, got {cmd:?}");
549 + };
550 + assert_eq!(control.id, Some(9));
551 + assert_eq!(payload, b"HEADTAIL");
552 + }
553 +
554 + #[test]
555 + fn frame_compose_dispatches_header_only() {
556 + let mut p = Parser::new();
557 + let cmd = p
558 + .feed(b"Ga=c,i=3,r=1,c=2")
559 + .expect("frame compose must dispatch header-only");
560 + let Command::FrameCompose { control } = cmd else {
561 + panic!("expected FrameCompose, got {cmd:?}");
562 + };
563 + assert_eq!(control.action, 'c');
564 + assert_eq!(control.id, Some(3));
565 + }
435 566 }
M crates/shop/src/main.rs +6
@@ -461,6 +461,12 @@
461 461 app.images.drop_all();
462 462 app.image_placement = None;
463 463 }
464 + // Multi-image placement + animation are parsed but not yet rendered
465 + // (shop tasks 509fd8cb, e61fb2a1). Dropping them here matches the
466 + // current single-image renderer.
467 + kgp::Command::Place { .. }
468 + | kgp::Command::FrameAppend { .. }
469 + | kgp::Command::FrameCompose { .. } => {}
464 470 }
465 471 }
466 472