diff options
| author | user <user@clank> | 2026-08-15 18:44:07 +0200 |
|---|---|---|
| committer | user <user@clank> | 2026-08-15 18:44:07 +0200 |
| commit | 93ce0b645babd0851edb34701bbfffa5565daf65 (patch) | |
| tree | b402f934e5624a731fef4988d5d4e640766817fa | |
| parent | sl0ppty: watch the config directory even when it is not there yet (diff) | |
| download | slosh-93ce0b645babd0851edb34701bbfffa5565daf65.tar.gz slosh-93ce0b645babd0851edb34701bbfffa5565daf65.tar.xz slosh-93ce0b645babd0851edb34701bbfffa5565daf65.zip | |
sl0ppty: measure chrome in columns, not characters
Good catch: a two-column bell mark shifted the pane title row. Everything in
app.c that positions chrome -- title budgets, button placement, the hit rects
that have to agree with them -- came from a `cells()` that counted codepoints,
and a glyph drawn in two columns was booked as one, so every glyph after it
landed a column early.
Widths now come from lib-vt's own table, via ghostty_unicode_grapheme_width.
That is deliberately the same table the terminal uses for pane content, so
chrome and content cannot disagree about how wide something is, and it is
cluster-aware: an emoji with a variation selector is two codepoints and one
glyph. A wide cell claims its right half as a tail (width 0), which the
renderer already knew how to skip.
HANDOVER said this was a known trap and to dump the actual cells before
touching arithmetic, which is exactly how this went: the first assertion I
wrote passed against a deliberately broken width function, because it measured
the frame's right corner -- drawn at an absolute position, so it does not move
-- rather than the columns the mark claims. The test now reads the mark's own
colour runs and demands 1 column for a narrow mark and 2 for a wide one, and it
fails when the width function is stubbed back to 1.
Two things fell out on the way:
ghostty_unicode_grapheme_width measures only the *first* cluster and returns
the codepoints it consumed. My first loop ignored that and asked "did the width
change when I added the next codepoint?", which concludes that an entire ASCII
string is a single cluster -- the cheatsheet modal collapsed to ten columns
wide with the text clipped. Use the return value.
A cluster can also be longer than a cell holds: a ZWJ family emoji is 18 bytes
against 16. Both the cell writer and the config's mark fields (now 32 bytes)
clamp on a codepoint boundary, because snprintf cuts at a byte and half a
codepoint is invalid UTF-8 -- which showed up as a decoder error in a test far
from the cause, not as a short mark.
| -rw-r--r-- | HANDOVER.md | 29 | ||||
| -rw-r--r-- | src/app.c | 16 | ||||
| -rw-r--r-- | src/config.c | 27 | ||||
| -rw-r--r-- | src/config.h | 9 | ||||
| -rw-r--r-- | src/screen.c | 119 | ||||
| -rw-r--r-- | src/sl0ppty.h | 7 | ||||
| -rw-r--r-- | tests/test_wide_marks.py | 114 |
7 files changed, 284 insertions, 37 deletions
diff --git a/HANDOVER.md b/HANDOVER.md index 93a8a25..fba59d8 100644 --- a/HANDOVER.md +++ b/HANDOVER.md @@ -47,13 +47,28 @@ prefixes that arm after the pointer rests. Anything that arms on dwell and is so it looks intermittent, which is the worst way to be wrong. This caught me twice, for `edge:` and then `corner:`. -**Glyph width and glyph weight are different problems.** `screen_text` books -every chrome cell as one column. An emoji drawn two columns wide shifts the -whole row. Separately, a glyph that does not *fill* its cell donates that space -to the gap beside it and looks badly spaced next to one that does — the cells -can be provably even and still look wrong. Three attempts at the frame buttons -established this; they are plain ASCII now. If someone reports spacing, dump -the actual cells before touching any arithmetic. +**Glyph width and glyph weight are different problems.** Width is now handled: +`screen_text` and `cells()` ask lib-vt's own table +(`ghostty_unicode_grapheme_width`), the same one the terminal uses for pane +content, so a two-column bell mark claims two columns and the wide cell's +right half is a tail the diff skips. Set `bell_mark "🔔"` and the title row +stays aligned. What is *not* solved is weight: a glyph that does not fill its +cell donates that space to the gap beside it and looks badly spaced next to +one that does — the cells can be provably even and still look wrong. Three +attempts at the frame buttons established this; they are plain ASCII now. If +someone reports spacing, dump the actual cells before touching any arithmetic. + +Two traps found while fixing the width, both worth knowing before touching +this code again. `ghostty_unicode_grapheme_width` measures **only the first +cluster** and tells you how many codepoints it ate; a loop that ignores the +return value and instead asks "did the width change when I added the next +codepoint?" concludes that an entire ASCII string is one cluster, which +silently shrank the cheatsheet modal to ten columns. And a grapheme cluster +can be longer than the 16 bytes a cell holds (a ZWJ family emoji is 18), so +both the cell writer and the config's mark fields clamp on a codepoint +boundary — truncating mid-codepoint put invalid UTF-8 into the snapshot, which +surfaced as a decode error in a test rather than anything resembling the +cause. **The test harness is not a session, and the difference hid a real bug.** The server set `signal(SIGCHLD, SIG_IGN)`, which does not merely skip a handler — @@ -305,15 +305,13 @@ static tab_t *cur(app_t *a) { return &a->tabs[a->cur]; } * Always asked with the rect that is about to be registered as the hit, so a * thing that lights up and the thing that would be clicked cannot drift apart. * A pointer already carrying something is busy and lights nothing. */ -/* Display columns of a short chrome string. Counts codepoints, which is exact - * for what chrome draws: every glyph here is one column, and a mark wide - * enough not to be is the thing the config warns you about. */ -static uint16_t cells(const char *str) { - uint16_t n = 0; - for (const char *q = str; q && *q; q++) - if (((unsigned char)*q & 0xC0) != 0x80) n++; - return n; -} +/* Display columns of a chrome string, from the terminal's own width table. + * + * This counted codepoints until a two-column bell mark shifted a pane's title + * row: booking one column for a glyph drawn in two moves everything after it, + * and every length in this file -- title budgets, button positions, the hit + * rects that must agree with them -- is computed from here. */ +static uint16_t cells(const char *str) { return screen_cells(str); } static bool ptr_on(const app_t *a, uint16_t x, uint16_t y, uint16_t w, uint16_t h) { diff --git a/src/config.c b/src/config.c index 9473e34..d1b4eb6 100644 --- a/src/config.c +++ b/src/config.c @@ -268,6 +268,21 @@ static void bind_add(config_t *c, int key, uint16_t mods, action_t action, * `declared` is whether the config named `unfocused` itself, in which case it * has said what it wants -- including `states { unfocused { } }`, which says * "nothing", and must not be quietly refilled. */ +/* Copy a mark, cutting only where a codepoint ends. + * + * snprintf truncates at a byte, and a mark is a grapheme cluster: cutting one + * mid-codepoint puts bytes on the wire that are not UTF-8 at all, which shows + * up far from here as a garbled screen rather than a too-short mark. */ +static void set_mark(char *dst, size_t cap, const char *src) { + size_t n = strlen(src); + if (n >= cap) { + n = cap - 1; + while (n && ((unsigned char)src[n] & 0xC0) == 0x80) n--; + } + memcpy(dst, src, n); + dst[n] = 0; +} + static void apply_dim_unfocused(config_t *c, bool declared) { if (declared) return; if (!c->dim_unfocused) { @@ -923,15 +938,15 @@ bool config_load(config_t *c, const char *path, char *err, size_t errcap) { c->pane_buttons = kdl_arg_bool(kdl_child(root, "pane_buttons"), 0, c->pane_buttons); const char *zm = kdl_arg(kdl_child(root, "zoom_mark"), 0, NULL); - if (zm) snprintf(c->zoom_mark, sizeof c->zoom_mark, "%s", zm); + if (zm) set_mark(c->zoom_mark, sizeof c->zoom_mark, zm); const char *zo = kdl_arg(kdl_child(root, "zoom_on_mark"), 0, NULL); - if (zo) snprintf(c->zoom_on_mark, sizeof c->zoom_on_mark, "%s", zo); + if (zo) set_mark(c->zoom_on_mark, sizeof c->zoom_on_mark, zo); const char *cm = kdl_arg(kdl_child(root, "close_mark"), 0, NULL); - if (cm) snprintf(c->close_mark, sizeof c->close_mark, "%s", cm); + if (cm) set_mark(c->close_mark, sizeof c->close_mark, cm); const char *mm = kdl_arg(kdl_child(root, "min_mark"), 0, NULL); - if (mm) snprintf(c->min_mark, sizeof c->min_mark, "%s", mm); + if (mm) set_mark(c->min_mark, sizeof c->min_mark, mm); const char *nt = kdl_arg(kdl_child(root, "newtab_mark"), 0, NULL); - if (nt) snprintf(c->newtab_mark, sizeof c->newtab_mark, "%s", nt); + if (nt) set_mark(c->newtab_mark, sizeof c->newtab_mark, nt); { /* `commands` (the default), `all`, or `none`. `true`/`false` are taken as * `all`/`none`, because that is what they used to mean here and a config @@ -954,7 +969,7 @@ bool config_load(config_t *c, const char *path, char *err, size_t errcap) { c->bell_indicator = kdl_arg_bool(kdl_child(root, "bell_indicator"), 0, c->bell_indicator); const char *bm = kdl_arg(kdl_child(root, "bell_mark"), 0, NULL); - if (bm) snprintf(c->bell_mark, sizeof c->bell_mark, "%s", bm); + if (bm) set_mark(c->bell_mark, sizeof c->bell_mark, bm); const kdl_node_t *mins = kdl_child(root, "min_split"); if (mins) { diff --git a/src/config.h b/src/config.h index 8c01fdb..36e9915 100644 --- a/src/config.h +++ b/src/config.h @@ -110,16 +110,19 @@ typedef struct { * reads as even: a glyph that floats in a mostly-empty cell donates its own * whitespace to the gap beside it and looks further away than its * neighbours, which is what an en dash did here. */ - char zoom_mark[16], zoom_on_mark[16], close_mark[16], min_mark[16]; + /* 32 bytes because a mark is a grapheme cluster, not a character: a + family emoji is eighteen bytes on its own. Truncating one mid- + codepoint used to put invalid UTF-8 on the wire. */ + char zoom_mark[32], zoom_on_mark[32], close_mark[32], min_mark[32]; /* The new-tab button at the end of the strip. Drawn with a space each side, * so a one-character mark is still a three-cell target. */ - char newtab_mark[16]; + char newtab_mark[32]; bool bell_indicator; /* What to draw. A short string rather than a fixed glyph: the obvious * choice is an emoji bell, and emoji are drawn two columns wide by many * terminals while chrome here is booked as one — so the default is narrow * and anyone who knows their terminal can say otherwise. */ - char bell_mark[16]; + char bell_mark[32]; /* Which dead panes stay, showing what they printed and offering to run * again, and which just go. * diff --git a/src/screen.c b/src/screen.c index b7175bb..aa0faa3 100644 --- a/src/screen.c +++ b/src/screen.c @@ -3,6 +3,8 @@ #define _GNU_SOURCE #include "sl0ppty.h" +#include <ghostty/vt.h> + #include <stdarg.h> #include <stdio.h> #include <stdlib.h> @@ -114,27 +116,121 @@ cell_t *screen_at(screen_t *s, uint16_t x, uint16_t y) { return &s->cur[(size_t)y * s->cols + x]; } +static size_t u8_len(unsigned char b) { + if (b < 0x80) return 1; + if ((b & 0xe0) == 0xc0) return 2; + if ((b & 0xf0) == 0xe0) return 3; + if ((b & 0xf8) == 0xf0) return 4; + return 1; +} + +/* Decode UTF-8 into codepoints, bounded. Returns how many were written and, + * via `used`, how many bytes they came from -- the caller needs both to walk a + * string cluster by cluster. */ +static size_t decode_utf8(const char *txt, size_t len, uint32_t *cps, + size_t max, size_t *used) { + size_t n = 0, i = 0; + while (i < len && n < max) { + size_t l = u8_len((unsigned char)txt[i]); + if (l == 0 || i + l > len) break; + uint32_t cp; + switch (l) { + case 1: cp = (unsigned char)txt[i]; break; + case 2: cp = ((uint32_t)((unsigned char)txt[i] & 0x1f) << 6) | + ((unsigned char)txt[i + 1] & 0x3f); break; + case 3: cp = ((uint32_t)((unsigned char)txt[i] & 0x0f) << 12) | + ((uint32_t)((unsigned char)txt[i + 1] & 0x3f) << 6) | + ((unsigned char)txt[i + 2] & 0x3f); break; + default: cp = ((uint32_t)((unsigned char)txt[i] & 0x07) << 18) | + ((uint32_t)((unsigned char)txt[i + 1] & 0x3f) << 12) | + ((uint32_t)((unsigned char)txt[i + 2] & 0x3f) << 6) | + ((unsigned char)txt[i + 3] & 0x3f); break; + } + cps[n++] = cp; + i += l; + } + if (used) *used = i; + return n; +} + +/* How many columns the first grapheme cluster takes, asked of the same table + * the terminal uses (lib-vt's). Guessing 1 for everything is what made a + * two-column bell mark shift the rest of a pane's title row: the cell was + * booked as one column and drawn as two, and every glyph after it landed a + * column early. */ +uint8_t screen_width_of(const char *txt, size_t len) { + uint32_t cps[16]; + size_t n = decode_utf8(txt, len, cps, sizeof cps / sizeof *cps, NULL); + if (!n) return 1; + uint8_t w = 1; + ghostty_unicode_grapheme_width(cps, n, &w); + /* A zero-width cluster still owns the cell it was written into: chrome is + * laid out in cells, and a mark that claims no space would be overwritten + * by whatever comes next. */ + return w ? w : 1; +} + +/* Columns a whole string occupies. + * + * Cluster by cluster, using the codepoint count lib-vt reports as consumed -- + * summing per-codepoint widths is a different and wrong answer, because an + * emoji with a variation selector is two codepoints and one cluster. */ +uint16_t screen_cells(const char *txt) { + if (!txt) return 0; + uint32_t cps[256]; + uint16_t total = 0; + size_t len = strlen(txt), at = 0; + while (at < len) { + size_t used = 0; + size_t n = decode_utf8(txt + at, len - at, cps, sizeof cps / sizeof *cps, + &used); + if (!n) break; + for (size_t i = 0; i < n;) { + uint8_t w = 1; + size_t eaten = ghostty_unicode_grapheme_width(cps + i, n - i, &w); + if (!eaten) break; + i += eaten; + total = (uint16_t)(total + w); + } + at += used; + } + return total; +} + void screen_put_utf8(screen_t *s, uint16_t x, uint16_t y, const char *txt, size_t len, color_t fg, color_t bg, uint16_t attrs) { cell_t *c = screen_at(s, x, y); if (!c) return; - if (len > sizeof c->text) len = sizeof c->text; + /* Clamped on a codepoint boundary. A grapheme cluster can be longer than + * this cell holds -- a family emoji is eighteen bytes -- and cutting one in + * the middle of a codepoint puts invalid UTF-8 on the wire, which is worse + * than losing the tail of a cluster nobody can see anyway. */ + if (len > sizeof c->text) { + len = sizeof c->text; + while (len && ((unsigned char)txt[len] & 0xC0) == 0x80) len--; + } memset(c->text, 0, sizeof c->text); memcpy(c->text, txt, len); c->len = (uint8_t)len; - c->width = 1; + c->width = screen_width_of(txt, len); + /* The second half of a wide cell is a tail: never painted, and skipped by + * the diff. Without claiming it, the cell to the right keeps whatever was + * there and the terminal draws both. */ + if (c->width == 2) { + cell_t *tail = screen_at(s, (uint16_t)(x + 1), y); + if (tail) { + memset(tail, 0, sizeof *tail); + tail->fg = fg; + tail->bg = bg; + } else { + c->width = 1; /* no room for the tail: do not claim what is not there */ + } + } c->fg = fg; c->bg = bg; c->attrs = attrs; } -static size_t u8_len(unsigned char b) { - if (b < 0x80) return 1; - if ((b & 0xe0) == 0xc0) return 2; - if ((b & 0xf0) == 0xe0) return 3; - if ((b & 0xf8) == 0xf0) return 4; - return 1; -} uint16_t screen_text(screen_t *s, uint16_t x, uint16_t y, const char *txt, color_t fg, color_t bg, uint16_t attrs) { @@ -143,10 +239,11 @@ uint16_t screen_text(screen_t *s, uint16_t x, uint16_t y, const char *txt, size_t len = u8_len((unsigned char)*p); if (x + n >= s->cols) break; screen_put_utf8(s, (uint16_t)(x + n), y, p, len, fg, bg, attrs); + cell_t *c = screen_at(s, (uint16_t)(x + n), y); + n = (uint16_t)(n + (c && c->width ? c->width : 1)); p += len; - n++; } - return n; /* cells written, so a caller can lay out what follows */ + return n; /* columns written, so a caller can lay out what follows */ } static bool color_eq(color_t a, color_t b) { diff --git a/src/sl0ppty.h b/src/sl0ppty.h index e937e2b..cb916c5 100644 --- a/src/sl0ppty.h +++ b/src/sl0ppty.h @@ -59,7 +59,12 @@ void screen_clear(screen_t *s); cell_t *screen_at(screen_t *s, uint16_t x, uint16_t y); void screen_put_utf8(screen_t *s, uint16_t x, uint16_t y, const char *txt, size_t len, color_t fg, color_t bg, uint16_t attrs); -/* Write a UTF-8 string one cell per codepoint; returns cells written. */ +/* Columns one grapheme cluster occupies, from lib-vt's own width table. */ +uint8_t screen_width_of(const char *txt, size_t len); +/* Columns a whole string occupies. What chrome must budget with. */ +uint16_t screen_cells(const char *txt); +/* Write a UTF-8 string; returns the *columns* written, which is not the number + * of characters when any of them is wide. */ uint16_t screen_text(screen_t *s, uint16_t x, uint16_t y, const char *txt, color_t fg, color_t bg, uint16_t attrs); /* Diff cur against prev into s->out (the minimal byte stream for a terminal). */ diff --git a/tests/test_wide_marks.py b/tests/test_wide_marks.py new file mode 100644 index 0000000..c53df57 --- /dev/null +++ b/tests/test_wide_marks.py @@ -0,0 +1,114 @@ +#!/usr/bin/env python3 +"""Chrome is laid out in columns, and some glyphs are two of them. + +The bug: a bell mark like 🔔 is one character and two columns. Every length in +app.c came from counting characters, so the mark was booked as one column and +drawn as two, and everything after it on that row -- the title, the buttons, +the frame's right corner -- landed a column early. + +Assertions here are on *columns*, taken from style runs rather than the text +dump: the dump collapses a wide cell into one character, so measuring a python +string is measuring the wrong thing and would have passed before the fix. +""" +import json +import os +import subprocess +import sys +import tempfile + +from harness import BIN, check, report + +LAYOUT = """layout { + tab { + pane focus=true + pane command="printf '\\\\033]2;ringer\\\\007'; printf '\\\\a'; sleep 30" + } +} +""" + +NARROW = "\u2022" # one column +WIDE = "\U0001f514" # bell emoji, two columns +CLUSTER = "\U0001f468\u200d\U0001f469\u200d\U0001f467" # ZWJ family: 18 bytes + + +def run(mark, cols=100, rows=14): + lay = tempfile.NamedTemporaryFile("w", suffix=".kdl", delete=False) + lay.write(LAYOUT) + lay.close() + cfg = tempfile.NamedTemporaryFile("w", suffix=".kdl", delete=False) + # An unmistakable bell colour: the assertion is "how many columns did the + # mark claim", which means finding those columns without guessing. + cfg.write('bell_mark "%s"\ntheme { bell "#00ff00" }\n' % mark) + cfg.close() + p = subprocess.run([BIN, "--script", "--cols", str(cols), "--rows", str(rows), + "--layout", lay.name, "--", "/bin/sh", "-c", "read x"], + input=b"settle 250\npanes\nsnapshot json\n", + capture_output=True, env={**os.environ, + "SL0PPTY_CONFIG": cfg.name}) + os.unlink(lay.name) + os.unlink(cfg.name) + return p.stdout + + +def columns_claimed_by_the_mark(mark): + """How many columns of the title row the mark actually owns. + + Measured from where it is drawn (one column in from the pane's left edge) + to the first column that is not the bell colour. This is the number that + was wrong: the glyph was drawn in two columns and booked as one, so the + dashes, title and buttons after it all moved a column left.""" + lines = run(mark).decode().strip().split("\n") + panes, snap = json.loads(lines[0]), json.loads(lines[1]) + rang = [q for q in panes if not q["focused"]][0] + y, start = rang["y"], rang["x"] + 1 + runs = {r["x"]: r for r in snap["styles"] if r["y"] == y} + n, x = 0, start + while x in runs and runs[x].get("fg") == "#00ff00": + n += runs[x]["w"] + x += runs[x]["w"] + return n + + +def test_a_mark_claims_exactly_the_columns_it_draws_in(): + for label, mark, want in (("narrow", NARROW, 1), ("wide", WIDE, 2), + ("two characters", "!!", 2)): + got = columns_claimed_by_the_mark(mark) + check("%s mark claims %d column(s)" % (label, want), got == want, + "claimed %d" % got) + + +def test_the_mark_is_actually_drawn(): + out = run(WIDE).decode() + check("a wide mark reaches the screen", WIDE in out, "no bell mark on screen") + + +def test_no_mark_can_put_invalid_utf8_on_the_wire(): + """A mark is a grapheme cluster, not a character. The config stored it in a + fixed buffer with snprintf, which truncates at a byte: an 18-byte family + emoji came out as 15 bytes ending mid-codepoint, and every consumer of the + snapshot then had a decoding error instead of a screen.""" + for label, mark in (("wide", WIDE), ("18-byte cluster", CLUSTER)): + raw = run(mark) + try: + raw.decode("utf-8") + ok = True + except UnicodeDecodeError as e: + ok = False + detail = repr(raw[max(0, e.start - 20):e.start + 12]) + check("%s: output is valid utf-8" % label, ok, + detail if not ok else "") + + +def test_widths_come_from_the_terminals_own_table(): + """Not a table of our own: the same one lib-vt uses for pane content, so + chrome and content cannot disagree about how wide something is.""" + src = open(os.path.join(os.path.dirname(__file__), "..", "src", "screen.c")).read() + check("width comes from lib-vt", + "ghostty_unicode_grapheme_width" in src, + "screen.c is measuring text some other way") + + +for name, fn in sorted(list(globals().items())): + if name.startswith("test_"): + fn() +sys.exit(report()) |
