Skip to content

feat: add visual mode text selection - #164

Open
ashupednekar wants to merge 1 commit into
gammons:mainfrom
ashupednekar:feat_visual_mode
Open

ashupednekar wants to merge 1 commit into
gammons:mainfrom
ashupednekar:feat_visual_mode

Conversation

@ashupednekar

Copy link
Copy Markdown

Summary

Add Vim-style visual mode so users can select and copy part of a channel message or thread reply with the keyboard.

Changes

  • Add v / VISUAL mode with h/j/k/l, arrow, 0/$, and endpoint-swap motions
  • Add y clipboard yank and Esc cancellation for messages and thread replies
  • Keep image preview available on O and add selection/mode coverage

Testing

  • go test ./... (2,176 tests passed)

@gammons

gammons commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Thanks for this. Let me start with the good news, because the CI failure looks worse than it is.

The compile errors are a stale base, not invented APIs. Your merge-base is f60601f (2026-06-11) — 222 commits behind main. statusbar.CopyFailedMsg did exist and was deleted by 3e5b662 ("feat: write clipboard through OSC 52", 2026-08-14), which also narrowed clipboardWrite from (clipboard.Format, []byte) <-chan struct{} to func(text string) tea.Cmd (callbacks.go:127-133). So you wrote against a real API that moved under you.

I verified the repair is mechanical: merged main, dropped the golang.design/x/clipboard import, swapped the closure for tea.Batch(a.clipboardWrite(text), ...), fixed the test's writer signature — go build, go vet and all of ./internal/ui/... pass. About 15 lines.

That said, I don't want to merge this yet, for reasons beyond the rebase.

1. The highlight never repaints in the messages pane. messages.Model.BeginVisualSelection and MoveVisualSelection never call m.dirty(). Your thread implementation does. dirty() bumps version, which is the panel cache key (view_messages.go:203, and the model's own comment at messages/model.go:308-312). Measured:

messages: version before=0  after Begin=0
messages: version after Move=0
BUG: MoveVisualSelection did not bump version -> panel cache will not repaint

So in the main messages pane, a user enters visual mode, presses l, and nothing moves. The tests don't catch it because they only assert SelectionText() — the data model — never the render. That's precisely what a render assertion is for.

2. App state is read from a tea.Cmd goroutine. mode_visual.go:29-36 returns a closure that reads a.clipboardAvailable and calls a.clipboardWrite(...) inside the command. tea.Cmds run on separate goroutines; Update runs on the event loop. Compare copyPermalinkOfSelected (app.go:1057-1058), which calls clipboardWrite in Update and batches the result.

3. Motions bypass the KeyMap. handleVisualMode switches on raw msg.String() while VisualMode is declared in the KeyMap. So h/j/k/l/0/$/o/y are invisible to help.FromKeyMap? will never document them — and they can't participate in the custom-keybinding-overrides roadmap item. Every other mode here uses key.Matches. It also swallows every unlisted key, so Q, G, Tab and ? are dead in visual mode.

4. It takes v from a documented feature with no doc updates. v is image preview, documented at wiki/Keybindings.md:38, wiki/Tradeoffs-and-Non-Goals.md:29 and docs/STATUS.md:61. Your 10 files are all internal/ui/** — no wiki/, no docs/.

5. It reintroduces the cgo clipboard into internal/ui, which 3e5b662 deliberately removed. Given I just had to ship 934733f to build darwin with cgo so paste works at all, that area is fragile and I want it going one direction only.

On scope — and this is the main thing. BeginVisualSelection anchors to the currently-selected message and MoveVisualSelection clamps to that one cache entry's linesPlain, so the selection is confined to a single message and j past its last line does nothing. That's actually the sane scope — cross-message character-wise selection over a virtualized, ANSI-styled, lazily-wrapped viewport is genuinely hard, and mouse drag-to-copy already covers the multi-message case. But the mode indicator says VISUAL, which promises vim semantics it doesn't deliver: no viewport scrolling while selecting, no w/b/e, no V line-wise, no counts. Someone will press G and find it dead.

And the value ratio is lopsided against #163, which does "copy this message" in ~35 lines for the 95% case. This is ~230 lines plus a whole new Mode for "copy part of one message."

What I'd like: let #163 land first, then open an issue for the visual-mode design. A new mode in a modal editor is the definition of a large feature addition, and I'd rather agree on the shape — scope, key, whether it scrolls — before you sink more time into it. If you still want it after that:

  1. Rebase; port to OSC 52 (drop golang.design/x/clipboard, use a.clipboardWrite, drop the clipboardAvailable gate — it gates the cgo path and is the wrong check for an OSC 52 write).
  2. Move state reads out of the tea.Cmd closure into Update.
  3. Add m.dirty() to both messages-pane methods and a render assertion that the highlight appears and moves. Non-negotiable — the feature is currently invisible there.
  4. Factor the duplicated ~70 lines of motion logic in messages/model.go and thread/model.go into one implementation. They've already diverged (dirty() in one, not the other), which is how this defect happened.
  5. Route visual keys through key.Matches so ? documents them.
  6. Decide v deliberately — if it moves, update all three docs and add a test that O still opens the preview.
  7. Document VISUAL mode; there's currently nothing.

Also a semantic nit: selection.Range is documented half-open [Start, End) (selection/selection.go:19), but your comment says you're emulating vim's inclusive visual cursor. o happens to be a no-op on the resulting text under Normalize so it works out, but extending left vs right is asymmetric by one cell.

@gammons gammons added the changes requested Blocking issues found in review label Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Blocking issues found in review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants