85df18a0
Add corrections section to waystty implementation plan
a73x 2026-04-08 05:40
Addresses review feedback without rewriting the plan: Blockers fixed (C1-C2, C7-C10, C12-C13): - Zig 0.15 stdlib API sweep (GPA, ArrayList, io.getStdOut, fcntl flags) - Task 0.2/0.3 ordering (build.zig must exist before zig fetch --save) - @cImport moved out of function body - mmap defer-before-null-check fixed - String literal casts for fontconfig/xkbcommon C APIs - FT_Bitmap null check + negative pitch handling - execve plumbing via std.posix.execveZ - Task 2.1 path discovery via ls instead of find Missing coverage (C3-C6, C11): - New Task 2.8: wire effect callbacks (WRITE_PTY, TITLE_CHANGED, etc.) - New Task 3.2: SIGCHLD / child-exit detection (terminal now exits when shell exits) - New Task 7.6: cursor rendering (previously no visible cursor) - sRGB color math documented - Key repeat carries current modifier state Structure (C14-C15): - Guidance to split 6.7, 6.8, 7.2, 7.3 into sub-tasks ad-hoc - Drop unused protocol bindings from Task 5.1 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/docs/superpowers/plans/2026-04-07-waystty-implementation.md b/docs/superpowers/plans/2026-04-07-waystty-implementation.md index 725453f..1e12ab6 100644 --- a/docs/superpowers/plans/2026-04-07-waystty-implementation.md +++ b/docs/superpowers/plans/2026-04-07-waystty-implementation.md @@ -26,6 +26,232 @@ When in doubt, consult these external references: --- ## Corrections (apply while executing) This plan was reviewed after writing. The following corrections apply throughout — read them before starting, and apply them when you reach the relevant task. ### C1. Zig 0.15 stdlib API sweep Multiple tasks use pre-0.15 Zig idioms. Apply these substitutions **everywhere**: - **`std.heap.GeneralPurposeAllocator(.{}){}`** → use `std.heap.DebugAllocator(.{}){}` for debug builds, or `std.heap.smp_allocator` for release. The GPA was renamed to `DebugAllocator` in 0.15. - **`std.io.getStdOut().writer()`** → use `std.fs.File.stdout().writer(&buf)` with an explicit `[4096]u8` buffer. Writergate changed the writer API in 0.15. (Affects Task 3.1.) - **`std.ArrayList(T).init(alloc)`** → ArrayList is unmanaged by default in 0.15. Use `std.ArrayList(T){}` or `.empty` as the initial value. Methods take an allocator: `list.append(alloc, item)`, `list.deinit(alloc)`. (Affects Task 5.4, 5.5, 7.3.) - **`std.posix.fcntl(..., flags | @as(u32, @bitCast(std.posix.O{...})))`** → the `flags` returned by fcntl is wider than `u32` on 64-bit. Use `std.posix.O.NONBLOCK` via the typed wrapper, or bitcast through `@as(@TypeOf(flags), ...)`. (Affects Task 1.1.) ### C2. Task 0.2 / 0.3 ordering is wrong `zig fetch --save` requires a valid `build.zig` to exist. The order must be: 1. **Task 0.2 (revised)**: Write `build.zig.zon` with an empty `.dependencies = .{}` block and placeholder fingerprint `0x0`. Do NOT run `zig fetch` yet. 2. **Task 0.3**: Write `build.zig` and `src/main.zig`. 3. **Task 0.3 extra step**: Run `zig build` — it will fail with a fingerprint error. Paste the printed fingerprint value back into `build.zig.zon`. Run `zig build` again; it should succeed and print `waystty`. 4. **Task 0.2b (new step)**: Run the `zig fetch --save` commands to populate dependencies. Each will update `build.zig.zon` with real hashes. 5. **Task 0.2c**: Pin ghostty to a specific commit hash by editing the URL to `git+https://github.com/ghostty-org/ghostty#<commit-sha>`. The spec requires pinning. ### C3. Add Task 2.8 — wire effect callbacks The spec lists six required effect callbacks. The plan omits this step, which means `vim` cursor positioning will break (needs `WRITE_PTY`), titles won't update, and DA queries won't respond. Add this task after Task 2.7: **Task 2.8: Wire effect callbacks** - [ ] Register `WRITE_PTY` callback that writes bytes to a provided `*Pty` via a context pointer. **Mandatory** — without this, the terminal cannot respond to DSR or cursor-position queries. - [ ] Register `DEVICE_ATTRIBUTES` callback returning VT220 conformance. - [ ] Register `SIZE` callback that returns current cols/rows. - [ ] Register `XTVERSION` callback returning `"waystty"`. - [ ] Register `TITLE_CHANGED` callback that stores the new title in a shared string. `main.zig` reads this after each frame and calls `xdg_toplevel.setTitle` when it changes. - [ ] Register `COLOR_SCHEME` callback as a stub returning `false`. The exact Zig API for registering these callbacks must be determined from `example/zig-vt/src/main.zig`. The C API uses `ghostty_terminal_set(term, OPT, callback)`; the Zig module likely exposes these as fields on an options struct passed to `Terminal.init`, or as individual setter methods. Add a test that feeds a DSR query (`\x1b[6n`) and verifies the write-pty callback fires with a cursor-position response. ### C4. Add Task 3.2 — SIGCHLD / child-exit detection The plan's main loop checks `p.child_pid > 0`, but `child_pid` never changes after spawn — the loop runs forever after the shell exits. Fix: **Task 3.2: Detect child exit** - [ ] Add a `waitpidNonblocking` helper to `pty.zig`: ```zig pub fn isChildAlive(self: *Pty) bool { var status: c_int = 0; const rc = c.waitpid(self.child_pid, &status, c.WNOHANG); if (rc == 0) return true; // still running if (rc == self.child_pid) return false; // exited return true; // error or no state change } ``` - [ ] In the main loop (Task 7.3), replace `while (!window.should_close and p.child_pid > 0)` with `while (!window.should_close and p.isChildAlive())`. - [ ] When `Pty.read` returns `0` (not `error.WouldBlock` — a clean `0` means EOF), break out of the loop. Add this check in both Task 3.1's `runHeadless` (already handles it) and Task 7.3's main loop (currently doesn't). ### C5. Add Task 7.6 — cursor rendering The plan never draws a cursor. libghostty's render state exposes cursor position and visibility. Add after Task 7.5: **Task 7.6: Render cursor** - [ ] Query cursor x/y/visible from `render_state` after each `update`. - [ ] If visible, append one extra `Instance` to the instance buffer at the cursor cell with inverted fg/bg colors (or a hardcoded bright color). Use a special UV rect pointing to an all-1.0 region of the atlas (reserve a 1x1 white pixel at atlas `(0,0)` during `Atlas.init`). - [ ] Draw the cursor instance as part of the same draw call. Add the white-pixel reservation to Task 4.3: after `atlas.pixels` is zeroed, set `atlas.pixels[0] = 255` and reserve cursor UV coordinates at `(0, 0)` to `(1/width, 1/height)`. ### C6. sRGB color correctness (Task 6.5 + 6.7) Mixing sRGB-encoded colors with linear glyph alpha produces visually wrong text. Pick one: - **Option A (recommended)**: Change swapchain format from `.b8g8r8a8_srgb` to `.b8g8r8a8_unorm`, and manually linearize cell colors before passing to the shader (`pow(x, 2.2)` approximation, or a proper sRGB→linear function), then the shader's `mix` works in linear space. Output also needs to be gamma-corrected before write. - **Option B (simpler, slightly wrong)**: Keep sRGB swapchain. Do `mix` in sRGB space (visually acceptable for terminal text, even if mathematically wrong). Document the choice. The plan originally assumed B implicitly; the review correctly flagged it. Choose B for v1 simplicity and document the limitation. ### C7. Task 7.3 — fix `@cImport` in function body `@cImport` inside a function body is a compile error. Move the dlopen wrapper to module scope: ```zig // At top of main.zig (not inside main) const dl = @cImport({ @cInclude("dlfcn.h"); }); var vk_lib_handle: ?*anyopaque = null; fn vkGetInstanceProcAddr(instance: anytype, name: [*:0]const u8) ?*const fn () callconv(.C) void { if (vk_lib_handle == null) { vk_lib_handle = dl.dlopen("libvulkan.so.1", dl.RTLD_NOW); } const handle = vk_lib_handle orelse return null; const sym = dl.dlsym(handle, "vkGetInstanceProcAddr") orelse return null; const get_proc: *const fn (@TypeOf(instance), [*:0]const u8) callconv(.C) ?*const fn () callconv(.C) void = @ptrCast(@alignCast(sym)); return get_proc(instance, name); } ``` The exact signature vulkan-zig expects must still be matched; consult its examples. Key fix: cImport and `vk_lib_handle` live at module scope. ### C8. Keyboard event queue Task 5.4 uses `ArrayList(KeyboardEvent).init(alloc)`. Apply C1 (unmanaged ArrayList), plus: - Pre-reserve capacity of 64 at init so `append` rarely allocates in the event callback. - If `append` fails (OOM or otherwise), log via `std.log.err` rather than silently dropping — this is the only path for user input. ### C9. Task 5.4 mmap defer ordering + xkbcommon casts ```zig .keymap => |k| { if (k.format != .xkb_v1) return; const map_mem = c.mmap( null, k.size, c.PROT_READ, c.MAP_PRIVATE, k.fd, 0, ); if (map_mem == c.MAP_FAILED) { _ = c.close(k.fd); return; } defer _ = c.munmap(map_mem, k.size); defer _ = c.close(k.fd); const new_keymap = c.xkb_keymap_new_from_string( kb.xkb_ctx, @ptrCast(@alignCast(map_mem.?)), c.XKB_KEYMAP_FORMAT_TEXT_V1, c.XKB_KEYMAP_COMPILE_NO_FLAGS, ) orelse return; // ... }, ``` Also cast modifier name strings explicitly: ```zig .ctrl = c.xkb_state_mod_name_is_active(state, @ptrCast("Control"), c.XKB_STATE_MODS_EFFECTIVE) > 0, .shift = c.xkb_state_mod_name_is_active(state, @ptrCast("Shift"), c.XKB_STATE_MODS_EFFECTIVE) > 0, ``` Same for all `FcPatternAddString` string literals in Task 4.1. ### C10. Task 4.2 — FT_Bitmap nullability and negative pitch ```zig if (w > 0 and h > 0) { const buffer = bitmap.buffer orelse return error.FtBitmapNull; const pitch: i32 = bitmap.pitch; const abs_pitch: u32 = @intCast(@abs(pitch)); const top_down = pitch > 0; var y: u32 = 0; while (y < h) : (y += 1) { const src_y = if (top_down) y else (h - 1 - y); const src_row = buffer + @as(usize, src_y) * abs_pitch; const dst_row = pixels.ptr + @as(usize, y) * w; @memcpy(dst_row[0..w], src_row[0..w]); } } ``` ### C11. Task 5.5 — repeat events must carry modifiers `tickRepeat` creates synthetic events with `modifiers = .{}`. This breaks `Ctrl-c` repeat. Change `Keyboard` to track `current_mods: Modifiers` (updated in the `.modifiers` handler) and use it when constructing the synthetic repeat event. ### C12. Task 7.3 — child process `execve` plumbing Use `std.posix.execveZ` instead of `std.c.execve` directly — it handles the type plumbing correctly: ```zig if (pid == 0) { _ = c.setenv("TERM", "xterm-256color", 1); var argv = [_:null]?[*:0]const u8{ @ptrCast(opts.shell.ptr), null }; std.posix.execveZ(@ptrCast(opts.shell.ptr), &argv, std.c.environ) catch {}; std.process.exit(1); } ``` Assume the caller passes a `[:0]const u8` shell path (tighten `SpawnOptions.shell` to `[:0]const u8`). ### C13. Task 2.1 — path discovery Instead of `find`, use the deterministic path: ```bash ls .zig-cache/p/ | grep ghostty ``` If the ghostty dep is lazy and hasn't been fetched yet, run `zig build` first (Task 0.4 triggers it). Then: ```bash ls .zig-cache/p/<ghostty-hash>/example/zig-vt/src/ ``` ### C14. Split the huge tasks (6.7, 6.8, 7.2, 7.3) Each of these should be split into 4-6 sub-tasks with a build check after each sub-task. For example, Task 6.7 splits into: - 6.7a: Shader module creation - 6.7b: Vertex input description + binding - 6.7c: Rasterizer + multisampling + color blend - 6.7d: Pipeline layout + push constants - 6.7e: Final `createGraphicsPipelines` call and smoke-compile Task 7.2's Context is even larger — split into init (with one subsystem per sub-task: instance, device, swapchain, render pass, pipeline, descriptors, commands, sync, buffers, atlas) and deinit. When executing, the implementer should perform this split ad-hoc rather than following the monolithic steps as-written. ### C15. Drop unused protocol bindings (nit) Task 5.1 binds `wp_cursor_shape_manager_v1`, `wp_fractional_scale_manager_v1`, `wp_viewporter`, `wl_output`. The plan never uses them. For a truly minimal v1, drop them from the scanner block and re-add when actually implementing cursor shape / DPI scaling. --- ## Phase 0: Project Scaffolding ### Task 0.1: Initialize project and git