From f66c44502ef21100955a22387981beca5e458870 Mon Sep 17 00:00:00 2001 From: Luca Padovan Date: Tue, 25 Aug 2026 22:47:43 +0200 Subject: [PATCH 1/6] Give the mouse and the arrow keys to the program that asked for them Reported from htop in the popup: arrows did nothing and clicking its tabs did nothing. Two separate causes, both of which affect every pane and not only the popup. Arrow keys have two encodings. Normally an arrow is CSI-introduced ("\033[A"); once a program sets DECCKM, which is what ncurses does when it calls keypad(), it is SS3-introduced ("\033OA"), and that is the form terminfo tells the program to expect. Send the wrong one and the program does not see a near miss, it sees no arrow key at all. The emulator has tracked the mode all along and nothing ever read it back, so termdock always sent the CSI form. Encoded per pane rather than once, because with synchronized input two panes need not be in the same mode. The mouse was never forwarded to anything. internal/vt10x records that a program turned on reporting (modes 1000/1002/1003/1006) and there was no Write anywhere in mouse.go: every click, drag and wheel event was consumed by termdock's own gestures, so htop, vim and less never heard about the mouse in any pane. They do now, with the state a terminal mouse report needs and termdock did not have to keep before: tcell names no button when it reports a release, and SGR reports have to name the one being released, so the button that went down is remembered. Shift takes it back, which is tmux's convention. It has to exist: without it a pane running a full-screen program could not be focused, selected from or resized with the mouse at all. Two details worth pinning, and both are tested. tcell's button order and X11's cross over (tcell's Button2 is the right button, X11's 2 is the right one and 1 is the middle), which is the kind of mapping that gets written backwards and never noticed. And the pre-SGR encoding puts a coordinate in one byte biased by 32, so a click past column 223 cannot be expressed: it is dropped rather than reported at the wrong cell. Documented in the README and in the manual's keys page, since which of the two owns a click is something you have to be told. --- README.md | 15 ++ docs/keys.html | 26 +++ docs/pages.py | 25 +++ internal/core/appcursor_test.go | 81 ++++++++++ internal/core/core.go | 3 + internal/core/keys.go | 66 +++++++- internal/core/mouse.go | 9 ++ internal/core/mousefwd.go | 254 +++++++++++++++++++++++++++++ internal/core/mousefwd_test.go | 278 ++++++++++++++++++++++++++++++++ internal/core/popup.go | 2 +- 10 files changed, 751 insertions(+), 8 deletions(-) create mode 100644 internal/core/appcursor_test.go create mode 100644 internal/core/mousefwd.go create mode 100644 internal/core/mousefwd_test.go diff --git a/README.md b/README.md index 0d24624..79a137e 100644 --- a/README.md +++ b/README.md @@ -927,6 +927,21 @@ Enter with `Ctrl-B [`. From there: scrolls; scrolling back to the bottom exits it automatically. - Drag in copy-mode: selects text and copies it on release. +**Programs that want the mouse get the mouse.** htop, vim, less, lazygit +and anything else that asks the terminal to report mouse events receives +your clicks, drags and wheel directly, in a pane or in the popup: the +list above applies to a pane sitting at a shell prompt, not to one +running a program that has other plans for a click. termdock knows which +is which because it emulates the terminal itself, so it sees the program +turn reporting on. + +Hold **Shift** to take the mouse back for termdock. Shift-click focuses a +pane running a full-screen program, shift-drag selects its text or its +border, and shift-wheel scrolls the pane's scrollback instead of the +program's own list. That is tmux's convention, and the reason it needs +one: without it, a pane running htop could not be resized with the mouse +at all. + ### ๐ŸŽจ Pane titles and borders Every pane is framed by a thin border, with its title embedded in the top diff --git a/docs/keys.html b/docs/keys.html index 4e704c8..b716aaf 100644 --- a/docs/keys.html +++ b/docs/keys.html @@ -42,6 +42,7 @@

On this page

Windows Finding things Copy-mode + The mouse Session and everything else Rebinding @@ -112,6 +113,31 @@

Copy-mode

q or EscLeave. +

The mouse

+ +

At a shell prompt the mouse drives termdock: click a pane to focus it, +click-drag its content to select and copy, drag a border to resize, drag +a pane's title onto a window tab to move it there, click a tab to switch +window, and use the wheel to scroll that pane's scrollback.

+ +
+Programs that want the mouse get the mouse. +htop, vim, less, lazygit and anything else that asks the terminal to +report mouse events receives your clicks, drags and wheel directly, in a +pane or in the popup. termdock knows which is which because it emulates +the terminal itself, so it sees the program turn reporting on. +
+ +

Hold Shift to take the mouse back. Shift-click +focuses a pane running a full-screen program, shift-drag selects its text +or drags its border, and shift-wheel scrolls the pane's scrollback rather +than the program's own list. Without an escape hatch of this kind a pane +running htop could not be resized with the mouse at all, which is why +tmux has the same one.

+ +

The whole thing is off if you set mouse off; see +Configuration.

+

Session and everything else

diff --git a/docs/pages.py b/docs/pages.py index e2a8108..55e1eda 100644 --- a/docs/pages.py +++ b/docs/pages.py @@ -208,6 +208,31 @@
q or EscLeave.
+

The mouse

+ +

At a shell prompt the mouse drives termdock: click a pane to focus it, +click-drag its content to select and copy, drag a border to resize, drag +a pane's title onto a window tab to move it there, click a tab to switch +window, and use the wheel to scroll that pane's scrollback.

+ +
+Programs that want the mouse get the mouse. +htop, vim, less, lazygit and anything else that asks the terminal to +report mouse events receives your clicks, drags and wheel directly, in a +pane or in the popup. termdock knows which is which because it emulates +the terminal itself, so it sees the program turn reporting on. +
+ +

Hold Shift to take the mouse back. Shift-click +focuses a pane running a full-screen program, shift-drag selects its text +or drags its border, and shift-wheel scrolls the pane's scrollback rather +than the program's own list. Without an escape hatch of this kind a pane +running htop could not be resized with the mouse at all, which is why +tmux has the same one.

+ +

The whole thing is off if you set mouse off; see +Configuration.

+

Session and everything else

diff --git a/internal/core/appcursor_test.go b/internal/core/appcursor_test.go new file mode 100644 index 0000000..b5d9eff --- /dev/null +++ b/internal/core/appcursor_test.go @@ -0,0 +1,81 @@ +package core + +import ( + "testing" + + "github.com/gdamore/tcell/v2" +) + +// An arrow key has two encodings, and which one is right depends on the +// program: CSI ("\033[A") normally, SS3 ("\033OA") once the program sets +// DECCKM, which is what ncurses does when it calls keypad(). Sending the +// wrong one is not a near miss, the program does not see an arrow key at +// all. termdock tracked the mode but never consulted it, so arrows did +// nothing in htop. +func TestArrowsFollowApplicationCursorMode(t *testing.T) { + for _, tc := range []struct { + key tcell.Key + normal string + appCursor string + }{ + {tcell.KeyUp, "\x1b[A", "\x1bOA"}, + {tcell.KeyDown, "\x1b[B", "\x1bOB"}, + {tcell.KeyRight, "\x1b[C", "\x1bOC"}, + {tcell.KeyLeft, "\x1b[D", "\x1bOD"}, + {tcell.KeyHome, "\x1b[H", "\x1bOH"}, + {tcell.KeyEnd, "\x1b[F", "\x1bOF"}, + } { + if got := string(keyBytes(tc.key, 0, false)); got != tc.normal { + t.Errorf("key %v without DECCKM = %q, want %q", tc.key, got, tc.normal) + } + if got := string(keyBytes(tc.key, 0, true)); got != tc.appCursor { + t.Errorf("key %v with DECCKM = %q, want %q", tc.key, got, tc.appCursor) + } + } +} + +// Everything else is unaffected by the mode: only the cursor keys and +// Home/End move to SS3, and quietly changing anything else would break +// programs that are working today. +func TestOnlyCursorKeysChangeWithTheMode(t *testing.T) { + for _, key := range []tcell.Key{ + tcell.KeyEnter, tcell.KeyTab, tcell.KeyEsc, tcell.KeyBackspace2, + tcell.KeyPgUp, tcell.KeyPgDn, tcell.KeyDelete, tcell.KeyInsert, + tcell.KeyF1, tcell.KeyF5, + } { + plain, app := keyBytes(key, 0, false), keyBytes(key, 0, true) + if string(plain) != string(app) { + t.Errorf("key %v changed with DECCKM: %q vs %q", key, plain, app) + } + } + if got := string(keyBytes(tcell.KeyRune, 'x', true)); got != "x" { + t.Errorf("a plain rune became %q under DECCKM", got) + } +} + +// And the mode has to be read from the pane the keys are going to, not +// assumed. This drives it the way a program does, through the emulator. +func TestAppCursorModeReadsThePanesTerminal(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + p := c.panes[c.win().active.ID] + if appCursorMode(p) { + t.Fatal("a fresh pane should not be in application cursor mode") + } + + p.Term().Write([]byte("\x1b[?1h")) // DECCKM set, as ncurses' smkx does + if !appCursorMode(p) { + t.Error("after \\033[?1h the pane should be in application cursor mode") + } + + p.Term().Write([]byte("\x1b[?1l")) // and rmkx on the way out + if appCursorMode(p) { + t.Error("after \\033[?1l the pane should be back in normal cursor mode") + } + + if appCursorMode(nil) { + t.Error("a nil pane should not report application cursor mode") + } +} diff --git a/internal/core/core.go b/internal/core/core.go index a962b23..135c577 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -187,6 +187,9 @@ type Core struct { contentPress *contentPressState titleDrag *titleDragState closedPanes []closedPane // undo stack behind Ctrl-B Z; see undoclose.go + // mouseFwd tracks a button held down in a pane whose program asked + // for mouse reporting; see mousefwd.go. + mouseFwd mouseForwardState // lastTabs is the window tab strip as the last Frame laid it out, so // a click resolves against what is on screen rather than a strip // re-derived after the labels have moved on. See tabAt. diff --git a/internal/core/keys.go b/internal/core/keys.go index 12a5465..cd533c8 100644 --- a/internal/core/keys.go +++ b/internal/core/keys.go @@ -6,6 +6,7 @@ import ( "github.com/padovanl/termdock/internal/layout" "github.com/padovanl/termdock/internal/pane" "github.com/padovanl/termdock/internal/proto" + "github.com/padovanl/termdock/internal/vt10x" "github.com/gdamore/tcell/v2" ) @@ -329,22 +330,49 @@ func (c *Core) requestQuit() { } func (c *Core) forwardKey(key tcell.Key, r rune) { - b := keyBytes(key, r) - if b == nil { - return - } + // Encoded per pane rather than once: which bytes an arrow key is + // depends on whether the program in that pane asked for application + // cursor keys, and with synchronized input the panes need not agree + // (one running vim, another sitting at a prompt). w := c.win() + send := func(p *pane.Pane) { + if b := keyBytes(key, r, appCursorMode(p)); b != nil { + p.Write(b) + } + } if w.syncPanes { for _, l := range c.broadcastTargets(w) { if p, ok := c.panes[l.ID]; ok { - p.Write(b) + send(p) } } return } if p, ok := c.panes[w.active.ID]; ok { - p.Write(b) + send(p) + } +} + +// appCursorMode reports whether the program running in p has switched +// the cursor keys into application mode (DECCKM, "\033[?1h"). +// +// It matters because in that mode an arrow key is SS3-introduced +// ("\033OA") rather than CSI-introduced ("\033[A"), and that is the form +// ncurses looks up in terminfo once a program calls keypad(). termdock +// tracked the mode in its emulator but never consulted it when sending +// keys, so it always sent the CSI form: arrow keys did nothing in htop, +// and in anything else that turns the mode on. +// +// Locked here, per this package's convention with vt10x accessors (see +// paneHistory): the mode is written by the pane's pump goroutine. +func appCursorMode(p *pane.Pane) bool { + if p == nil { + return false } + t := p.Term() + t.Lock() + defer t.Unlock() + return t.Mode()&vt10x.ModeAppCursor != 0 } // handleConfirmKey answers a pending confirm prompt (see @@ -382,7 +410,31 @@ func (c *Core) handleResizeKey(key tcell.Key, r rune) { c.relayoutLocked() } -func keyBytes(key tcell.Key, r rune) []byte { +// keyBytes is the byte sequence a key sends to the program in a pane. +// +// appCursor selects between the two encodings the cursor keys have: +// CSI ("\033[A") in the default mode, SS3 ("\033OA") once a program has +// set DECCKM. Passing the wrong one is not a near miss โ€” the program +// simply does not recognise the key. See appCursorMode. +func keyBytes(key tcell.Key, r rune, appCursor bool) []byte { + if appCursor { + // Home and End move with the arrows: xterm sends all six through + // SS3 in application mode, and terminfo's smkx entries expect it. + switch key { + case tcell.KeyUp: + return []byte("\x1bOA") + case tcell.KeyDown: + return []byte("\x1bOB") + case tcell.KeyRight: + return []byte("\x1bOC") + case tcell.KeyLeft: + return []byte("\x1bOD") + case tcell.KeyHome: + return []byte("\x1bOH") + case tcell.KeyEnd: + return []byte("\x1bOF") + } + } switch key { case tcell.KeyRune: return []byte(string(r)) diff --git a/internal/core/mouse.go b/internal/core/mouse.go index 343ce44..ca0c71f 100644 --- a/internal/core/mouse.go +++ b/internal/core/mouse.go @@ -48,6 +48,15 @@ func (c *Core) handleMouse(m proto.ClientMsg) Result { return Result{} } + // Before any of termdock's own gestures: a pane whose program asked + // for mouse reporting owns the mouse inside itself, wheel included โ€” + // scrolling htop's list is the program's job, not the scrollback's. + // See mousefwd.go, and shift-click to take it back. + if c.forwardMouse(m) { + c.markDirty() + return Result{} + } + // Wheel-scrolling only makes sense over the normal pane layout or // while already scrolled back in copy-mode; with the popup or // overview covering the screen, there's no window underneath for it diff --git a/internal/core/mousefwd.go b/internal/core/mousefwd.go new file mode 100644 index 0000000..2572b12 --- /dev/null +++ b/internal/core/mousefwd.go @@ -0,0 +1,254 @@ +package core + +import ( + "fmt" + + "github.com/gdamore/tcell/v2" + + "github.com/padovanl/termdock/internal/pane" + "github.com/padovanl/termdock/internal/proto" + "github.com/padovanl/termdock/internal/vt10x" +) + +// Forwarding the mouse to the program in a pane. +// +// termdock has always used the mouse for its own gestures: click to +// focus, drag a divider, drag a title onto a window tab, drag to select +// text. That is right for a shell prompt, which has no use for a click. +// It is wrong for htop, vim, less or anything else that asks the +// terminal to report mouse events, because those programs then never +// hear about a click at all: the emulator recorded that the program had +// turned reporting on (see internal/vt10x, modes 1000/1002/1003/1006) +// and nothing ever read that back. Clicking htop's tabs did nothing, in +// a pane or in the popup. +// +// So: a pane whose program asked for the mouse gets the mouse, and +// holding shift takes it back for termdock, which is the escape hatch +// tmux uses and the one people already know. + +// mouseForwardState remembers what the button was doing, because a +// terminal mouse report is a state machine and a single event does not +// say enough on its own. tcell reports a release as "no buttons", naming +// no button, and SGR reports have to name the button being released; and +// a drag is only distinguishable from a press by whether the button was +// already down. +type mouseForwardState struct { + down bool + button int // the X11 button code currently held + col int // last reported cell, to avoid repeating identical motion + row int +} + +// wantsMouse reports whether the program in p has asked for mouse +// reporting at all. +func wantsMouse(p *pane.Pane) bool { + if p == nil { + return false + } + t := p.Term() + t.Lock() + defer t.Unlock() + return t.Mode()&vt10x.ModeMouseMask != 0 +} + +// mouseModes reads the two things the encoding depends on: whether the +// program wants SGR-style reports (mode 1006) and whether it wants +// motion while a button is held (1002/1003). +func mouseModes(p *pane.Pane) (sgr, motion bool) { + if p == nil { + return false, false + } + t := p.Term() + t.Lock() + defer t.Unlock() + m := t.Mode() + return m&vt10x.ModeMouseSgr != 0, m&(vt10x.ModeMouseMotion|vt10x.ModeMouseMany) != 0 +} + +// buttonCode maps tcell's button mask to the X11 code a terminal report +// uses. The orders differ, which is an easy thing to get backwards: +// tcell's Button2 is the *right* button while X11's 2 is the right one +// and 1 is the middle, so they cross over. +func buttonCode(b tcell.ButtonMask) (int, bool) { + switch { + case b&tcell.Button1 != 0: // left + return 0, true + case b&tcell.Button3 != 0: // tcell's middle + return 1, true + case b&tcell.Button2 != 0: // tcell's right + return 2, true + case b&tcell.WheelUp != 0: + return 64, true + case b&tcell.WheelDown != 0: + return 65, true + case b&tcell.WheelLeft != 0: + return 66, true + case b&tcell.WheelRight != 0: + return 67, true + } + return 0, false +} + +// modifierBits are the modifier flags a report carries alongside the +// button. Shift is deliberately absent: it is termdock's escape hatch, +// so a shifted click never reaches the program to begin with and saying +// "shift was held" about an event the program is not receiving would be +// meaningless. +func modifierBits(mod tcell.ModMask) int { + bits := 0 + if mod&tcell.ModAlt != 0 { + bits |= 8 + } + if mod&tcell.ModCtrl != 0 { + bits |= 16 + } + return bits +} + +// maxLegacyCoord is the highest cell a pre-SGR report can address: the +// coordinate is sent as a single byte biased by 32, so 223 is the end of +// the road. Programs that care about wider terminals ask for SGR (1006), +// which has no such limit; for the ones that do not, a click past the +// limit is dropped rather than reported at a wrong cell. +const maxLegacyCoord = 223 + +// mouseReport encodes one event. col and row are zero-based within the +// pane; the wire format is one-based. +func mouseReport(sgr bool, code, col, row int, release bool) []byte { + if col < 0 || row < 0 { + return nil + } + if sgr { + final := 'M' + if release { + final = 'm' + } + return []byte(fmt.Sprintf("\x1b[<%d;%d;%d%c", code, col+1, row+1, final)) + } + // The legacy encoding cannot name the button being released, so every + // release is button 3. This is why SGR exists. + if release { + code = 3 + } + if col > maxLegacyCoord || row > maxLegacyCoord || code > 255-32 { + return nil + } + return []byte{0x1b, '[', 'M', byte(32 + code), byte(32 + col + 1), byte(32 + row + 1)} +} + +// forwardMouse hands the event to the program in the pane under the +// pointer, and reports whether it did. False means termdock should treat +// the event as its own gesture, which is the answer for a plain shell +// prompt, for a shift-held click, and for everything outside a pane. +func (c *Core) forwardMouse(m proto.ClientMsg) bool { + if c.mode != ModeNormal && c.mode != ModePopup { + return false // termdock's own modal UI owns the mouse while it is up + } + mod := tcell.ModMask(m.MouseMod) + if mod&tcell.ModShift != 0 { + // The escape hatch: shift-click always drives termdock, so a pane + // running a full-screen program can still be focused, selected + // from, and resized. + return false + } + + target, originX, originY := c.mouseTarget(m.MouseX, m.MouseY) + if target == nil || !wantsMouse(target) { + return false + } + + buttons := tcell.ButtonMask(m.MouseButtons) + col, row := m.MouseX-originX, m.MouseY-originY + sgr, motion := mouseModes(target) + + switch { + case buttons == tcell.ButtonNone: + if !c.mouseFwd.down { + return false // a release we never saw the press for + } + code := c.mouseFwd.button + modifierBits(mod) + c.mouseFwd = mouseForwardState{} + return writeReport(target, mouseReport(sgr, code, col, row, true)) + + case buttons&(tcell.WheelUp|tcell.WheelDown|tcell.WheelLeft|tcell.WheelRight) != 0: + code, ok := buttonCode(buttons) + if !ok { + return false + } + // A wheel report is a press with no matching release, so it must + // not disturb whatever the buttons are doing. + return writeReport(target, mouseReport(sgr, code+modifierBits(mod), col, row, false)) + + case c.mouseFwd.down: + // Still held: this is a drag. Only programs that asked for motion + // want to hear about it, and only when the cell actually changed, + // since tcell repeats the event as the pointer moves within a cell. + if !motion || (col == c.mouseFwd.col && row == c.mouseFwd.row) { + return true // still ours to swallow: the button is down in their pane + } + c.mouseFwd.col, c.mouseFwd.row = col, row + code := c.mouseFwd.button + 32 + modifierBits(mod) + return writeReport(target, mouseReport(sgr, code, col, row, false)) + + default: + code, ok := buttonCode(buttons) + if !ok { + return false + } + // Clicking a pane still focuses it, even though the click then + // belongs to the program: otherwise clicking into a pane running + // htop would leave the keyboard pointed somewhere else. + if leaf := c.leafAt(m.MouseX, m.MouseY); leaf != nil && c.mode == ModeNormal { + c.setActive(leaf) + } + c.mouseFwd = mouseForwardState{down: true, button: code, col: col, row: row} + return writeReport(target, mouseReport(sgr, code+modifierBits(mod), col, row, false)) + } +} + +// writeReport sends the encoded bytes, reporting whether there were any. +// An event the encoding cannot express (a click past column 223 without +// SGR) is dropped rather than sent at the wrong cell, and dropping it is +// still "handled": the program owns that pane, and quietly running +// termdock's own gesture instead would be worse than nothing happening. +func writeReport(p *pane.Pane, b []byte) bool { + if b != nil { + p.Write(b) + } + return true +} + +// mouseTarget is the pane under (x, y) and the screen position of that +// pane's top-left content cell, which is what turns an absolute click +// into the pane-relative cell a report names. The pane is nil when the +// point is on a border, on the status bar, or outside the popup while +// the popup is up. +// +// Only the origin is returned rather than a rectangle: the two rect +// types in play here (layout.Rect for a pane, proto.Rect for the popup) +// are different types, and the offset is the only part either one is +// needed for. +func (c *Core) mouseTarget(x, y int) (p *pane.Pane, originX, originY int) { + if c.mode == ModePopup { + if !c.popupFits() { + return nil, 0, 0 + } + r := c.popupRect() + // The popup draws a border of its own, so the program inside it + // starts one cell in from the box. + x0, y0 := r.X+1, r.Y+1 + if x < x0 || x >= x0+r.W-2 || y < y0 || y >= y0+r.H-2 { + return nil, 0, 0 + } + return c.popup, x0, y0 + } + leaf := c.leafAt(x, y) + if leaf == nil { + return nil, 0, 0 + } + pane, ok := c.panes[leaf.ID] + if !ok { + return nil, 0, 0 + } + return pane, leaf.Rect.X, leaf.Rect.Y +} diff --git a/internal/core/mousefwd_test.go b/internal/core/mousefwd_test.go new file mode 100644 index 0000000..f437b32 --- /dev/null +++ b/internal/core/mousefwd_test.go @@ -0,0 +1,278 @@ +package core + +import ( + "testing" + + "github.com/gdamore/tcell/v2" + + "github.com/padovanl/termdock/internal/layout" + + "github.com/padovanl/termdock/internal/proto" +) + +// enableMouse makes a pane's program ask for mouse reporting, the way +// htop or vim does, by driving the emulator with the real sequences. +func enableMouse(t *testing.T, c *Core, id int, seq string) { + t.Helper() + c.panes[id].Term().Write([]byte(seq)) +} + +// paneOutput drains what termdock wrote back towards the program. The +// pane's pty echoes, so reading the emulator is the wrong place to look; +// this reads the bytes the pane was asked to write. +func fwdMsg(x, y int, buttons tcell.ButtonMask, mod tcell.ModMask) proto.ClientMsg { + return proto.ClientMsg{ + Kind: "mouse", MouseX: x, MouseY: y, + MouseButtons: int32(buttons), MouseMod: int32(mod), + } +} + +// The encoding is the part a program actually parses, so it is worth +// pinning exactly rather than testing that "something" was sent. +func TestMouseReportEncoding(t *testing.T) { + for _, tc := range []struct { + name string + sgr bool + code, col, row int + release bool + want string + }{ + {"legacy press, top-left", false, 0, 0, 0, false, "\x1b[M\x20\x21\x21"}, + {"legacy press, further in", false, 0, 9, 4, false, "\x1b[M\x20\x2a\x25"}, + // The legacy form cannot name which button was released. + {"legacy release is always button 3", false, 2, 0, 0, true, "\x1b[M\x23\x21\x21"}, + {"sgr press keeps the button", true, 2, 9, 4, false, "\x1b[<2;10;5M"}, + {"sgr release keeps the button, lowercase m", true, 2, 9, 4, true, "\x1b[<2;10;5m"}, + {"sgr wheel up", true, 64, 0, 0, false, "\x1b[<64;1;1M"}, + } { + got := string(mouseReport(tc.sgr, tc.code, tc.col, tc.row, tc.release)) + if got != tc.want { + t.Errorf("%s: got %q, want %q", tc.name, got, tc.want) + } + } + + // Past the byte-encodable range the legacy form must drop the event + // rather than report a wrong cell; SGR has no such limit. + if got := mouseReport(false, 0, maxLegacyCoord+1, 0, false); got != nil { + t.Errorf("legacy report past column %d returned %q, want nothing", maxLegacyCoord, got) + } + if got := mouseReport(true, 0, 999, 999, false); string(got) != "\x1b[<0;1000;1000M" { + t.Errorf("sgr past the legacy limit = %q, want it encoded anyway", got) + } +} + +// tcell's button order and X11's cross over, which is exactly the sort +// of mapping that gets written backwards and never noticed. +func TestButtonCodesMatchX11NotTcellOrder(t *testing.T) { + for _, tc := range []struct { + mask tcell.ButtonMask + want int + what string + }{ + {tcell.Button1, 0, "left"}, + {tcell.Button3, 1, "middle (tcell calls it Button3, X11 calls it 1)"}, + {tcell.Button2, 2, "right (tcell calls it Button2, X11 calls it 2)"}, + {tcell.WheelUp, 64, "wheel up"}, + {tcell.WheelDown, 65, "wheel down"}, + } { + got, ok := buttonCode(tc.mask) + if !ok || got != tc.want { + t.Errorf("%s: got %d (ok=%v), want %d", tc.what, got, ok, tc.want) + } + } + if _, ok := buttonCode(tcell.ButtonNone); ok { + t.Error("no buttons should not map to a button code") + } +} + +// A shell prompt has no use for a click, so termdock keeps its own +// gestures until a program says otherwise. +func TestMouseGoesToTermdockUntilAProgramAsksForIt(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + id := c.win().active.ID + r := c.win().active.Rect + msg := fwdMsg(r.X+1, r.Y+1, tcell.Button1, 0) + + if c.forwardMouse(msg) { + t.Fatal("a plain pane forwarded the mouse; termdock's own gestures would stop working") + } + + enableMouse(t, c, id, "\x1b[?1000h") + if !c.forwardMouse(msg) { + t.Error("after the program enabled mouse reporting the click should go to it") + } +} + +// Shift is the way back to termdock's own gestures while a full-screen +// program is running, and it has to work even mid-drag. +func TestShiftTakesTheMouseBackFromTheProgram(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + id := c.win().active.ID + enableMouse(t, c, id, "\x1b[?1000h") + r := c.win().active.Rect + + if c.forwardMouse(fwdMsg(r.X+1, r.Y+1, tcell.Button1, tcell.ModShift)) { + t.Error("a shift-click was forwarded to the program instead of driving termdock") + } +} + +// A release names no button in tcell, so the state has to remember which +// one went down; and a release with no press behind it is not ours. +func TestReleaseWithoutAPressIsNotForwarded(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + enableMouse(t, c, c.win().active.ID, "\x1b[?1000h\x1b[?1006h") + r := c.win().active.Rect + + if c.forwardMouse(fwdMsg(r.X+1, r.Y+1, tcell.ButtonNone, 0)) { + t.Error("a release with no press before it was forwarded") + } + if c.mouseFwd.down { + t.Error("no button should be recorded as held") + } + + c.forwardMouse(fwdMsg(r.X+1, r.Y+1, tcell.Button2, 0)) + if !c.mouseFwd.down || c.mouseFwd.button != 2 { + t.Fatalf("after a right-click press: down=%v button=%d, want true/2", c.mouseFwd.down, c.mouseFwd.button) + } + if !c.forwardMouse(fwdMsg(r.X+1, r.Y+1, tcell.ButtonNone, 0)) { + t.Error("the matching release was not forwarded") + } + if c.mouseFwd.down { + t.Error("the release should clear the held button") + } +} + +// Coordinates are pane-relative: a click on the second pane's first +// content cell is column 1 of that pane, not of the screen. The pane is +// offset from the screen origin, so the two genuinely differ. +func TestCoordinatesAreRelativeToThePaneNotTheScreen(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + c.doSplit(layout.Vertical) + leaves := layout.Leaves(c.win().root) + if len(leaves) != 2 { + t.Fatalf("expected 2 panes, got %d", len(leaves)) + } + right := leaves[1] + if right.Rect.X == 0 { + t.Fatalf("a vertical split left the second pane at column 0, so this proves nothing") + } + + enableMouse(t, c, right.ID, "\x1b[?1000h\x1b[?1006h") + _, originX, originY := c.mouseTarget(right.Rect.X, right.Rect.Y) + if originX != right.Rect.X || originY != right.Rect.Y { + t.Fatalf("origin for the right pane = (%d,%d), want (%d,%d)", + originX, originY, right.Rect.X, right.Rect.Y) + } + + // A click three cells into that pane must report column 4, one-based, + // regardless of where the pane sits on screen. + x, y := right.Rect.X+3, right.Rect.Y+2 + if !c.forwardMouse(fwdMsg(x, y, tcell.Button1, 0)) { + t.Fatal("the click was not forwarded") + } + want := "\x1b[<0;4;3M" + if got := string(mouseReport(true, 0, x-originX, y-originY, false)); got != want { + t.Errorf("report for a click 3 cells in = %q, want %q", got, want) + } +} + +// While a modal of termdock's own is up it owns the mouse, whatever the +// program underneath asked for. +func TestTermdockModesKeepTheMouse(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + enableMouse(t, c, c.win().active.ID, "\x1b[?1000h") + r := c.win().active.Rect + msg := fwdMsg(r.X+1, r.Y+1, tcell.Button1, 0) + + for _, m := range []Mode{ModeCopy, ModeOverview, ModeSettings, ModeHistory, ModeTimeline, ModePicker} { + c.mode = m + if c.forwardMouse(msg) { + t.Errorf("mode %v forwarded the mouse to the program", m) + } + } +} + +// The mode the program asked for decides the encoding, and asking for +// motion is separate from asking for buttons. +func TestModesAreReadBackFromTheProgram(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + p := c.panes[c.win().active.ID] + if sgr, motion := mouseModes(p); sgr || motion { + t.Fatalf("a fresh pane reported sgr=%v motion=%v, want both off", sgr, motion) + } + p.Term().Write([]byte("\x1b[?1002h")) + if _, motion := mouseModes(p); !motion { + t.Error("mode 1002 should ask for motion") + } + p.Term().Write([]byte("\x1b[?1006h")) + if sgr, _ := mouseModes(p); !sgr { + t.Error("mode 1006 should ask for SGR reports") + } + p.Term().Write([]byte("\x1b[?1000l\x1b[?1002l\x1b[?1003l")) + if wantsMouse(p) { + t.Error("with every reporting mode turned off the pane should not want the mouse") + } +} + +// The case this was reported from: htop in the floating popup, where +// clicking its tabs did nothing. The popup draws its own border, so the +// program starts one cell inside the box, and a click has to be reported +// relative to that rather than to the box or the screen. +func TestPopupForwardsTheMouseToItsProgram(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + c.togglePopup() + if c.popup == nil || c.mode != ModePopup { + t.Skip("the popup did not open in this environment") + } + if !c.popupFits() { + t.Skip("the test terminal is too small for the popup") + } + + r := c.popupRect() + inside := fwdMsg(r.X+1, r.Y+1, tcell.Button1, 0) + + // A shell in the popup leaves the mouse to termdock. + if c.forwardMouse(inside) { + t.Error("a popup running a shell forwarded the mouse") + } + + c.popup.Term().Write([]byte("\x1b[?1000h\x1b[?1006h")) + target, originX, originY := c.mouseTarget(r.X+1, r.Y+1) + if target != c.popup { + t.Fatal("a point inside the popup did not resolve to the popup's pane") + } + if originX != r.X+1 || originY != r.Y+1 { + t.Errorf("popup content origin = (%d,%d), want (%d,%d) โ€” one cell in from its border", + originX, originY, r.X+1, r.Y+1) + } + if !c.forwardMouse(inside) { + t.Error("a click inside the popup was not forwarded to its program") + } + + // Outside the box is not the program's, so clicking away still + // dismisses the popup the way it always did. + if out, _, _ := c.mouseTarget(r.X-1, r.Y-1); out != nil { + t.Error("a point outside the popup resolved to the popup's pane") + } +} diff --git a/internal/core/popup.go b/internal/core/popup.go index f06fe70..e417200 100644 --- a/internal/core/popup.go +++ b/internal/core/popup.go @@ -174,7 +174,7 @@ func (c *Core) writeToPopup(key tcell.Key, r rune) { if c.popup == nil { return } - if b := keyBytes(key, r); b != nil { + if b := keyBytes(key, r, appCursorMode(c.popup)); b != nil { c.popup.Write(b) } } From 88c0a7606d4e0df7722f1645050e9d921b74b0f3 Mon Sep 17 00:00:00 2001 From: Luca Padovan Date: Thu, 27 Aug 2026 00:20:35 +0200 Subject: [PATCH 2/6] Stop a CSI parameter from crashing the daemon, and a "-t" spec from lying Found by hunting rather than by report, which is the useful kind. An escape sequence could take down every session at once. "\033[9223372036854775807P" is a well-formed delete-characters, and the handler does "cursor column + n" and slices the line with the result: near the top of int64 that addition wraps negative, and the slice panics. The daemon holds every session in one process, so that is every pane of every session, from one line of output. Any program printing garbage could do it by accident. Clamped where the parameters are parsed rather than in the thirty-odd handlers that use one, since they all do arithmetic of that shape and fixing them one at a time would be waiting for the next report. The cap is xterm's, 65535, and negatives read as zero: ECMA-48 has no negative parameters but strconv.Atoi is happy to return one for "-1", which indexes backwards just as badly. Nothing legitimate notices, because every handler already clamps to the real grid. Separately, "termdock send-keys -t main:-1" typed into the *active* window without complaining. -1 is parseTarget's own "no window given" sentinel and a negative number parsed straight into it, so a spec that should have failed silently selected a different window instead. For a scripting interface whose main verb types keystrokes, acting on the wrong window is worse than refusing. Negative numbers are now read as a name, which fails to resolve and says so. Both come with the test that reproduces them: removing the clamp panics with the same negative slice bound, and removing the sign check puts the sentinel back. parseTarget and extractTarget had no tests at all, and now cover the specs worth being sure about. The hunt that turned these up is committed too: one test drives a whole session with random keys, clicks and resizes through every mode, and one throws well-formed but extreme escape sequences at the emulator. The second is what found the panic. --- cli.go | 8 +- clitarget_parse_test.go | 90 +++++++++++++++++ internal/core/sessionfuzz_test.go | 96 ++++++++++++++++++ internal/vt10x/csi.go | 33 +++++- internal/vt10x/extremeparams_test.go | 146 +++++++++++++++++++++++++++ 5 files changed, 371 insertions(+), 2 deletions(-) create mode 100644 clitarget_parse_test.go create mode 100644 internal/core/sessionfuzz_test.go create mode 100644 internal/vt10x/extremeparams_test.go diff --git a/cli.go b/cli.go index a697938..660d44e 100644 --- a/cli.go +++ b/cli.go @@ -190,7 +190,13 @@ func parseTarget(spec string) (session string, windowIdx int, windowName string, return session, windowIdx, windowName, paneIdx } wp := strings.SplitN(parts[1], ".", 2) - if n, err := strconv.Atoi(wp[0]); err == nil { + // A negative number is not an index, and must not be taken as one: + // -1 is this function's own "no window given" sentinel, so "-t + // main:-1" would have resolved to the *active* window without + // complaint. Silently acting on the wrong window is the worst + // outcome for a scripting interface whose main verb is send-keys. + // Left as a name instead, so it fails with "no such window". + if n, err := strconv.Atoi(wp[0]); err == nil && n >= 0 { windowIdx = n } else { windowName = wp[0] diff --git a/clitarget_parse_test.go b/clitarget_parse_test.go new file mode 100644 index 0000000..277fb30 --- /dev/null +++ b/clitarget_parse_test.go @@ -0,0 +1,90 @@ +package main + +import "testing" + +// parseTarget reads the "-t" spec that every scripting verb takes, and +// send-keys types whatever it is given into whatever it resolves to. A +// spec that resolves to the wrong pane is therefore worse than one that +// fails, which is what these pin down. +func TestParseTarget(t *testing.T) { + for _, tc := range []struct { + spec string + session string + winIdx int + winName string + pane int + }{ + {"main", "main", -1, "", 0}, + {"main:", "main", -1, "", 0}, + {"main:2", "main", 2, "", 0}, + {"main:0", "main", 0, "", 0}, + {"main:2.3", "main", 2, "", 3}, + {"main:build", "main", -1, "build", 0}, + {"main:build.2", "main", -1, "build", 2}, + // A trailing dot names no pane, which means the window's active one. + {"main:2.", "main", 2, "", 0}, + // A window name containing a dot survives only up to the first one, + // same as tmux; the rest is read as the pane. + {"main:my.win", "main", -1, "my", 0}, + // Colons past the first belong to the window part. + {"main:a:b", "main", -1, "a:b", 0}, + // A negative number is not an index. -1 is the "no window given" + // sentinel, so treating it as one would quietly select the active + // window; as a name it fails to resolve, which is the honest answer. + {"main:-1", "main", -1, "-1", 0}, + {"main:-3", "main", -1, "-3", 0}, + } { + s, wi, wn, p := parseTarget(tc.spec) + if s != tc.session || wi != tc.winIdx || wn != tc.winName || p != tc.pane { + t.Errorf("parseTarget(%q) = (%q, %d, %q, %d), want (%q, %d, %q, %d)", + tc.spec, s, wi, wn, p, tc.session, tc.winIdx, tc.winName, tc.pane) + } + } +} + +// A negative window index must never come back as the sentinel, whatever +// the number: that is the case where the command would act on a window +// nobody named. +func TestParseTargetNeverTurnsANegativeIntoTheActiveWindow(t *testing.T) { + for _, spec := range []string{"s:-1", "s:-2", "s:-99", "s:-1.1"} { + _, idx, name, _ := parseTarget(spec) + if idx != -1 { + t.Errorf("parseTarget(%q) gave window index %d", spec, idx) + } + if name == "" { + t.Errorf("parseTarget(%q) left both the index and the name empty, which resolves to the active window", spec) + } + } +} + +// extractTarget pulls "-t X" out of an argument list and hands back what +// is left, which is the command's own arguments. +func TestExtractTarget(t *testing.T) { + for _, tc := range []struct { + args []string + target string + rest []string + }{ + {[]string{"-t", "main", "hello"}, "main", []string{"hello"}}, + {[]string{"hello", "-t", "main"}, "main", []string{"hello"}}, + {[]string{"--target", "main:1", "a", "b"}, "main:1", []string{"a", "b"}}, + {[]string{"hello"}, "", []string{"hello"}}, + // A dangling -t names nothing, and must not eat past the end. + {[]string{"hello", "-t"}, "", []string{"hello", "-t"}}, + } { + target, rest := extractTarget(tc.args) + if target != tc.target { + t.Errorf("extractTarget(%q) target = %q, want %q", tc.args, target, tc.target) + } + if len(rest) != len(tc.rest) { + t.Errorf("extractTarget(%q) rest = %q, want %q", tc.args, rest, tc.rest) + continue + } + for i := range rest { + if rest[i] != tc.rest[i] { + t.Errorf("extractTarget(%q) rest = %q, want %q", tc.args, rest, tc.rest) + break + } + } + } +} diff --git a/internal/core/sessionfuzz_test.go b/internal/core/sessionfuzz_test.go new file mode 100644 index 0000000..f4960d5 --- /dev/null +++ b/internal/core/sessionfuzz_test.go @@ -0,0 +1,96 @@ +package core + +import ( + "math/rand" + "os" + "strconv" + "testing" + + "github.com/gdamore/tcell/v2" + + "github.com/padovanl/termdock/internal/proto" +) + +// TestSessionSurvivesRandomInput drives a whole session the way a person +// with a keyboard and a mouse would, only faster and without aim: every +// mode entered and left in any order, panes split and killed underneath +// the modes that index into them, the terminal resized to sizes nothing +// fits in, and the mouse clicked at coordinates that are not on screen. +// +// The daemon holds every session in one process, so a panic anywhere in +// here takes down every pane of every session at once. That makes "never +// panics, whatever the order" a property worth hammering at rather than +// reasoning about: the modes each keep indices into state the others are +// busy rewriting. +func TestSessionSurvivesRandomInput(t *testing.T) { + c := newTestCore(t) + t.Cleanup(func() { closeAllPanes(c) }) + + seed := int64(20260822) + if v := os.Getenv("TERMDOCK_FUZZ_SEED"); v != "" { + if n, err := strconv.ParseInt(v, 10, 64); err == nil { + seed = n + } + } + t.Logf("seed %d (set TERMDOCK_FUZZ_SEED to reproduce a failure)", seed) + rng := rand.New(rand.NewSource(seed)) + + keys := []tcell.Key{ + tcell.KeyRune, tcell.KeyEnter, tcell.KeyEsc, tcell.KeyUp, tcell.KeyDown, + tcell.KeyLeft, tcell.KeyRight, tcell.KeyPgUp, tcell.KeyPgDn, tcell.KeyTab, + tcell.KeyBackspace2, tcell.KeyHome, tcell.KeyEnd, tcell.KeyCtrlB, + } + // Every prefix-key command, plus junk runes that are bound to nothing. + runes := []rune("vs%\"hjklozxrc np;wWgS/P!Q: RZmOHTLA,.$&]=dCq?y[123450abcdefiu~@^") + + sizes := [][2]int{{80, 24}, {40, 12}, {200, 60}, {20, 6}, {5, 3}, {1, 1}, {120, 40}} + + for i := 0; i < 4000; i++ { + switch rng.Intn(10) { + case 0: + // A resize, including sizes too small for anything to fit. + s := sizes[rng.Intn(len(sizes))] + c.Resize(s[0], s[1]) + case 1: + // A click anywhere, on screen or off it, with or without a + // button, sometimes with shift. + mod := tcell.ModNone + if rng.Intn(4) == 0 { + mod = tcell.ModShift + } + buttons := []tcell.ButtonMask{ + tcell.Button1, tcell.Button2, tcell.Button3, + tcell.WheelUp, tcell.WheelDown, tcell.ButtonNone, + }[rng.Intn(6)] + c.HandleClientMsg(proto.ClientMsg{ + Kind: "mouse", MouseX: rng.Intn(140) - 5, MouseY: rng.Intn(50) - 5, + MouseButtons: int32(buttons), MouseMod: int32(mod), + }) + case 2: + // The prefix, so the next key is taken as a command. + c.HandleClientMsg(proto.ClientMsg{Kind: "key", KeyCode: int32(tcell.KeyCtrlB)}) + default: + k := keys[rng.Intn(len(keys))] + r := rune(0) + if k == tcell.KeyRune { + r = runes[rng.Intn(len(runes))] + } + c.HandleClientMsg(proto.ClientMsg{Kind: "key", KeyCode: int32(k), KeyRune: r}) + } + + // Building a frame is what every attached client does many times a + // second, and it reads all the state the loop above is mangling. + f := c.Frame() + if f.Cols < 0 || f.Rows < 0 { + t.Fatalf("iteration %d: frame reports a negative size %dx%d", i, f.Cols, f.Rows) + } + for _, p := range f.Panes { + if p.Rect.W < 0 || p.Rect.H < 0 { + t.Fatalf("iteration %d: pane %d has a negative size %v", i, p.ID, p.Rect) + } + } + if c.PaneCount() == 0 { + break // the session killed its last pane; nothing left to drive + } + } +} diff --git a/internal/vt10x/csi.go b/internal/vt10x/csi.go index f5df174..e115f6c 100644 --- a/internal/vt10x/csi.go +++ b/internal/vt10x/csi.go @@ -50,10 +50,41 @@ func (c *csiEscape) parse() { //t.logf("invalid CSI arg '%s'\n", p) break } - c.args = append(c.args, i) + c.args = append(c.args, clampCSIParam(i)) } } +// maxCSIParam is the largest value a CSI parameter is allowed to carry, +// matching what xterm caps them at. +// +// Clamping happens here, once, rather than in each of the thirty-odd +// handlers that use a parameter, because the failure is not a wrong +// picture but a crash: the handlers do arithmetic like "cursor column + +// n" and index the line with the result, so a parameter near the top of +// int64 overflows to a negative index and panics. "\033[9223372036854775807P" +// was enough to take down the daemon, and with it every pane of every +// session in the process. +// +// ECMA-48 has no negative parameters; strconv.Atoi is happy to return +// one for "-1", which would index backwards just as badly. Zero is the +// safe reading, and is what a parameter that was never meant to exist +// should mean. +// +// The cap costs nothing real: parameters address screen positions and +// counts, and every handler already clamps to the actual grid, so no +// legitimate sequence notices the difference. +const maxCSIParam = 65535 + +func clampCSIParam(n int) int { + switch { + case n < 0: + return 0 + case n > maxCSIParam: + return maxCSIParam + } + return n +} + func (c *csiEscape) arg(i, def int) int { if i >= len(c.args) || i < 0 { return def diff --git a/internal/vt10x/extremeparams_test.go b/internal/vt10x/extremeparams_test.go new file mode 100644 index 0000000..56eeb79 --- /dev/null +++ b/internal/vt10x/extremeparams_test.go @@ -0,0 +1,146 @@ +package vt10x + +import ( + "fmt" + "strings" + "testing" +) + +// Sequences that are well-formed but extreme: the parser has to survive +// parameters far outside any real terminal's range, since a program can +// emit them by accident (a bad ncurses build, a corrupted stream) and +// the daemon renders every session in one process. +func TestParserSurvivesExtremeParameters(t *testing.T) { + term := New(WithSize(80, 24), WithHistoryLimit(200)) + + huge := []string{ + "999999999", "2147483647", "4294967295", "9223372036854775807", + "99999999999999999999", "0", "-1", "", + } + finals := "ABCDEFGHJKLMPSTXZ@`abcdefghilmnpqrstuvwxyz" + + for _, p := range huge { + for _, f := range finals { + term.Write([]byte("\x1b[" + p + string(f))) + term.Write([]byte("\x1b[" + p + ";" + p + string(f))) + term.Write([]byte("\x1b[?" + p + "h")) + term.Write([]byte("\x1b[?" + p + "l")) + } + } + + // Many parameters in one sequence. + term.Write([]byte("\x1b[" + strings.Repeat("1;", 500) + "m")) + // A scroll region beyond the screen, then output that has to scroll. + term.Write([]byte("\x1b[9999;99999r")) + term.Write([]byte(strings.Repeat("line\r\n", 300))) + // Cursor addressing far outside the grid. + for _, s := range []string{"\x1b[99999;99999H", "\x1b[0;0H", "\x1b[;H"} { + term.Write([]byte(s)) + term.Write([]byte("x")) + } + // Tab stops, insert/delete of absurd counts. + term.Write([]byte("\x1b[99999b\x1b[99999@\x1b[99999P\x1b[99999L\x1b[99999M\x1b[99999X")) + // OSC with an enormous payload and no terminator, then one with. + term.Write([]byte("\x1b]0;" + strings.Repeat("t", 100000))) + term.Write([]byte("\x07")) + term.Write([]byte("\x1b]133;" + strings.Repeat("A", 10000) + "\x07")) + + cols, rows := term.Size() + if cols <= 0 || rows <= 0 { + t.Fatalf("terminal reports a %dx%d size after the stress", cols, rows) + } + // Every cell must still be readable: that is what the renderer does + // on the next frame, and an out-of-range read here panics the daemon. + for y := 0; y < rows; y++ { + for x := 0; x < cols; x++ { + _ = term.Cell(x, y) + } + } + for n := 0; n < term.HistoryLen(); n++ { + for x := 0; x < cols; x++ { + _ = term.HistoryCell(n, x) + } + } + for _, m := range term.Marks() { + _ = m + } + fmt.Fprint(&strings.Builder{}, term.String()) +} + +// Resizing while the buffer is full, repeatedly, in both directions and +// through degenerate sizes. +func TestResizeThroughDegenerateSizes(t *testing.T) { + term := New(WithSize(80, 24), WithHistoryLimit(100)) + term.Write([]byte(strings.Repeat("filling the screen with text\r\n", 200))) + + for _, sz := range [][2]int{ + {1, 1}, {0, 0}, {-1, -1}, {200, 60}, {1, 200}, {200, 1}, + {80, 24}, {0, 24}, {80, 0}, {3, 3}, {500, 500}, {80, 24}, + } { + term.Resize(sz[0], sz[1]) + term.Write([]byte("after resize\r\n")) + cols, rows := term.Size() + for y := 0; y < rows; y++ { + for x := 0; x < cols; x++ { + _ = term.Cell(x, y) + } + } + if cols < 0 || rows < 0 { + t.Fatalf("resize to %v left a %dx%d terminal", sz, cols, rows) + } + } +} + +// The exact sequence that brought the daemon down, and its relatives. +// Each handler here does arithmetic on a parameter and then indexes a +// line with the result: "\033[P" made "cursor column + n" overflow +// int64 into a negative index. A crash in the emulator is not a wrong +// picture on one pane, it is every pane of every session in the process, +// since they all live in the one daemon. +func TestHugeParametersCannotOverflowIntoANegativeIndex(t *testing.T) { + for _, seq := range []string{ + "\x1b[9223372036854775807P", // DCH, the one that was reported + "\x1b[9223372036854775807@", // ICH + "\x1b[9223372036854775807X", // ECH + "\x1b[9223372036854775807L", // IL + "\x1b[9223372036854775807M", // DL + "\x1b[9223372036854775807C", // CUF + "\x1b[9223372036854775807D", // CUB + "\x1b[9223372036854775807b", // REP + "\x1b[9223372036854775807;9223372036854775807H", + "\x1b[9223372036854775807;9223372036854775807r", + "\x1b[-1P", // Atoi accepts a sign; ECMA-48 has no negative parameters + "\x1b[-1@", + "\x1b[-99;-99H", + } { + term := New(WithSize(20, 5), WithHistoryLimit(50)) + term.Write([]byte("some text on the line")) + term.Write([]byte("\x1b[3;5H")) // somewhere in the middle + term.Write([]byte(seq)) // must not panic + + cols, rows := term.Size() + for y := 0; y < rows; y++ { + for x := 0; x < cols; x++ { + _ = term.Cell(x, y) + } + } + } +} + +// The clamp is what makes the above safe, so pin its edges: a negative +// parameter reads as zero, and anything past the cap stops there. +func TestCSIParametersAreClamped(t *testing.T) { + for _, tc := range []struct{ in, want int }{ + {0, 0}, + {1, 1}, + {maxCSIParam, maxCSIParam}, + {maxCSIParam + 1, maxCSIParam}, + {1 << 62, maxCSIParam}, + {-1, 0}, + {-(1 << 62), 0}, + } { + if got := clampCSIParam(tc.in); got != tc.want { + t.Errorf("clampCSIParam(%d) = %d, want %d", tc.in, got, tc.want) + } + } +} From a1c9fdc8fa5238e9f9f625e55a11bb9e1ada8fdb Mon Sep 17 00:00:00 2001 From: Luca Padovan Date: Thu, 27 Aug 2026 00:27:17 +0200 Subject: [PATCH 3/6] Stop preview blanks painting themselves black over the theme The jump picker's preview showed a black slab beside any pane that was not full of text, and the braille thumbnail had black behind its dots. Reported a while back, half-fixed then: the earlier pass changed what draws the box, and this is what was actually filling it. A preview cell carries the raw colour values vt10x uses, where "default" is 1<<24 and up and anything under 256 is a palette index. Zero is therefore not "unset", it is palette entry 0, which is black. Every blank cell was built as proto.Cell{Ch: ' '}, so it went out as black on black and was drawn as black whatever theme was set, right next to panes the same theme was colouring correctly. Three places built cells that way, including the one that draws the braille dots, so the dots had a black backing too. All three now go through one blankCell, which spells the default colours out, so there is no longer a way to write the wrong thing by leaving a field off. The test asserts on the colour values rather than on pixels, since that is where the mistake lives: a zero colour is the bug, and "not zero" is not enough to say, so it checks for the default range specifically. --- internal/core/picker.go | 26 ++++- internal/core/previewbg_test.go | 63 +++++++++++++ internal/vt10x/hostilesequences_test.go | 120 ++++++++++++++++++++++++ 3 files changed, 206 insertions(+), 3 deletions(-) create mode 100644 internal/core/previewbg_test.go create mode 100644 internal/vt10x/hostilesequences_test.go diff --git a/internal/core/picker.go b/internal/core/picker.go index ab80f87..58c1e88 100644 --- a/internal/core/picker.go +++ b/internal/core/picker.go @@ -10,6 +10,7 @@ import ( "github.com/padovanl/termdock/internal/layout" "github.com/padovanl/termdock/internal/proto" + "github.com/padovanl/termdock/internal/vt10x" ) // The jump picker (Ctrl-B w) is termdock's answer to tmux's choose-tree: @@ -291,7 +292,7 @@ func (c *Core) buildPreview(paneID int, maxW, maxH int) [][]proto.Cell { row[x] = glyphToCell(t.Cell(x, yOff+y)) continue } - row[x] = proto.Cell{Ch: ' '} + row[x] = blankCell() } cells[y] = row } @@ -368,11 +369,12 @@ func (c *Core) buildThumbnail(paneID int, maxW, maxH int) [][]proto.Cell { } } } - cell := proto.Cell{Ch: ' '} + cell := blankCell() if dots != 0 { // Colored from the first bit of real text in the block, so // a green prompt still reads as green at thumbnail scale. - cell = proto.Cell{Ch: 0x2800 | dots, FG: fg} + cell.Ch = 0x2800 | dots + cell.FG = fg } row[cx] = cell } @@ -381,6 +383,24 @@ func (c *Core) buildThumbnail(paneID int, maxW, maxH int) [][]proto.Cell { return cells } +// blankCell is an empty cell in a preview: a space in the *default* +// colours, meaning "whatever the theme paints a pane with". +// +// Spelling the colours out matters because zero is not neutral here. A +// proto.Cell's colours are the raw values vt10x uses, where "default" is +// 1<<24 and up (see convColor in internal/client) and anything under 256 +// is a palette index. So a cell built as proto.Cell{Ch: ' '} carries +// palette index 0 twice over: black on black, drawn as black whatever +// theme is set. That is what filled the empty part of the jump picker's +// preview with a black slab, and sat behind the braille dots too. +func blankCell() proto.Cell { + return proto.Cell{ + Ch: ' ', + FG: uint32(vt10x.DefaultFG), + BG: uint32(vt10x.DefaultBG), + } +} + func minInt(a, b int) int { if a < b { return a diff --git a/internal/core/previewbg_test.go b/internal/core/previewbg_test.go new file mode 100644 index 0000000..4c86edd --- /dev/null +++ b/internal/core/previewbg_test.go @@ -0,0 +1,63 @@ +package core + +import ( + "testing" + + "github.com/padovanl/termdock/internal/proto" + "github.com/padovanl/termdock/internal/vt10x" +) + +// A preview cell's colours are the raw vt10x values, where "default" +// is 1<<24 and up and anything below 256 is a palette index. Zero is +// therefore not "unset", it is palette entry 0: black. Building an empty +// cell as proto.Cell{Ch: ' '} made every blank part of a preview black +// on black, whatever theme was set, which is the black slab that showed +// in the jump picker beside a mostly empty pane. +func isDefaultColour(v uint32) bool { return v >= 1<<24 } + +func TestPreviewBlanksUseTheThemeNotBlack(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + id := c.win().active.ID + + // Both preview kinds: the picker's braille thumbnail, and the + // character-for-character one. + for name, cells := range map[string][][]proto.Cell{ + "thumbnail": c.buildThumbnail(id, 20, 8), + "preview": c.buildPreview(id, 20, 8), + } { + if len(cells) == 0 { + t.Fatalf("%s produced no cells", name) + } + for y, row := range cells { + for x, cell := range row { + if !isDefaultColour(cell.BG) { + t.Errorf("%s cell (%d,%d) has background %d, want a default colour so the theme decides", + name, x, y, cell.BG) + } + if cell.Ch == ' ' && !isDefaultColour(cell.FG) { + t.Errorf("%s blank cell (%d,%d) has foreground %d, want a default colour", + name, x, y, cell.FG) + } + } + } + } +} + +// blankCell is the single place that gets this right, so pin it. +func TestBlankCellIsDefaultOnDefault(t *testing.T) { + b := blankCell() + if b.Ch != ' ' { + t.Errorf("blank cell char = %q, want a space", b.Ch) + } + if b.FG != uint32(vt10x.DefaultFG) || b.BG != uint32(vt10x.DefaultBG) { + t.Errorf("blank cell colours = fg %d / bg %d, want the vt10x defaults %d / %d", + b.FG, b.BG, vt10x.DefaultFG, vt10x.DefaultBG) + } + // The specific trap: zero would be palette index 0, which is black. + if b.BG == 0 || b.FG == 0 { + t.Error("a zero colour is palette entry 0, which renders as black rather than as the theme's") + } +} diff --git a/internal/vt10x/hostilesequences_test.go b/internal/vt10x/hostilesequences_test.go new file mode 100644 index 0000000..7991e39 --- /dev/null +++ b/internal/vt10x/hostilesequences_test.go @@ -0,0 +1,120 @@ +package vt10x + +import ( + "math/rand" + "strings" + "testing" +) + +// Beyond the CSI parameters: the other sequence families, each driven +// with well-formed but hostile content. Same stake as before, a panic +// here is every pane of every session in the daemon. +func TestOSCAndDCSSurviveHostileContent(t *testing.T) { + term := New(WithSize(40, 10), WithHistoryLimit(100)) + + payloads := []string{ + "", ";", ";;;;", strings.Repeat(";", 1000), + strings.Repeat("A", 70000), + "\x00\x01\x02", "\xff\xfe", + strings.Repeat("๐Ÿ˜€", 500), + "0;title", "2;", "133;", "133;D;notanumber", "133;D;-5", + "133;D;99999999999999999999", + "8;;http://example.com", "52;c;" + strings.Repeat("b", 10000), + "10;?", "11;?", "4;256;rgb:ff/ff/ff", + } + terminators := []string{"\x07", "\x1b\\", ""} + + for _, p := range payloads { + for _, term2 := range terminators { + term.Write([]byte("\x1b]" + p + term2)) + term.Write([]byte("\x1bP" + p + term2)) // DCS + term.Write([]byte("\x1b_" + p + term2)) // APC + term.Write([]byte("\x1b^" + p + term2)) // PM + term.Write([]byte("\x1bX" + p + term2)) // SOS + } + } + readEverything(t, term) +} + +// Charset selection, tab stops, alt screen and scroll regions, driven +// together: each keeps state the others move underneath it. +func TestCharsetsTabsAltScreenAndScrollRegions(t *testing.T) { + term := New(WithSize(30, 8), WithHistoryLimit(80)) + + seqs := []string{ + "\x1b(0", "\x1b(B", "\x1b)0", "\x1b)B", "\x1b*0", "\x1b+0", + "\x1b(?", "\x1b(\xff", // designations nothing defines + "\x0e", "\x0f", // shift out / shift in + "\x1b[?1049h", "\x1b[?1049l", "\x1b[?47h", "\x1b[?47l", + "\x1b[1;3r", "\x1b[3;1r", "\x1b[0;0r", "\x1b[r", + "\x1bH", "\x1b[0g", "\x1b[3g", "\x1b[g", + "\x1bM", "\x1bD", "\x1bE", "\x1b7", "\x1b8", + "\x1b[?6h", "\x1b[?6l", // origin mode + "\x1b[?7h", "\x1b[?7l", // wrap + "\x1b#8", // DEC alignment test, fills the screen + } + + rng := rand.New(rand.NewSource(7)) + for i := 0; i < 6000; i++ { + term.Write([]byte(seqs[rng.Intn(len(seqs))])) + if rng.Intn(3) == 0 { + term.Write([]byte(strings.Repeat("x", rng.Intn(60)))) + } + if rng.Intn(20) == 0 { + term.Write([]byte("\r\n")) + } + if rng.Intn(50) == 0 { + term.Resize(1+rng.Intn(60), 1+rng.Intn(20)) + } + } + readEverything(t, term) +} + +// Text that is not simple: wide glyphs, combining marks, zero-width +// joiners and invalid UTF-8, written at the right-hand edge where a +// wide glyph does not fit. +func TestAwkwardTextAtTheEdge(t *testing.T) { + for _, cols := range []int{1, 2, 3, 10, 40} { + term := New(WithSize(cols, 4), WithHistoryLimit(20)) + for _, s := range []string{ + "ๆผขๅญ—ใƒ†ใ‚นใƒˆ", "aฬฬ‚ฬƒ", "๐Ÿ‘จโ€๐Ÿ‘ฉโ€๐Ÿ‘งโ€๐Ÿ‘ฆ", "๐Ÿ‡ฎ๐Ÿ‡น๐Ÿ‡ฏ๐Ÿ‡ต", + "\xff\xfe\xfd", "\xc3", "\xe2\x82", // truncated UTF-8 + "\u200b\u200d\ufeff", // zero-width space, ZWJ, BOM + strings.Repeat("ๆผข", 50), + } { + for i := 0; i < 3; i++ { + term.Write([]byte(s)) + term.Write([]byte("\r\n")) + } + } + readEverything(t, term) + } +} + +// readEverything does what the renderer does on the next frame: touch +// every cell the terminal says it has, plus its scrollback and marks. +// An out-of-range read here is the crash, so this is the assertion. +func readEverything(t *testing.T, term Terminal) { + t.Helper() + cols, rows := term.Size() + if cols < 0 || rows < 0 { + t.Fatalf("terminal reports a %dx%d size", cols, rows) + } + for y := 0; y < rows; y++ { + for x := 0; x < cols; x++ { + _ = term.Cell(x, y) + } + } + for n := 0; n < term.HistoryLen(); n++ { + for x := 0; x < cols; x++ { + _ = term.HistoryCell(n, x) + } + } + for _, m := range term.Marks() { + if m.Line < 0 { + t.Fatalf("a mark points at line %d", m.Line) + } + } + _ = term.String() + _ = term.Cursor() +} From cfc19ab4ff7b8f2447e33ee028a83d16dbbf063c Mon Sep 17 00:00:00 2001 From: Luca Padovan Date: Thu, 27 Aug 2026 00:37:43 +0200 Subject: [PATCH 4/6] Say it out loud when the settings screen writes a file, or refuses a value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing S on the settings screen wrote termdock.conf and said so, into a status bar that was already showing "โ†‘โ†“ move, enter to type a value, S save to file, esc close". Every modal screen keeps its own instructions there for as long as it is open, and those won, so from the one screen where knowing the file was written is the whole point, nothing appeared to happen. A flash is a message that outranks a screen's own hint for a few seconds and then gets out of the way, which is the right trade for both: the instructions are what you need for as long as the screen is open, the confirmation is what you need immediately after acting. The same fault made two worse messages invisible, both found looking for others like it. Typing a value the settings screen refuses left the screen open with the reason hidden, so a rejected value looked accepted. And pressing an arrow on a free-text setting explained that it has no list to step through, silently. flash schedules a redraw for when it expires. A frame is only built when something asks for one, and the periodic tick is every fifteen seconds, so without that the message would outstay its welcome by ten seconds or more and "a few seconds" would be a duration nothing honoured. --- internal/core/core.go | 8 ++- internal/core/flash.go | 52 +++++++++++++++ internal/core/flash_test.go | 130 ++++++++++++++++++++++++++++++++++++ internal/core/frame.go | 6 ++ internal/core/settings.go | 17 +++-- 5 files changed, 205 insertions(+), 8 deletions(-) create mode 100644 internal/core/flash.go create mode 100644 internal/core/flash_test.go diff --git a/internal/core/core.go b/internal/core/core.go index 135c577..eb141d4 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -137,8 +137,12 @@ type Core struct { prefix bool prefixKey tcell.Key statusMsg string - shellName string - hostname string + // flashMsg/flashUntil are a message that outranks a modal screen's + // own status-bar hint for a few seconds; see flash.go. + flashMsg string + flashUntil time.Time + shellName string + hostname string bindings map[rune]action // defaultBindings, overridden per-key by config's "bind" setting; see SetBindOverrides diff --git a/internal/core/flash.go b/internal/core/flash.go new file mode 100644 index 0000000..48c41d8 --- /dev/null +++ b/internal/core/flash.go @@ -0,0 +1,52 @@ +package core + +import ( + "fmt" + "time" +) + +// A flash is a message that takes over the status bar briefly and then +// gets out of the way. +// +// c.statusMsg is not enough on its own. Every modal screen puts its own +// instructions in the status bar for as long as it is open (see +// statusHint), so a message set from inside one is written and never +// seen: saving a setting to the config file said so, underneath "โ†‘โ†“ +// move, enter to type a value, S save to file, esc close", which won. +// Nothing appeared to happen, from the one screen where knowing that the +// file was written is the whole point. +// +// So a flash outranks the mode's own hint, and only for a few seconds: +// the hint is what you need for as long as the screen is open, and the +// confirmation is what you need immediately after acting. + +// flashFor is how long a flash holds the status bar. Long enough to read +// a path in it without hurrying, short enough that the instructions it +// covers are not gone when you look back for them. +const flashFor = 4 * time.Second + +// flash puts msg in the status bar for flashFor. +func (c *Core) flash(format string, args ...any) { + c.flashMsg = fmt.Sprintf(format, args...) + c.flashUntil = time.Now().Add(flashFor) + // Set as well, not instead: outside a modal screen the status bar + // shows statusMsg anyway, and leaving it behind afterwards is the + // existing behaviour for every other message. + c.statusMsg = c.flashMsg + + // A frame is only built when something asks for one. Without this the + // message would sit there until the next keypress or the 15-second + // clock tick, whichever came first, and "a few seconds" would be a + // duration nothing honoured. markDirty is a non-blocking send, so + // calling it from the timer's goroutine is safe and cannot stall it. + time.AfterFunc(flashFor, c.markDirty) +} + +// activeFlash is the message currently owed the status bar, or "" when +// none is live. +func (c *Core) activeFlash() string { + if c.flashMsg == "" || time.Now().After(c.flashUntil) { + return "" + } + return c.flashMsg +} diff --git a/internal/core/flash_test.go b/internal/core/flash_test.go new file mode 100644 index 0000000..8fc0075 --- /dev/null +++ b/internal/core/flash_test.go @@ -0,0 +1,130 @@ +package core + +import ( + "strings" + "testing" + "time" + + "github.com/padovanl/termdock/internal/config" +) + +// Saving a setting to the config file said so in c.statusMsg, and the +// settings screen never showed it: every modal screen puts its own +// instructions in the status bar for as long as it is open, and those +// won. From the one screen where knowing the file was written is the +// point, nothing appeared to happen. +func TestSavingASettingSaysSoOverTheScreensOwnHint(t *testing.T) { + dir := t.TempDir() + t.Setenv("TERMDOCK_CONFIG", dir+"/termdock.conf") + + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + c.enterSettings() + if c.mode != ModeSettings { + t.Fatal("the settings screen did not open") + } + + // Before saving, the screen's own instructions have the bar. + hint, _ := c.statusHint() + if !strings.Contains(hint, "save to file") { + t.Fatalf("with nothing flashed the bar reads %q, expected the screen's instructions", hint) + } + + // Put the cursor on a setting that has a value to save. + for i, s := range config.Settings() { + if s.Key == "history-limit" { + c.settings.sel = i + } + } + c.saveSelectedSetting() + + hint, _ = c.statusHint() + if strings.Contains(hint, "save to file") { + t.Errorf("after saving, the bar still reads the screen's instructions: %q", hint) + } + if !strings.Contains(hint, "history-limit") || !strings.Contains(hint, "saved") { + t.Errorf("after saving, the bar reads %q; it should say what was written and where", hint) + } + + // And it gets out of the way again, leaving the instructions you need + // for as long as the screen is open. + c.flashUntil = time.Now().Add(-time.Second) + hint, _ = c.statusHint() + if !strings.Contains(hint, "save to file") { + t.Errorf("once the flash expired the bar reads %q, expected the screen's instructions back", hint) + } +} + +// A failure has to be as visible as a success, for the same reason. +func TestAFailedSaveIsAlsoShown(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + c.enterSettings() + // "shell" reads as "(your $SHELL)" when unset, which is not a value + // that can be written to a file. + for i, s := range config.Settings() { + if s.Key == "shell" { + c.settings.sel = i + } + } + c.saveSelectedSetting() + + hint, _ := c.statusHint() + if !strings.Contains(hint, "shell") { + t.Errorf("the refusal was not shown: bar reads %q", hint) + } +} + +// The flash is a general mechanism, so pin its edges rather than only +// the one caller. +func TestFlashExpires(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + if got := c.activeFlash(); got != "" { + t.Errorf("a fresh session has a flash: %q", got) + } + c.flash("wrote %d lines", 3) + if got := c.activeFlash(); got != "wrote 3 lines" { + t.Errorf("activeFlash = %q, want the message just flashed", got) + } + if c.statusMsg != "wrote 3 lines" { + t.Errorf("statusMsg = %q; a flash should also leave the ordinary message behind", c.statusMsg) + } + c.flashUntil = time.Now().Add(-time.Millisecond) + if got := c.activeFlash(); got != "" { + t.Errorf("an expired flash still reports %q", got) + } +} + +// A value the settings screen refuses is the case that matters most: +// the screen stays open afterwards, so without a flash its own +// instructions cover the reason and the value looks accepted. +func TestARefusedValueSaysWhy(t *testing.T) { + c := newTestCore(t) + c.mu.Lock() + defer c.mu.Unlock() + + c.enterSettings() + for i, s := range config.Settings() { + if s.Key == "history-limit" { + c.settings.sel = i + } + } + c.settings.editing = true + c.settings.buffer = []rune("not a number") + c.commitEditedSetting() + + hint, _ := c.statusHint() + if strings.Contains(hint, "save to file") { + t.Fatalf("after a refusal the bar still reads the screen's instructions: %q", hint) + } + if !strings.Contains(hint, "history-limit") { + t.Errorf("the refusal does not name the setting: %q", hint) + } +} diff --git a/internal/core/frame.go b/internal/core/frame.go index 8e13b08..4f4873f 100644 --- a/internal/core/frame.go +++ b/internal/core/frame.go @@ -290,6 +290,12 @@ func (c *Core) statusHint() (hint, style string) { case c.statusMsg != "": hint = c.statusMsg } + // A flash outranks a modal screen's own instructions, briefly. Set + // after the switch rather than before it so it keeps that screen's + // styling and only replaces the words. See flash.go. + if msg := c.activeFlash(); msg != "" { + hint = msg + } return hint, style } diff --git a/internal/core/settings.go b/internal/core/settings.go index 589782e..eab922b 100644 --- a/internal/core/settings.go +++ b/internal/core/settings.go @@ -301,7 +301,7 @@ func (c *Core) stepSelectedSetting(delta int) { return } if len(s.Choices()) == 0 { - c.statusMsg = fmt.Sprintf("%s has no fixed set of values โ€” press enter to type one", s.Key) + c.flash("%s has no fixed set of values โ€” press enter to type one", s.Key) return } updated := c.cfg @@ -370,12 +370,15 @@ func (c *Core) commitEditedSetting() { c.settings.buffer = nil updated := c.cfg + // Flashed rather than set: the screen stays open after a refusal, so + // its own instructions would cover the reason and the value would + // look as though it had been accepted. if err := config.Set(&updated, s.Key, value); err != nil { - c.statusMsg = s.Key + ": " + err.Error() + c.flash("%s: %v", s.Key, err) return } if err := config.CheckSetting(&updated, s.Key); err != nil { - c.statusMsg = s.Key + ": " + err.Error() + c.flash("%s: %v", s.Key, err) return } c.applySettingChange(s, updated, config.Get(&updated, s.Key)) @@ -392,14 +395,16 @@ func (c *Core) saveSelectedSetting() { } value := config.Get(&c.cfg, s.Key) if strings.HasPrefix(value, "(") { - c.statusMsg = fmt.Sprintf("%s isn't set to anything to save", s.Key) + c.flash("%s isn't set to anything to save", s.Key) return } if err := c.persistSetting(s.Key, value); err != nil { - c.statusMsg = fmt.Sprintf("could not save %s: %v", s.Key, err) + c.flash("could not save %s: %v", s.Key, err) return } - c.statusMsg = fmt.Sprintf("%s = %s โ€” saved to %s", s.Key, value, config.Path()) + // Flashed, not merely set: this screen keeps its own instructions in + // the status bar, so a plain message here was written and never seen. + c.flash("saved %s = %s to %s", s.Key, value, config.Path()) } // applySettingChange is the one place a change made on this screen takes From d85f646a6397ba4d1fe13a688a7a37c0c2b76704 Mon Sep 17 00:00:00 2001 From: Luca Padovan Date: Thu, 27 Aug 2026 00:44:06 +0200 Subject: [PATCH 5/6] Guard the pane lifecycle against leaking a goroutine each time Every pane has a pump goroutine blocked reading its pty, and a session is meant to stay up for weeks, so one left behind per closed pane is a slow leak in a process that never restarts. Opening and closing twenty-four panes leaves the goroutine count exactly where it started, which is what this pins. Nothing was wrong: Close kills the process and closes the pty, which is what unblocks the read, and both paths that remove a pane call it first. Worth a test anyway, because the property is invisible until it has been failing for a fortnight and the failure is a daemon that has quietly grown to thousands of goroutines. The comment records the trap the first version of this test fell into. Closing panes with detachLeafIn leaks a goroutine every time and looks precisely like the bug being hunted, because detachLeafIn only takes a pane out of the layout tree: break-pane and move-to-window use it on panes they mean to keep running. --- internal/core/paneleak_test.go | 67 ++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 internal/core/paneleak_test.go diff --git a/internal/core/paneleak_test.go b/internal/core/paneleak_test.go new file mode 100644 index 0000000..d5ee6c9 --- /dev/null +++ b/internal/core/paneleak_test.go @@ -0,0 +1,67 @@ +package core + +import ( + "runtime" + "testing" + "time" + + "github.com/padovanl/termdock/internal/layout" +) + +// A session is meant to stay up for weeks, so a goroutine left behind by +// every pane that closes is a slow leak in a process that never +// restarts. Each pane has a pump goroutine blocked reading its pty, and +// what unblocks it is the pty being closed. +// +// It has to close panes the way the program does, through killActive: +// detachLeafIn only takes a pane out of the layout tree and is used by +// break-pane and move-to-window, where the pane is deliberately kept +// alive. Closing them that way in a test leaks a goroutine per pane and +// looks exactly like the bug this is looking for. +func TestPanesDoNotLeakGoroutines(t *testing.T) { + c := newTestCore(t) + t.Cleanup(func() { closeAllPanes(c) }) + + settle := func() int { + for i := 0; i < 60; i++ { + runtime.GC() + time.Sleep(20 * time.Millisecond) + } + return runtime.NumGoroutine() + } + + // Warm up: the first few panes create shared machinery. + for i := 0; i < 3; i++ { + c.mu.Lock() + c.doSplit(layout.Vertical) + c.mu.Unlock() + } + c.mu.Lock() + for len(layout.Leaves(c.win().root)) > 1 { + c.setActive(layout.Leaves(c.win().root)[1]) + c.killActive() + } + c.mu.Unlock() + base := settle() + + for round := 0; round < 12; round++ { + c.mu.Lock() + c.doSplit(layout.Vertical) + c.doSplit(layout.Horizontal) + c.mu.Unlock() + c.mu.Lock() + for len(layout.Leaves(c.win().root)) > 1 { + c.setActive(layout.Leaves(c.win().root)[1]) + c.killActive() + } + c.mu.Unlock() + } + + after := settle() + t.Logf("goroutine: %d prima, %d dopo 24 pane aperti e chiusi", base, after) + if after > base+4 { + buf := make([]byte, 1<<16) + n := runtime.Stack(buf, true) + t.Fatalf("goroutines went %d -> %d after opening and closing 24 panes\n%s", base, after, buf[:n]) + } +} From dbdbf309d476b4f9cb30b76659275965aef8c0f3 Mon Sep 17 00:00:00 2001 From: Luca Padovan Date: Thu, 27 Aug 2026 00:57:15 +0200 Subject: [PATCH 6/6] Pin that attaching and detaching leaks nothing either The counterpart to the pane lifecycle guard. Attach and detach is what a session does most over its life, and a goroutine left behind by each one would only show up in a session old enough that restarting it costs real work. Fifteen cycles, same goroutine count at the end as at the start. Nothing was wrong here either. Both of these are cheap to keep and would have caught the leak on the day it was introduced rather than a fortnight later. --- internal/server/attachleak_test.go | 48 ++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 internal/server/attachleak_test.go diff --git a/internal/server/attachleak_test.go b/internal/server/attachleak_test.go new file mode 100644 index 0000000..a1c4d96 --- /dev/null +++ b/internal/server/attachleak_test.go @@ -0,0 +1,48 @@ +package server + +import ( + "runtime" + "testing" + "time" +) + +// Attaching and detaching is the most repeated thing a session does over +// its life: reattaching from a new terminal every morning for a month is +// dozens of round trips through handleConn. A goroutine left behind by +// each one is a leak that only surfaces in a session old enough that +// restarting it loses real work, which is the kind this daemon exists to +// avoid losing. +func TestAttachAndDetachDoNotLeakGoroutines(t *testing.T) { + sock, kill := startSession(t, "leak-probe") + defer kill() + + cycle := func() { + conn, _, _ := dial(t, sock, false) + conn.Close() + } + + settle := func() int { + for i := 0; i < 50; i++ { + runtime.GC() + time.Sleep(20 * time.Millisecond) + } + return runtime.NumGoroutine() + } + + for i := 0; i < 3; i++ { // warm up: the first attach builds shared state + cycle() + } + base := settle() + + for i := 0; i < 15; i++ { + cycle() + } + after := settle() + + t.Logf("goroutines: %d before, %d after 15 attach/detach cycles", base, after) + if after > base+3 { + buf := make([]byte, 1<<16) + n := runtime.Stack(buf, true) + t.Fatalf("goroutines went %d -> %d over 15 attach/detach cycles\n%s", base, after, buf[:n]) + } +}