diff options
| author | Gabriel Schneider <[email protected]> | 2026-08-11 18:30:13 -0300 |
|---|---|---|
| committer | Gabriel Schneider <[email protected]> | 2026-08-12 13:04:32 -0300 |
| commit | 08f32cdde672740b71608f3dfccd629dcbac78f7 (patch) | |
| tree | 56d51eaf64df7a0bc53586e9550ba2be11d23263 | |
| parent | a28c3b71a917dcaae6ba4cd87b356eeab94462c4 (diff) | |
| download | pardes-08f32cdde672740b71608f3dfccd629dcbac78f7.tar.gz pardes-08f32cdde672740b71608f3dfccd629dcbac78f7.zip | |
fonts: the picker showed 512 faces on a machine with a thousand
max_fonts was 512 and this desktop has 1071 monospace faces installed. The
walk stopped at the cap, and because the sort runs AFTER the cut, the picker
did not look truncated -- it ran A to z with four hundred faces missing out of
the middle of it, which is a far worse way to be wrong than a short list.
The cap is now 4096 and the array comes from the arena rather than the stack:
4096 * {name, path} is 128 KiB, which is a fine thing to hand an arena that
resets at the end of the keystroke and not a thing to put on a call stack. It
was only ever a MEMORY bound anyway -- the work is bounded by max_steps, since
a face has to be walked past before it can be found -- and the comment now
says so instead of implying the number was about how long a list can be read.
Costs nothing measurable: the walk is what takes the time, not the four sfnt
reads per file. 512 faces warm was 31ms, 1071 is 36ms. (The 11s I first
measured was a cold page cache reading every font file on the disk once.)
Two guards, because the reason this went unnoticed is more interesting than
the off-by-a-cap:
- src/fonts.zig is imported behind `platform == .gui or .macos`, so on the tty
build nothing analyses it and zig collected no tests from it. It HAD tests;
they never ran. It now has its own libc-linked module in unit-test, which is
the hazard build.zig already writes down next to shell_bin.zig.
- a canary test asserting installed.len < max_fonts. Reaching the cap means
the list handed to the picker is a lie, and it should fail loudly rather
than quietly serve half a machine. Verified it fails at 512 and passes at
4096.
Verified in a real SDL window: SPC t f then a dump, 1092 rows in +Fonts.
unit-test 186/186 (5 of them newly reachable).
| -rw-r--r-- | build.zig | 13 | ||||
| -rw-r--r-- | src/fonts.zig | 42 |
2 files changed, 49 insertions, 6 deletions
@@ -1091,6 +1091,19 @@ pub fn build(b: *std.Build) void { .link_libc = true, }) }); unit_step.dependOn(&b.addRunArtifact(nested_test).step); + // fonts.zig is the same shape once more, and it needs its own module + // for the reason spelled out above rather than as a convention: the + // core imports it behind `platform == .gui or .macos`, so on this build + // nothing analyses it and its tests would silently not exist. That is + // how a picker capped at 512 faces on a machine with a thousand of them + // went unnoticed. + const fonts_test = b.addTest(.{ .root_module = b.createModule(.{ + .target = target, + .optimize = optimize, + .root_source_file = b.path("src/fonts.zig"), + .link_libc = true, + }) }); + unit_step.dependOn(&b.addRunArtifact(fonts_test).step); // crt.zig is pure std — the mouse-mapping-vs-shader-formula test const crt_test = b.addTest(.{ .root_module = b.createModule(.{ .target = target, diff --git a/src/fonts.zig b/src/fonts.zig index 28789e3d..e4744daf 100644 --- a/src/fonts.zig +++ b/src/fonts.zig @@ -39,12 +39,20 @@ const home_dirs = switch (builtin.os.tag) { else => [_][]const u8{ ".local/share/fonts", ".fonts" }, }; -/// The same three safety rails look.find has, for the same reason: this walk -/// runs INSIDE the keystroke that asked for it, so it must end whatever it is +/// The same safety rails look.find has, for the same reason: this walk runs +/// INSIDE the keystroke that asked for it, so it must end whatever it is /// pointed at. A font tree is shallow and wide (one directory per family), so -/// the depth cap is lower than find's and the file cap is what a picker can -/// still be read as a list. -const max_fonts = 512; +/// the depth cap is lower than find's. +/// +/// `max_fonts` is a MEMORY bound and nothing else — the work is already bounded +/// by `max_steps`, since a face has to be walked past before it can be found. +/// It used to be 512, which is not a machine-sized number: a desktop with the +/// Noto and Nerd Font sets installed has nearly a thousand monospace faces, and +/// the picker silently served the first 512 the directories happened to hand +/// back. Sorting happens AFTER the cut, so the loss did not even look like +/// truncation — the list ran A to z with half the fonts missing out of the +/// middle of it, which is a far worse way to be wrong than a short list. +const max_fonts = 4096; const max_steps = 20_000; const max_depth = 8; @@ -96,7 +104,12 @@ pub fn fallbacks(arena: std.mem.Allocator) []const Font { /// chain. `wanted == null` means every monospace face; named scans accept /// proportional symbol/general faces and preserve the requested priority. fn scan(arena: std.mem.Allocator, wanted: ?[]const []const u8) []const Font { - var found: [max_fonts]Font = undefined; + // ...from the arena, not the stack: 4096 faces of {name, path} is 128 KiB, + // which is a fine thing to hand back to an arena that resets at the end of + // the keystroke and not a thing to put on a call stack. An allocator that + // cannot spare it answers with the embedded face, like every other failure + // in here. + const found = arena.alloc(Font, max_fonts) catch return &.{}; var found_len: usize = 0; // Zig 0.16 moved the filesystem behind std.Io; the blocking // single-threaded implementation is the synchronous walk a sans-IO core @@ -210,6 +223,23 @@ test "fallback discovery is unique and preserves candidate priority" { } } +test "the picker lists every monospace face rather than the first max_fonts" { + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const installed = list(arena_state.allocator(), null); + // A canary, and only meaningful on a machine that HAS fonts: reaching the + // cap means the list handed to the picker is a silent lie, sorted after + // the cut so it reads as complete while missing faces out of the middle. + // That shipped once at max_fonts = 512 on a desktop with ~1000 of them. + try std.testing.expect(installed.len < max_fonts); + // ...and what it does return is sorted, which is what makes the picker the + // same list twice running. + for (installed, 0..) |font, i| { + try std.testing.expect(font.name.len != 0); + if (i > 0) try std.testing.expect(!std.mem.lessThan(u8, font.name, installed[i - 1].name)); + } +} + /// Is every glyph in this font the same width? The terminal grid IS a /// monospace cell — one advance for every column, chosen once from 'M' — so a /// proportional font does not render badly in it, it renders as rubble: every |
