Skip to content

on-terminal cannot say which terminal it is about: canvas.TerminalState carries no pty key, and the markup refuses an authored payload #224

Description

@sonhyrd

on-terminal cannot say which terminal it is about: canvas.TerminalState carries no pty key, and the markup refuses to let the app attach one. With N <terminal> widgets mounted at once, every state echo is anonymous.

The mechanism

The payload is canvas.TerminalState (src/primitives/canvas/terminal_grid.zig:172):

pub const TerminalState = struct {
    scrollback: u32 = 0,
    history: u32 = 0,
    cols: u16 = 0,
    rows: u16 = 0,
};

Four numbers, no identity. Two panes with the same geometry report byte-identical states.

And the markup will not let the app add one: on-terminal takes a bare tag only, and an authored payload is a hard error —

on-terminal takes a bare Msg tag whose payload is the post-change terminal view state (a canvas.TerminalState variant, ...)

(src/primitives/canvas/ui_markup.zig:1282; refused at ui_markup_view.zig:1935 and ui_markup_compiled.zig:1992, with a test pinning it — "on-terminal requires a bare tag: an authored payload is refused", ui_markup_view_tests.zig:2918). So on-terminal="term_state:{pty_key}" — the obvious workaround, and the shape every other handler in the markup accepts — is exactly the thing the schema rejects.

Which leaves no channel at all. The mounting shape this breaks on is the ordinary one:

<for each="panes" key="pane_id" as="p">
  <terminal pty="{p.pty_key}" grow="1" label="{'Shell ' ++ p.number}" />
</for>

Every one of those widgets constructs Msg{ .term_state = state } from a struct that is indistinguishable across panes.

The key is in hand at the dispatch site and dropped

This is what makes it look like an oversight rather than a design. WidgetLayoutNode carries the bound key — ui.zig:756, pty: u64, described there as "the MODEL-OWNED u64 effect key the app's ptySpawn named". And the wheel path reads it three times immediately before dispatching (src/runtime/ui_app.zig:5547-5556):

if (!self.terminal_sessions.hasSession(node.widget.terminal.pty)) return false;
if (self.terminal_sessions.wheel(node.widget.terminal.pty, input_event.delta_y)) {
    ...
    if (self.terminal_sessions.currentState(node.widget.terminal.pty)) |state| {
        if (tree.msgForTerminal(node.widget.id, state)) |msg| {

msgForTerminal(id, state) (ui.zig:1264) even takes the widget's ObjectId — it just does not pass it on, and that id is not the pty key the model reasons in anyway.

What it costs

A per-pane scrollback: u32 in the model is unwritable: update can never correctly attribute an echo to a pane, and a field it can never write is a lie in the model, so the field has to go. That much is survivable — each pane is its own widget and the runtime retains its offset across rebuilds, so the common case works without the app's help.

What is not survivable is anything where the app has to react to a specific pane's state: persisting per-pane scroll position, showing "scrolled up" chrome on the right pane's header, or driving a jump-to-bottom button. All of those need to know which pane just spoke.

The ask

Put the key in the payload:

pub const TerminalState = struct {
    /// The bound pty key (`<terminal pty={key}>`) this state belongs
    /// to — the same u64 the app's `ptySpawn` named. 0 on an unbound
    /// terminal.
    pty: u64 = 0,
    scrollback: u32 = 0,
    history: u32 = 0,
    cols: u16 = 0,
    rows: u16 = 0,
};

It is already available everywhere the state is built, it costs 8 bytes on a struct the app copies once per event, and terminalMsg (ui.zig:1125) needs no change at all — the tag's payload just grows a field.

One caveat found while trying it: translatedTerminalMsg's declared-record path (:1140) maps by name, but the structural matcher behind it pins an exact field count, so a naive addition silently un-matches every transpiled core's mirror. Admitting pty as an optional field keeps old cores on four and lets new ones declare five.

Passing the key as a second constructor argument instead would work equally well; putting it in the struct is just the smaller diff.

Verified against ea98365.

A patch is up: #221. Filed separately as the defect record — close whichever of the two is the redundant one.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions