From 9c857ae0cc7649ee565ed01134f7943953b9f8c4 Mon Sep 17 00:00:00 2001 From: rzen Date: Fri, 7 Aug 2026 15:10:35 -0400 Subject: [PATCH] =?UTF-8?q?Selected-ness=20rides=20down=20as=20a=20compare?= =?UTF-8?q?d=20parameter=20=E2=80=94=20a=20marquee=20crossing=20repaints?= =?UTF-8?q?=20its=20faces,=20not=20the=20board?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A selection change re-ran every CardFaceView on the board (180 bodies ≈ 85 ms on the 6×30 fixture, 515 ≈ 233 ms on a real 515-card board, debug): the face's body read store.selection in three places — isSelected, the drag replica's count, and the context menu's styleTarget — and Observation invalidates every reader of the property, past the equatable gate entirely. The band overlay stayed cheap, which is why the marquee tracked the cursor while the highlight lagged ~0.4 s behind. Now LaneView and TrashLaneView hoist one selection read per body and hand each face isSelected/selectedCount as compared parameters; StyleMenuItems takes its target as a deferred closure; TrashLaneRowView gains the same treatment plus the Equatable gate it never needed before. Select-one-card: 180 bodies → 1. A growing band costs the selection's own running size; the real board's crossing fell 233 → 112 ms — the remainder is lane bodies re-measuring their masonry, a separate lane-level finding recorded in RENDER-INSTRUMENTATION.md. Also: select() gains defaultsSoleMember — the marquee's explicit nils never avoided the sole-member default, so a one-card band acquired a selectionHead and could scroll the lane out from under its own drag. MarqueeRenderCostTests pins the shape: redundant samples cost zero bodies, a growing band pays per crossing, and selectionStillRepaints holds a ≤8 budget. --- CHANGELOG.md | 2 + Kanban/LiveStore/BoardStore.swift | 15 +- Kanban/LiveStore/TransientBoardState.swift | 29 +- Kanban/UI/Board/CardFaceView.swift | 77 +++-- Kanban/UI/Board/LaneView.swift | 23 +- Kanban/UI/Board/SelectionClicks.swift | 14 +- Kanban/UI/Board/TrashLaneRowView.swift | 53 +++- Kanban/UI/Board/TrashLaneView.swift | 25 +- Kanban/UI/StyleEditor.swift | 20 +- KanbanTests/BoardRenderPerformanceTests.swift | 20 +- KanbanTests/MarqueeRenderCostTests.swift | 285 ++++++++++++++++++ KanbanTests/TransientBoardStateTests.swift | 36 +++ KanbanTests/ViewEquatableTests.swift | 265 ++++++++++++++-- RENDER-INSTRUMENTATION.md | 10 +- 14 files changed, 795 insertions(+), 79 deletions(-) create mode 100644 KanbanTests/MarqueeRenderCostTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cc79e5..3aaf0b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ **August 2026** +The rubber band now highlights cards the moment it touches them, instead of lagging behind on large boards. + The app's appearance can now be set to Light, Dark, or Auto from the View menu or the toolbar's new Appearance item. A board can now wear a background image, painted across the whole window with a frosted strip keeping the title bar legible. diff --git a/Kanban/LiveStore/BoardStore.swift b/Kanban/LiveStore/BoardStore.swift index 71262d8..f2c9385 100644 --- a/Kanban/LiveStore/BoardStore.swift +++ b/Kanban/LiveStore/BoardStore.swift @@ -4545,8 +4545,19 @@ public final class BoardStore: HealHost { /// for "the lane that most recently held selection or a creation", and a *card* selection is /// its lane holding selection just as much as the lane's own header click is — so both are /// noted here, and creation notes itself in `beginPlaceholder`. - public func select(_ ids: Set, in container: ItemContainer, anchor: ItemID? = nil, head: ItemID? = nil) { - transient.select(ids, in: container, anchor: anchor, head: head) + /// + /// - Parameter defaultsSoleMember: forwarded verbatim — see `TransientBoardState.select`, whose + /// sole-member default the rubber band opts out of. + public func select( + _ ids: Set, + in container: ItemContainer, + anchor: ItemID? = nil, + head: ItemID? = nil, + defaultsSoleMember: Bool = true + ) { + transient.select( + ids, in: container, anchor: anchor, head: head, defaultsSoleMember: defaultsSoleMember + ) transient.noteActiveLane(Self.lane(holding: ids, in: snapshot)) } diff --git a/Kanban/LiveStore/TransientBoardState.swift b/Kanban/LiveStore/TransientBoardState.swift index fd1112c..bea540c 100644 --- a/Kanban/LiveStore/TransientBoardState.swift +++ b/Kanban/LiveStore/TransientBoardState.swift @@ -389,17 +389,32 @@ public final class TransientBoardState { /// The grammar itself is `SelectionGrammar`'s — pure, testable, and the one funnel every click /// surface goes through (`BoardStore.click`). This is the storage half, and its only rule of its /// own is the **anchor default**, which the head shares: `nil` with a sole member takes that - /// member, `nil` with any other count takes nothing. That makes the two callers that pass - /// nothing behave exactly as they should — a one-item selection made by any route is a - /// legitimate range origin *and* a legitimate place to arrow from, while a marquee or a Select - /// All names no gesture and so leaves a ⇧-click acting plain and the arrows re-deriving a - /// position from the set's last member. + /// member, `nil` with any other count takes nothing. That makes the callers that pass nothing + /// behave exactly as they should — a one-item selection made by any route is a legitimate range + /// origin *and* a legitimate place to arrow from. + /// + /// **The default is opt-out, because one caller genuinely names no gesture.** A Select All over a + /// one-card board is still a selection the user pointed at, but a rubber band is not: it names no + /// click to range from and no item to arrow from *whatever* it happens to enclose, and passing + /// `nil` cannot say so — `nil` is what asks for the default. So `defaultsSoleMember: false` takes + /// `anchor` and `head` verbatim, `nil` included, and a band that sweeps exactly one card leaves + /// both cursors empty instead of quietly acquiring them (`MarqueeControl.gesture`; a + /// `selectionHead` set mid-band would additionally fire `LaneView.cardStack`'s scroll-to under the + /// user's own drag). /// /// Deliberately **not** filtered against the snapshot: a caller selects what it is rendering, and /// `resolve(against:)` on the next reload is what keeps the set honest over time. - public func select(_ ids: Set, in container: ItemContainer, anchor: ItemID? = nil, head: ItemID? = nil) { + /// + /// - Parameter defaultsSoleMember: whether a `nil` anchor or head falls back to a sole member. + public func select( + _ ids: Set, + in container: ItemContainer, + anchor: ItemID? = nil, + head: ItemID? = nil, + defaultsSoleMember: Bool = true + ) { selection = ItemReferenceSet(ids: ids, container: container) - let sole = ids.count == 1 ? ids.first : nil + let sole = defaultsSoleMember && ids.count == 1 ? ids.first : nil selectionAnchor = anchor ?? sole selectionHead = head ?? sole } diff --git a/Kanban/UI/Board/CardFaceView.swift b/Kanban/UI/Board/CardFaceView.swift index 0521027..7abe00a 100644 --- a/Kanban/UI/Board/CardFaceView.swift +++ b/Kanban/UI/Board/CardFaceView.swift @@ -141,6 +141,25 @@ struct CardFaceView: View, Equatable { /// registry (the resting grid's input) and starts the card drag session from `.onDrag`. let drops: BoardDropContext + /// Whether this card is in the selection **of its own container** — a parameter rather than a + /// read off the store, and that is the whole of the fix RENDER-INSTRUMENTATION.md ▸ "Selection is + /// O(board) in card bodies" asked for. + /// + /// Observation tracks whole properties, so a body that reads `store.selection` is subscribed to + /// *every* selection change on the board — 180 faces re-running per marquee sample on the 6×30 + /// fixture, 515 on a real board, and `.equatable()` powerless over any of it because a direct + /// Observation invalidation never consults the gate. Taking selected-ness as a compared input + /// instead moves the subscription up one level, to the parent that already holds it: the lane's + /// body reads the selection once for its own header, hands each face the answer, and the gate + /// below then re-runs exactly the faces whose flag actually flipped. + let isSelected: Bool + + /// The size of the selection this face belongs to — **1 when unselected**, which the parent + /// computes and normalizes so the drag replica's fan and count badge need no store read of their + /// own (`dragReplica`). Deliberately not `Int?`: "how many ride along" has an answer for every + /// face, and one is it. + let selectedCount: Int + /// The app-wide quick-style recents — see `LaneView`'s own note. @Environment(AppModel.self) private var appModel @@ -179,16 +198,32 @@ struct CardFaceView: View, Equatable { /// The whole of what this face is a function of **as far as its parent is concerned**: the card /// value (`Card` is `Equatable` down to its attachment names and its parsed document), which home - /// it is drawn in (`CardFaceRole.isEquivalent(to:)`), and the three window-lived collaborators — - /// the store by identity, the band and the drop machinery by their own equivalence tests, which - /// exist because the strip rebuilds both structs, closures and all, on every body pass. + /// it is drawn in (`CardFaceRole.isEquivalent(to:)`), the two selection figures the parent + /// resolves for it, and the three window-lived collaborators — the store by identity, the band + /// and the drop machinery by their own equivalence tests, which exist because the strip rebuilds + /// both structs, closures and all, on every body pass. /// - /// **What the gate does not suppress is the point.** Everything this body reads through - /// Observation — `store.selection`, `store.searchFilter`, `store.transient.pendingCut` and the - /// rename editor, `drops.session.isDragging`, `appModel.styleRecents` — invalidates this view - /// directly, and `.equatable()` has no say in that. The gate only stops the *parent* handing a - /// face a new-but-identical set of inputs and re-running it for nothing, which during a card drag - /// is what every proposal change does to every face in the lane. + /// **Selection is a compared input now, and that is what makes the gate reach it.** It used to be + /// an Observation read — `isSelected` off `store.selection` — which meant a click anywhere on the + /// board invalidated every face on it *directly*, past the gate entirely, and the measured cost + /// of a marquee sample was the whole board's worth of bodies. Selected-ness now rides down as + /// `isSelected`/`selectedCount` on the parent's own subscription (`LaneView` already reads the + /// selection for its header; `TrashLaneView` gained one hoisted read to match), so a selection + /// change re-runs the lane bodies that were subscribed anyway plus exactly the faces whose flag + /// flipped. See RENDER-INSTRUMENTATION.md ▸ Selection is O(board) in card bodies. + /// + /// **What the gate still does not suppress is the point.** What remains of this body's own + /// Observation reads — `store.transient.pendingCut` and the rename editor, + /// `drops.session.isDragging`, `appModel.styleRecents` — invalidates this view directly, and + /// `.equatable()` has no say in that. The gate only stops the *parent* handing a face a + /// new-but-identical set of inputs and re-running it for nothing, which during a card drag is + /// what every proposal change does to every face in the lane. + /// + /// `store.searchFilter` is deliberately absent from that list: its only use here is `ownSlotSeed`, + /// reached from `startBoardCardDrag`, which runs when a drag begins rather than when the body + /// does — an event-time read subscribes nothing. `draggedIDs`, the context menus' `targetIDs` and + /// the deferred `styleTarget` read the selection the same way, inside actions, which is why they + /// stayed as they were. /// /// Deliberately NOT compared: `@State` (per-identity, preserved across updates anyway), /// `@Environment` values (SwiftUI invalidates on those itself), and the `.board` role's @@ -196,6 +231,8 @@ struct CardFaceView: View, Equatable { nonisolated static func == (lhs: CardFaceView, rhs: CardFaceView) -> Bool { lhs.card == rhs.card && lhs.role.isEquivalent(to: rhs.role) + && lhs.isSelected == rhs.isSelected + && lhs.selectedCount == rhs.selectedCount && lhs.store === rhs.store && lhs.marquee.isEquivalent(to: rhs.marquee) && lhs.drops.isEquivalent(to: rhs.drops) @@ -505,10 +542,15 @@ struct CardFaceView: View, Equatable { /// The image travelling under the cursor: this card's face at its real size, fanned with ghosts /// and a count badge when the whole multi-selection rides along (03-board-ui.md § Motion). + /// + /// The count is `selectedCount`, not a fresh read of the selection: `.onDrag(_:preview:)`'s + /// preview builder is **non-escaping**, so it evaluates while the body does, and a `store.selection` + /// read here would have kept every face on the board subscribed no matter what the rest of this + /// view did. The parent already normalizes the figure to 1 for an unselected face, so the branch + /// that used to compute it is gone rather than moved (see `isSelected`'s note); the `max` is belt + /// over those braces, because a zero here would draw a badge reading nothing. private var dragReplica: some View { - let count = store.selection.container == role.container && store.selection.ids.contains(card.id) - ? max(1, store.selection.ids.count) - : 1 + let count = max(1, selectedCount) return ZStack { if count > 2 { replicaFace.offset(x: 10, y: 10).opacity(0.45) } if count > 1 { replicaFace.offset(x: 5, y: 5).opacity(0.7) } @@ -578,7 +620,12 @@ struct CardFaceView: View, Equatable { Button("Rename") { beginRename() } .disabled(!store.acceptsBoardMutations) - StyleMenuItems(store: store, recents: appModel.styleRecents, target: styleTarget) + // The target is **deferred**, and that is not a style choice: `.contextMenu`'s builder is + // non-escaping, so it runs while this body does, and computing a `StyleTarget` eagerly meant + // reading `store.selection` at body time — the O(board) subscription this view was rebuilt to + // shed (`isSelected`). Passed as a closure, `styleTarget` evaluates when a row *acts*, which + // is where every other menu target here is already read from (`deleteTargets`). + StyleMenuItems(store: store, recents: appModel.styleRecents, target: { styleTarget }) Divider() @@ -779,10 +826,6 @@ struct CardFaceView: View, Equatable { // MARK: - Selection and rename plumbing - private var isSelected: Bool { - store.selection.container == role.container && store.selection.ids.contains(card.id) - } - /// What the plate's edge is painted with: the accent when this card is selected or a Finder file /// drag is hovering it, a separator hairline under Increase Contrast, and nothing otherwise. /// diff --git a/Kanban/UI/Board/LaneView.swift b/Kanban/UI/Board/LaneView.swift index 580b22b..be7362a 100644 --- a/Kanban/UI/Board/LaneView.swift +++ b/Kanban/UI/Board/LaneView.swift @@ -343,7 +343,9 @@ struct LaneView: View, Equatable { Divider() - StyleMenuItems(store: store, recents: appModel.styleRecents, target: styleTarget) + // Deferred, `CardFaceView`'s reason (`StyleMenuItems`): a non-escaping menu builder makes an + // eagerly computed target a body-time selection read. + StyleMenuItems(store: store, recents: appModel.styleRecents, target: { styleTarget }) Divider() @@ -734,6 +736,18 @@ struct LaneView: View, Equatable { // the session's hidden-member resolution) once per element — O(n²) per lane body, which // a drag pickup's synchronous whole-board layout multiplied into a visible stall. let slots = self.slots + // **The board-side selection, read once for the whole lane.** This body is already subscribed + // to it — the header's own `isSelected` reads it — so hoisting the set here costs nothing new + // and lets every face take its selected-ness as a *compared parameter* instead of reading the + // store itself. That is the difference between a selection change re-running one body per + // lane and re-running one per card on the board (`CardFaceView.isSelected`, + // RENDER-INSTRUMENTATION.md ▸ Selection is O(board) in card bodies). + // + // Hoisted for `slots`' reason too: reading it inside the `ForEach` closure would re-derive + // the container branch and re-count the set once per element. + let selection = store.selection + let selectedIDs = selection.container == .board ? selection.ids : [] + let selectedCount = max(1, selectedIDs.count) return ScrollView(.vertical) { // Cards stay standard width whatever the lane spans: at a slot width of // `units × standard + (units - 1) × gap`, `MasonryLayout` divides back into exactly @@ -748,7 +762,12 @@ struct LaneView: View, Equatable { card: card, role: .board(openCard: openCard), marquee: marquee, - drops: drops + drops: drops, + isSelected: selectedIDs.contains(card.id), + // 1 for an unselected face: the replica's fan and count badge want + // "how many ride along", and a card outside the selection drags + // alone (`CardFaceView.draggedIDs`). + selectedCount: selectedIDs.contains(card.id) ? selectedCount : 1 ) // **The value gate** (`CardFaceView.==`) — the lane's own, one level // down: this body re-runs on every proposal change while a drag is over diff --git a/Kanban/UI/Board/SelectionClicks.swift b/Kanban/UI/Board/SelectionClicks.swift index b022f31..ab841f7 100644 --- a/Kanban/UI/Board/SelectionClicks.swift +++ b/Kanban/UI/Board/SelectionClicks.swift @@ -122,10 +122,16 @@ struct MarqueeControl { // Neither cursor: a band names no click to range from and no item to arrow from, // so a ⇧-click after one acts plain and an arrow re-derives a position from the // set's last member (`TransientBoardState.selectionAnchor`, `selectionHead`). - // Both are spelled out rather than defaulted, because `select`'s sole-member - // default would otherwise pick one up the moment a band happened to sweep - // exactly one card. - store.select(ids, in: session.container, anchor: nil, head: nil) + // + // **`defaultsSoleMember: false` is what says that, and the explicit `nil`s never + // did.** `nil` *is* the default, so a band that swept exactly one card used to + // pick up both cursors anyway — harmless for the anchor, not for the head: + // `LaneView.cardStack` watches `selectionHead` and scrolls the lane to it, which + // means a one-card band could scroll the board out from under the drag that was + // drawing it. The flag is the only way to spell "no gesture named these". + store.select( + ids, in: session.container, anchor: nil, head: nil, defaultsSoleMember: false + ) } } .onEnded { _ in session.end() } diff --git a/Kanban/UI/Board/TrashLaneRowView.swift b/Kanban/UI/Board/TrashLaneRowView.swift index 4abeefd..04f7433 100644 --- a/Kanban/UI/Board/TrashLaneRowView.swift +++ b/Kanban/UI/Board/TrashLaneRowView.swift @@ -36,7 +36,7 @@ import SwiftUI /// `renameTarget`/`boardStyleTarget`/`LaneWidthCommands`, all of which require a `.board` /// selection. /// - **No Finder file drop** — the column's own delegate clears the file highlight (`TrashDrop`). -struct TrashLaneRowView: View { +struct TrashLaneRowView: View, Equatable { let store: BoardStore let lane: TrashedLane @@ -56,6 +56,16 @@ struct TrashLaneRowView: View { /// geometry and the band's begin guard (`MarqueeTargetRegistry`). let marquee: MarqueeControl + /// Whether this row is in the trash-side selection — a parameter for `CardFaceView.isSelected`'s + /// reason, and resolved by the same hoisted read in `TrashLaneView.scrollableCards` that feeds + /// the faces beside it, so a row and a card in one column can never disagree about what is + /// selected. + let isSelected: Bool + + /// The size of the selection this row belongs to — **1 when unselected**, normalized by the + /// parent. The drag replica's fan and count badge are its only reader (`dragReplica`). + let selectedCount: Int + /// Increase Contrast, for the plate's borders — `CardFaceView`'s rule, so a selected row and a /// selected card wear the same ring at the same strength. @Environment(\.colorSchemeContrast) private var contrast @@ -74,6 +84,34 @@ struct TrashLaneRowView: View { /// This row's drawn width — the drag replica's, measured for `CardFaceView`'s reason. @State private var measuredWidth: CGFloat = 0 + /// The row's rebuild gate — `CardFaceView.==`'s twin, one level over: the lane value (a + /// `TrashedLane` is `Equatable` down to its title and held-card count), the two selection figures + /// the column resolves, the store and the confirmation host by identity, and the band and the + /// drop machinery by their own equivalence tests, which exist because the window rebuilds both + /// structs — closures and all — on every body pass. + /// + /// **This row had no gate until the selection came down as a parameter, and that is the reason it + /// has one now.** Before, the column's body re-ran only when the trash's contents changed, so + /// there was nothing worth suppressing; hoisting the selection read into + /// `TrashLaneView.scrollableCards` made that body re-run on every selection change on the trash + /// side, and without a gate here every row would rebuild on each one — the very shape the change + /// exists to remove (`CardFaceView.isSelected`). Applied through `.equatable()` at the call site, + /// exactly as the card face beside it is. + /// + /// Deliberately NOT compared: `@State` (per-identity, preserved across updates anyway) and + /// `@Environment` values (SwiftUI invalidates on those itself). And, as with the face, the gate + /// has no say over this body's remaining Observation reads — `store.transient.pendingCut`, + /// `drops.session.isDragging` — which invalidate it directly. + nonisolated static func == (lhs: TrashLaneRowView, rhs: TrashLaneRowView) -> Bool { + lhs.lane == rhs.lane + && lhs.isSelected == rhs.isSelected + && lhs.selectedCount == rhs.selectedCount + && lhs.store === rhs.store + && lhs.confirmations === rhs.confirmations + && lhs.marquee.isEquivalent(to: rhs.marquee) + && lhs.drops.isEquivalent(to: rhs.drops) + } + var body: some View { row .contextMenu { menu } @@ -247,10 +285,13 @@ struct TrashLaneRowView: View { /// The image under the cursor: this row at its drawn width, fanned when the whole selection /// rides along — `CardFaceView.dragReplica`'s treatment, so a restore looks like every other /// drag on the board. + /// + /// Off `selectedCount` rather than the store, for the face's reason exactly: `.onDrag`'s preview + /// builder is non-escaping, so a read here happens at body time and would keep this row + /// subscribed to every selection change on the board. The `max` is belt over the parent's braces, + /// `CardFaceView.dragReplica`'s note again. private var dragReplica: some View { - let count = store.selection.container == .trash && store.selection.ids.contains(lane.id) - ? max(1, store.selection.ids.count) - : 1 + let count = max(1, selectedCount) return ZStack { if count > 2 { replicaFace.offset(x: 10, y: 10).opacity(0.45) } if count > 1 { replicaFace.offset(x: 5, y: 5).opacity(0.7) } @@ -342,10 +383,6 @@ struct TrashLaneRowView: View { // MARK: - Selection treatment - private var isSelected: Bool { - store.selection.container == .trash && store.selection.ids.contains(lane.id) - } - /// The accent ring when selected, a separator hairline under Increase Contrast, nothing /// otherwise — `CardFaceView.plateStroke`'s three-way branch, minus the file-drop hover the /// trash never has. diff --git a/Kanban/UI/Board/TrashLaneView.swift b/Kanban/UI/Board/TrashLaneView.swift index 8963df4..a795ced 100644 --- a/Kanban/UI/Board/TrashLaneView.swift +++ b/Kanban/UI/Board/TrashLaneView.swift @@ -327,7 +327,18 @@ struct TrashLaneView: View { } private var scrollableCards: some View { - ScrollView(.vertical) { + // **The trash-side selection, read once for the whole column — and this is a new subscription, + // deliberately.** `LaneView` was already reading the selection for its header, so hoisting it + // there was free; this body was not, and now is. The trade is the point: one column body per + // selection change, in place of one body per *row* in it, which is what every face and row + // here cost while each read `store.selection` for itself (`CardFaceView.isSelected`, + // RENDER-INSTRUMENTATION.md ▸ Selection is O(board) in card bodies). A trash holds one board's + // deletions, so even the bad side of that trade is small — but the shape is what matters, and + // the shape is now O(1) bodies rather than O(rows). + let selection = store.selection + let selectedIDs = selection.container == .trash ? selection.ids : [] + let selectedCount = max(1, selectedIDs.count) + return ScrollView(.vertical) { // **`MasonryLayout` at one column, and a plain `VStack` deliberately not.** The trash is // one width unit, so its masonry is a single column — but it is the *same* layout the // lanes use, which is what makes the drag's make-room reflow read as positional slides @@ -348,7 +359,9 @@ struct TrashLaneView: View { card: card, role: .trash(confirmations: confirmations), marquee: marquee, - drops: drops + drops: drops, + isSelected: selectedIDs.contains(card.id), + selectedCount: selectedIDs.contains(card.id) ? selectedCount : 1 ) // The value gate, `LaneView`'s rule on the trash side (`CardFaceView.==`). .equatable() @@ -361,8 +374,14 @@ struct TrashLaneView: View { lane: lane, confirmations: confirmations, drops: drops, - marquee: marquee + marquee: marquee, + isSelected: selectedIDs.contains(lane.id), + selectedCount: selectedIDs.contains(lane.id) ? selectedCount : 1 ) + // The row's own gate (`TrashLaneRowView.==`) — worth having now that the + // column's body re-runs on every selection change rather than only on a + // reload. + .equatable() case .shadow: // The delete gesture's shadow, holding the topmost row open // (04-interactions.md ▸ The trash). At the nominal card height: the cards diff --git a/Kanban/UI/StyleEditor.swift b/Kanban/UI/StyleEditor.swift index 57a0be4..57b340e 100644 --- a/Kanban/UI/StyleEditor.swift +++ b/Kanban/UI/StyleEditor.swift @@ -656,15 +656,21 @@ private struct StyleWellGrid: View { /// One view for both menus because the entries are identical on a card and on a lane: only the /// *target* differs, and that is the caller's to compute (the clicked item, or the selection it /// belongs to). +/// +/// **The target arrives as a closure, deliberately.** `.contextMenu`'s builder is non-escaping, so a +/// caller's menu is assembled while its own body runs — and a target computed eagerly is a +/// `store.selection` read at body time, which under Observation subscribes that body to every +/// selection change on the board. Deferring it means a card face's context menu costs its face +/// nothing until a row actually acts (`CardFaceView.isSelected` has the measurement). struct StyleMenuItems: View { let store: BoardStore let recents: StyleRecents - let target: StyleTarget + let target: () -> StyleTarget var body: some View { Button("Style…") { - store.transient.beginStyleEditor(for: target) + store.transient.beginStyleEditor(for: target()) } .disabled(!store.acceptsBoardMutations) @@ -683,11 +689,15 @@ struct StyleMenuItems: View { /// /// **Absent until it has something to offer.** A brand-new install has no recents, and an empty /// picker in a context menu is a row that looks broken. +/// +/// The target is deferred for `StyleMenuItems`' reason, and this row is where the deferral has to +/// hold: the `Binding` below is read when the menu shows a checkmark and written when a dot is +/// picked, both of them the picker's own doing rather than the enclosing card face's body. struct QuickStyleRow: View { let store: BoardStore let recents: StyleRecents - let target: StyleTarget + let target: () -> StyleTarget /// A sentinel for "the current value is not one of these", so a mixed batch — or a background /// that has aged out of the recents — leaves the row unchecked rather than checking the wrong @@ -717,13 +727,13 @@ struct QuickStyleRow: View { private var selection: Binding { Binding( get: { - let state = StyleFieldState.resolve(store.styleSubjects(of: target).map(\.background)) + let state = StyleFieldState.resolve(store.styleSubjects(of: target()).map(\.background)) guard case let .uniform(value) = state, recents.backgrounds.contains(value) else { return .other } return .value(value) }, set: { picked in guard case let .value(name) = picked else { return } - StyleCommand.apply(background: .set(name), to: target, in: store, recents: recents) + StyleCommand.apply(background: .set(name), to: target(), in: store, recents: recents) } ) } diff --git a/KanbanTests/BoardRenderPerformanceTests.swift b/KanbanTests/BoardRenderPerformanceTests.swift index e3d9198..a4248f7 100644 --- a/KanbanTests/BoardRenderPerformanceTests.swift +++ b/KanbanTests/BoardRenderPerformanceTests.swift @@ -389,12 +389,20 @@ struct BoardRenderPerformanceTests { // run. The other half of the gate: too strict a `==` would show up here as a zero. #expect(selected.cards > 0, "selecting a card repainted nothing") - // What it actually costs is **every face on the board**, and that is the design rather than a - // defect: `CardFaceView.body` reads `store.selection` (`isSelected`), so a selection change - // invalidates all of them directly — the Observation half the gates explicitly do not cover. - // Recorded here as a number rather than asserted as a budget: narrowing it would mean each - // face taking its own selected-ness as a compared parameter, which is a design change and not - // this card's. See RENDER-INSTRUMENTATION.md ▸ What the first run found. + // **And it costs a handful of faces, not the board.** This used to be "every face on the + // board" — `CardFaceView.body` read `store.selection` for its own `isSelected`, so under + // Observation's property-level tracking one click invalidated all \(laneCount * cardsPerLane) + // of them *directly*, past the gate entirely (RENDER-INSTRUMENTATION.md ▸ Selection is + // O(board) in card bodies). Selected-ness is a compared parameter now — the lane hoists the + // selection once and hands each face its answer — so what re-runs is the lane bodies that + // were subscribed anyway plus the faces whose flag actually flipped. + // + // The budget is the one-card-edit budget above and it is loose for the same reason: SwiftUI + // evaluates a body more than once per update, so a single flipped face is worth several + // counts. What it rules out is the old shape, which was two orders of magnitude over this. + #expect(selected.cards <= 8, + "selecting one card re-rendered \(selected.cards) of \(laneCount * cardsPerLane) card faces") + #expect(selected.strips >= 1, "the strip did not re-run for a selection change") } diff --git a/KanbanTests/MarqueeRenderCostTests.swift b/KanbanTests/MarqueeRenderCostTests.swift new file mode 100644 index 0000000..7c280c7 --- /dev/null +++ b/KanbanTests/MarqueeRenderCostTests.swift @@ -0,0 +1,285 @@ +import AppKit +import Foundation +import SwiftUI +import Testing +@testable import Kanban + +/// **What a marquee drag sample costs the renderer** — the regression suite for the selection fix +/// (2026-08-07), grown out of the investigation that found it. +/// +/// `MarqueeControl.gesture`'s `onChanged` calls `store.select(ids, …)` on EVERY mouse sample, +/// whether or not the swept set changed, and `TransientBoardState.select` writes its `@Observable` +/// properties unconditionally — Observation notifies on every set, equal or not. So a band drag is a +/// stream of selection changes at pointer rate, and whatever one selection change costs the board, +/// the band pays it sixty times a second. +/// +/// The investigation measured that cost at **180 card bodies and ~85 ms per sample** on the 6×30 +/// fixture below (515 bodies, ~233 ms on a real 515-card board, debug): every face on the board, +/// every sample, because `CardFaceView.body` read `store.selection` for its own `isSelected` and +/// Observation tracks whole properties. `.equatable()` could not help — a direct Observation +/// invalidation never consults the gate (RENDER-INSTRUMENTATION.md ▸ Selection is O(board) in card +/// bodies). +/// +/// The fix made selected-ness a **compared parameter**: `LaneView` and `TrashLaneView` hoist one +/// selection read per body and hand each face `isSelected`/`selectedCount`, so a selection change +/// re-runs the lane bodies that were subscribed anyway plus the faces whose flag actually flipped. +/// These tests pin that shape — a growing band pays per *crossing*, not per card on the board — and +/// the redundant streams pin the other half: a sample that changes nothing costs nothing. +/// +/// The prints stay: a budget says whether the shape held, and the numbers beside it say by how much. + +// MARK: - Fixture (BoardRenderPerformanceTests' shape) + +private let laneCount = 6 +private let cardsPerLane = 30 + +private func laneName(_ lane: Int) -> String { + String(format: "1%07d-1111-4111-8111-111111111111", lane) +} + +private func cardName(_ lane: Int, _ card: Int) -> String { + String(format: "2%03d%04d-2222-4222-8222-222222222222", lane, card) +} + +@MainActor +private func makeFixture() throws -> WriterFixture { + let fixture = try WriterFixture() + try fixture.board(title: "Marquee Cost Board") + for lane in 0.. NSWindow? + let confirmations: TrashConfirmations + let openCard: @MainActor (ItemID) -> Void + let search: BoardSearchPresentation + + @Environment(AppModel.self) private var appModel + + var body: some View { + BoardView( + store: store, + window: window, + confirmations: confirmations, + openCard: openCard, + search: search + ) + .environment(\.boardZoom, appModel.zoom.context) + } +} + +@MainActor +private final class HostedBoard { + let store: BoardStore + let appModel: AppModel + let window: NSWindow + let view: NSView + private let scratch: URL + private let preferencesDomain: String + + init(store: BoardStore, scratch: URL) { + self.store = store + self.scratch = scratch + preferencesDomain = "dev.rzen.indie.Kanban.marquee-cost.\(UUID().uuidString)" + appModel = AppModel( + registryStorageURL: scratch.appendingPathComponent("board-registry.json"), + clipboardStagingRoot: scratch.appendingPathComponent("Clipboard", isDirectory: true), + preferences: UserDefaults(suiteName: preferencesDomain)! + ) + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 1600, height: 1000), + styleMask: [.titled], backing: .buffered, defer: false + ) + self.window = window + let root = ZoomedBoard( + store: store, + window: { [weak window] in window }, + confirmations: TrashConfirmations(), + openCard: { _ in }, + search: BoardSearchPresentation() + ) + .environment(appModel) + let hosting = NSHostingView(rootView: root) + hosting.frame = NSRect(x: 0, y: 0, width: 1600, height: 1000) + view = hosting + window.contentView = hosting + window.orderBack(nil) + settle() + } + + deinit { + window.orderOut(nil) + window.contentView = nil + try? FileManager.default.removeItem(at: scratch) + UserDefaults.standard.removePersistentDomain(forName: preferencesDomain) + } + + func settle(turns: Int = 6) { + for _ in 0.. HostedBoard { + let scratch = FileManager.default.temporaryDirectory + .appendingPathComponent("MarqueeCost-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: scratch, withIntermediateDirectories: true) + return HostedBoard(store: try BoardStore(rootURL: fixture.root), scratch: scratch) +} + +// MARK: - The measurements + +@MainActor +@Suite("Marquee drag-sample render cost", .serialized) +struct MarqueeRenderCostTests { + + @Test("A stream of redundant selects — the marquee's steady state between card crossings") + func redundantSelectStream() throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + let board = try host(fixture) + let store = board.store + + // The band is sweeping ten cards and the cursor is moving inside the same footprint — + // every sample recomputes the same set, exactly as MarqueeControl.gesture does today. + let swept = Set((0..<10).map { ItemID(rawValue: cardName(1, $0)) }) + + // First select: the legitimate change. Not measured here. + store.select(swept, in: .board, anchor: nil, head: nil) + board.settle() + + let samples = 30 + BoardRenderMetrics.reset() + let t0 = CACurrentMediaTime() + for _ in 0.. = [ItemID(rawValue: cardName(1, 0))] + store.select(swept, in: .board, anchor: nil, head: nil) + board.settle() + + BoardRenderMetrics.reset() + let t0 = CACurrentMediaTime() + for i in 1...samples { + swept.insert(ItemID(rawValue: cardName(1, i % cardsPerLane))) + store.select(swept, in: .board, anchor: nil, head: nil) + board.settleOnce() + } + let elapsed = (CACurrentMediaTime() - t0) * 1000 + + let cards = BoardRenderMetrics.cardBodyEvaluations + print(String( + format: "── %d growing selects — %d card bodies, %.1f ms total (%.2f ms/sample)", + samples, cards, elapsed, elapsed / Double(samples) + )) + + // Something has to repaint: each sample adds one card to the band, and that card's face has + // to grow an accent ring. + #expect(cards > 0, "a growing band repainted nothing") + + // **The regression pin, and what it is a budget *of*.** A sample re-runs the faces whose + // parameters moved, and a growing band moves two things: the newcomer's `isSelected`, and + // `selectedCount` for everyone already in the band — the drag replica's fan and count badge + // are drawn from it, so a card that now travels with five others is genuinely a different + // face than one that travelled with four (`CardFaceView.dragReplica`). So the floor is the + // **selection's own running size, summed over the stream** — 2 members after the first + // sample, \(samples + 1) after the last — and not one flip per sample. + // + // That is the shape the fix bought: the cost follows what the user has selected, not what the + // board holds. Before it, every sample re-ran all \(laneCount * cardsPerLane) faces on the + // board whatever the band had swept — 3,600 bodies over this stream, an order of magnitude + // over the budget below and independent of the selection entirely. + // + // The ×3 is `BoardRenderPerformanceTests`' slack rationale: SwiftUI evaluates a body more + // than once per update, so a changed face is worth more than one count. Measured at exactly + // the floor today. + let selectionWork = (2...(samples + 1)).reduce(0, +) + #expect(cards <= selectionWork * 3, + "\(samples) growing selects cost \(cards) card bodies — floor \(selectionWork), budget \(selectionWork * 3)") + } + + @Test("A stream of redundant clears — the band sweeping empty space") + func redundantClearStream() throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + let board = try host(fixture) + let store = board.store + + store.clearSelection() + board.settle() + + let samples = 30 + BoardRenderMetrics.reset() + let t0 = CACurrentMediaTime() + for _ in 0.. TrashedLane { + TrashedLane( + id: ItemID(rawValue: Ident.lane3), + schema: 1, + title: .valid(title), + modified: .missing, + order: 1024, + heldCards: heldCards, + document: FrontmatterDocument(body: "") + ) + } + + @MainActor + private func makeView( + store: BoardStore, + lane: TrashedLane, + confirmations: TrashConfirmations, + drops: BoardDropContext, + marquee: MarqueeControl, + isSelected: Bool = false, + selectedCount: Int = 1 + ) -> TrashLaneRowView { + TrashLaneRowView( + store: store, + lane: lane, + confirmations: confirmations, + drops: drops, + marquee: marquee, + isSelected: isSelected, + selectedCount: selectedCount + ) + } + + @Test("Identical inputs compare equal, a freshly rebuilt drop context included") + func identicalInputsAreEqual() throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let session = DragSession() + let registry = LaneDropRegistry() + let marquee = MarqueeControl( + session: MarqueeSession(), registry: MarqueeTargetRegistry(), store: store + ) + let confirmations = TrashConfirmations() + let lane = makeRow(title: "Retired") + + // Two body passes of the window: same collaborators, three brand-new closures in the drop + // context each time — a gate that compared any of them would never suppress anything. + #expect(makeView( + store: store, lane: lane, confirmations: confirmations, + drops: makeDrops(store: store, session: session, registry: registry), marquee: marquee + ) == makeView( + store: store, lane: lane, confirmations: confirmations, + drops: makeDrops(store: store, session: session, registry: registry), marquee: marquee + )) + } + + @Test("A different lane value is unequal — the gate never withholds a repaint") + func aDifferentLaneValueIsADifference() throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let marquee = MarqueeControl( + session: MarqueeSession(), registry: MarqueeTargetRegistry(), store: store + ) + let drops = makeDrops(store: store, session: DragSession(), registry: LaneDropRegistry()) + let confirmations = TrashConfirmations() + let base = makeView(store: store, lane: makeRow(title: "Retired"), + confirmations: confirmations, drops: drops, marquee: marquee) + + // The row draws exactly two things — the title and the held-card count — so both have to be + // differences, and `TrashedLane` being `Equatable` is what makes them one comparison. + #expect(base != makeView(store: store, lane: makeRow(title: "Retired lanes"), + confirmations: confirmations, drops: drops, marquee: marquee)) + #expect(base != makeView(store: store, lane: makeRow(title: "Retired", heldCards: 6), + confirmations: confirmations, drops: drops, marquee: marquee)) + } + + @Test("Selected-ness and the selection's size are compared, the card face's rule") + func selectednessIsADifference() throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let marquee = MarqueeControl( + session: MarqueeSession(), registry: MarqueeTargetRegistry(), store: store + ) + let drops = makeDrops(store: store, session: DragSession(), registry: LaneDropRegistry()) + let confirmations = TrashConfirmations() + let lane = makeRow(title: "Retired") + let base = makeView(store: store, lane: lane, confirmations: confirmations, + drops: drops, marquee: marquee) + + #expect(base != makeView(store: store, lane: lane, confirmations: confirmations, + drops: drops, marquee: marquee, isSelected: true)) + #expect(makeView(store: store, lane: lane, confirmations: confirmations, drops: drops, + marquee: marquee, isSelected: true, selectedCount: 1) + != makeView(store: store, lane: lane, confirmations: confirmations, drops: drops, + marquee: marquee, isSelected: true, selectedCount: 4)) + } + + @Test("The window-lived collaborators are compared by identity, the strip's gap by value") + func theCollaboratorsAreComparedByIdentity() throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + let store = try BoardStore(rootURL: fixture.root) + let other = try BoardStore(rootURL: fixture.root) + let session = DragSession() + let registry = LaneDropRegistry() + let bandSession = MarqueeSession() + let bandRegistry = MarqueeTargetRegistry() + let marquee = MarqueeControl(session: bandSession, registry: bandRegistry, store: store) + let drops = makeDrops(store: store, session: session, registry: registry) + let confirmations = TrashConfirmations() + let lane = makeRow(title: "Retired") + let base = makeView(store: store, lane: lane, confirmations: confirmations, + drops: drops, marquee: marquee) + + #expect(base != makeView(store: other, lane: lane, confirmations: confirmations, + drops: drops, marquee: marquee)) + // Window-lived state the row's permanent Delete goes through — identity is meaningful here + // for `CardFaceRole.isEquivalent(to:)`'s reason exactly. + #expect(base != makeView(store: store, lane: lane, confirmations: TrashConfirmations(), + drops: drops, marquee: marquee)) + #expect(base != makeView( + store: store, lane: lane, confirmations: confirmations, + drops: makeDrops(store: store, session: DragSession(), registry: registry), + marquee: marquee + )) + #expect(base != makeView( + store: store, lane: lane, confirmations: confirmations, + drops: makeDrops(store: store, session: session, registry: LaneDropRegistry()), + marquee: marquee + )) + #expect(base != makeView( + store: store, lane: lane, confirmations: confirmations, + drops: makeDrops(store: store, session: session, registry: registry, gap: 20), + marquee: marquee + )) + #expect(base != makeView( + store: store, lane: lane, confirmations: confirmations, drops: drops, + marquee: MarqueeControl(session: MarqueeSession(), registry: bandRegistry, store: store) + )) + #expect(base != makeView( + store: store, lane: lane, confirmations: confirmations, drops: drops, + marquee: MarqueeControl(session: bandSession, registry: MarqueeTargetRegistry(), store: store) )) } } diff --git a/RENDER-INSTRUMENTATION.md b/RENDER-INSTRUMENTATION.md index f9514f4..9f3fd27 100644 --- a/RENDER-INSTRUMENTATION.md +++ b/RENDER-INSTRUMENTATION.md @@ -52,7 +52,15 @@ LaneView.body → header → .boardTextInk(headerInk) → headerInk Observation tracks whole **properties**. Reading `.background` off `store.snapshot` subscribes that body to the entire snapshot, so a reload that changes one card anywhere invalidates every lane on the board directly — and `.equatable()` has no say over a direct invalidation. The fix is to resolve the board's ink once in `BoardView` and pass it down as a compared parameter, the way `slotWidth` and `columns` already are. Until then the container budget in the test is written to the number the tree actually produces, with the reason, and `theLaneCostFollowsTheBoard` is a tripwire in both directions: it fails if the number goes **down**, which is the fix landing. -**Selection is O(board) in card bodies.** Selecting one card re-runs all 180 faces, because `CardFaceView.body` reads `store.selection` through `isSelected`. This is the Observation half the gates explicitly do not cover, and narrowing it would mean each face taking its own selected-ness as a compared parameter — a design change, not a gate. +**Selection is O(board) in card bodies** — *fixed 2026-08-07; kept for the history of the table above.* Selecting one card re-ran all 180 faces, because `CardFaceView.body` read `store.selection` through `isSelected`. This was the Observation half the gates explicitly do not cover, and narrowing it meant exactly what the next section describes: each face taking its own selected-ness as a compared parameter. + +### The selection storm, measured and fixed (2026-08-07) + +What surfaced it: drag-selecting felt sluggish — ~0.4 s between the marquee reaching a card and its highlight. `KanbanTests/MarqueeRenderCostTests.swift` (now the regression suite) measured a marquee sample two ways. A sample whose swept set is **unchanged** was already free — SwiftUI prunes equal-value `@Observable` writes before any body runs, so no dedupe guard was ever needed. A sample that **changes** the set cost the whole board: 180 bodies ≈ 85 ms on the 6×30 fixture, 515 bodies ≈ 233 ms on a copy of the real 515-card Redesign board (debug builds). The asymmetry the user feels is exactly that split: the band overlay is cheap and tracks the cursor, while the highlight waits for the full-board pass. The face's body reached `store.selection` in **three** places — `isSelected`, the drag replica's count (`.onDrag`'s preview builder is non-escaping), and the context menu's `styleTarget` (`.contextMenu`'s builder likewise) — and all three had to be re-sourced. + +The fix: `LaneView`/`TrashLaneView` hoist one selection read per body and hand each face `isSelected`/`selectedCount` as compared parameters; `StyleMenuItems`' target became a deferred closure. After: selecting one card re-runs **1** face; the growing-band stream costs the selection's own running size (the count parameter genuinely changes for every swept face) instead of the board — 230 bodies over 20 samples where it was 3,600. + +**What remains, and where the next fix lives.** Wall-clock per crossing only roughly halved — 85 → 46 ms on the fixture, 233 → 112 ms on the 515-card board — because every lane body still re-runs per selection change (headers legitimately read the selection) and each lane pass re-measures its masonry: ~1,030 fresh `masonryMeasurements` (+~520 cache hits) per sample on the real board. The bodies are fixed; the residue is *layout*. The levers are the `headerInk` hoist above (stop lane bodies re-running for board-level reads) and the masonry measurement cache not surviving a lane body re-run — both lane-level, neither touched by the card-face change. ### The zoom pair (2026-08-03)