diff --git a/architecture/visual-mode.md b/architecture/visual-mode.md index de7eaf2..009e4b4 100644 --- a/architecture/visual-mode.md +++ b/architecture/visual-mode.md @@ -42,65 +42,75 @@ preference — trivially changed in `bindings.rs`; `f` is the chosen default.* ### D2 — Paint-toggle semantics: sweep toggles, endpoints included 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 - mark** (vim includes the row you start on). A lone `v … v` thus behaves like a - single `s`. +- **Enter** (`v`/`V` while normal): activate, **anchor at the current row**, and + **toggle its mark** (vim includes the row you start on). A lone `v … v` thus + behaves like a single `s`. - **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 - range `(old_cursor, new_cursor]` (excludes the row you left, includes every - row up to and including the one you land on). This makes jumps (`G`, `Ctrl-d`) - paint the whole span, not just the endpoint. Sweeping **back** re-toggles - (un-paints) the rows re-entered — the "toggle" the user asked for. + move, then reconcile marks to the contiguous **range `[anchor, cursor]`** — + toggle exactly the rows whose membership in that range **changed** (relative to + the anchor). Growing the range marks the rows entered; **shrinking it unmarks + the rows left**, so moving back down/up **cleanly reverses** a move and the row + 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 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 - (`Tab`) also exits — the swept indices would otherwise be stale. + (`Tab`) also exits — the anchor/indices would otherwise be stale. ```d2 direction: right shape: sequence_diagram Normal Visual -Normal -> Visual: "v / V (toggle current row)" -Visual -> Visual: "j k g G C-d C-u (move, then toggle swept range)" +Normal -> Visual: "v / V (anchor here, toggle current row)" +Visual -> Visual: "j k g G C-d C-u (reconcile marks to [anchor, cursor])" Visual -> Normal: "v / V / Esc (marks kept)" Visual -> Normal: "a / Enter / w / … (exit, then act on marks)" 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 -v -> {0} (enter toggles current) -j -> {0,1} paint (0,1] -j -> {0,1,2} paint (1,2] -G -> {0..9} paint (2,9] (jump paints the span) -k -> {0..8} paint (9,8] -> un-paints 9 -v -> exit, marks {0..8} kept -a -> append 9 rows +v -> {0} anchor 0, toggle current +j -> {0,1} range [0,1] +j -> {0,1,2} range [0,2] +G -> {0..9} range [0,9] (jump reconciles the span) +k -> {0..8} range [0,8] -> 9 leaves, unmarked +g -> {0} range [0,0] -> 1..8 leave, unmarked +v -> exit, marks {0} kept ``` ### D3 — Where the state lives and how dispatch routes it -`App` gains a `bool` "visual active" flag. `App::dispatch` is the single choke -point (it already maps every `Action`): +`Library` owns the visual state as `visual: Option` — `Some(anchor_view)` +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()` - (the anchor). Ignored unless the library is focused. -- The six movement actions → when the flag is set and the library is focused: - read the cursor, run the existing move, read the cursor again, and call a new - `Library::paint_between(old_view, new_view)`; otherwise unchanged. -- `LibraryAscend`/`LibraryDive`/`CycleFocus` → clear the flag, then proceed. -- `ClearSearch` (`Esc`) → if the flag is set, just clear it (do not also clear +- `LibraryVisualMode` → `library.toggle_visual()` (activate: anchor at the cursor + and toggle its mark; or deactivate). Only reachable while the library is + focused (Library-scope binding). +- The six movement actions → a `library_move` helper: when visual is active, read + the cursor, run the existing move, read the cursor again, and call + `Library::paint_between(old_view, new_view)`; otherwise just move. +- `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. -- 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 -`paint_between(from_view, to_view)` — toggle the mark (respecting `is_queable`) -of every view index in the half-open sweep, mapping each through the `/` filter -to its real index exactly as `toggle_mark` does. No change to `select`, so +`paint_between(from_view, to_view)` — using the stored `anchor`, toggle the mark +(respecting `is_queable`) of every view index whose membership in `[anchor, +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. ### 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, and the `ToggleSpectrum` chord moves `v`→`f`. All dispatch/help/label logic is already derived from the table. -- **`app/mod.rs`** (`App`): the visual flag + the dispatch routing above. -- **`app/library.rs`** (`Library`): `selected_view`, `paint_between`, and the - title indicator. Marks, filter, and `select` are reused unchanged. +- **`app/mod.rs`** (`App`): the `was_visual` guard + `library_move` + the + dispatch routing above. +- **`app/library.rs`** (`Library`): the `visual: Option` 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 -- **Back-sweep un-paints.** Overshooting then correcting toggles rows off — the - literal "toggle" the request asked for, but a user expecting a monotonic vim - range-select may be mildly surprised. Documented in the help text ("toggles"). - An anchored range-select (never un-paints within one session) is a possible - future refinement. +- **Anchor semantics chosen over per-step toggle.** An earlier half-open + per-step design stranded the turnaround row (down-then-up left the furthest row + marked). The anchored `[anchor, cursor]` range reconciliation fixes that: + moving back cleanly reverses. Sweeping over a pre-existing (`s`) mark still + *toggles* it against range membership — reversible, but worth knowing. - **Filter interaction.** With a `/` filter active, paint sweeps **view** indices 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. diff --git a/cbd-tui/src/app/library.rs b/cbd-tui/src/app/library.rs index 2561d0a..330e650 100644 --- a/cbd-tui/src/app/library.rs +++ b/cbd-tui/src/app/library.rs @@ -29,9 +29,11 @@ pub struct Library { filter: Filter, parent: Option, positions: HashMap, - /// Visual (paint-select) mode: while true, movement toggles the mark of - /// every row swept over (architecture/visual-mode.md). - visual: bool, + /// Visual (paint-select) mode: `Some(anchor_view)` while active. Movement + /// marks the contiguous range between the anchor (where `v` was pressed) + /// and the cursor, toggling rows as they enter/leave it — so moving back + /// cleanly reverses (architecture/visual-mode.md). + visual: Option, tx: Sender, } @@ -46,7 +48,7 @@ impl Library { filter: Filter::default(), positions: HashMap::new(), parent: None, - visual: false, + visual: None, tx, } } @@ -197,7 +199,7 @@ impl Library { /// Whether visual (paint-select) mode is active. pub fn is_visual(&self) -> bool { - self.visual + self.visual.is_some() } /// Titles of the currently marked rows, in list order (inspection helper). @@ -209,18 +211,21 @@ impl Library { .collect() } - /// Enter or leave visual mode. Entering also toggles the current row's - /// mark (vim includes the row you start on, D2); leaving keeps the marks. + /// Enter or leave visual mode. Entering anchors at the current row and + /// toggles its mark (vim includes the row you start on, D2); leaving keeps + /// the marks. pub fn toggle_visual(&mut self) { - self.visual = !self.visual; - if self.visual { + if self.visual.is_some() { + self.visual = None; + } else { + self.visual = Some(self.list_state.selected().unwrap_or(0)); self.toggle_mark(); } } /// Leave visual mode (marks kept). Idempotent. pub fn exit_visual(&mut self) { - self.visual = false; + self.visual = None; } /// The current cursor position as a view index. @@ -228,20 +233,21 @@ impl Library { self.list_state.selected() } - /// Paint the sweep from `from_view` to `to_view`: toggle the mark of every - /// view index in the half-open range `(from_view, to_view]` (excludes the - /// row left, includes each row swept into, in either direction). Used after - /// a movement while visual mode is active. + /// Repaint after a visual-mode move from `from_view` to `to_view`. The + /// selection is the contiguous range `[anchor, cursor]`; a move that grows + /// or shrinks it toggles exactly the rows whose range membership changed + /// (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) { - if from_view == to_view { + let Some(anchor) = self.visual else { return; - } - if to_view > from_view { - for view in (from_view + 1)..=to_view { - self.toggle_mark_view(view); - } - } else { - for view in to_view..from_view { + }; + let (old_lo, old_hi) = (anchor.min(from_view), anchor.max(from_view)); + let (new_lo, new_hi) = (anchor.min(to_view), anchor.max(to_view)); + for view in old_lo.min(new_lo)..=old_hi.max(new_hi) { + let in_old = (old_lo..=old_hi).contains(&view); + let in_new = (new_lo..=new_hi).contains(&view); + if in_old != in_new { self.toggle_mark_view(view); } } @@ -383,7 +389,7 @@ impl Library { } else { COLOR_PRIMARY_DARK })) - .title(if self.visual { + .title(if self.visual.is_some() { // Visual (paint-select) mode: movement toggles marks. format!("{} — VISUAL", self.title) } else if let Some(query) = self.filter.query() { diff --git a/cbd-tui/src/app/mod.rs b/cbd-tui/src/app/mod.rs index 4733258..6ae47d2 100644 --- a/cbd-tui/src/app/mod.rs +++ b/cbd-tui/src/app/mod.rs @@ -885,16 +885,37 @@ mod tests { #[test] 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(); app.library .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); // gamma - let _ = app.dispatch(Action::LibraryPrev); // back onto beta -> unmark + let _ = app.dispatch(Action::LibraryPrev); // back onto beta assert_eq!( 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" ); } diff --git a/plan/summary.md b/plan/summary.md index 06a48d6..333bef3 100644 --- a/plan/summary.md +++ b/plan/summary.md @@ -1188,13 +1188,13 @@ leaves it, marks kept. range when visual is active; `Esc` in visual leaves the mode without clearing the `/` filter; node/focus changes leave it. -Decisions/deviations: **paint semantics are toggle-based** (back-sweeping -un-paints — the literal "toggle" requested; an anchored never-un-paint range is -a noted future refinement); **library-only** — the queue has no marks yet (a -standing `queue.rs` FIXME), so `v`/`V` are unbound there (D5); **spectrum key -`f` is a -chosen default**, trivially changed; **web-client parity deferred** (D6). Jump -moves (`g`/`G`, `Ctrl-d`/`Ctrl-u`) paint the whole span. +Decisions/deviations: **paint is anchored** — the selection is the contiguous +range `[anchor, cursor]`, and a move toggles the rows whose range membership +changed, so moving back cleanly reverses (a first per-step-toggle attempt +stranded the turnaround row — fixed); **library-only** — the queue has no marks +yet (a standing `queue.rs` FIXME), so `v`/`V` are unbound there (D5); **spectrum +key `f` is a chosen default**, trivially changed; **web-client parity deferred** +(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 covering enter/step/jump/back-sweep/exit/Esc/non-move-exit/node-focus-exit/