From 5d9b2da19db1f7bb33fd59f707537d43f893de8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:25:07 +0200 Subject: [PATCH 1/9] feat: move the cursor and paste in the TUI text fields The filter and the option value editor kept the cursor at the end of the text, drew it as a fake underscore and ignored paste. Both now keep a cursor position, handle left/right/home/end/delete, accept bracketed paste (cleaned of control characters) and show the real terminal cursor, also while a long value is scrolled. --- internal/tui/helpers_test.go | 20 ++ internal/tui/model.go | 35 ++- internal/tui/options.go | 23 +- internal/tui/optionsview.go | 58 ++-- internal/tui/optionsview_test.go | 2 +- internal/tui/testdata/options-editing.golden | 2 +- internal/tui/textedit.go | 98 +++++++ internal/tui/textedit_test.go | 271 +++++++++++++++++++ internal/tui/view.go | 39 ++- 9 files changed, 502 insertions(+), 46 deletions(-) create mode 100644 internal/tui/textedit.go create mode 100644 internal/tui/textedit_test.go diff --git a/internal/tui/helpers_test.go b/internal/tui/helpers_test.go index b28f7d5..408427d 100644 --- a/internal/tui/helpers_test.go +++ b/internal/tui/helpers_test.go @@ -201,6 +201,20 @@ func key(name string) tea.KeyPressMsg { return tea.KeyPressMsg{Code: tea.KeySpace, Text: " "} case "backspace": return tea.KeyPressMsg{Code: tea.KeyBackspace} + case "left": + return tea.KeyPressMsg{Code: tea.KeyLeft} + case "right": + return tea.KeyPressMsg{Code: tea.KeyRight} + case "home": + return tea.KeyPressMsg{Code: tea.KeyHome} + case "end": + return tea.KeyPressMsg{Code: tea.KeyEnd} + case "delete": + return tea.KeyPressMsg{Code: tea.KeyDelete} + case "ctrl+a": + return tea.KeyPressMsg{Code: 'a', Mod: tea.ModCtrl} + case "ctrl+e": + return tea.KeyPressMsg{Code: 'e', Mod: tea.ModCtrl} case "ctrl+c": return tea.KeyPressMsg{Code: 'c', Mod: tea.ModCtrl} default: @@ -219,6 +233,12 @@ func press(t *testing.T, m Model, names ...string) Model { return m } +// paste delivers text the way a terminal with bracketed paste does. +func paste(m Model, text string) Model { + next, _ := m.Update(tea.PasteMsg{Content: text}) + return next.(Model) +} + // sized returns m after a terminal of w x h cells has been reported. func sized(m Model, w, h int) Model { next, _ := m.Update(tea.WindowSizeMsg{Width: w, Height: h}) diff --git a/internal/tui/model.go b/internal/tui/model.go index fee1af0..dae4418 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -22,6 +22,7 @@ type Model struct { visible []int // indexes into views that match the filter, in order cursor int // position within visible filter string + filterPos int // the cursor within filter, as a rune index filtering bool // true while the user is typing into the filter status string // the last error to show, cleared by the next key press pending bool // true from pressing space until the write has finished @@ -71,6 +72,8 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.applyOptions(msg), nil case optionWrittenMsg: return m.applyOptionWritten(msg), nil + case tea.PasteMsg: + return m.applyPaste(msg.Content), nil case tea.KeyPressMsg: m.status = "" switch m.screen { @@ -103,9 +106,10 @@ func (m Model) updateBrowsing(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m.openOptions() case "/": m.filtering = true + m.filterPos = len([]rune(m.filter)) case "esc": if m.filter != "" { - m.filter = "" + m.filter, m.filterPos = "", 0 m.applyFilter() } } @@ -120,22 +124,33 @@ func (m Model) updateFiltering(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { m.filtering = false case "esc": m.filtering = false - m.filter = "" + m.filter, m.filterPos = "", 0 m.applyFilter() - case "backspace": - if runes := []rune(m.filter); len(runes) > 0 { - m.filter = string(runes[:len(runes)-1]) - m.applyFilter() - } default: - if msg.Text != "" { - m.filter += msg.Text - m.applyFilter() + if text, pos, ok := editText(m.filter, m.filterPos, msg); ok { + changed := text != m.filter + m.filter, m.filterPos = text, pos + if changed { + m.applyFilter() + } } } return m, nil } +// applyPaste puts pasted text into the text field that has the keyboard, and +// ignores it everywhere else. +func (m Model) applyPaste(content string) Model { + switch { + case m.screen == screenOptions: + return m.pasteOption(content) + case m.screen == screenBrowser && m.filtering: + m.filter, m.filterPos = pasteText(m.filter, m.filterPos, content) + m.applyFilter() + } + return m +} + // move shifts the cursor by delta, staying put at either end of the list. func (m *Model) move(delta int) { next := m.cursor + delta diff --git a/internal/tui/options.go b/internal/tui/options.go index f9de572..9f46a6f 100644 --- a/internal/tui/options.go +++ b/internal/tui/options.go @@ -16,6 +16,7 @@ type optionsState struct { cursor int editing bool // true while the user types a new value input string // the value typed so far + pos int // the cursor within input, as a rune index } // optionsMsg carries the options of a module back to Update after `o`. @@ -104,7 +105,7 @@ func (m Model) applyOptionWritten(msg optionWrittenMsg) Model { m.options.rows = rows m.options.cursor = min(m.options.cursor, max(len(rows)-1, 0)) m.options.editing = false - m.options.input = "" + m.options.input, m.options.pos = "", 0 m.rememberOptions(msg.id, rows) return m } @@ -204,6 +205,7 @@ func (m Model) updateOptions(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { if row, ok := m.selectedOption(); ok && row.Editable && (row.Type == "string" || row.Type == "int") { m.options.editing = true m.options.input = row.Value + m.options.pos = len([]rune(row.Value)) } } return m, nil @@ -216,23 +218,26 @@ func (m Model) updateOptionInput(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m, tea.Quit case "esc": m.options.editing = false - m.options.input = "" + m.options.input, m.options.pos = "", 0 case "enter": if row, ok := m.selectedOption(); ok { return m.writeOption(row.Key, m.options.input) } - case "backspace": - if runes := []rune(m.options.input); len(runes) > 0 { - m.options.input = string(runes[:len(runes)-1]) - } default: - if msg.Text != "" { - m.options.input += sanitize(msg.Text) - } + m.options.input, m.options.pos, _ = editText(m.options.input, m.options.pos, msg) } return m, nil } +// pasteOption inserts pasted text into the value being typed. +func (m Model) pasteOption(content string) Model { + if m.pending || !m.options.editing { + return m + } + m.options.input, m.options.pos = pasteText(m.options.input, m.options.pos, content) + return m +} + // cycleEnum moves an enum option to the next (delta 1) or previous (delta -1) // allowed value, wrapping around, and writes it. func (m Model) cycleEnum(delta int) (tea.Model, tea.Cmd) { diff --git a/internal/tui/optionsview.go b/internal/tui/optionsview.go index bb11b75..80d5f12 100644 --- a/internal/tui/optionsview.go +++ b/internal/tui/optionsview.go @@ -5,6 +5,7 @@ import ( "slices" "strings" + tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" "github.com/charmbracelet/x/ansi" @@ -56,7 +57,7 @@ func (m Model) renderOptionsFooter() string { case m.pending: return dimStyle.Inline(true).MaxWidth(m.width).Render("saving…") case m.options.editing: - return dimStyle.Inline(true).MaxWidth(m.width).Render("type a value · enter save · esc cancel · ctrl+c quit") + return dimStyle.Inline(true).MaxWidth(m.width).Render("type a value · ←/→ move · enter save · esc cancel · ctrl+c quit") } return dimStyle.Inline(true).MaxWidth(m.width). Render("↑/↓ move · space toggle · ←/→ change · enter edit · esc back · q quit") @@ -68,10 +69,7 @@ func (m Model) optionTable(n int) []string { if len(rows) == 0 { return []string{dimStyle.Render("No options")} } - start := 0 - if m.options.cursor >= n { - start = m.options.cursor - n + 1 - } + start := m.optionTableStart(n) end := min(start+n, len(rows)) lines := make([]string, 0, end-start) @@ -95,12 +93,38 @@ func (m Model) optionTable(n int) []string { return lines } +// optionTableStart is the index of the first option line shown when n lines fit, +// scrolled so the cursor is visible. +func (m Model) optionTableStart(n int) int { + if m.options.cursor >= n { + return m.options.cursor - n + 1 + } + return 0 +} + +// optionCursor is where the terminal cursor belongs while a value is typed, or +// nil when it is not. +func (m Model) optionCursor() *tea.Cursor { + if !m.options.editing || m.pending { + return nil + } + tableRows := max(m.bodyRows()-optionDetailLines, 1) + _, col := editCell(m.options.input, m.options.pos) + x := 2 + keyColumn + col // the marker, the key column, then the cell + y := headerLines + m.options.cursor - m.optionTableStart(tableRows) + if x >= m.width { + return nil + } + return tea.NewCursor(x, y) +} + // optionValueCell is what the value column shows for one option: the text being // typed for the row under edit, otherwise the value with a note where it is not // a plain, explicitly set value. func (m Model) optionValueCell(pos int, row modedit.OptionView) string { if m.options.editing && pos == m.options.cursor { - return editCell(m.options.input) + cell, _ := editCell(m.options.input, m.options.pos) + return cell } return row.Value + valueNote(row) } @@ -117,21 +141,13 @@ func valueNote(row modedit.OptionView) string { return "" } -// editCell draws the text being typed between brackets. A long input is cut on -// the left, so the end of it and the cursor stay inside the value column. -func editCell(input string) string { - const room = valueColumn - 1 - 3 // the column keeps one cell free; "[", "_" and "]" take three - if w := ansi.StringWidth(input); w > room { - // TruncateLeft keeps a wide character that is cut in the middle, so the - // result can be one cell too wide: cut a little more until it fits. - for n := w - (room - 1); ; n++ { - if cut := ansi.TruncateLeft(input, n, "…"); ansi.StringWidth(cut) <= room { - input = cut - break - } - } - } - return "[" + input + "_]" +// editCell draws the text being typed between brackets. A long input is cut so +// the cursor stays inside the value column; the second result is the cursor's +// column within the cell. +func editCell(input string, pos int) (string, int) { + const room = valueColumn - 1 - 2 // the column keeps one cell free; "[" and "]" take two + text, col := editWindow(input, pos, room) + return "[" + text + "]", col + 1 } // pad cuts s to width cells (with an ellipsis) or pads it with spaces. diff --git a/internal/tui/optionsview_test.go b/internal/tui/optionsview_test.go index 935eabf..1b859da 100644 --- a/internal/tui/optionsview_test.go +++ b/internal/tui/optionsview_test.go @@ -129,7 +129,7 @@ func TestOptionsEditShowsTheTypedTextInPlaceOfTheValue(t *testing.T) { if !strings.Contains(out, "[abcx_]") { t.Fatalf("the input cell is missing:\n%s", out) } - if !strings.Contains(out, "type a value · enter save · esc cancel") { + if !strings.Contains(out, "type a value · ←/→ move · enter save · esc cancel") { t.Fatalf("the editing footer is missing:\n%s", out) } } diff --git a/internal/tui/testdata/options-editing.golden b/internal/tui/testdata/options-editing.golden index 564b50a..bc2a745 100644 --- a/internal/tui/testdata/options-editing.golden +++ b/internal/tui/testdata/options-editing.golden @@ -17,4 +17,4 @@ Note: this module is not enabled; its options apply once it is -type a value · enter save · esc cancel · ctrl+c quit \ No newline at end of file +type a value · ←/→ move · enter save · esc cancel · ctrl+c quit \ No newline at end of file diff --git a/internal/tui/textedit.go b/internal/tui/textedit.go new file mode 100644 index 0000000..6b1218b --- /dev/null +++ b/internal/tui/textedit.go @@ -0,0 +1,98 @@ +package tui + +import ( + tea "charm.land/bubbletea/v2" + "github.com/charmbracelet/x/ansi" +) + +// The two text fields (the filter and an option value) keep their text as a +// string and the cursor as a rune index into it, so the Model stays a plain +// value and tests can read the text directly. + +// editText applies one key press to text, with the cursor at pos, and returns +// the new text and cursor. ok is false for keys it does not handle, so the +// caller can give them their own meaning (enter, esc, ctrl+c). +func editText(text string, pos int, msg tea.KeyPressMsg) (string, int, bool) { + runes := []rune(text) + pos = min(max(pos, 0), len(runes)) + switch msg.String() { + case "left", "ctrl+b": + pos = max(pos-1, 0) + case "right", "ctrl+f": + pos = min(pos+1, len(runes)) + case "home", "ctrl+a": + pos = 0 + case "end", "ctrl+e": + pos = len(runes) + case "backspace": + if pos > 0 { + runes = append(runes[:pos-1], runes[pos:]...) + pos-- + } + case "delete", "ctrl+d": + if pos < len(runes) { + runes = append(runes[:pos], runes[pos+1:]...) + } + default: + if msg.Text == "" { + return text, pos, false + } + text, pos = pasteText(text, pos, msg.Text) + return text, pos, true + } + return string(runes), pos, true +} + +// pasteText inserts content, cleaned of control characters, at pos and returns +// the new text and the cursor behind the inserted part. +func pasteText(text string, pos int, content string) (string, int) { + runes := []rune(text) + pos = min(max(pos, 0), len(runes)) + add := []rune(sanitize(content)) + out := make([]rune, 0, len(runes)+len(add)) + out = append(out, runes[:pos]...) + out = append(out, add...) + out = append(out, runes[pos:]...) + return string(out), pos + len(add) +} + +// editWindow returns what fits into room cells of text with the cursor at pos, +// and the cell column of the cursor. When the cursor is behind the text, an +// underscore marks it. A text that does not fit is cut on the left, and on the +// right when much of it follows the cursor, so the cursor is always inside. +func editWindow(text string, pos, room int) (string, int) { + runes := []rune(text) + pos = min(max(pos, 0), len(runes)) + before, after := string(runes[:pos]), string(runes[pos:]) + atEnd := after == "" + + if ansi.StringWidth(before)+max(ansi.StringWidth(after), 1) <= room { + if atEnd { + after = "_" + } + return before + after, ansi.StringWidth(before) + } + + // Not everything fits: give the part after the cursor at most a third of + // the cell, and the rest to the part before it. + rightBudget := min(max(ansi.StringWidth(after), 1), max(room/3, 1)) + right := after + if ansi.StringWidth(right) > rightBudget { + right = ansi.Truncate(right, rightBudget, "…") + } + if atEnd { + right = "_" + } + leftBudget := max(room-max(ansi.StringWidth(right), 1), 1) + if w := ansi.StringWidth(before); w > leftBudget { + // TruncateLeft keeps a wide character that is cut in the middle, so the + // result can be a cell too wide: cut a little more until it fits. + for n := w - (leftBudget - 1); ; n++ { + if cut := ansi.TruncateLeft(before, n, "…"); ansi.StringWidth(cut) <= leftBudget { + before = cut + break + } + } + } + return before + right, ansi.StringWidth(before) +} diff --git a/internal/tui/textedit_test.go b/internal/tui/textedit_test.go new file mode 100644 index 0000000..4bee3da --- /dev/null +++ b/internal/tui/textedit_test.go @@ -0,0 +1,271 @@ +package tui + +import ( + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/charmbracelet/x/ansi" +) + +func TestEditTextMovesTheCursorAndEditsAtIt(t *testing.T) { + text, pos := "abcd", 4 + step := func(name string) { + t.Helper() + var ok bool + text, pos, ok = editText(text, pos, key(name)) + if !ok { + t.Fatalf("%s must be handled", name) + } + } + + step("left") + step("left") + if pos != 2 { + t.Fatalf("pos = %d after two lefts, want 2", pos) + } + text, pos, _ = editText(text, pos, key("X")) + if text != "abXcd" || pos != 3 { + t.Fatalf("text=%q pos=%d, want X inserted at the cursor", text, pos) + } + step("backspace") + if text != "abcd" || pos != 2 { + t.Fatalf("text=%q pos=%d, want the character before the cursor removed", text, pos) + } + step("delete") + if text != "abd" || pos != 2 { + t.Fatalf("text=%q pos=%d, want the character under the cursor removed", text, pos) + } + step("home") + step("delete") + if text != "bd" || pos != 0 { + t.Fatalf("text=%q pos=%d after home and delete", text, pos) + } + step("backspace") // at the start: nothing to remove + if text != "bd" || pos != 0 { + t.Fatalf("backspace at the start changed text=%q pos=%d", text, pos) + } + step("end") + step("right") // at the end: stays + if pos != 2 { + t.Fatalf("pos = %d at the end, want 2", pos) + } + step("ctrl+a") + if pos != 0 { + t.Fatalf("ctrl+a: pos = %d, want 0", pos) + } + step("ctrl+e") + if pos != 2 { + t.Fatalf("ctrl+e: pos = %d, want 2", pos) + } +} + +func TestEditTextCountsRunesNotBytes(t *testing.T) { + text, pos, _ := editText("äö日", 3, key("left")) + text, pos, _ = editText(text, pos, key("backspace")) + if text != "ä日" || pos != 1 { + t.Fatalf("text=%q pos=%d, want the rune before the cursor removed", text, pos) + } +} + +func TestEditTextLeavesOtherKeysAlone(t *testing.T) { + for _, name := range []string{"up", "down", "enter", "esc", "ctrl+c"} { + if _, _, ok := editText("abc", 1, key(name)); ok { + t.Fatalf("%s must be left to the caller", name) + } + } + _ = tea.KeyPressMsg{} +} + +func TestPasteTextInsertsAtTheCursorAndCleansIt(t *testing.T) { + text, pos := pasteText("ad", 1, "b\x1b[31mc\n") + if strings.ContainsAny(text, "\x1b\n") { + t.Fatalf("pasted control characters must be cleaned: %q", text) + } + if !strings.HasPrefix(text, "ab") || !strings.HasSuffix(text, "d") || pos != len([]rune(text))-1 { + t.Fatalf("text=%q pos=%d, want the paste between a and d with the cursor behind it", text, pos) + } +} + +func TestEditWindowKeepsTheCursorInsideTheCell(t *testing.T) { + long := strings.Repeat("0123456789", 6) + for _, pos := range []int{0, 1, 10, 30, 59, 60} { + cell, col := editWindow(long, pos, 23) + if w := ansi.StringWidth(cell); w > 23 { + t.Fatalf("pos %d: cell %q is %d cells wide, want at most 23", pos, cell, w) + } + if col < 0 || col >= 23 { + t.Fatalf("pos %d: cursor column %d outside the cell", pos, col) + } + } +} + +func TestEditWindowShowsTheWholeShortText(t *testing.T) { + cell, col := editWindow("abc", 3, 23) + if cell != "abc_" || col != 3 { + t.Fatalf("cell=%q col=%d, want abc_ with the cursor on the underscore", cell, col) + } + cell, col = editWindow("abc", 1, 23) + if cell != "abc" || col != 1 { + t.Fatalf("cell=%q col=%d, want abc with the cursor on b", cell, col) + } +} + +func TestEditWindowWithWideCharacters(t *testing.T) { + long := strings.Repeat("日本語", 10) + for _, pos := range []int{0, 7, 15, 30} { + cell, col := editWindow(long, pos, 23) + if ansi.StringWidth(cell) > 23 || col >= 23 { + t.Fatalf("pos %d: cell %q (%d cells) col %d does not fit", pos, cell, ansi.StringWidth(cell), col) + } + } +} + +func TestOptionInputCursorMovesAndEditsInTheMiddle(t *testing.T) { + m, b := withOptions() + m = onOption(t, openFzfOptions(t, m), "prefix") // value "abc" + + m = press(t, m, "enter", "left", "left", "X") + if m.options.input != "aXbc" { + t.Fatalf("input = %q, want X inserted before b", m.options.input) + } + m = press(t, m, "home", "Y", "end", "Z") + if m.options.input != "YaXbcZ" { + t.Fatalf("input = %q, want Y at the start and Z at the end", m.options.input) + } + m = press(t, m, "left", "left", "delete") + if m.options.input != "YaXbZ" { + t.Fatalf("input = %q after delete, want the character under the cursor gone", m.options.input) + } + if len(b.optionCalls) != 0 { + t.Fatal("moving and typing must not write") + } +} + +func TestOptionInputShowsARealCursorAtTheEditPosition(t *testing.T) { + m, _ := withOptions() + m = onOption(t, openFzfOptions(t, m), "prefix") + + if m.View().Cursor != nil { + t.Fatal("no cursor expected before editing starts") + } + m = press(t, m, "enter") + cur := m.View().Cursor + if cur == nil { + t.Fatal("the editing cell needs a visible cursor") + } + lines := strings.Split(plain(m), "\n") + line := lines[cur.Y] + if !strings.Contains(line, "prefix") { + t.Fatalf("the cursor is on line %d %q, want the row being edited", cur.Y, line) + } + // "[abc_]": the cursor sits on the underscore behind the text. + if got := ansi.Cut(line, cur.X, cur.X+1); got != "_" { + t.Fatalf("the cursor is on %q at column %d of %q, want the underscore", got, cur.X, line) + } + + m = press(t, m, "left") + cur = m.View().Cursor + if got := ansi.Cut(strings.Split(plain(m), "\n")[cur.Y], cur.X, cur.X+1); got != "c" { + t.Fatalf("after left the cursor is on %q, want c", got) + } +} + +func TestOptionInputCursorStaysVisibleWhileScrolled(t *testing.T) { + m, b := withOptions() + b.optionRows["fzf"][2].Value, b.optionRows["fzf"][2].Set = strings.Repeat("0123456789", 6), true + m = onOption(t, openFzfOptions(t, m), "prefix") + m = press(t, m, "enter", "home") + + cur := m.View().Cursor + if cur == nil { + t.Fatal("cursor missing") + } + line := strings.Split(plain(m), "\n")[cur.Y] + if got := ansi.Cut(line, cur.X, cur.X+1); got != "0" { + t.Fatalf("at the start the cursor is on %q, want the first character:\n%s", got, plain(m)) + } + assertFits(t, plain(m), 80, 20) +} + +func TestPasteIntoAnOptionValueGoesInAtTheCursor(t *testing.T) { + m, _ := withOptions() + m = onOption(t, openFzfOptions(t, m), "prefix") + m = press(t, m, "enter", "left") + + m = paste(m, "XY\x1b") + if strings.ContainsRune(m.options.input, '\x1b') || !strings.HasPrefix(m.options.input, "abXY") || !strings.HasSuffix(m.options.input, "c") { + t.Fatalf("input = %q, want the cleaned paste between b and c", m.options.input) + } +} + +func TestPasteIsIgnoredOutsideATextField(t *testing.T) { + m, _ := withOptions() + m = openFzfOptions(t, m) + before := m + m = paste(m, "abc") + if m.options.input != before.options.input || m.options.editing || m.filter != "" { + t.Fatal("a paste on the options list must not start editing or filtering") + } + + browser := newTestModel(sampleViews()) + browser = paste(sized(browser, 80, 20), "abc") + if browser.filter != "" || browser.filtering { + t.Fatal("a paste into the browser outside the filter must be ignored") + } +} + +func TestFilterCursorMovesAndPasteWorks(t *testing.T) { + m := sized(newTestModel(sampleViews()), 80, 20) + m = press(t, m, "/", "f", "z") + m = press(t, m, "left", "x") + if m.filter != "fxz" { + t.Fatalf("filter = %q, want x inserted before z", m.filter) + } + m = press(t, m, "backspace", "backspace") + if m.filter != "z" { + t.Fatalf("filter = %q, want z", m.filter) + } + + m = paste(m, "f") + if m.filter != "fz" && m.filter != "zf" { + t.Fatalf("filter = %q, want the paste inserted", m.filter) + } + cur := m.View().Cursor + if cur == nil || cur.Y != 0 { + t.Fatalf("the filter needs a cursor on the header line, got %+v", cur) + } +} + +func TestFilterPasteIsCleanedAndFilters(t *testing.T) { + m := sized(newTestModel(sampleViews()), 80, 20) + m = press(t, m, "/") + m = paste(m, "fz\x1b[2J\n") + if strings.ContainsAny(m.filter, "\x1b\n") { + t.Fatalf("filter = %q, control characters must be cleaned", m.filter) + } + if len(m.visible) != 0 { + t.Fatalf("visible = %v, a cleaned multi-word paste matches nothing", visibleIDs(m)) + } + m.filter = "" + m.applyFilter() + m = paste(m, "fzf") + if got := visibleIDs(m); len(got) != 1 || got[0] != "fzf" { + t.Fatalf("visible = %v, want only fzf after pasting fzf", got) + } +} + +func TestNoCursorOutsideTextFields(t *testing.T) { + m := sized(newTestModel(sampleViews()), 80, 20) + if m.View().Cursor != nil { + t.Fatal("the browser has no text cursor while not filtering") + } + m = press(t, m, "/") + if m.View().Cursor == nil { + t.Fatal("filtering needs a cursor") + } + m = press(t, m, "enter") + if m.View().Cursor != nil { + t.Fatal("a kept filter has no cursor") + } +} diff --git a/internal/tui/view.go b/internal/tui/view.go index c347fde..22cc353 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -32,6 +32,7 @@ var ( func (m Model) View() tea.View { v := tea.NewView(m.render()) v.AltScreen = true + v.Cursor = m.textCursor() return v } @@ -68,17 +69,47 @@ func (m Model) render() string { } func (m Model) renderHeader() string { - line := titleStyle.Render("omnishell") + - dimStyle.Render(fmt.Sprintf(" %d modules · %s", len(m.views), changesText(m.changes()))) + line := m.headerBase() + filter := sanitize(m.filter) switch { case m.filtering: - line += " filter: " + m.filter + "_" + line += " filter: " + filter + if m.filterPos >= len([]rune(filter)) { + line += "_" // the cursor is behind the text + } case m.filter != "": - line += fmt.Sprintf(" filter: %s (%d shown)", m.filter, len(m.visible)) + line += fmt.Sprintf(" filter: %s (%d shown)", filter, len(m.visible)) } return lipgloss.NewStyle().Inline(true).MaxWidth(m.width).Render(line) } +// headerBase is the start of the browser's header line, before the filter. +func (m Model) headerBase() string { + return titleStyle.Render("omnishell") + + dimStyle.Render(fmt.Sprintf(" %d modules · %s", len(m.views), changesText(m.changes()))) +} + +// textCursor is where the terminal cursor belongs: in the filter or in the +// option value being typed, and nowhere else (nil hides it). +func (m Model) textCursor() *tea.Cursor { + if m.width < minWidth || m.height < minHeight { + return nil + } + switch { + case m.screen == screenOptions: + return m.optionCursor() + case m.screen == screenBrowser && m.filtering: + runes := []rune(sanitize(m.filter)) + before := string(runes[:min(m.filterPos, len(runes))]) + x := ansi.StringWidth(m.headerBase() + " filter: " + before) + if x >= m.width { + return nil + } + return tea.NewCursor(x, 0) + } + return nil +} + func (m Model) renderFooter() string { if m.status != "" { return statusStyle.Render(ansi.Truncate("! "+m.status, m.width, "…")) From 87d241cf7e09fa0b6bef2f1828923dd99e6ab688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:25:46 +0200 Subject: [PATCH 2/9] fix: clean manifest text before it reaches the terminal Move the control-character cleaning into modedit.Clean so that omnishell list uses it too, and clean ModuleView.Unavailable and the filter header in the TUI. --- internal/cli/list.go | 12 ++++---- internal/cli/list_test.go | 50 ++++++++++++++++++++++++++++++++++ internal/modedit/clean.go | 23 ++++++++++++++++ internal/modedit/clean_test.go | 20 ++++++++++++++ internal/tui/sanitize.go | 22 ++++----------- internal/tui/sanitize_test.go | 24 ++++++++++++++++ 6 files changed, 129 insertions(+), 22 deletions(-) create mode 100644 internal/modedit/clean.go create mode 100644 internal/modedit/clean_test.go diff --git a/internal/cli/list.go b/internal/cli/list.go index 1897ab4..4c83d5c 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -49,14 +49,16 @@ PACKAGES is one of: rows := make([]listRow, 0, len(views)) for _, v := range views { + // Manifest text can come from a stranger's module: it must not + // reach the terminal as a control sequence. rows = append(rows, listRow{ - Module: v.ID, + Module: modedit.Clean(v.ID), Status: string(v.Status), - Description: v.Description, - Homepage: v.Homepage, + Description: modedit.Clean(v.Description), + Homepage: modedit.Clean(v.Homepage), Packages: string(v.Packages), - Platforms: strings.Join(v.Platforms, ","), - Shells: strings.Join(v.Shells, ","), + Platforms: modedit.Clean(strings.Join(v.Platforms, ",")), + Shells: modedit.Clean(strings.Join(v.Shells, ",")), Src: string(v.Origin), }) } diff --git a/internal/cli/list_test.go b/internal/cli/list_test.go index 2bbcbad..ceee5a3 100644 --- a/internal/cli/list_test.go +++ b/internal/cli/list_test.go @@ -117,3 +117,53 @@ func findRow(table, id string) string { } return "" } + +// A module manifest can come from a stranger; `list` writes its text to a +// terminal, so control sequences in it must not survive. +func TestListNeutralisesControlSequencesInManifestText(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + configDir := filepath.Join(home, ".config", "omnishell") + installFixtureModules(t, configDir) + + manifest := filepath.Join(configDir, "modules", "completion", "manifest.toml") + data, err := os.ReadFile(manifest) + if err != nil { + t.Fatal(err) + } + hostile := strings.Replace(string(data), `description = "compinit"`, + `description = "compinit\u001b[2J\u0007\u202eevil"`, 1) + hostile = strings.Replace(hostile, `version = "1.0.0"`, + "version = \"1.0.0\"\nhomepage = \"https://example.com/\\u001b]0;pwned\\u0007\"", 1) + if hostile == string(data) { + t.Fatal("the fixture manifest changed shape; adapt the test") + } + if err := os.WriteFile(manifest, []byte(hostile), 0o644); err != nil { + t.Fatal(err) + } + + cli.SetLookPathForTest(func(string) (string, error) { return "", os.ErrNotExist }) + defer cli.SetLookPathForTest(nil) + cli.SetRunnerForTest(&pkgmgr.MockRunner{}) + defer cli.SetRunnerForTest(nil) + var initOut, initErr bytes.Buffer + if code := cli.Execute([]string{"init"}, &initOut, &initErr); code != 0 { + t.Fatalf("init exit %d: %s", code, initErr.String()) + } + + for _, args := range [][]string{{"list"}, {"list", "--json"}} { + var out, errOut bytes.Buffer + if code := cli.Execute(args, &out, &errOut); code != 0 { + t.Fatalf("%v exit %d: %s", args, code, errOut.String()) + } + for _, bad := range []string{"\x1b", "\x07", "\u202e"} { + if strings.Contains(out.String(), bad) { + t.Fatalf("%v output contains %q:\n%q", args, bad, out.String()) + } + } + if !strings.Contains(out.String(), "compinit") { + t.Fatalf("%v lost the description:\n%s", args, out.String()) + } + } +} diff --git a/internal/modedit/clean.go b/internal/modedit/clean.go new file mode 100644 index 0000000..13c4c4c --- /dev/null +++ b/internal/modedit/clean.go @@ -0,0 +1,23 @@ +package modedit + +import ( + "strings" + "unicode" +) + +// Clean replaces control characters (ESC, BEL, CR, newline, tab, C1) and bidi +// override characters with a space, so text that comes from a module manifest +// can neither act as a terminal command nor reorder the text around it. Every +// front end that prints manifest text to a terminal runs it through Clean. +func Clean(s string) string { + return strings.Map(func(r rune) rune { + if unicode.IsControl(r) || isBidiControl(r) { + return ' ' + } + return r + }, s) +} + +func isBidiControl(r rune) bool { + return (r >= 0x202A && r <= 0x202E) || (r >= 0x2066 && r <= 0x2069) +} diff --git a/internal/modedit/clean_test.go b/internal/modedit/clean_test.go new file mode 100644 index 0000000..5277f59 --- /dev/null +++ b/internal/modedit/clean_test.go @@ -0,0 +1,20 @@ +package modedit + +import "testing" + +func TestCleanReplacesControlAndBidiCharactersWithSpaces(t *testing.T) { + cases := map[string]string{ + "plain text": "plain text", + "ä日 ok": "ä日 ok", + "a\x1b]0;x\x07b": "a ]0;x b", + "a\x1b[2Jb": "a [2Jb", + "one\ntwo\r\tthree": "one two three", + "a\u202etxt\u2066b\u2069": "a txt b ", + "c1\u009bcontrol": "c1 control", + } + for in, want := range cases { + if got := Clean(in); got != want { + t.Errorf("Clean(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/tui/sanitize.go b/internal/tui/sanitize.go index d628224..45407e6 100644 --- a/internal/tui/sanitize.go +++ b/internal/tui/sanitize.go @@ -2,8 +2,6 @@ package tui import ( "slices" - "strings" - "unicode" "github.com/JtheGunner/omnishell/internal/modedit" ) @@ -14,21 +12,10 @@ import ( // module's text read as another's. Everything shown is therefore cleaned once, // when the model is built. -// sanitize replaces control characters (ESC, BEL, CR, newline, tab, C1) and -// bidi override characters with a space, so nothing in s can act as a -// terminal command or reorder the text around it. -func sanitize(s string) string { - return strings.Map(func(r rune) rune { - if unicode.IsControl(r) || isBidiControl(r) { - return ' ' - } - return r - }, s) -} - -func isBidiControl(r rune) bool { - return (r >= 0x202A && r <= 0x202E) || (r >= 0x2066 && r <= 0x2069) -} +// sanitize is modedit.Clean: it replaces control characters and bidi override +// characters with a space, so nothing in s can act as a terminal command or +// reorder the text around it. +func sanitize(s string) string { return modedit.Clean(s) } // sanitizeViews returns a cleaned copy of views; the caller's slice and the // slices inside it are left alone. @@ -39,6 +26,7 @@ func sanitizeViews(views []modedit.ModuleView) []modedit.ModuleView { v.Name = sanitize(v.Name) v.Description = sanitize(v.Description) v.Homepage = sanitize(v.Homepage) + v.Unavailable = sanitize(v.Unavailable) v.Platforms = sanitizeAll(v.Platforms) v.Shells = sanitizeAll(v.Shells) out[i] = v diff --git a/internal/tui/sanitize_test.go b/internal/tui/sanitize_test.go index 8a6ec6b..14a0d28 100644 --- a/internal/tui/sanitize_test.go +++ b/internal/tui/sanitize_test.go @@ -18,6 +18,7 @@ func hostileViews() []modedit.ModuleView { Homepage: "https://example.com/\x1b[1;1H", Platforms: []string{"linux\x1b[2J"}, Shells: []string{"zsh\x07"}, + Unavailable: "needs zsh\x1b[2J\x07", Status: modedit.StatusDisabled, Packages: modedit.PackagesNA, Origin: modedit.OriginUser, @@ -55,3 +56,26 @@ func TestNewDoesNotMutateTheCallersViews(t *testing.T) { t.Fatalf("New must copy: caller's view changed to %+v", views[0]) } } + +func TestHostFieldIsNeutralised(t *testing.T) { + m := sized(newTestModel(hostileViews()), 100, 30) + out := m.View().Content + + for _, bad := range []string{"\x1b[2J", "\x07"} { + if strings.Contains(out, bad) { + t.Fatalf("the Host line carries %q from the host check:\n%q", bad, out) + } + } + if m.views[0].Unavailable != "needs zsh [2J " { + t.Fatalf("Unavailable = %q, want it cleaned when the model is built", m.views[0].Unavailable) + } +} + +func TestFilterHeaderIsNeutralised(t *testing.T) { + m := sized(newTestModel(sampleViews()), 100, 30) + m.filter, m.filtering = "a\x1b[2Jb", true // as if set by a path that skipped the key handler + + if out := m.View().Content; strings.Contains(out, "\x1b[2J") { + t.Fatalf("the filter text reached the header raw:\n%q", out) + } +} From cabac615e59de8dd18e580f7022d8daca9a0ab51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:26:39 +0200 Subject: [PATCH 3/9] fix: wrap long status messages and cap the too-small notice A rejection such as the managed-shells message was cut at 80 columns. The footer now wraps it over up to three lines, and the body shrinks by the same amount. The "Terminal too small" line is cut to the terminal width. --- internal/tui/optionsview.go | 2 +- internal/tui/status_test.go | 73 +++++++++++++++++++++++++++++++++++++ internal/tui/view.go | 47 +++++++++++++++++++++--- internal/tui/view_test.go | 2 +- 4 files changed, 117 insertions(+), 7 deletions(-) create mode 100644 internal/tui/status_test.go diff --git a/internal/tui/optionsview.go b/internal/tui/optionsview.go index 80d5f12..1e6541f 100644 --- a/internal/tui/optionsview.go +++ b/internal/tui/optionsview.go @@ -53,7 +53,7 @@ func (m Model) renderOptionsHeader() string { func (m Model) renderOptionsFooter() string { switch { case m.status != "": - return statusStyle.Render(ansi.Truncate("! "+m.status, m.width, "…")) + return m.renderStatus() case m.pending: return dimStyle.Inline(true).MaxWidth(m.width).Render("saving…") case m.options.editing: diff --git a/internal/tui/status_test.go b/internal/tui/status_test.go new file mode 100644 index 0000000..6bbf8e0 --- /dev/null +++ b/internal/tui/status_test.go @@ -0,0 +1,73 @@ +package tui + +import ( + "errors" + "strings" + "testing" + + "github.com/charmbracelet/x/ansi" +) + +// The message modedit really produces when a module cannot run with the managed +// shells; it is longer than an 80-column terminal. +const longRejection = `module "zshonly" only supports zsh, but none of your managed shells (bash) do — install one of those shells first` + +// footerText joins the lines below the body (everything after the last line +// that is part of the box or table) into one string with single spaces. +func footerText(out string, lines int) string { + all := strings.Split(out, "\n") + return strings.Join(strings.Fields(strings.Join(all[len(all)-lines:], " ")), " ") +} + +func TestALongRejectionIsFullyReadableInTheBrowser(t *testing.T) { + m, b := newBackedModel(sampleViews()) + b.toggleErr = errors.New(longRejection) + m = space(press(t, sized(m, 80, 20), "down", "down")) + + out := plain(m) + if got := footerText(out, 2); got != "! "+longRejection { + t.Fatalf("the footer reads %q, want the whole message", got) + } + assertFits(t, out, 80, 20) +} + +func TestALongRejectionIsFullyReadableOnTheOptionsScreen(t *testing.T) { + m, b := withOptions() + b.setOptionErr = errors.New(strings.Repeat("reason ", 20) + "end") + m = onOption(t, openFzfOptions(t, m), "prefix") + m = press(t, m, "enter", "x") + m = settleKey(t, m, "enter") + + out := plain(m) + if got := footerText(out, 3); !strings.HasSuffix(got, "end") || !strings.HasPrefix(got, "! reason") { + t.Fatalf("the footer reads %q, want the whole message", got) + } + assertFits(t, out, 80, 20) +} + +func TestAnAbsurdlyLongMessageIsCutWithAnEllipsisInsteadOfEatingTheScreen(t *testing.T) { + m, b := newBackedModel(sampleViews()) + b.toggleErr = errors.New(strings.Repeat("word ", 200)) + m = space(press(t, sized(m, 80, 20), "down")) + + out := plain(m) + lines := strings.Split(out, "\n") + if !strings.HasSuffix(strings.TrimRight(lines[len(lines)-1], " "), "…") { + t.Fatalf("the last footer line must end with an ellipsis:\n%s", out) + } + assertFits(t, out, 80, 20) +} + +func TestTheTooSmallNoticeNeverExceedsTheTerminalWidth(t *testing.T) { + for _, size := range [][2]int{{40, 10}, {20, 5}, {10, 3}, {79, 19}} { + out := plain(sized(newTestModel(sampleViews()), size[0], size[1])) + for _, line := range strings.Split(out, "\n") { + if w := ansi.StringWidth(line); w > size[0] { + t.Fatalf("%dx%d: the notice is %d cells wide: %q", size[0], size[1], w, line) + } + } + if !strings.Contains(out, "Terminal too small") && size[0] >= 20 { + t.Fatalf("%dx%d: the notice is missing: %q", size[0], size[1], out) + } + } +} diff --git a/internal/tui/view.go b/internal/tui/view.go index 22cc353..098afc3 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -41,8 +41,8 @@ func (m Model) render() string { case m.width == 0 || m.height == 0: return "" // the size is unknown until the first tea.WindowSizeMsg case m.width < minWidth || m.height < minHeight: - return fmt.Sprintf("Terminal too small: need at least %dx%d, have %dx%d", - minWidth, minHeight, m.width, m.height) + return ansi.Truncate(fmt.Sprintf("Terminal too small: need at least %dx%d, have %dx%d", + minWidth, minHeight, m.width, m.height), m.width, "…") } switch m.screen { @@ -52,7 +52,7 @@ func (m Model) render() string { return m.renderOptions() } - bodyHeight := m.height - headerLines - footerLines + bodyHeight := m.bodyRows() rows := bodyHeight - boxChromeV rightWidth := m.width - listWidth @@ -112,7 +112,7 @@ func (m Model) textCursor() *tea.Cursor { func (m Model) renderFooter() string { if m.status != "" { - return statusStyle.Render(ansi.Truncate("! "+m.status, m.width, "…")) + return m.renderStatus() } if m.pending { return dimStyle.Inline(true).MaxWidth(m.width).Render("saving…") @@ -126,7 +126,44 @@ func (m Model) renderFooter() string { // bodyRows is how many lines fit between the header and the footer. func (m Model) bodyRows() int { - return max(m.height-headerLines-footerLines, 1) + return max(m.height-headerLines-m.footerHeight(), 1) +} + +// maxStatusLines is how many lines an error message may take in the footer. +const maxStatusLines = 3 + +// statusLines is the status message, wrapped to the terminal width so a long +// reason stays readable; one that still does not fit in maxStatusLines ends +// with an ellipsis. It is nil when there is no message. +func (m Model) statusLines() []string { + if m.status == "" { + return nil + } + lines := strings.Split(ansi.Wrap("! "+m.status, max(m.width, 1), ""), "\n") + if len(lines) > maxStatusLines { + lines = lines[:maxStatusLines] + last := strings.TrimRight(lines[maxStatusLines-1], " ") + lines[maxStatusLines-1] = ansi.Truncate(last, max(m.width-1, 1), "") + "…" + } + return lines +} + +// footerHeight is how many lines the footer takes: one, or more while a long +// message is shown. +func (m Model) footerHeight() int { + if m.screen == screenPlan { + return footerLines // the plan screen has no status line + } + return max(len(m.statusLines()), footerLines) +} + +// renderStatus draws the wrapped status message. +func (m Model) renderStatus() string { + lines := m.statusLines() + for i, line := range lines { + lines[i] = statusStyle.Render(line) + } + return strings.Join(lines, "\n") } // changesText says how many modules differ from the state the browser started diff --git a/internal/tui/view_test.go b/internal/tui/view_test.go index 0d5c081..ab25520 100644 --- a/internal/tui/view_test.go +++ b/internal/tui/view_test.go @@ -13,7 +13,7 @@ func TestViewIsEmptyUntilTheTerminalSizeIsKnown(t *testing.T) { } func TestViewShowsATooSmallMessageBelowTheMinimumSize(t *testing.T) { - cases := []struct{ w, h int }{{79, 20}, {80, 19}, {10, 5}} + cases := []struct{ w, h int }{{79, 20}, {80, 19}, {80, 5}} for _, c := range cases { out := plain(sized(newTestModel(sampleViews()), c.w, c.h)) if !strings.Contains(out, "Terminal too small: need at least 80x20") { From fc116d20012b7fc858fddc6c8272366340f9c201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:27:06 +0200 Subject: [PATCH 4/9] fix: indent wrapped detail lines under their value Also say in the Host line that a module that only lacks OS support can still be enabled, but apply skips it. --- internal/modedit/views.go | 3 +- internal/modedit/views_test.go | 2 +- internal/tui/testdata/browser-rejected.golden | 2 +- .../tui/testdata/browser-unavailable.golden | 2 +- internal/tui/view.go | 25 +++++++++------- internal/tui/view_test.go | 29 +++++++++++++++++++ 6 files changed, 49 insertions(+), 14 deletions(-) diff --git a/internal/modedit/views.go b/internal/modedit/views.go index 2b97e72..2afb18e 100644 --- a/internal/modedit/views.go +++ b/internal/modedit/views.go @@ -177,7 +177,8 @@ func (ed Editor) packageState(mf module.Manifest) PackageState { func (ed Editor) unavailableReason(cfg config.Config, mf module.Manifest) string { osName := string(ed.Engine.Platform.OS) if !contains(mf.Platforms, osName) { - return fmt.Sprintf("not supported on %s (module supports %s)", osName, strings.Join(mf.Platforms, ", ")) + return fmt.Sprintf("not supported on %s (module supports %s); you can enable it, but apply skips it", + osName, strings.Join(mf.Platforms, ", ")) } managed := engine.ManagedShells(cfg, ed.Engine.Platform) if !sharesAny(mf.Shells, managed) { diff --git a/internal/modedit/views_test.go b/internal/modedit/views_test.go index caa79d2..7c6a6d3 100644 --- a/internal/modedit/views_test.go +++ b/internal/modedit/views_test.go @@ -224,7 +224,7 @@ func TestViewsExplainAModuleThatNeedsAnotherOS(t *testing.T) { if err != nil { t.Fatalf("Views: %v", err) } - want := "not supported on linux (module supports macos)" + want := "not supported on linux (module supports macos); you can enable it, but apply skips it" if got := viewByID(t, views, "macosonly").Unavailable; got != want { t.Fatalf("Unavailable = %q, want %q", got, want) } diff --git a/internal/tui/testdata/browser-rejected.golden b/internal/tui/testdata/browser-rejected.golden index 4fc5fb7..7f3881b 100644 --- a/internal/tui/testdata/browser-rejected.golden +++ b/internal/tui/testdata/browser-rejected.golden @@ -10,7 +10,7 @@ omnishell 3 modules · 0 changes since start │ ││ Platforms: linux │ │ ││ Shells: zsh │ │ ││ Host: needs zsh, but your managed shells │ -│ ││ are bash │ +│ ││ are bash │ │ ││ Options: 0 │ │ ││ Homepage: — │ │ ││ │ diff --git a/internal/tui/testdata/browser-unavailable.golden b/internal/tui/testdata/browser-unavailable.golden index 1ef5a2b..2b988c5 100644 --- a/internal/tui/testdata/browser-unavailable.golden +++ b/internal/tui/testdata/browser-unavailable.golden @@ -10,7 +10,7 @@ omnishell 3 modules · 0 changes since start │ ││ Platforms: linux │ │ ││ Shells: zsh │ │ ││ Host: needs zsh, but your managed shells │ -│ ││ are bash │ +│ ││ are bash │ │ ││ Options: 0 │ │ ││ Homepage: — │ │ ││ │ diff --git a/internal/tui/view.go b/internal/tui/view.go index 098afc3..7380777 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -228,28 +228,33 @@ func (m Model) renderDetail(rows, width int) string { "", v.Description, "", - field("Status", string(v.Status)), - field("Packages", string(v.Packages)), - field("Platforms", strings.Join(v.Platforms, ", ")), - field("Shells", strings.Join(v.Shells, ", ")), + field("Status", string(v.Status), width), + field("Packages", string(v.Packages), width), + field("Platforms", strings.Join(v.Platforms, ", "), width), + field("Shells", strings.Join(v.Shells, ", "), width), } if v.Unavailable != "" { - lines = append(lines, field("Host", v.Unavailable)) + lines = append(lines, field("Host", v.Unavailable, width)) } lines = append(lines, - field("Options", fmt.Sprint(v.OptionCount)), - field("Homepage", homepage), + field("Options", fmt.Sprint(v.OptionCount), width), + field("Homepage", homepage, width), ) text := strings.Join(lines, "\n") return lipgloss.NewStyle().Width(width).MaxHeight(rows).Render(text) } -// field renders one "Label: value" line of the detail pane. -func field(label, value string) string { - return fmt.Sprintf("%-10s %s", label+":", value) +// field renders one "Label: value" entry of the detail pane, wrapped to width +// with the continuation lines under the value rather than under the label. +func field(label, value string, width int) string { + return strings.Join(wrapValue(fmt.Sprintf("%-10s ", label+":"), value, width, maxFieldLines), "\n") } +// maxFieldLines is a safety cap on how many lines one detail field may take; +// the pane clips to its own height anyway. +const maxFieldLines = 12 + func statusBox(s modedit.Status) string { switch s { case modedit.StatusEnabled: diff --git a/internal/tui/view_test.go b/internal/tui/view_test.go index ab25520..5e4204c 100644 --- a/internal/tui/view_test.go +++ b/internal/tui/view_test.go @@ -289,3 +289,32 @@ func TestViewGoldenFiles(t *testing.T) { assertGolden(t, name, plain(m)) } } + +// A detail line that wraps continues under its value, not under its label. +func TestDetailFieldsWrapWithAHangingIndent(t *testing.T) { + views := sampleViews() + views[2].Unavailable = "not supported on linux (module supports macos); you can enable it, but apply skips it" + m := sized(newTestModel(views), 80, 20) + m = press(t, m, "down", "down") + + lines := strings.Split(plain(m), "\n") + var host int + for i, line := range lines { + if strings.Contains(line, "Host:") { + host = i + } + } + if host == 0 { + t.Fatalf("no Host line:\n%s", plain(m)) + } + first := lines[host] + next := lines[host+1] + column := strings.Index(first, "not supported") + if column < 0 || !strings.HasPrefix(next[strings.Index(first, "Host:")-0:], strings.Repeat(" ", column-strings.Index(first, "Host:"))) { + t.Fatalf("the second line must start under the value:\n%s\n%s", first, next) + } + if strings.TrimSpace(next[column:]) == "" || strings.TrimSpace(next[strings.Index(first, "Host:"):column]) != "" { + t.Fatalf("the continuation must carry text under the value, nothing under the label:\n%s\n%s", first, next) + } + assertFits(t, plain(m), 80, 20) +} From d4a1849d8273ec7bc6f08ff0b127bf09df4560fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:28:02 +0200 Subject: [PATCH 5/9] fix: wrap, scroll and guard the plan screen Long plan lines and errors wrap instead of being cut, and the error scrolls like the plan. y/enter only hand over to apply while the plan is on screen, a second plan is not requested while one is still being computed, and the footer says apply asks again. --- internal/tui/model.go | 1 + internal/tui/plan.go | 20 +++- internal/tui/plan_test.go | 101 +++++++++++++++++++-- internal/tui/planview.go | 54 ++++++++--- internal/tui/testdata/plan-error.golden | 40 ++++---- internal/tui/testdata/plan-ready.golden | 40 ++++---- internal/tui/testdata/plan-scrolled.golden | 40 ++++---- 7 files changed, 211 insertions(+), 85 deletions(-) diff --git a/internal/tui/model.go b/internal/tui/model.go index dae4418..9547e00 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -35,6 +35,7 @@ type Model struct { optionNow map[string]map[string]string plan planState planSeq int // counts plan requests, so a stale answer can be told from the current one + planRunning bool // true from asking for a plan until its answer arrives, even if the user left applyRequested bool // set when the user confirmed on the plan screen width int // 0 until the first tea.WindowSizeMsg height int diff --git a/internal/tui/plan.go b/internal/tui/plan.go index 6125856..0ba89f8 100644 --- a/internal/tui/plan.go +++ b/internal/tui/plan.go @@ -51,13 +51,15 @@ func planCmd(b Backend, seq int) tea.Cmd { } // openPlan switches to the plan screen and starts computing the plan. It does -// nothing while a toggle is being written: the plan would describe a config -// that is about to change. +// nothing while a toggle is being written (the plan would describe a config +// that is about to change) or while an earlier computation is still running +// (it cannot be cancelled and queries the package manager). func (m Model) openPlan() (tea.Model, tea.Cmd) { - if m.pending { + if m.pending || m.planRunning { return m, nil } m.screen = screenPlan + m.planRunning = true m.planSeq++ m.plan = planState{loading: true} return m, planCmd(m.backend, m.planSeq) @@ -66,6 +68,7 @@ func (m Model) openPlan() (tea.Model, tea.Cmd) { // applyPlan stores a computed plan, unless the user has left the plan screen // (or asked for a newer plan) since it was requested. func (m Model) applyPlan(msg planMsg) Model { + m.planRunning = false if m.screen != screenPlan || msg.seq != m.planSeq { return m } @@ -96,7 +99,7 @@ func (m Model) updatePlan(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { m.planSeq++ // whatever is still being computed is no longer wanted m.plan = planState{} case "y", "enter": - if m.plan.canApply() { + if m.plan.canApply() && m.planVisible() { m.applyRequested = true return m, tea.Quit } @@ -116,11 +119,18 @@ func (m Model) updatePlan(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m, nil } +// planVisible reports whether the plan is on screen: a terminal below the +// minimum size shows only the too-small notice, and confirming something that +// cannot be seen would be blind. +func (m Model) planVisible() bool { + return m.width >= minWidth && m.height >= minHeight +} + // planRows is how many plan lines fit on the screen. func (m Model) planRows() int { return m.bodyRows() } func (m Model) maxPlanOffset() int { - return max(len(m.plan.lines)-m.planRows(), 0) + return max(len(m.planDisplay())-m.planRows(), 0) } // scrollPlan moves the visible window by delta lines, stopping at both ends. diff --git a/internal/tui/plan_test.go b/internal/tui/plan_test.go index 83ea228..b8fae9b 100644 --- a/internal/tui/plan_test.go +++ b/internal/tui/plan_test.go @@ -151,19 +151,29 @@ func TestAPlanThatArrivesAfterGoingBackIsDropped(t *testing.T) { } } -func TestAnOldPlanDoesNotAnswerANewerRequest(t *testing.T) { - m, _ := withPlan(PlanPreview{Text: "fresh", NeedsApply: true}, nil) +// A plan computation cannot be cancelled, and it queries the package manager: +// asking again while one is still running would pile up queries. +func TestAIsIgnoredWhileAnEarlierPlanIsStillBeingComputed(t *testing.T) { + m, b := withPlan(PlanPreview{Text: "fresh", NeedsApply: true}, nil) first, firstCmd := m.Update(key("a")) back := press(t, first.(Model), "esc") - second, secondCmd := back.Update(key("a")) - afterOld := settle(second.(Model), firstCmd) - if !afterOld.plan.loading { - t.Fatal("the answer to the first request must not fill the second screen") + again, againCmd := back.Update(key("a")) + if againCmd != nil || again.(Model).screen != screenBrowser { + t.Fatal("a must be ignored while the earlier plan is still being computed") } - afterNew := settle(afterOld, secondCmd) - if afterNew.plan.loading || len(afterNew.plan.lines) == 0 { - t.Fatal("the answer to the second request must be shown") + + afterOld := settle(again.(Model), firstCmd) // the old answer arrives and is dropped + if afterOld.screen != screenBrowser { + t.Fatal("the dropped answer must leave the browser alone") + } + + next, cmd := afterOld.Update(key("a")) + if cmd == nil || next.(Model).screen != screenPlan { + t.Fatal("a must work again once the earlier computation has finished") + } + if b.planCalls != 1 { + t.Fatalf("planCalls = %d, want only the first request to have run", b.planCalls) } } @@ -287,3 +297,76 @@ func TestResizingKeepsThePlanScrollPositionInRange(t *testing.T) { t.Fatalf("offset %d is past the end (%d) after growing to %d rows", m.plan.offset, m.maxPlanOffset(), rows) } } + +func TestYAndEnterDoNothingWhenThePlanIsNotOnScreen(t *testing.T) { + for _, name := range []string{"y", "enter"} { + m, _ := withPlan(PlanPreview{Text: "Plan", NeedsApply: true}, nil) + m = sized(openPlan(m), 70, 15) // the terminal shrank: the too-small notice replaces the plan + + next, cmd := m.Update(key(name)) + + if cmd != nil || next.(Model).applyRequested { + t.Fatalf("%s must not hand over to apply while the plan is hidden", name) + } + } +} + +func TestThePlanFooterSaysApplyAsksAgain(t *testing.T) { + m, _ := withPlan(PlanPreview{Text: "Plan", NeedsApply: true}, nil) + out := plain(openPlan(m)) + + if !strings.Contains(out, "y/enter continue to apply (apply asks again)") { + t.Fatalf("the footer must say that apply asks once more:\n%s", out) + } + assertFits(t, out, 80, 20) +} + +func TestLongPlanLinesWrapInsteadOfBeingCut(t *testing.T) { + long := " install " + strings.Repeat("alpha-beta ", 20) + "THE-END" + m, _ := withPlan(PlanPreview{Text: "Plan\n" + long, NeedsApply: true}, nil) + out := plain(openPlan(m)) + + if !strings.Contains(strings.Join(strings.Fields(out), " "), "alpha-beta THE-END") { + t.Fatalf("the end of a long line is missing:\n%s", out) + } + assertFits(t, out, 80, 20) + for _, line := range strings.Split(out, "\n")[2:6] { + if strings.TrimSpace(line) != "" && !strings.HasPrefix(line, " ") { + t.Fatalf("a wrapped line lost its indentation: %q", line) + } + } +} + +func TestWrappedPlanLinesAreWhatScrollingAndTheHeaderCount(t *testing.T) { + text := strings.Repeat(strings.Repeat("word ", 40)+"\n", 10) // ten lines, three screen lines each + m, _ := withPlan(PlanPreview{Text: text, NeedsApply: true}, nil) + m = openPlan(m) + + if got := len(m.planDisplay()); got < 20 { + t.Fatalf("display lines = %d, want the wrapped lines counted", got) + } + m = press(t, m, "end") + if m.plan.offset != m.maxPlanOffset() || m.maxPlanOffset() == 0 { + t.Fatalf("offset=%d max=%d, want the end reachable", m.plan.offset, m.maxPlanOffset()) + } + assertFits(t, plain(m), 80, 20) +} + +func TestALongPlanErrorCanBeScrolled(t *testing.T) { + words := make([]string, 600) + for i := range words { + words[i] = fmt.Sprintf("w%03d", i) + } + m, _ := withPlan(PlanPreview{}, errors.New(strings.Join(words, " "))) + m = openPlan(m) + + if strings.Contains(plain(m), "w599") { + t.Fatal("the end of the error should be below the first screen") + } + m = press(t, m, "end") + out := plain(m) + if !strings.Contains(out, "w599") { + t.Fatalf("scrolling must reach the end of the error:\n%s", out) + } + assertFits(t, out, 80, 20) +} diff --git a/internal/tui/planview.go b/internal/tui/planview.go index e8d2c74..a407338 100644 --- a/internal/tui/planview.go +++ b/internal/tui/planview.go @@ -26,18 +26,50 @@ func (m Model) renderPlan() string { ) } -// planBody returns at most rows lines: a waiting note, the error, or the -// visible window of the plan. +// planBody returns at most rows lines: the visible window of what planDisplay +// shows. func (m Model) planBody(rows int) []string { + lines := m.planDisplay() + start := min(m.plan.offset, len(lines)) + end := min(start+rows, len(lines)) + return append([]string(nil), lines[start:end]...) +} + +// planDisplay is everything the plan screen has to show, wrapped to the +// terminal width: a waiting note, the error, or the plan. Long lines wrap +// instead of being cut, and the error scrolls like the plan does. +func (m Model) planDisplay() []string { switch { case m.plan.loading: return []string{dimStyle.Render("computing plan…")} case m.plan.err != "": - wrapped := lipgloss.NewStyle().Width(m.width).Render("Could not compute the plan: " + m.plan.err) - return clip(strings.Split(wrapped, "\n"), rows) + return strings.Split(ansi.Wrap("Could not compute the plan: "+m.plan.err, max(m.width, 1), ""), "\n") + } + var out []string + for _, line := range m.plan.lines { + out = append(out, wrapIndented(line, max(m.width, 1))...) + } + return out +} + +// wrapIndented wraps line to width cells. A line that starts with spaces keeps +// them, and its continuation lines are indented two cells further. +func wrapIndented(line string, width int) []string { + trimmed := strings.TrimLeft(line, " ") + indent := len(line) - len(trimmed) + if ansi.StringWidth(line) <= width || indent >= width-10 { + return strings.Split(ansi.Wrap(line, width, ""), "\n") } - end := min(m.plan.offset+rows, len(m.plan.lines)) - return append([]string(nil), m.plan.lines[m.plan.offset:end]...) + avail := width - indent - 2 + parts := strings.Split(ansi.Wrap(trimmed, avail, ""), "\n") + for i, part := range parts { + if i == 0 { + parts[i] = strings.Repeat(" ", indent) + part + } else { + parts[i] = strings.Repeat(" ", indent+2) + part + } + } + return parts } func clip(lines []string, n int) []string { @@ -49,9 +81,9 @@ func clip(lines []string, n int) []string { func (m Model) renderPlanHeader(rows int) string { line := titleStyle.Render("omnishell") + dimStyle.Render(" plan preview") - if len(m.plan.lines) > rows { - last := min(m.plan.offset+rows, len(m.plan.lines)) - line += dimStyle.Render(fmt.Sprintf(" lines %d-%d of %d", m.plan.offset+1, last, len(m.plan.lines))) + if all := len(m.planDisplay()); all > rows { + last := min(m.plan.offset+rows, all) + line += dimStyle.Render(fmt.Sprintf(" lines %d-%d of %d", m.plan.offset+1, last, all)) } return lipgloss.NewStyle().Inline(true).MaxWidth(m.width).Render(line) } @@ -62,9 +94,9 @@ func (m Model) renderPlanFooter() string { case m.plan.loading: help = "esc back · q quit" case m.plan.err != "": - help = "esc back · q quit" + help = "↑/↓ scroll · esc back · q quit" case m.plan.canApply(): - help = "↑/↓ scroll · y/enter continue to apply · esc back · q quit" + help = "↑/↓ scroll · y/enter continue to apply (apply asks again) · esc back · q quit" } return dimStyle.Inline(true).MaxWidth(m.width).Render(help) } diff --git a/internal/tui/testdata/plan-error.golden b/internal/tui/testdata/plan-error.golden index eef4c2f..62e4904 100644 --- a/internal/tui/testdata/plan-error.golden +++ b/internal/tui/testdata/plan-error.golden @@ -1,20 +1,20 @@ -omnishell plan preview -Could not compute the plan: module "fzf": option ctrl_r: "yes please" is not a -bool - - - - - - - - - - - - - - - - -esc back · q quit \ No newline at end of file +omnishell plan preview +Could not compute the plan: module "fzf": option ctrl_r: "yes please" is not a +bool + + + + + + + + + + + + + + + + +↑/↓ scroll · esc back · q quit \ No newline at end of file diff --git a/internal/tui/testdata/plan-ready.golden b/internal/tui/testdata/plan-ready.golden index 18e0f80..58d792a 100644 --- a/internal/tui/testdata/plan-ready.golden +++ b/internal/tui/testdata/plan-ready.golden @@ -1,20 +1,20 @@ -omnishell plan preview -Plan (package manager: apt, shells: bash) - - install completion snippet: bash - install fzf snippet: bash packages: fzf - skip macosonly not supported on linux - -2 to install, 0 to update, 1 skipped - - - - - - - - - - - -↑/↓ scroll · y/enter continue to apply · esc back · q quit \ No newline at end of file +omnishell plan preview +Plan (package manager: apt, shells: bash) + + install completion snippet: bash + install fzf snippet: bash packages: fzf + skip macosonly not supported on linux + +2 to install, 0 to update, 1 skipped + + + + + + + + + + + +↑/↓ scroll · y/enter continue to apply (apply asks again) · esc back · q quit \ No newline at end of file diff --git a/internal/tui/testdata/plan-scrolled.golden b/internal/tui/testdata/plan-scrolled.golden index 6b447fc..d0561a1 100644 --- a/internal/tui/testdata/plan-scrolled.golden +++ b/internal/tui/testdata/plan-scrolled.golden @@ -1,20 +1,20 @@ -omnishell plan preview lines 20-37 of 100 -line 020 -line 021 -line 022 -line 023 -line 024 -line 025 -line 026 -line 027 -line 028 -line 029 -line 030 -line 031 -line 032 -line 033 -line 034 -line 035 -line 036 -line 037 -↑/↓ scroll · y/enter continue to apply · esc back · q quit \ No newline at end of file +omnishell plan preview lines 20-37 of 100 +line 020 +line 021 +line 022 +line 023 +line 024 +line 025 +line 026 +line 027 +line 028 +line 029 +line 030 +line 031 +line 032 +line 033 +line 034 +line 035 +line 036 +line 037 +↑/↓ scroll · y/enter continue to apply (apply asks again) · esc back · q quit \ No newline at end of file From 0b7bd19952e6325a51bbe0e2a6a3a5799bedfac3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:29:08 +0200 Subject: [PATCH 6/9] fix: keep the TUI truthful when a re-read fails after a write A toggle whose write succeeded but whose status re-read failed now shows the written state, and the options screen locks its value keys (r reads again) instead of computing the next change from stale rows. An external SIGINT ends the UI quietly. --- internal/tui/options.go | 50 ++++++++++++++++++++++++--- internal/tui/options_test.go | 66 ++++++++++++++++++++++++++++++++++++ internal/tui/optionsview.go | 2 ++ internal/tui/run.go | 14 ++++++-- internal/tui/run_test.go | 24 +++++++++++++ internal/tui/toggle.go | 22 ++++++++++-- internal/tui/toggle_test.go | 33 ++++++++++++++++++ 7 files changed, 202 insertions(+), 9 deletions(-) diff --git a/internal/tui/options.go b/internal/tui/options.go index 9f46a6f..f636d3e 100644 --- a/internal/tui/options.go +++ b/internal/tui/options.go @@ -16,6 +16,7 @@ type optionsState struct { cursor int editing bool // true while the user types a new value input string // the value typed so far + stale bool // true when a write went through but re-reading the rows failed pos int // the cursor within input, as a rune index } @@ -29,9 +30,10 @@ type optionsMsg struct { // optionWrittenMsg reports the outcome of writing one option: the error that // stopped it, or the module's options re-read after the write. type optionWrittenMsg struct { - id string - rows []modedit.OptionView - err error + id string + rows []modedit.OptionView + written bool // true once config.toml holds the new value, even if err is set + err error } // optionsCmd reads the options of a module in the background. @@ -50,7 +52,16 @@ func optionWriteCmd(b Backend, id, key, raw string) tea.Cmd { return optionWrittenMsg{id: id, err: err} } rows, err := b.Options(id) - return optionWrittenMsg{id: id, rows: rows, err: err} + return optionWrittenMsg{id: id, rows: rows, written: true, err: err} + } +} + +// optionRereadCmd reads a module's options again after a write whose re-read +// failed; it answers like a write, so the screen unlocks when it succeeds. +func optionRereadCmd(b Backend, id string) tea.Cmd { + return func() tea.Msg { + rows, err := b.Options(id) + return optionWrittenMsg{id: id, rows: rows, written: true, err: err} } } @@ -99,9 +110,17 @@ func (m Model) applyOptionWritten(msg optionWrittenMsg) Model { } if msg.err != nil { m.status = sanitize(msg.err.Error()) + if msg.written { + // The value is in config.toml but the rows are older than it: lock + // the keys that compute from them until a read succeeds. + m.options.stale = true + m.options.editing = false + m.options.input, m.options.pos = "", 0 + } return m } rows := sanitizeOptions(msg.rows) + m.options.stale = false m.options.rows = rows m.options.cursor = min(m.options.cursor, max(len(rows)-1, 0)) m.options.editing = false @@ -179,6 +198,9 @@ func (m Model) updateOptions(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { if m.options.editing { return m.updateOptionInput(msg) } + if m.options.stale { + return m.updateStaleOptions(msg) + } switch msg.String() { case "ctrl+c", "q": return m, tea.Quit @@ -264,3 +286,23 @@ func (m Model) writeOption(key, raw string) (tea.Model, tea.Cmd) { m.pending = true return m, optionWriteCmd(m.backend, m.options.id, key, raw) } + +// updateStaleOptions handles a key press while the rows are older than the file: +// moving, leaving and reading again work; everything that would write is off. +func (m Model) updateStaleOptions(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { + switch msg.String() { + case "ctrl+c", "q": + return m, tea.Quit + case "esc", "b": + m.screen = screenBrowser + m.options = optionsState{} + case "up", "k": + m.options.cursor = max(m.options.cursor-1, 0) + case "down", "j": + m.options.cursor = min(m.options.cursor+1, max(len(m.options.rows)-1, 0)) + case "r": + m.pending = true + return m, optionRereadCmd(m.backend, m.options.id) + } + return m, nil +} diff --git a/internal/tui/options_test.go b/internal/tui/options_test.go index d14a4e7..419fa81 100644 --- a/internal/tui/options_test.go +++ b/internal/tui/options_test.go @@ -509,3 +509,69 @@ func TestAnOptionsLoadResultThatArrivesOnAnotherScreenIsDropped(t *testing.T) { t.Fatalf("screen=%v pending=%v rows=%d, want the plan screen untouched and the wait over", got.screen, got.pending, len(got.options.rows)) } } + +// If the options cannot be re-read after a successful write, the rows are older +// than the file: a bool toggled from them would write what is already there. +func TestAfterAFailedRereadTheValueKeysAreLockedUntilAReadSucceeds(t *testing.T) { + m, b := withOptions() + m = openFzfOptions(t, m) // the cursor is on ctrl_r (bool, true) + b.optionsErr = errors.New("cannot re-read") + m = settleKey(t, m, "space") + if len(b.optionCalls) != 1 || !m.options.stale { + t.Fatalf("calls=%v stale=%v, want one write and the screen marked as out of date", b.optionCalls, m.options.stale) + } + + m = press(t, m, "space", "left", "right", "enter") + if len(b.optionCalls) != 1 || m.options.editing { + t.Fatalf("calls=%v editing=%v: the value keys must do nothing while the rows are out of date", b.optionCalls, m.options.editing) + } + m = press(t, m, "down", "up") // moving is harmless + if m.options.cursor != 0 { + t.Fatalf("cursor = %d, want movement to keep working", m.options.cursor) + } + if out := plain(m); !strings.Contains(out, "out of date") || !strings.Contains(out, "r re-read") { + t.Fatalf("the footer must say what is wrong and how to fix it:\n%s", out) + } + + m = settleKey(t, m, "r") // still failing + if !m.options.stale || m.pending { + t.Fatalf("stale=%v pending=%v, want it to stay locked while the read fails", m.options.stale, m.pending) + } + + b.optionsErr = nil + m = settleKey(t, m, "r") + if m.options.stale { + t.Fatal("a successful re-read must unlock the screen") + } + if got := optionRow(t, m, "ctrl_r").Value; got != "true" && got != "false" { + t.Fatalf("value = %q", got) + } + m = settleKey(t, m, "space") + if len(b.optionCalls) != 2 { + t.Fatalf("calls = %v, want the toggle to work again after the re-read", b.optionCalls) + } +} + +func TestAWriteThatFailsDoesNotMarkTheScreenAsOutOfDate(t *testing.T) { + m, b := withOptions() + b.setOptionErr = errors.New("nope") + m = settleKey(t, openFzfOptions(t, m), "space") + + if m.options.stale { + t.Fatal("the rows are still what the file holds after a rejected write") + } +} + +func TestLeavingTheOptionsScreenForgetsTheOutOfDateMark(t *testing.T) { + m, b := withOptions() + m = openFzfOptions(t, m) + b.optionsErr = errors.New("cannot re-read") + m = settleKey(t, m, "space") + b.optionsErr = nil + + m = press(t, m, "esc") + m = openFzfOptions(t, m) + if m.options.stale { + t.Fatal("a freshly opened screen has just been read") + } +} diff --git a/internal/tui/optionsview.go b/internal/tui/optionsview.go index 1e6541f..a99b76d 100644 --- a/internal/tui/optionsview.go +++ b/internal/tui/optionsview.go @@ -56,6 +56,8 @@ func (m Model) renderOptionsFooter() string { return m.renderStatus() case m.pending: return dimStyle.Inline(true).MaxWidth(m.width).Render("saving…") + case m.options.stale: + return dimStyle.Inline(true).MaxWidth(m.width).Render("values may be out of date · r re-read · esc back · q quit") case m.options.editing: return dimStyle.Inline(true).MaxWidth(m.width).Render("type a value · ←/→ move · enter save · esc cancel · ctrl+c quit") } diff --git a/internal/tui/run.go b/internal/tui/run.go index 5a883c6..9311ba7 100644 --- a/internal/tui/run.go +++ b/internal/tui/run.go @@ -1,6 +1,7 @@ package tui import ( + "errors" "fmt" "io" @@ -24,8 +25,17 @@ func Run(b Backend, in io.Reader, out io.Writer) (Result, error) { if err != nil { return Result{}, err } - final, err := tea.NewProgram(New(b, views), tea.WithInput(in), tea.WithOutput(out)).Run() - if err != nil { + return finish(tea.NewProgram(New(b, views), tea.WithInput(in), tea.WithOutput(out)).Run()) +} + +// finish turns what the program ended with into Run's result. An interrupt +// (SIGINT from outside) is the user leaving: it ends quietly and never leads on +// to apply. +func finish(final tea.Model, err error) (Result, error) { + switch { + case errors.Is(err, tea.ErrInterrupted): + return Result{}, nil + case err != nil: return Result{}, fmt.Errorf("run terminal UI: %w", err) } return resultOf(final), nil diff --git a/internal/tui/run_test.go b/internal/tui/run_test.go index 7ab31f3..6321d28 100644 --- a/internal/tui/run_test.go +++ b/internal/tui/run_test.go @@ -3,9 +3,12 @@ package tui import ( "bytes" "errors" + "fmt" "strings" "testing" "time" + + tea "charm.land/bubbletea/v2" ) func TestRunReturnsTheBackendErrorWithoutTouchingTheTerminal(t *testing.T) { @@ -59,3 +62,24 @@ func TestResultOfReadsTheUsersDecisionOffTheFinalModel(t *testing.T) { t.Fatal("a missing model must not request an apply") } } + +// SIGINT from outside (kill -INT) ends the program with tea.ErrInterrupted; +// that is the user leaving, not a failure to report. +func TestAnInterruptEndsTheUIWithoutAnError(t *testing.T) { + for _, err := range []error{tea.ErrInterrupted, fmt.Errorf("wrapped: %w", tea.ErrInterrupted)} { + result, got := finish(Model{applyRequested: true}, err) + if got != nil { + t.Fatalf("finish(%v) = %v, want no error", err, got) + } + if result.ApplyRequested { + t.Fatal("an interrupt must never request an apply, even if the plan was confirmed") + } + } +} + +func TestOtherRunErrorsStillSurface(t *testing.T) { + boom := errors.New("boom") + if _, got := finish(nil, boom); !errors.Is(got, boom) { + t.Fatalf("finish = %v, want the error to be wrapped and returned", got) + } +} diff --git a/internal/tui/toggle.go b/internal/tui/toggle.go index 0c01fad..1c1f06e 100644 --- a/internal/tui/toggle.go +++ b/internal/tui/toggle.go @@ -11,6 +11,9 @@ import ( // toggledMsg reports the outcome of flipping one module: either the error that // stopped it, or every module's state as config.toml now has it. type toggledMsg struct { + id string // the module that was flipped + enable bool // what it was flipped to + written bool // true once config.toml holds the new state, even if err is set statuses map[string]modedit.Status err error } @@ -41,10 +44,10 @@ func toggleCmd(b Backend, id string, enable bool) tea.Cmd { err = b.Disable(id) } if err != nil { - return toggledMsg{err: err} + return toggledMsg{id: id, enable: enable, err: err} } statuses, err := b.Statuses() - return toggledMsg{statuses: statuses, err: err} + return toggledMsg{id: id, enable: enable, written: true, statuses: statuses, err: err} } } @@ -54,11 +57,24 @@ func toggleCmd(b Backend, id string, enable bool) tea.Cmd { // depend on the status. func (m Model) applyToggled(msg toggledMsg) Model { m.pending = false + m.views = slices.Clone(m.views) if msg.err != nil { m.status = sanitize(msg.err.Error()) + if msg.written { + // The write went through and only the re-read failed: show what + // config.toml now holds rather than the state from before. + next := modedit.StatusDisabled + if msg.enable { + next = modedit.StatusEnabled + } + for i, v := range m.views { + if v.ID == msg.id { + m.views[i].Status = next + } + } + } return m } - m.views = slices.Clone(m.views) for i, v := range m.views { if status, ok := msg.statuses[v.ID]; ok { m.views[i].Status = status diff --git a/internal/tui/toggle_test.go b/internal/tui/toggle_test.go index 70ababd..e24aa1f 100644 --- a/internal/tui/toggle_test.go +++ b/internal/tui/toggle_test.go @@ -270,3 +270,36 @@ func statusOfID(m Model, id string) modedit.Status { } return "" } + +// The write succeeded, only the re-read failed: config.toml has changed, so the +// checkbox must show that and not the state from before the write. +func TestFailedRefreshAfterASuccessfulWriteStillShowsTheNewState(t *testing.T) { + m, b := newBackedModel(sampleViews()) + b.statusesErr = errors.New("cannot re-read config") + + m = space(m) // completion was enabled: now disabled + + if got := m.views[0].Status; got != modedit.StatusDisabled { + t.Fatalf("status = %q, want the checkbox to show what was written", got) + } + if m.changes() != 1 { + t.Fatalf("changes = %d, want the written change counted", m.changes()) + } + b.statusesErr = nil + m = press(t, m, "down") + m = space(m) // fzf: now enabled + if m.views[0].Status != modedit.StatusDisabled || m.views[1].Status != modedit.StatusEnabled { + t.Fatalf("after a later refresh the states are %q and %q", m.views[0].Status, m.views[1].Status) + } +} + +func TestAWriteThatFailedLeavesTheCheckboxAsItWas(t *testing.T) { + m, b := newBackedModel(sampleViews()) + b.toggleErr = errors.New("read-only") + + m = space(m) + + if m.views[0].Status != modedit.StatusEnabled || m.changes() != 0 { + t.Fatalf("a rejected write must change nothing: status=%q changes=%d", m.views[0].Status, m.changes()) + } +} From 0f19c20b6391ac863e30ac30af66f74a628f38b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:29:38 +0200 Subject: [PATCH 7/9] fix: say so when apply finds nothing to do after a TUI hand-off --- internal/cli/apply.go | 3 +++ internal/cli/tui_test.go | 25 +++++++++++++++++++++++++ internal/engine/apply.go | 4 ++++ 3 files changed, 32 insertions(+) diff --git a/internal/cli/apply.go b/internal/cli/apply.go index c0b5b90..29efb10 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -99,6 +99,9 @@ func runApply(cmd *cobra.Command, forceDryRun bool) error { } printApplySummary(out, res) + if res.NothingToDo { + _, _ = fmt.Fprintln(out, "Nothing to apply: no module or package changes are planned.") + } // --reload re-execs the login shell, but only after a fully successful, // non-dry-run apply and only in an interactive session. diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index f22b134..b7fe459 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -532,3 +532,28 @@ func TestTUIBackendOptionsOfAnUnknownModuleIsAnError(t *testing.T) { t.Fatalf("err = %v, want unknown module", err) } } + +// The plan said there was something to apply, but the config changed in +// between and apply finds nothing to do: that must be said, not left silent. +func TestTUIHandoffSaysWhenApplyFindsNothingToDo(t *testing.T) { + zshHostWithCompletion(t) + handoffRun(t, tui.Result{ApplyRequested: true}, nil) + askedQuestions(t, true) + + var out, errb bytes.Buffer + if code := cli.Execute([]string{"tui"}, &out, &errb); code != 0 { + t.Fatalf("first run: exit = %d (stderr: %s)", code, errb.String()) + } + if strings.Contains(out.String(), "Nothing to apply") { + t.Fatalf("the first apply had work to do, but said otherwise:\n%s", out.String()) + } + + out.Reset() + errb.Reset() + if code := cli.Execute([]string{"tui"}, &out, &errb); code != 0 { + t.Fatalf("second run: exit = %d (stderr: %s)", code, errb.String()) + } + if !strings.Contains(out.String(), "Nothing to apply: no module or package changes are planned.") { + t.Fatalf("a hand-off that finds nothing to do must say so; stdout:\n%s", out.String()) + } +} diff --git a/internal/engine/apply.go b/internal/engine/apply.go index 358867e..ce908a0 100644 --- a/internal/engine/apply.go +++ b/internal/engine/apply.go @@ -52,6 +52,9 @@ type Result struct { InitFilesWritten []string BackupDir string Changed bool + // NothingToDo is true when apply took its idempotent no-op path: no planned + // changes and init and rc files that still match the lockfile. + NothingToDo bool } // Apply brings the system to the desired state described by cfg. It loads the @@ -89,6 +92,7 @@ func (e Engine) Apply(cfg config.Config, cfgPath, lockPath string, opts ApplyOpt res.Modules = summarise(plan, pd) return res, ErrDegraded } + res.NothingToDo = true return res, nil } From b4d8e78c0501556f66c35172ccd43f352c68b189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:30:06 +0200 Subject: [PATCH 8/9] docs: describe the TUI polish and wrap the tui help text Rewrap the tui --help text to 80 columns, mention the OS-only case in the README and Host line, name Enter next to y in CLAUDE.md, rewrap the changelog entry and keep editors from stripping the golden files. --- .gitattributes | 3 +++ CHANGELOG.md | 16 +++++++++++----- CLAUDE.md | 14 ++++++++------ README.md | 2 +- internal/cli/tui.go | 11 ++++++----- internal/cli/tui_test.go | 15 +++++++++++++++ 6 files changed, 44 insertions(+), 17 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..895ff1c --- /dev/null +++ b/.gitattributes @@ -0,0 +1,3 @@ +# The TUI golden files compare rendered screens, whose lines end in spaces. +# Keep editors and `git diff --check` from treating them as errors. +internal/tui/testdata/*.golden -whitespace diff --git a/CHANGELOG.md b/CHANGELOG.md index f890177..8c383e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,13 +9,19 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). homepage, package status, platforms, shells) with a `/` filter. Space enables or disables the selected module in `config.toml`, like `enable` / `disable`. `o` edits the module's options (bool, enum, string and int), validated like - `omnishell set`. `a` previews the plan; confirming it closes the UI and runs `omnishell apply`, - which still asks before it changes anything. Modules that cannot run on this - host are dimmed with the reason shown. It needs an interactive terminal. It - brings the first runtime dependencies, Bubble Tea and Lip Gloss, so the - binary grows by about 1.5 MiB. + `omnishell set`; text fields have a movable cursor and accept pasted text. + `a` previews the plan; confirming it closes the UI and runs + `omnishell apply`, which still asks before it changes anything. Modules that + cannot run on this host are dimmed with the reason shown; one that only lacks + OS support can still be enabled, but `apply` skips it. It needs an + interactive terminal. It brings the first runtime dependencies, Bubble Tea + and Lip Gloss, so the binary grows by about 1.5 MiB. +- `apply` now says "Nothing to apply" when it finds nothing to do, instead of + printing nothing. ### Fixed +- `list` no longer prints control characters from a module manifest's text to + the terminal. - `list`, `diff` / `apply --dry-run` and `doctor` no longer print the package manager's answers (for example `brew list --versions fzf`) into their own output. `list --json` was not valid JSON on hosts where the package manager diff --git a/CLAUDE.md b/CLAUDE.md index 0dc9375..2e6a901 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -163,16 +163,18 @@ files matters more than any single file: change on one key press, string and int options are typed and validated on enter (a rejected value keeps the typed text), list options are shown but not editable. The header's change count includes options whose value differs from - the first value the screen showed. The plan screen (`a`) asks `Backend.Plan`, which is - `Engine.Preview`: the plan text plus apply's own "nothing to do" rule, so the - screen never promises a change apply would not make. `y` only sets - `Result.ApplyRequested`; once the UI has closed, `internal/cli/tui.go` runs - the real apply command (`handOffToApply`), so its prompt, sudo and exit codes + the first value the screen showed. The plan screen (`a`) asks `Backend.Plan`, + which is `Engine.Preview`: the plan text plus apply's own "nothing to do" + rule, so the screen never promises a change apply would not make. `y` or + Enter only sets `Result.ApplyRequested` (and only while the plan is on + screen); once the UI has closed, `internal/cli/tui.go` runs the real apply + command (`handOffToApply`), so its prompt, sudo and exit codes stay the gate. The UI's own engine is built with `io.Discard` writers, because the runner copies package-manager output to them and the UI owns the terminal. Any text that comes from a backend or a manifest (module fields, plan text, - error messages) must go through `sanitize` before it is drawn. Rendering + error messages) must go through `sanitize` (`modedit.Clean`, which `omnishell + list` uses too) before it is drawn. Rendering tests compare `View().Content` (styling stripped) with golden files in `internal/tui/testdata`; regenerate them with `go test ./internal/tui -run Golden -update` and review the diff by eye. diff --git a/README.md b/README.md index 54e287b..2ddcab3 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,7 @@ your system until you run `omnishell apply`. |----------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `omnishell init` | Create the omnishell config and hook it into your shells. Idempotent. | — | | `omnishell list` | List known modules and their status (enabled/disabled, packages, platforms, shells, source, description). | `--json` (emit a JSON array instead of the table; also includes each module's `homepage`) | -| `omnishell tui` | Browse and toggle modules in a full-screen terminal UI: a module list with description, homepage, package status, platforms and shells, a `/` filter, and Space to enable or disable the selected module (written to `config.toml` at once, like `enable` / `disable`). `o` edits the module's options (bool, enum, string and int; each change is written at once and validated like `omnishell set`). `a` previews the plan; confirming it closes the UI and runs `omnishell apply`, which still asks before it changes anything. Modules that cannot run on this host are dimmed. Needs an interactive terminal (exit 2 otherwise). | — | +| `omnishell tui` | Browse and toggle modules in a full-screen terminal UI: a module list with description, homepage, package status, platforms and shells, a `/` filter, and Space to enable or disable the selected module (written to `config.toml` at once, like `enable` / `disable`). `o` edits the module's options (bool, enum, string and int; each change is written at once and validated like `omnishell set`). `a` previews the plan; confirming it closes the UI and runs `omnishell apply`, which still asks before it changes anything. Modules that cannot run on this host are dimmed with the reason; a module that only lacks OS support can still be enabled, but `apply` skips it. Needs an interactive terminal (exit 2 otherwise). | — | | `omnishell enable ` | Enable a module in the config. Does not apply. | — | | `omnishell disable ` | Disable a module in the config. Does not apply. | — | | `omnishell set . ` | Set a module option in the config, validated against that module's option schema. | — | diff --git a/internal/cli/tui.go b/internal/cli/tui.go index 0971285..b1791ba 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -105,11 +105,12 @@ func newTUICmd() *cobra.Command { Long: `Browse modules in a full-screen terminal UI. Shows every known module with its description, homepage, package status, -platforms and shells. Move with the arrow keys, press space to enable or disable -the selected module, press o to edit its options, type / to filter, q to quit. Space writes config.toml at -once, exactly like 'omnishell enable' and 'disable'; it never touches your -shells. Press a to preview the plan; confirming it closes the UI and runs -'omnishell apply', which still asks before it changes anything. +platforms and shells. Move with the arrow keys, press space to enable or +disable the selected module, press o to edit its options, type / to filter, +q to quit. Space writes config.toml at once, exactly like 'omnishell enable' +and 'disable'; it never touches your shells. Press a to preview the plan; +confirming it closes the UI and runs 'omnishell apply', which still asks +before it changes anything. Needs an interactive terminal; in scripts use 'omnishell list'.`, Args: cobra.NoArgs, diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index b7fe459..1b0aaca 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -11,6 +11,7 @@ import ( "sort" "strings" "testing" + "unicode/utf8" "github.com/JtheGunner/omnishell/internal/cli" "github.com/JtheGunner/omnishell/internal/config" @@ -557,3 +558,17 @@ func TestTUIHandoffSaysWhenApplyFindsNothingToDo(t *testing.T) { t.Fatalf("a hand-off that finds nothing to do must say so; stdout:\n%s", out.String()) } } + +// A help line longer than a classic 80-column terminal wraps in the middle of a +// word. +func TestTUIHelpFitsAnEightyColumnTerminal(t *testing.T) { + var out, errb bytes.Buffer + if code := cli.Execute([]string{"tui", "--help"}, &out, &errb); code != 0 { + t.Fatalf("exit = %d (stderr: %s)", code, errb.String()) + } + for _, line := range strings.Split(out.String(), "\n") { + if n := utf8.RuneCountInString(line); n > 80 { + t.Errorf("help line is %d characters wide: %q", n, line) + } + } +} From 34a16506a9808d56f0f3b0ed17fbeb105de19f26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 16:47:08 +0200 Subject: [PATCH 9/9] fix: count a written option after a failed re-read and explain the ignored plan key Review findings: an option whose write succeeded but whose re-read failed was missing from the header's change count, and pressing a while an earlier plan was still being computed looked like a dead key. --- internal/tui/options.go | 15 ++++++++++++++- internal/tui/options_test.go | 22 ++++++++++++++++++++++ internal/tui/plan.go | 6 +++++- internal/tui/plan_test.go | 3 +++ 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/internal/tui/options.go b/internal/tui/options.go index f636d3e..7c05702 100644 --- a/internal/tui/options.go +++ b/internal/tui/options.go @@ -31,6 +31,8 @@ type optionsMsg struct { // stopped it, or the module's options re-read after the write. type optionWrittenMsg struct { id string + key string // the option that was written + raw string // the value that was written, as typed or chosen rows []modedit.OptionView written bool // true once config.toml holds the new value, even if err is set err error @@ -52,7 +54,7 @@ func optionWriteCmd(b Backend, id, key, raw string) tea.Cmd { return optionWrittenMsg{id: id, err: err} } rows, err := b.Options(id) - return optionWrittenMsg{id: id, rows: rows, written: true, err: err} + return optionWrittenMsg{id: id, key: key, raw: raw, rows: rows, written: true, err: err} } } @@ -116,6 +118,17 @@ func (m Model) applyOptionWritten(msg optionWrittenMsg) Model { m.options.stale = true m.options.editing = false m.options.input, m.options.pos = "", 0 + if msg.key != "" { + // Show and count what was written, as far as it is known. + rows := slices.Clone(m.options.rows) + for i := range rows { + if rows[i].Key == msg.key { + rows[i].Value, rows[i].Set = sanitize(msg.raw), true + } + } + m.options.rows = rows + m.rememberOptions(msg.id, rows) + } } return m } diff --git a/internal/tui/options_test.go b/internal/tui/options_test.go index 419fa81..5664bfe 100644 --- a/internal/tui/options_test.go +++ b/internal/tui/options_test.go @@ -575,3 +575,25 @@ func TestLeavingTheOptionsScreenForgetsTheOutOfDateMark(t *testing.T) { t.Fatal("a freshly opened screen has just been read") } } + +// The write went through, only the re-read failed: config.toml has the new +// value, so the header must count it even though the rows are out of date. +func TestAFailedRereadStillCountsTheWrittenOptionAsAChange(t *testing.T) { + m, b := withOptions() + m = openFzfOptions(t, m) + b.optionsErr = errors.New("cannot re-read") + m = settleKey(t, m, "space") // ctrl_r: true -> false + + if m.changes() != 1 { + t.Fatalf("changes = %d, want the written option counted", m.changes()) + } + if got := optionRow(t, m, "ctrl_r").Value; got != "false" { + t.Fatalf("ctrl_r shows %q, want what was written", got) + } + + b.optionsErr = nil + m = settleKey(t, m, "r") + if m.changes() != 1 { + t.Fatalf("changes = %d after the re-read, want it unchanged", m.changes()) + } +} diff --git a/internal/tui/plan.go b/internal/tui/plan.go index 0ba89f8..ced7739 100644 --- a/internal/tui/plan.go +++ b/internal/tui/plan.go @@ -55,7 +55,11 @@ func planCmd(b Backend, seq int) tea.Cmd { // that is about to change) or while an earlier computation is still running // (it cannot be cancelled and queries the package manager). func (m Model) openPlan() (tea.Model, tea.Cmd) { - if m.pending || m.planRunning { + if m.pending { + return m, nil + } + if m.planRunning { + m.status = "the previous plan is still being computed" return m, nil } m.screen = screenPlan diff --git a/internal/tui/plan_test.go b/internal/tui/plan_test.go index b8fae9b..689f6eb 100644 --- a/internal/tui/plan_test.go +++ b/internal/tui/plan_test.go @@ -162,6 +162,9 @@ func TestAIsIgnoredWhileAnEarlierPlanIsStillBeingComputed(t *testing.T) { if againCmd != nil || again.(Model).screen != screenBrowser { t.Fatal("a must be ignored while the earlier plan is still being computed") } + if !strings.Contains(again.(Model).status, "still being computed") { + t.Fatalf("status = %q, want a note so the key does not look dead", again.(Model).status) + } afterOld := settle(again.(Model), firstCmd) // the old answer arrives and is dropped if afterOld.screen != screenBrowser {