Repository navigation
feat(ui): restore contiguous shift-click range selection in the cue list - #851
Merged
Merged
Conversation
#786 collapsed ⌘ and ⇧ into one `isExtending` flag to keep the tap truth table at eight rows, which cost the cue list contiguous range selection entirely. Splitting them costs four more rows; pin all twelve, plus the precedence for ⌘⇧ pressed together (⇧ wins — #790 does not ask for Finder's "add range to selection", so that is a decision on record rather than an accident of which `if` came first). `CueRangeSelection` is pinned against the *displayed* id array, never against `Cue.ID` or cue time: "between" can only mean between the rows as drawn. A stale or missing anchor degrades to selecting the clicked row — it must not range from a phantom position. Red: `cannot find 'CueRangeSelection' in scope`, `cannot find 'CueRowTapModifier' in scope`.
…ist (#790) ⌘ keeps toggling one row; ⇧ now selects the inclusive range between the anchor and the clicked row, replacing the selection. The anchor is the last row selected *without* ⇧, and a ⇧-click never moves it, so repeated ⇧-clicks re-range from the same origin. `CueRangeSelection.range(in:from:to:)` takes the **displayed** id array rather than sorting ids or cue times: the user is pointing at rows on screen, so "between" can only mean between them as drawn. That is also what makes the issue's filter criterion correct by construction — a row-hiding filter would hand in a shorter array and need no change here. (There is no such filter today: `CueListPane.cues` is the item's whole cue array and the GO-type filter only dims rows via `rowOpacity`, so the criterion cannot be exercised end to end yet. Pinned by unit test over a pre-filtered array instead of claimed.) `CueRowTap.intent` is pinned into golden vector 8 with a C# mirror, which the issue does not mention, so the vector moves in the same commit: the `rowTapIntent` group goes 8 → 12 cases keyed on a `modifier` string instead of the `isExtending` bool, and `rangeSelection` joins as a twelfth group. Vector 8 having frozen `isExtending` as correct is precisely the shape of bug this is fixing. Verified the displayed-order pin is not vacuous: an id-sorting mutant was killed by `test_rangeFollowsDisplayedOrder_notIdOrder`, `test_forwardRange_isInclusiveOfBothEnds` and the vector drift guard.
#790) Vector 8 moved on the macOS side, so the C# core moves with it or the "Golden vectors (Windows, C# core)" job goes red: `rowTapIntent` is keyed on a `modifier` string instead of `isExtending` and carries 12 cases, and `rangeSelection` joins as a twelfth group. `CueRangeSelection.Range` returns a set and walks `displayed` for both ends. Reading the platform's modifier state into `CueRowTapModifier` is UI glue and deliberately stays out of the core — only the decision it feeds is pinned. The golden-vector verifier compares the range as a `HashSet<Guid>`: the vector emits displayed order only so the JSON diffs readably, and Swift's `uuidString` uppercases the hex where `Guid.ToString()` lowercases it. Not taken on trust — 566 green proves nothing by itself. Two mutants, both confirmed applied and both killed: sorting ids instead of following the displayed order died on "the range follows displayed order, not id order"; re-collapsing Range into ToggleSelection died on three `modifier=range` cases. Only the shuffled fixture kills the first, which is what it is there for — reversing a list leaves the same contiguous set.
The range-select path had no XCUITest. This adds one, and the awkward part is that a multi-row selection publishes nothing an XCUITest can read: the selection paints a row background (not queryable) and the `.isSelected` trait is spent on the playhead's current cue. Measured, not assumed, before settling on the two signals below: - `List.onDeleteCommand` needs the list first-responder, and clicking a stripe (a `Button`) never gives it focus — a diagnostic run pressed Delete after a plain stripe click and all six cues stayed. So "select the range, delete it, count what's left" is not available. - `XCUIElement.performWithKeyModifiers` is declared in the macOS SDK with only `ios`/`macCatalyst` availability annotations; it does work here, and pushing the modifier as global state is exactly what the row's `NSEvent.modifierFlags` read sees. So the test asserts what the app does publish: the playhead (a ⇧-click is `extendRange` and never seeks, where a dropped ⇧ would fall through to the stripe's plain `selectAndSeek` and jump to 165s), and the "Renumber Selected…" sheet, gated on `selection.count >= 2` and titled with the count. The title is localized, so the digits are read out of the AX value rather than matched against the English string. Mutation-proven rather than merely green: reverting `CueRowTapModifier` to #786's collapse (⇧ → `.toggle`) fails the test with "2" is not equal to "3" — the regression's own signature.
`architecture.md` still claimed `List`'s ⇧-range selection was "no longer reachable in the cue list" and `ui-sections.md` still read ⌘ and ⇧ as one extend gesture — both were true of #786 and are not true now. The two specs are left as the historical records they are: the #786 spec gets a superseded-in-part note next to the `isExtending` snippet rather than a rewritten truth table, and the M1d vector table gains the `rangeSelection` group with the case counts corrected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Splits
⌘from⇧on a cue-row click:⌘keeps toggling one row,⇧selects the contiguous run from the anchor to the clicked row. Contiguous range selection has been unreachable in the cue list since #786.Closes #790
Motivation
#786 gave the three text columns to inline editing, so the row's own tap handlers now claim the click before
Listsees it and the native⇧-range selection stopped working. Both modifiers were collapsed into oneisExtending: Bool— a deliberate trade at the time to keep the truth table at eight rows — which made⇧-click a toggle. On a list that routinely runs to hundreds of cues, "select these twenty cues" became twenty clicks.Implementation
CueRowTapModifier(plain/toggle/range) replaces the boolean, andCueRowTapIntent.extendSelectionsplits into.toggleSelectionand.extendRange. ReadingNSEvent.modifierFlagsinto that enum is UI glue; the precedence decision is not, so it is pinned:⇧wins over⌘⇧. Finder's "add a range to the selection" is out of scope, so⌘⇧simply behaves as⇧and the table stays at twelve rows.CueRangeSelection.range(in:from:to:)takes the displayed id array, an optional anchor and the target, and returns the inclusive run. Resolving against displayed order — not ids, not cue times — is the whole design: the user is pointing at rows on screen, so "between" can only mean between them as drawn, and a future row-hiding filter needs no change here, only a shorter list. No usable anchor (nil, or one no longer displayed) degrades to selecting just the clicked row; the click has to do something, and selecting what was clicked is the answer that does not surprise.The anchor lives in
CueListPaneas the last row selected without⇧. A⇧-click deliberately does not move it, so repeated⇧-clicks re-range from the same origin, matching Finder and every spreadsheet.Beyond the issue's scope, deliberately:
CueRowTap.intentturned out to be already pinned ingolden/cue-presentation-v1.jsonwith a C# mirror and its own CI job — which #790 never mentions. Changing its signature without the mirror would have broken "Golden vectors (Windows, C# core)", soCueRangeSelectionis pinned alongside it and mirrored inOnlyCue.Core, keeping the epic #728 invariant vector-pinned ⇔ present inOnlyCue.Core.Changes
OnlyCue/UI/CueRowTapIntent.swift—CueRowTapModifier+ itsNSEvent.ModifierFlagsreading; the five-case intent.OnlyCue/UI/CueRangeSelection.swift(new) — the range resolver.OnlyCue/UI/CueRowView.swift—onToggleSelection/onExtendRangereplaceonExtendSelection; the old staticisExtendingscratch state is gone.OnlyCue/UI/CueListPane.swift—selectionAnchorstate and the two new row callbacks, both keepinglastRowTapSelectionin sync so the reveal-the-rowonChangestill skips self-inflicted changes.OnlyCueTests/CueRangeSelectionTests.swift(new),OnlyCueTests/CueRowTapIntentTests.swift— 8 + 13 tests.golden/cue-presentation-v1.json+ its Swift fixtures/emitters —rowTapIntent8 → 12 cases, newrangeSelectiongroup (8 cases), twelve groups total.windows/OnlyCue.Core/Presentation/CueRowTapIntent.cs,CueRangeSelection.cs(new), and the vector DTO/tests — the C# mirror.docs/architecture.md,docs/ui-sections.md— corrected; the two specs get superseded notes rather than rewritten history.Screenshots / Demo
No visual change — the selection paints with the existing row background. What changes is the gesture: click a stripe,
⇧-click a stripe further down, and every row between them is selected instead of just the two.Test Plan
⇧-click row 9 → rows 3–9 inclusive (test_forwardRange_isInclusiveOfBothEnds).⇧-click row 5 → rows 3–5; the anchor did not move to 9 (CueListPaneleaves it put; unit-covered by the repeated-range cases).⇧-click backwards selects the same run (test_backwardRange_equalsForwardRange).⌘-click inside a range removes only that row (CueRowTapIntentTests,.toggle→.toggleSelection+formSymmetricDifference).⇧-click never opens aTextFieldand never moves the playhead — asserted in the new UI test against the transport readout.stripe, modifier=range, readOnly=true).CueListSingleClickEditUITests).Not covered end to end, stated rather than glossed: the criterion "with a filter active, a
⇧-click range covers only rows visible under the filter". There is no row-hiding filter in the app today —CueListPane.cuesis the active item's full cue array and the GO-type filter only dims rows viarowOpacity— so there is nothing to click through. The design answer is thatCueRangeSelectiontakes the displayed array and is therefore correct by construction; it is unit-tested with a pre-filtered, shuffled array (test_rangeFollowsDisplayedOrder_notIdOrder).Observability note from writing the UI test:
List.onDeleteCommandneeds the list to be first responder, and clicking a stripe (aButton) does not give it focus — measured with a throwaway diagnostic, six cues survived a Delete keypress. So "select the range, press ⌫, count what's left" is not available as an assertion, and the test reads the playhead plus the "Renumber Selected…" sheet's count instead. Worth a separate issue; out of scope here.OnlyCue verification (required)
Spec link:
docs/architecture.md#cue-selection,docs/ui-sections.md"Cue Row Interaction",docs/superpowers/specs/2026-08-28-cue-list-single-click-edit.mdCloses: #790
102cf5ais the failing test commit,065446dturns it green)test_shiftClickOnColourStripe_selectsTheRangeFromTheAnchor; the filter scenario is unmappable today, see above)docs/devand do not run on PRs — the new XCUITest was verified locally, 4/4)Mutation proof
Green-on-the-first-try proves nothing, so each layer was mutated and the mutant confirmed applied before trusting the verdict:
CueRangeSelection.rangesorts ids instead of following the arraytest_rangeFollowsDisplayedOrder_notIdOrder,test_forwardRange_isInclusiveOfBothEnds,test_committedVectors_matchCurrentSwiftOutputRangesorts idsrangeSelectioncase — and only that one, because reversing a list preserves the contiguous set, which is exactly why the shuffled fixture existsCueRowTapModifier.Range => ToggleSelectionmodifier=rangerowTapIntentcasesCueRowTapModifier.init(flags:)reverted to #786's collapse (⇧ →.toggle)"2" is not equal to "3"— the regression's own signatureLocal: 1842 Swift unit tests, 4/4 cue-list UI tests, 566 C# tests, SwiftLint
--strictclean.