cbd-tui: fix visual mode stranding the turnaround row (anchor the range)

The per-step paint toggled the row you arrived at, so going down then
back up toggled off the rows re-entered but never the furthest row you
turned around on — it stayed marked. Anchor the selection instead: on
entering visual mode record the anchor row, and on each move reconcile
marks to the contiguous range [anchor, cursor], toggling only the rows
whose membership changed. Moving back now cleanly reverses; jumps
reconcile the whole span. visual state becomes Option<usize> (the
anchor). Adds a regression test (down then fully up leaves only the
anchor); 90 cbd-tui tests green, clippy/fmt clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Test User 2026-07-24 16:29:26 +02:00
parent 0a9456e173
commit 2e94760a69
4 changed files with 113 additions and 74 deletions

View File

@ -42,65 +42,75 @@ preference — trivially changed in `bindings.rs`; `f` is the chosen default.*
### D2 — Paint-toggle semantics: sweep toggles, endpoints included ### D2 — Paint-toggle semantics: sweep toggles, endpoints included
Visual mode holds one piece of state — that it is **active** (the anchor is Visual mode holds one piece of state — that it is **active** (the anchor is
implicit in the cursor + the running mark set). Behavior: the **anchor** — the view index where `v` was pressed — plus the cursor).
Behavior:
- **Enter** (`v`/`V` while normal): activate, and **toggle the current row's - **Enter** (`v`/`V` while normal): activate, **anchor at the current row**, and
mark** (vim includes the row you start on). A lone `v … v` thus behaves like a **toggle its mark** (vim includes the row you start on). A lone `v … v` thus
single `s`. behaves like a single `s`.
- **Move** (any of `j`/`k`, `g`/`G`, `Ctrl-d`/`Ctrl-u` while active): perform the - **Move** (any of `j`/`k`, `g`/`G`, `Ctrl-d`/`Ctrl-u` while active): perform the
move, then **toggle the mark of every row swept into** — the half-open view move, then reconcile marks to the contiguous **range `[anchor, cursor]`**
range `(old_cursor, new_cursor]` (excludes the row you left, includes every toggle exactly the rows whose membership in that range **changed** (relative to
row up to and including the one you land on). This makes jumps (`G`, `Ctrl-d`) the anchor). Growing the range marks the rows entered; **shrinking it unmarks
paint the whole span, not just the endpoint. Sweeping **back** re-toggles the rows left**, so moving back down/up **cleanly reverses** a move and the row
(un-paints) the rows re-entered — the "toggle" the user asked for. you turn around on is never stranded. Jumps (`G`, `Ctrl-d`) reconcile the whole
span at once. It is a *toggle* against range membership, so sweeping over a
row that was already marked (by `s`) flips it, and sweeping back flips it back.
- **Exit**: `v`/`V` again, or `Esc`, deactivates (marks persist). Any other - **Exit**: `v`/`V` again, or `Esc`, deactivates (marks persist). Any other
action key (`a`, `Enter`, `w`, …) also **exits first, then runs normally**, so action key (`a`, `Enter`, `w`, …) also **exits first, then runs normally**, so
`v j j a` selects three rows and appends them. Changing node (`h`/`l`) or focus `v j j a` selects three rows and appends them. Changing node (`h`/`l`) or focus
(`Tab`) also exits — the swept indices would otherwise be stale. (`Tab`) also exits — the anchor/indices would otherwise be stale.
```d2 ```d2
direction: right direction: right
shape: sequence_diagram shape: sequence_diagram
Normal Normal
Visual Visual
Normal -> Visual: "v / V (toggle current row)" Normal -> Visual: "v / V (anchor here, toggle current row)"
Visual -> Visual: "j k g G C-d C-u (move, then toggle swept range)" Visual -> Visual: "j k g G C-d C-u (reconcile marks to [anchor, cursor])"
Visual -> Normal: "v / V / Esc (marks kept)" Visual -> Normal: "v / V / Esc (marks kept)"
Visual -> Normal: "a / Enter / w / … (exit, then act on marks)" Visual -> Normal: "a / Enter / w / … (exit, then act on marks)"
Visual -> Normal: "h / l / Tab (node/focus change)" Visual -> Normal: "h / l / Tab (node/focus change)"
``` ```
Worked example (rows `0..9`, all unmarked, cursor at `0`): Worked example (rows `0..9`, all unmarked, cursor at `0` = anchor):
```text ```text
v -> {0} (enter toggles current) v -> {0} anchor 0, toggle current
j -> {0,1} paint (0,1] j -> {0,1} range [0,1]
j -> {0,1,2} paint (1,2] j -> {0,1,2} range [0,2]
G -> {0..9} paint (2,9] (jump paints the span) G -> {0..9} range [0,9] (jump reconciles the span)
k -> {0..8} paint (9,8] -> un-paints 9 k -> {0..8} range [0,8] -> 9 leaves, unmarked
v -> exit, marks {0..8} kept g -> {0} range [0,0] -> 1..8 leave, unmarked
a -> append 9 rows v -> exit, marks {0} kept
``` ```
### D3 — Where the state lives and how dispatch routes it ### D3 — Where the state lives and how dispatch routes it
`App` gains a `bool` "visual active" flag. `App::dispatch` is the single choke `Library` owns the visual state as `visual: Option<usize>``Some(anchor_view)`
point (it already maps every `Action`): while active, so the mode and its anchor are one field, exposed as `is_visual`,
`toggle_visual`, `exit_visual`. `App::dispatch` is the single choke point (it
already maps every `Action`):
- `LibraryVisualMode` → toggle the flag; on activate call `library.toggle_mark()` - `LibraryVisualMode``library.toggle_visual()` (activate: anchor at the cursor
(the anchor). Ignored unless the library is focused. and toggle its mark; or deactivate). Only reachable while the library is
- The six movement actions → when the flag is set and the library is focused: focused (Library-scope binding).
read the cursor, run the existing move, read the cursor again, and call a new - The six movement actions → a `library_move` helper: when visual is active, read
`Library::paint_between(old_view, new_view)`; otherwise unchanged. the cursor, run the existing move, read the cursor again, and call
- `LibraryAscend`/`LibraryDive`/`CycleFocus` → clear the flag, then proceed. `Library::paint_between(old_view, new_view)`; otherwise just move.
- `ClearSearch` (`Esc`) → if the flag is set, just clear it (do not also clear - `LibraryAscend`/`LibraryDive`/`CycleFocus` → covered by the catch-all below
(they exit visual, then proceed).
- `ClearSearch` (`Esc`) → if visual is active, just leave it (do not also clear
the search filter); else unchanged. the search filter); else unchanged.
- Every other action → clear the flag, then proceed. - Every other action → leave visual mode first, then proceed. Implemented as a
guard at the top of `dispatch`: capture `was_visual`, and exit unless the
action is one of the six movements, `LibraryVisualMode`, or `ClearSearch`.
`Library` gains `selected_view()` (the current view index) and `Library` gains `selected_view()` (the current view index) and
`paint_between(from_view, to_view)` — toggle the mark (respecting `is_queable`) `paint_between(from_view, to_view)` — using the stored `anchor`, toggle the mark
of every view index in the half-open sweep, mapping each through the `/` filter (respecting `is_queable`) of every view index whose membership in `[anchor,
to its real index exactly as `toggle_mark` does. No change to `select`, so cursor]` changed between the old and new cursor, mapping each through the `/`
filter to its real index exactly as `toggle_mark` does. No change to `select`, so
non-visual selection (filter re-select, `update_selection`) never paints. non-visual selection (filter re-select, `update_selection`) never paints.
### D4 — Visual indicator ### D4 — Visual indicator
@ -127,17 +137,19 @@ mode to the queue is gated on giving the queue a mark set (the existing
- **`bindings.rs`** (pure data): `+LibraryVisualMode`, its two Library bindings, - **`bindings.rs`** (pure data): `+LibraryVisualMode`, its two Library bindings,
and the `ToggleSpectrum` chord moves `v`→`f`. All dispatch/help/label logic is and the `ToggleSpectrum` chord moves `v`→`f`. All dispatch/help/label logic is
already derived from the table. already derived from the table.
- **`app/mod.rs`** (`App`): the visual flag + the dispatch routing above. - **`app/mod.rs`** (`App`): the `was_visual` guard + `library_move` + the
- **`app/library.rs`** (`Library`): `selected_view`, `paint_between`, and the dispatch routing above.
title indicator. Marks, filter, and `select` are reused unchanged. - **`app/library.rs`** (`Library`): the `visual: Option<usize>` anchor,
`is_visual`/`toggle_visual`/`exit_visual`, `selected_view`, `paint_between`, and
the title indicator. Marks, filter, and `select` are reused unchanged.
## Risks and open questions ## Risks and open questions
- **Back-sweep un-paints.** Overshooting then correcting toggles rows off — the - **Anchor semantics chosen over per-step toggle.** An earlier half-open
literal "toggle" the request asked for, but a user expecting a monotonic vim per-step design stranded the turnaround row (down-then-up left the furthest row
range-select may be mildly surprised. Documented in the help text ("toggles"). marked). The anchored `[anchor, cursor]` range reconciliation fixes that:
An anchored range-select (never un-paints within one session) is a possible moving back cleanly reverses. Sweeping over a pre-existing (`s`) mark still
future refinement. *toggles* it against range membership — reversible, but worth knowing.
- **Filter interaction.** With a `/` filter active, paint sweeps **view** indices - **Filter interaction.** With a `/` filter active, paint sweeps **view** indices
and toggles their real rows, so only visible rows are affected — consistent and toggles their real rows, so only visible rows are affected — consistent
with how `s` and `get_selected` already treat marks vs. the filtered view. with how `s` and `get_selected` already treat marks vs. the filtered view.

View File

@ -29,9 +29,11 @@ pub struct Library {
filter: Filter, filter: Filter,
parent: Option<String>, parent: Option<String>,
positions: HashMap<String, usize>, positions: HashMap<String, usize>,
/// Visual (paint-select) mode: while true, movement toggles the mark of /// Visual (paint-select) mode: `Some(anchor_view)` while active. Movement
/// every row swept over (architecture/visual-mode.md). /// marks the contiguous range between the anchor (where `v` was pressed)
visual: bool, /// and the cursor, toggling rows as they enter/leave it — so moving back
/// cleanly reverses (architecture/visual-mode.md).
visual: Option<usize>,
tx: Sender<MessageFromUi>, tx: Sender<MessageFromUi>,
} }
@ -46,7 +48,7 @@ impl Library {
filter: Filter::default(), filter: Filter::default(),
positions: HashMap::new(), positions: HashMap::new(),
parent: None, parent: None,
visual: false, visual: None,
tx, tx,
} }
} }
@ -197,7 +199,7 @@ impl Library {
/// Whether visual (paint-select) mode is active. /// Whether visual (paint-select) mode is active.
pub fn is_visual(&self) -> bool { pub fn is_visual(&self) -> bool {
self.visual self.visual.is_some()
} }
/// Titles of the currently marked rows, in list order (inspection helper). /// Titles of the currently marked rows, in list order (inspection helper).
@ -209,18 +211,21 @@ impl Library {
.collect() .collect()
} }
/// Enter or leave visual mode. Entering also toggles the current row's /// Enter or leave visual mode. Entering anchors at the current row and
/// mark (vim includes the row you start on, D2); leaving keeps the marks. /// toggles its mark (vim includes the row you start on, D2); leaving keeps
/// the marks.
pub fn toggle_visual(&mut self) { pub fn toggle_visual(&mut self) {
self.visual = !self.visual; if self.visual.is_some() {
if self.visual { self.visual = None;
} else {
self.visual = Some(self.list_state.selected().unwrap_or(0));
self.toggle_mark(); self.toggle_mark();
} }
} }
/// Leave visual mode (marks kept). Idempotent. /// Leave visual mode (marks kept). Idempotent.
pub fn exit_visual(&mut self) { pub fn exit_visual(&mut self) {
self.visual = false; self.visual = None;
} }
/// The current cursor position as a view index. /// The current cursor position as a view index.
@ -228,20 +233,21 @@ impl Library {
self.list_state.selected() self.list_state.selected()
} }
/// Paint the sweep from `from_view` to `to_view`: toggle the mark of every /// Repaint after a visual-mode move from `from_view` to `to_view`. The
/// view index in the half-open range `(from_view, to_view]` (excludes the /// selection is the contiguous range `[anchor, cursor]`; a move that grows
/// row left, includes each row swept into, in either direction). Used after /// or shrinks it toggles exactly the rows whose range membership changed
/// a movement while visual mode is active. /// (relative to the anchor), so moving back reverses a move cleanly and the
/// row you turn around on is never stranded. No-op unless visual is active.
pub fn paint_between(&mut self, from_view: usize, to_view: usize) { pub fn paint_between(&mut self, from_view: usize, to_view: usize) {
if from_view == to_view { let Some(anchor) = self.visual else {
return; return;
} };
if to_view > from_view { let (old_lo, old_hi) = (anchor.min(from_view), anchor.max(from_view));
for view in (from_view + 1)..=to_view { let (new_lo, new_hi) = (anchor.min(to_view), anchor.max(to_view));
self.toggle_mark_view(view); for view in old_lo.min(new_lo)..=old_hi.max(new_hi) {
} let in_old = (old_lo..=old_hi).contains(&view);
} else { let in_new = (new_lo..=new_hi).contains(&view);
for view in to_view..from_view { if in_old != in_new {
self.toggle_mark_view(view); self.toggle_mark_view(view);
} }
} }
@ -383,7 +389,7 @@ impl Library {
} else { } else {
COLOR_PRIMARY_DARK COLOR_PRIMARY_DARK
})) }))
.title(if self.visual { .title(if self.visual.is_some() {
// Visual (paint-select) mode: movement toggles marks. // Visual (paint-select) mode: movement toggles marks.
format!("{} — VISUAL", self.title) format!("{} — VISUAL", self.title)
} else if let Some(query) = self.filter.query() { } else if let Some(query) = self.filter.query() {

View File

@ -885,16 +885,37 @@ mod tests {
#[test] #[test]
fn visual_back_sweep_unpaints() { fn visual_back_sweep_unpaints() {
// Retreating shrinks the [anchor, cursor] range: the row turned around
// on (gamma) is unmarked, not stranded.
let (mut app, _rx) = app(); let (mut app, _rx) = app();
app.library app.library
.update(children_listing(&["alpha", "beta", "gamma"])); .update(children_listing(&["alpha", "beta", "gamma"]));
let _ = app.dispatch(Action::LibraryVisualMode); // alpha let _ = app.dispatch(Action::LibraryVisualMode); // alpha (anchor)
let _ = app.dispatch(Action::LibraryNext); // beta let _ = app.dispatch(Action::LibraryNext); // beta
let _ = app.dispatch(Action::LibraryNext); // gamma let _ = app.dispatch(Action::LibraryNext); // gamma
let _ = app.dispatch(Action::LibraryPrev); // back onto beta -> unmark let _ = app.dispatch(Action::LibraryPrev); // back onto beta
assert_eq!( assert_eq!(
app.library.marked_titles(), app.library.marked_titles(),
vec!["alpha".to_string(), "gamma".to_string()] vec!["alpha".to_string(), "beta".to_string()]
);
}
#[test]
fn visual_down_then_fully_up_leaves_only_the_anchor() {
// Regression: going down and back up to the start must not strand the
// furthest row marked (the reported bug).
let (mut app, _rx) = app();
app.library
.update(children_listing(&["alpha", "beta", "gamma"]));
let _ = app.dispatch(Action::LibraryVisualMode); // alpha (anchor)
let _ = app.dispatch(Action::LibraryNext); // beta
let _ = app.dispatch(Action::LibraryNext); // gamma
let _ = app.dispatch(Action::LibraryPrev); // beta
let _ = app.dispatch(Action::LibraryPrev); // alpha
assert_eq!(
app.library.marked_titles(),
vec!["alpha".to_string()],
"only the anchor stays marked after returning to it"
); );
} }

View File

@ -1188,13 +1188,13 @@ leaves it, marks kept.
range when visual is active; `Esc` in visual leaves the mode without clearing range when visual is active; `Esc` in visual leaves the mode without clearing
the `/` filter; node/focus changes leave it. the `/` filter; node/focus changes leave it.
Decisions/deviations: **paint semantics are toggle-based** (back-sweeping Decisions/deviations: **paint is anchored** — the selection is the contiguous
un-paints — the literal "toggle" requested; an anchored never-un-paint range is range `[anchor, cursor]`, and a move toggles the rows whose range membership
a noted future refinement); **library-only** — the queue has no marks yet (a changed, so moving back cleanly reverses (a first per-step-toggle attempt
standing `queue.rs` FIXME), so `v`/`V` are unbound there (D5); **spectrum key stranded the turnaround row — fixed); **library-only** — the queue has no marks
`f` is a yet (a standing `queue.rs` FIXME), so `v`/`V` are unbound there (D5); **spectrum
chosen default**, trivially changed; **web-client parity deferred** (D6). Jump key `f` is a chosen default**, trivially changed; **web-client parity deferred**
moves (`g`/`G`, `Ctrl-d`/`Ctrl-u`) paint the whole span. (D6). Jump moves (`g`/`G`, `Ctrl-d`/`Ctrl-u`) reconcile the whole span.
Verification: `cbd-tui` 89 tests green (14 new: 2 binding + 12 dispatch/paint Verification: `cbd-tui` 89 tests green (14 new: 2 binding + 12 dispatch/paint
covering enter/step/jump/back-sweep/exit/Esc/non-move-exit/node-focus-exit/ covering enter/step/jump/back-sweep/exit/Esc/non-move-exit/node-focus-exit/