From 25f62c36f7fdd5027e747fce7092d08998002775 Mon Sep 17 00:00:00 2001 From: Oleg <18466855+EvaTheSalmon@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:08:56 +0300 Subject: [PATCH] fix(grid): shift selection indices on step insert/remove The grid caches the current selection as a list of step indices. When a step was inserted or removed at or before a selected step, the control's own selection model shifted to keep the same items selected, but the cached indices were left untouched and went stale. Consumers that read the cache (copy, cut, delete, paste insert position) then acted on the wrong steps. Maintain the cache in the shared surface instead of relying on a control signal. OnMutation already runs on every mutation and carries the signal payload, so shift the cached indices arithmetically there: insert shifts survivors up, remove drops the removed set and shifts survivors down, and a full rebuild clears the selection. This holds for both the canonical DataGrid, which exposes no index-shift signal, and the transposed list, and it keeps the invariant even for the orientation-flipped surface whose view is detached. The selection is snapshotted at the top of OnMutation and re-applied after the collection mutates, guarded by a sequence-equality check, so a mid-mutation push from the transposed list does not double-shift. The drop-only clamp it replaces is removed. --- Docs/architecture/recipe-grid-surface.md | 12 + ...17-stale-selection-indices-after-insert.md | 153 +++++++++++ .../CanonicalSelectionShiftTests.cs | 247 ++++++++++++++++++ .../TransposedSelectionIndexTests.cs | 10 +- .../TransposedSelectionShiftTests.cs | 203 ++++++++++++++ .../RecipeGrid/RecipeGridSurfaceBase.cs | 67 +++-- 6 files changed, 668 insertions(+), 24 deletions(-) create mode 100644 Docs/plans/completed/20260717-stale-selection-indices-after-insert.md create mode 100644 SemiStep/SemiStep.Tests/UI/RecipeGrid/CanonicalSelectionShiftTests.cs create mode 100644 SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionShiftTests.cs diff --git a/Docs/architecture/recipe-grid-surface.md b/Docs/architecture/recipe-grid-surface.md index bf8f34f..2ceb3d3 100644 --- a/Docs/architecture/recipe-grid-surface.md +++ b/Docs/architecture/recipe-grid-surface.md @@ -171,6 +171,18 @@ and every dead member would have to be implemented and contract-tested by each f subscribes and sets its native selection (`null` clears). `RequestSelection` is a safe no-op after `Dispose()`. +**Index-shift reconciliation.** An index-shifting mutation (an insert or remove at/before a +selected step) moves the steps the cached `SelectedStepIndices` point at. The cache is +reconciled arithmetically inside `RecipeGridSurfaceBase.OnMutation` via `ShiftSelection`, +driven by the `MutationSignal`, not by any control-level selection event: `StepsInserted` shifts +each index at/after the insert up by the count, `StepRemoved`/`StepsRemoved` drop the removed +indices and shift survivors down, and `RecipeReplaced` clears the cache. This keeps the +invariant on both surfaces without an `ISelectionModel`, which matters because the canonical +`DataGrid` fires no selection push on an insert-before or a remove-of-selected. `ShiftSelection` +is a pure function of `(previous indices, signal)`; the snapshot is taken at the top of +`OnMutation` and assigned after the item mutation, guarded by sequence-equality so a mid-mutation +control push (the transposed `ListBox` on remove-selected) is not double-applied. + ## Orientation switching `ActiveRecipeGridSurface` is the single owner of orientation. Everything else observes or diff --git a/Docs/plans/completed/20260717-stale-selection-indices-after-insert.md b/Docs/plans/completed/20260717-stale-selection-indices-after-insert.md new file mode 100644 index 0000000..4ab9f60 --- /dev/null +++ b/Docs/plans/completed/20260717-stale-selection-indices-after-insert.md @@ -0,0 +1,153 @@ +# Stale `SelectedStepIndices` After an Index-Shifting Insert/Remove + +## Overview +- Correctness bug (data-integrity class, NOT perf), pre-existing, recorded as an out-of-scope follow-up from `Docs/plans/completed/20260715-transposed-selection-perf-fix.md` and `...-recycle-in-place-panel.md`. +- The view-model caches the current selection as `IReadOnlyList SelectedStepIndices` (`RecipeGridSurfaceBase.cs:101`). When a step is inserted or removed at an index at/before a selected step, the cached indices are NOT shifted, so the cache points at the wrong steps. Consumers that read the cache (`ClipboardViewModel`, `RecipeCommandsViewModel`) then act on the wrong steps. +- Fix: maintain the cache in the shared surface. `OnMutation` already runs on every mutation and already carries the signal payload (`RecipeGridSurfaceBase.cs:136`); shift the cached indices arithmetically there, driven by the mutation signal, so the invariant holds for BOTH grids without depending on any control-level signal. + +## Context (from discovery) +- `SemiStep/SemiStep.UI/RecipeGrid/RecipeGridSurfaceBase.cs` — shared base for both grids. + - `SelectedStepIndices` get / private set (`:101`); `UpdateSelection` pushes a materialized list from the view (`:115`). + - `SelectedStepIndex` singular derives `indices[0]` (`:99`, `:59`); `CanDeleteStep` derives `Count > 0` (`:54`). + - `Items` is the `ObservableCollection` bound as the control's ItemsSource (`:51`). + - `OnMutation(MutationSignal)` — the mutation pipeline, `switch` over signal at `:150`, calls `ReconcileSelectionWithItems()` at `:194`. + - `ReconcileSelectionWithItems` (`:216`) — drop-only clamp: early-returns when `All(index => index < itemCount)` (`:225`), never shifts. + - `CollectSelectedSteps` reads the cache to map indices → steps (`:534`). +- `SemiStep/SemiStep.UI/Coordinator/MutationSignal.cs` — `StepAppended` (`:7`), `StepsInserted(StartIndex, Count)` (`:9`), `StepRemoved(RemovedIndex)` (`:11`), `StepsRemoved(ImmutableArray)` (`:13`), `StepActionChanged` (`:15`), `PropertyUpdated` (`:17`), `RecipeReplaced` (`:19`), `StateRefreshed` (`:21`). +- `SemiStep/SemiStep.UI/Coordinator/RecipeCoordinator.cs` — which command emits which signal: `InsertStep`→`StepsInserted` (`:256`), `InsertSteps`→`StepsInserted` (`:277`), `RemoveStep`→`StepRemoved` (`:263`), `RemoveSteps`→`StepsRemoved` (`:270`), `AppendStep`→`StepAppended` (`:250`), `ChangeStepAction`→`StepActionChanged` (`:284`), `Undo`→`RecipeReplaced` (`:310`), `Redo`→`RecipeReplaced` (`:315`), `NewRecipe`→`RecipeReplaced` (`:320`). +- `SemiStep/SemiStep.UI/RecipeGrid/Transposed/TransposedRecipeGridView.axaml.cs` — `OnSelectionChanged` re-reads `StepListBox.Selection.SelectedIndexes` and calls `UpdateSelection` (`:191`), wired to the control `SelectionChanged` event (`:58`, disposed `:98`), guarded by `_syncingSelectionFromSurface` (`:34`, `:193`). `StepListBox.Selection` is an `ISelectionModel`. +- `SemiStep/SemiStep.UI/RecipeGrid/CanonicalRecipeGridView.axaml.cs` — `OnSelectionChanged` re-derives indices via `RecipeRows.IndexOf` over `RecipeGrid.SelectedItems` (`:134`), wired at `:25`. The DataGrid selection is `RecipeGrid.SelectedItems`; it is NOT an `ISelectionModel` and exposes no index-shift signal. +- Consumers of the cache: + - `SemiStep/SemiStep.UI/Clipboard/ClipboardViewModel.cs` — Copy (`:89`), Cut → `RemoveSteps(SelectedStepIndices)` then `RequestSelection` (`:112`, `:119`), Paste insert-position `SelectedStepIndices.Max() + 1` then `RequestSelection` (`:148`, `:159`). + - `SemiStep/SemiStep.UI/RecipeGrid/RecipeCommandsViewModel.cs` — AddStep uses `SelectedStepIndex + 1` then `RequestSelection` (`:73`, `:79`), DeleteStep uses `SelectedStepIndices` then `RequestSelection` (`:85`, `:97`), Undo/Redo call the coordinator with no follow-up reselect (`:101`, `:106`). +- Test harness: `UIFixture` exposes `Coordinator` (`:42`), `SeedRecipe` (`:133`), `CreateCanonicalSurface` (`:95`), `CreateTransposedSurface` (`:110`). Canonical view harness `ShowViewWithControl` (`CanonicalRecipeGridViewTests.cs:180`); transposed view harness `ShowView` + `UseTransposedColumnsPanel` (`TransposedSelectionIndexTests.cs:162`). +- Package versions: `Directory.Packages.props` pins `Avalonia` 12.0.5 (`:8`), `Avalonia.Controls.DataGrid` 12.0.1 (`:10`). The fix depends on neither — it is version-independent. + +## Development Approach +- **testing approach**: Regular (code first, then tests), but LEAD with red surface-level tests that reproduce the stale cache so the fix is falsifiable. +- The fix lives entirely in the shared `RecipeGridSurfaceBase` — one code path, both grids, no per-view work and no Avalonia selection-model dependency. +- **every task includes tests**; all tests pass before the next task starts. +- Preserve existing behavior: multi-select, the O(S) `OnSelectionChanged` read of `Selection.SelectedIndexes`, the `_syncingSelectionFromSurface` guard, `RequestSelection`/`ScrollIntoView`. + +## Testing Strategy +- **unit / surface-level tests** (`[AvaloniaFact]`, `Component=UI`, `Area=RecipeGrid`): drive mutations through `UIFixture.Coordinator` and assert `surface.SelectedStepIndices` after `Dispatcher.UIThread.RunJobs()`. These need no realized control, so they cover the canonical grid too (which otherwise has no testable selection signal). +- **live-control agreement tests**: with the control shown, assert cache == control selection indices after the dispatcher settles, for the one case where the control pushes mid-mutation (transposed remove-selected). +- **regression**: existing selection suites stay green — `TransposedSelectionIndexTests`, `TransposedColumnsPanelContractTests` multi-select round-trip, and the selection-cost probe discrimination. One existing test asserts the OLD buggy behavior and MUST be updated (see Task 2). +- **no perf claim**: correctness fix; no baseline/gate. + +## Acceptance Evidence +Reproduction is automatable; the evidence is a runnable test, not a description. + +- **Baseline, measured 2026-07-22** (throwaway headless probe against current master, since deleted): + - Canonical (DataGrid), select row 2 then `InsertStep(0)`: cache `[2]` → `[2]` (STALE); the DataGrid shifted its own `SelectedIndex` 2→3 keeping the same item, and fired NO `SelectionChanged` push. + - Canonical (DataGrid), multi-select rows `{0,2,4}` then `RemoveStep(2)`: cache `[0,2,4]` → `[0,2,4]` (STALE); the DataGrid dropped to 2 selected rows and fired NO push. + - Transposed (ListBox), select row 2 then `RemoveStep(2)`: control fires `SelectionChanged`, cache reconciles correctly (`TransposedSelectionIndexTests.cs:92` pins this — NOT stale). + - Transposed (ListBox), select row 2 then `InsertStep(0)`: cache stays `[2]` STALE — `TransposedSelectionIndexTests.cs:130` currently pins this staleness as the known follow-up. + - Undo/Redo (`RecipeReplaced` → full rebuild): BOTH controls clear the cache to `[]` (selection dropped, not stale). +- **Pass condition after the fix** — run: + `dotnet test SemiStep/SemiStep.Tests/SemiStep.Tests.csproj --filter "FullyQualifiedName~SelectionShift"` + - Insert-before-selection shifts the cache up by the insert count, on BOTH grids. + - Remove-before-selection shifts survivors down and drops removed indices, on BOTH grids. + - Insert/remove AFTER the selection leaves the cache unchanged, on BOTH grids. + - Consumer end-to-end: select → insert-before → `RemoveSteps(SelectedStepIndices)` deletes the originally-selected step (now at its shifted index), on BOTH grids. + - `RecipeReplaced` clears the cache to `[]`. + +## Progress Tracking +- mark completed items `[x]` immediately. +- add newly discovered tasks with ➕ prefix; blockers with ⚠️ prefix. +- keep this plan in sync with actual work. + +## Solution Overview +Maintain `SelectedStepIndices` as a pure function of `(previous indices, mutation signal)`, applied in the shared surface at mutation time. This is the same transaction that drove the `Items` mutation applied to the selection projection — not a hand-maintained duplicate of a control's state. + +Why here and not via a control signal (the rejected alternative): +- The canonical grid is an Avalonia `DataGrid` whose selection is `RecipeGrid.SelectedItems`, with no `ISelectionModel` and no `IndexesChanged`. The 2026-07-22 probe confirmed it fires no push on either insert-before or remove-selected, so there is no signal to subscribe. A control-signal fix would work on the ListBox and silently fail on the DataGrid. +- Both surfaces subscribe `coordinator.Mutated` in the base constructor (`RecipeGridSurfaceBase.cs:80`), so `OnMutation` runs even for the orientation-flipped-away grid whose view is detached. A view-side fix cannot cover that surface; a surface-side fix does. +- `OnMutation` sees every signal including `RecipeReplaced`, so the cache-clear-on-rebuild lives in the same place. + +Reentrancy (the one sharp edge): inside `OnMutation`, `Items.Insert/RemoveAt` raises `CollectionChanged` synchronously and the transposed ListBox reacts before the code after the `switch` runs. On a remove that includes a selected index the ListBox fires `SelectionChanged` mid-`switch` and pushes the correct post-shift indices. To avoid double-shifting, snapshot the cache at the TOP of `OnMutation`, compute the shift from the snapshot, and assign AFTER the `switch`. For the transposed remove-selected case the snapshot-computed value equals the control's pushed value, so a sequence-equality guard makes the assignment a no-op; for every other case (all DataGrid cases, ListBox insert-before) the surface is the sole writer. Verified by the 2026-07-22 probe: the only control that pushes mid-mutation is the ListBox on remove-selected, and it agrees. + +## Technical Details +Add a pure static shift function and call it from `OnMutation`; retire `ReconcileSelectionWithItems`. + +The shift applies to the four index-affecting signals ONLY. Every other signal leaves the cache untouched — assigning for them is what would fight the control (see `StepActionChanged` below). + +`ShiftSelection(IReadOnlyList selection, MutationSignal signal) -> IReadOnlyList`, defined for: +- `StepsInserted(startIndex, count)`: each `i` → `i >= startIndex ? i + count : i`. +- `StepRemoved(removedIndex)`: drop `i == removedIndex`; each survivor → `i > removedIndex ? i - 1 : i`. +- `StepsRemoved(removedIndices)`: drop any `i` in `removedIndices`; each survivor → `i - (number of removed indices < i)`. +- `RecipeReplaced`: return `[]` (full rebuild invalidates every index). + +Signals the shift does NOT assign for (cache left as-is): +- `StepAppended` — adds at the tail, past every selected index; no index moves. +- `PropertyUpdated` — no structural change. +- `StepActionChanged` — `RebuildItem` (`:461`) does `Items[stepIndex] = item`, replacing the reference at a stable index. The cached index stays valid, so the shift leaves it alone. In the live UI the control drops the replaced item and `OnActionChanged` (`:324`) calls `RequestSelection(result.Value)` (`:346`), which re-selects and repushes the cache. Assigning here would re-assert the pre-mutation snapshot and fight that push — hence no assignment for this signal. +- `StateRefreshed` — `OnMutation` already `return`s early at `:181` before reaching the shift. + +`OnMutation` shape: +- At the top, before the `switch`: `var selectionSnapshot = SelectedStepIndices;` +- Keep the existing `switch` that mutates `Items` unchanged, preserving both early-outs that bypass the shift: the `StateRefreshed` `return` (`:181`) and the `UnknownActionKeyException` catch-`return` (`:191`). +- Replace the `ReconcileSelectionWithItems()` call (`:194`) with: for `StepsInserted`/`StepRemoved`/`StepsRemoved`/`RecipeReplaced` only, compute `var shifted = ShiftSelection(selectionSnapshot, signal);` and assign `SelectedStepIndices = shifted` when `shifted` is not sequence-equal to the current `SelectedStepIndices` (a mid-`switch` control push may already have set the same value — the guard avoids churn and, for the transposed remove-selected case, a redundant re-assign). For all other signals, do nothing. +- Delete `ReconcileSelectionWithItems` (`:216`): a correct shift drops out-of-range indices on removal and clears on rebuild, so the drop-only clamp is subsumed. Leaving it would be a second, disagreeing writer. + +Invariant after the change: for the four index-affecting signals, `SelectedStepIndices` is consistent with `Items` and equal to what the live control selection would report — independent of whether a control is attached. Non-index signals leave the cache untouched; where selection needs to move for them (`StepActionChanged`), the existing `RequestSelection` path governs it. + +## What Goes Where +- **Implementation Steps** (`[ ]`): the shift function, the `OnMutation` change, test coverage, doc note. +- **Post-Completion** (no checkboxes): manual smoke on the live app; unrelated follow-ups tracked separately. + +## Implementation Steps + +### Task 1: Red surface-level tests reproducing the stale cache on both grids + +**Files:** +- Create: `SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionShiftTests.cs` +- Create: `SemiStep/SemiStep.Tests/UI/RecipeGrid/CanonicalSelectionShiftTests.cs` + +- [x] Transposed: seed a recipe, select index 2, `Coordinator.InsertStep(0, ...)`, `RunJobs`, assert `surface.SelectedStepIndices` == `[3]` (currently FAILS — proves the bug). +- [x] Canonical: same scenario via the DataGrid harness, assert cache == `[3]` (currently FAILS). +- [x] Canonical: multi-select `{0,2,4}`, `Coordinator.RemoveStep(2)`, assert cache == `[0,3]` (currently FAILS — DataGrid fires no push). +- [x] Run — the new tests FAIL for the right reason (stale indices); the rest of the suite stays green. + +### Task 2: Shift the cache in `OnMutation`; retire the clamp + +**Files:** +- Modify: `SemiStep/SemiStep.UI/RecipeGrid/RecipeGridSurfaceBase.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionIndexTests.cs` + +- [x] Add the pure `ShiftSelection(selection, signal)` per Technical Details — defined for `StepsInserted`/`StepRemoved`/`StepsRemoved` (shift) and `RecipeReplaced` (clear). +- [x] Snapshot `SelectedStepIndices` at the top of `OnMutation`; after the `switch`, for those four signals ONLY assign `ShiftSelection(snapshot, signal)` guarded by sequence-equality; assign nothing for other signals; preserve the `StateRefreshed` (`:181`) and exception-catch (`:191`) early returns; remove the `ReconcileSelectionWithItems()` call and delete the method. +- [x] Update `TransposedSelectionIndexTests.cs:130` (`InsertStepBeforeSelection_...LeavesSurfaceIndicesUntouched`) — it pins the OLD stale behavior; rename/retarget it to assert the shifted indices and the control/cache agreement. This is a deliberate expectation flip, not a regression. +- [x] Build green; the Task 1 red tests now PASS. +- [x] Run the existing selection suites — all green. + +### Task 3: Cover both directions, multi-select blocks, and the consumers + +**Files:** +- Modify: `SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionShiftTests.cs` +- Modify: `SemiStep/SemiStep.Tests/UI/RecipeGrid/CanonicalSelectionShiftTests.cs` + +- [x] Both grids: insert-before shifts up by the count; remove-before shifts survivors down; insert/remove AFTER the selection leaves the cache unchanged; a multi-select range shifts as a block. +- [x] Both grids: `RecipeReplaced` (drive via `Coordinator.Undo()` after an undoable mutation) clears the cache to `[]`. +- [x] Both grids: `Coordinator.ChangeStepAction(k)` on a selected step `k` leaves the cached index unchanged and still valid (`[k]`) — pins that the shift does not clobber the action-change case, whose live re-selection is governed by `OnActionChanged`→`RequestSelection` (`:346`), not the shift. +- [x] Consumer end-to-end, at least one per grid: select → insert-before → `RemoveSteps(surface.SelectedStepIndices)` removes the originally-selected step (verify by identity/content, not index). +- [x] Transposed live-control agreement: with the view shown, remove-selected leaves cache == `Selection.SelectedIndexes`. +- [x] Run the full suite — green. + +### Task 4: Verify acceptance criteria +- [x] `dotnet test SemiStep/SemiStep.Tests/SemiStep.Tests.csproj --filter "FullyQualifiedName~SelectionShift"` — all pass (18 passed, 0 failed). +- [x] `dotnet test SemiStep/SemiStep.Tests/SemiStep.Tests.csproj` — full suite green (1462 passed, 0 failed, 0 skipped run by default; 8 Performance explicit probes not run). +- [x] `dotnet format SemiStep.slnx --verify-no-changes` — exit 0, no formatting diffs. +- [x] Confirm the O(S) `OnSelectionChanged` read of `Selection.SelectedIndexes` is untouched (no reintroduced O(S·N) scan). `TransposedRecipeGridView.axaml.cs:200` still reads `StepListBox.Selection.SelectedIndexes.ToList()`. `ShiftSelection` (`RecipeGridSurfaceBase.cs:218`) is O(S) arithmetic: insert/remove map over the selection only; StepsRemoved is O(S·R) over the small removed set; no `IndexOf`-per-item scan over all N items. + +### Task 5: Documentation + cleanup +- [x] If `Docs/architecture/recipe-grid-surface.md` documents selection sync, note that index shifts are maintained in `OnMutation`, not via a control signal. +- [x] Move this plan to `Docs/plans/completed/`. (deferred to delivery — plan not moved by exec) + +## Post-Completion +*Manual / external — no checkboxes* +- Manual smoke on the live app: with a multi-selection active, exercise Cut/Paste/Delete after an index-shifting mutation and confirm the intended steps are affected. Under current UI gestures the stale window is largely latent (every consumer command calls `RequestSelection` after mutating, and undo clears), so the surface-level tests are the primary evidence; the smoke is a sanity pass. +- Related follow-ups tracked separately (NOT this plan): `TransposedGridSelectionController.ExtendSelectionTo` shift-select batching; the deferred black-box perf harness (`Docs/plans/20260717-blackbox-perf-harness.md`). This bug is correctness and invisible to the perf harness by construction. + +**Executed by exec:** +- branch: selection-index-shift diff --git a/SemiStep/SemiStep.Tests/UI/RecipeGrid/CanonicalSelectionShiftTests.cs b/SemiStep/SemiStep.Tests/UI/RecipeGrid/CanonicalSelectionShiftTests.cs new file mode 100644 index 0000000..d8b746f --- /dev/null +++ b/SemiStep/SemiStep.Tests/UI/RecipeGrid/CanonicalSelectionShiftTests.cs @@ -0,0 +1,247 @@ +using Avalonia.Controls; +using Avalonia.Headless.XUnit; +using Avalonia.Threading; + +using FluentAssertions; + +using SemiStep.Tests.Core.Helpers; +using SemiStep.Tests.UI.Helpers; + +using SemiStep.UI.RecipeGrid; + +using Xunit; + +namespace SemiStep.Tests.UI.RecipeGrid; + +// Reproduces the stale-index bug on the canonical DataGrid, which fires no SelectionChanged push +// on an index-shifting insert or on a remove that drops selected rows. The cache must still shift +// so it keeps pointing at the same logical steps. These are red tests: they assert the correct +// post-fix values and therefore fail against the current unfixed surface. +[Trait("Component", "UI")] +[Trait("Area", "RecipeGrid")] +[Trait("Category", "Integration")] +public sealed class CanonicalSelectionShiftTests : IAsyncLifetime +{ + private const int SeededStepCount = 6; + + private readonly UIFixture _fixture = new(); + private CanonicalRecipeGridSurface _surface = null!; + private Window? _window; + + public async ValueTask InitializeAsync() + { + await _fixture.InitializeAsync(); + _fixture.SeedRecipe(SeededStepCount); + + _surface = _fixture.CreateCanonicalSurface(); + _surface.Initialize(); + } + + public async ValueTask DisposeAsync() + { + _window?.Close(); + _surface.Dispose(); + await _fixture.DisposeAsync(); + } + + [AvaloniaFact] + public void InsertStepBeforeSelection_ShiftsCachedIndexUp() + { + var dataGrid = ShowView(); + + dataGrid.SelectedIndex = 2; + Dispatcher.UIThread.RunJobs(); + + // Inserting at index 0 pushes the selected step from index 2 to index 3. + _fixture.Coordinator.InsertStep(0, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(3); + } + + [AvaloniaFact] + public void RemoveSelectedStep_FromMultiSelection_ShiftsSurvivorsDownAndDropsRemoved() + { + var dataGrid = ShowView(); + + dataGrid.SelectedItems.Add(_surface.RecipeRows[0]); + Dispatcher.UIThread.RunJobs(); + dataGrid.SelectedItems.Add(_surface.RecipeRows[2]); + Dispatcher.UIThread.RunJobs(); + dataGrid.SelectedItems.Add(_surface.RecipeRows[4]); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(0, 2, 4); + + // Removing step 2 drops it and shifts the surviving step 4 down to 3. + _fixture.Coordinator.RemoveStep(2); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(0, 3); + } + + [AvaloniaFact] + public void RemoveStepBeforeSelection_ShiftsSurvivorDown() + { + _surface.UpdateSelection(new[] { 3 }); + + // Removing step 1 (before the selection) shifts the surviving step 3 down to 2. + _fixture.Coordinator.RemoveStep(1); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(2); + } + + [AvaloniaFact] + public void InsertStepAfterSelection_LeavesCacheUnchanged() + { + _surface.UpdateSelection(new[] { 1 }); + + _fixture.Coordinator.InsertStep(4, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(1); + } + + [AvaloniaFact] + public void RemoveStepAfterSelection_LeavesCacheUnchanged() + { + _surface.UpdateSelection(new[] { 1 }); + + _fixture.Coordinator.RemoveStep(4); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(1); + } + + [AvaloniaFact] + public void InsertBeforeContiguousRange_ShiftsBlockUp() + { + _surface.UpdateSelection(new[] { 2, 3, 4 }); + + // Inserting one step before the whole block moves {2, 3, 4} up to {3, 4, 5}. + _fixture.Coordinator.InsertStep(0, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(3, 4, 5); + } + + [AvaloniaFact] + public void Undo_RecipeReplaced_ClearsCache() + { + // An append leaves undo history without disturbing the selection at index 2. + _fixture.Coordinator.InsertStep(SeededStepCount, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.UpdateSelection(new[] { 2 }); + + // RecipeReplaced from a full rebuild invalidates every index, so the cache clears. + _fixture.Coordinator.Undo(); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().BeEmpty(); + } + + [AvaloniaFact] + public void ChangeStepAction_OnSelectedStep_LeavesCacheUnchanged() + { + _surface.UpdateSelection(new[] { 2 }); + + // StepActionChanged replaces the row at a stable index; the shift must not touch the cache. + // The live re-selection is governed by OnActionChanged -> RequestSelection, not the shift. + _fixture.Coordinator.ChangeStepAction(2, RecipeTestDriver.PauseActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(2); + } + + [AvaloniaFact] + public void InsertBeforeThenRemoveSelection_DeletesOriginallySelectedStep() + { + // Give the selected step a unique content signature so identity is provable by value. + _fixture.Coordinator.UpdateStepProperty(2, RecipeTestDriver.StepDurationColumn, "777"); + Dispatcher.UIThread.RunJobs(); + var originallySelectedStep = _fixture.Coordinator.CurrentRecipe.Steps[2]; + + _surface.UpdateSelection(new[] { 2 }); + + // Inserting before the selection shifts the cache from [2] to [3]. + _fixture.Coordinator.InsertStep(0, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + _surface.SelectedStepIndices.Should().Equal(3); + + // Deleting via the shifted cache must remove the originally selected step, not a bystander. + _fixture.Coordinator.RemoveSteps(_surface.SelectedStepIndices); + Dispatcher.UIThread.RunJobs(); + + _fixture.Coordinator.CurrentRecipe.Steps.Should().NotContain(originallySelectedStep); + } + + [AvaloniaFact] + public void InsertMultipleStepsBeforeSelection_ShiftsCachedIndexByCount() + { + _surface.UpdateSelection(new[] { 4 }); + + // Two steps inserted before the selection push the cached index up by the full count, 4 -> 6. + var stepsToInsert = _fixture.Coordinator.CurrentRecipe.Steps.Take(2).ToList(); + _fixture.Coordinator.InsertSteps(0, stepsToInsert); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(6); + } + + [AvaloniaFact] + public void InsertStepAtSelectedIndex_ShiftsCachedIndexUp() + { + _surface.UpdateSelection(new[] { 2 }); + + // Inserting exactly at the selected index still displaces it: index >= startIndex shifts, 2 -> 3. + _fixture.Coordinator.InsertStep(2, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(3); + } + + [AvaloniaFact] + public void RemoveSteps_DropsSelectedAndOffsetsSurvivorsPastEachRemovedIndex() + { + _surface.UpdateSelection(new[] { 2, 3, 5 }); + + // Removing {2, 4}: index 2 is dropped; 3 shifts past one removal to 2; 5 shifts past two removals to 3. + _fixture.Coordinator.RemoveSteps(new[] { 2, 4 }); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(2, 3); + } + + [AvaloniaFact] + public void AppendStep_LeavesSelectionUnchanged() + { + _surface.UpdateSelection(new[] { 2 }); + + // Appending past the tail adds no index at or before the selection, so the cache is untouched. + _fixture.Coordinator.AppendStep(RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(2); + } + + private DataGrid ShowView() + { + var view = new CanonicalRecipeGridView { DataContext = _surface }; + _window = new Window + { + Width = 1200, + Height = 600, + Content = view, + }; + + _window.Show(); + Dispatcher.UIThread.RunJobs(); + + var dataGrid = view.FindControl("RecipeGrid"); + dataGrid.Should().NotBeNull(); + + return dataGrid!; + } +} diff --git a/SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionIndexTests.cs b/SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionIndexTests.cs index c7c9512..fbd5fdd 100644 --- a/SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionIndexTests.cs +++ b/SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionIndexTests.cs @@ -128,7 +128,7 @@ public void OnSelectionChanged_MaterializesIndices_SnapshotDoesNotTrackLaterSele } [AvaloniaFact] - public void InsertStepBeforeSelection_DoesNotRaiseSelectionChanged_LeavesSurfaceIndicesUntouched() + public void InsertStepBeforeSelection_ShiftsSurfaceIndicesUp() { var stepListBox = ShowView(); @@ -136,15 +136,13 @@ public void InsertStepBeforeSelection_DoesNotRaiseSelectionChanged_LeavesSurface Select(stepListBox, 4); Select(stepListBox, 5); - var beforeInsert = _surface.SelectedStepIndices.ToList(); - // An index-shifting insert raises IndexesChanged, not SelectionChanged, so OnSelectionChanged - // never runs and the surface indices stay as they were. The stale-after-insert gap is a - // recorded follow-up; this test only pins that the insert does not route through OnSelectionChanged. + // never runs. OnMutation shifts the cached indices arithmetically, so inserting one step before + // the selection moves {3, 4, 5} up to {4, 5, 6}. _fixture.Coordinator.InsertStep(0, RecipeTestDriver.WaitActionId); Dispatcher.UIThread.RunJobs(); - _surface.SelectedStepIndices.Should().Equal(beforeInsert); + _surface.SelectedStepIndices.Should().Equal(4, 5, 6); } private static void Select(ListBox stepListBox, int index) diff --git a/SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionShiftTests.cs b/SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionShiftTests.cs new file mode 100644 index 0000000..0236aa4 --- /dev/null +++ b/SemiStep/SemiStep.Tests/UI/RecipeGrid/Transposed/TransposedSelectionShiftTests.cs @@ -0,0 +1,203 @@ +using Avalonia.Controls; +using Avalonia.Headless.XUnit; +using Avalonia.Threading; + +using FluentAssertions; + +using SemiStep.Tests.Core.Helpers; +using SemiStep.Tests.UI.Helpers; + +using SemiStep.UI.RecipeGrid.Transposed; + +using Xunit; + +namespace SemiStep.Tests.UI.RecipeGrid.Transposed; + +// Reproduces the stale-index bug: when a step is inserted or removed at/before a selected step, +// the cached SelectedStepIndices must shift so it keeps pointing at the same logical steps. +// These are red tests: they assert the correct post-fix values and therefore fail against the +// current unfixed surface, which does not shift the cache on an insert that raises no +// SelectionChanged. +[Trait("Component", "UI")] +[Trait("Area", "RecipeGrid")] +[Trait("Category", "Integration")] +public sealed class TransposedSelectionShiftTests : IAsyncLifetime +{ + private const int SeededStepCount = 6; + + private readonly UIFixture _fixture = new(); + private TransposedRecipeGridSurface _surface = null!; + private Window? _window; + + public async ValueTask InitializeAsync() + { + await _fixture.InitializeAsync(); + _fixture.SeedRecipe(SeededStepCount); + + _surface = _fixture.CreateTransposedSurface(); + _surface.Initialize(); + } + + public async ValueTask DisposeAsync() + { + _window?.Close(); + _surface.Dispose(); + await _fixture.DisposeAsync(); + } + + [AvaloniaFact] + public void InsertStepBeforeSelection_ShiftsCachedIndexUp() + { + var stepListBox = ShowView(); + + Select(stepListBox, 2); + + // Inserting at index 0 pushes the selected step from index 2 to index 3. + _fixture.Coordinator.InsertStep(0, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(3); + } + + [AvaloniaFact] + public void RemoveStepBeforeSelection_ShiftsSurvivorDown() + { + _surface.UpdateSelection(new[] { 3 }); + + // Removing step 1 (before the selection) shifts the surviving step 3 down to 2. + _fixture.Coordinator.RemoveStep(1); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(2); + } + + [AvaloniaFact] + public void InsertStepAfterSelection_LeavesCacheUnchanged() + { + _surface.UpdateSelection(new[] { 1 }); + + _fixture.Coordinator.InsertStep(4, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(1); + } + + [AvaloniaFact] + public void RemoveStepAfterSelection_LeavesCacheUnchanged() + { + _surface.UpdateSelection(new[] { 1 }); + + _fixture.Coordinator.RemoveStep(4); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(1); + } + + [AvaloniaFact] + public void InsertBeforeContiguousRange_ShiftsBlockUp() + { + _surface.UpdateSelection(new[] { 2, 3, 4 }); + + // Inserting one step before the whole block moves {2, 3, 4} up to {3, 4, 5}. + _fixture.Coordinator.InsertStep(0, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(3, 4, 5); + } + + [AvaloniaFact] + public void Undo_RecipeReplaced_ClearsCache() + { + // An append leaves undo history without disturbing the selection at index 2. + _fixture.Coordinator.InsertStep(SeededStepCount, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.UpdateSelection(new[] { 2 }); + + // RecipeReplaced from a full rebuild invalidates every index, so the cache clears. + _fixture.Coordinator.Undo(); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().BeEmpty(); + } + + [AvaloniaFact] + public void ChangeStepAction_OnSelectedStep_LeavesCacheUnchanged() + { + _surface.UpdateSelection(new[] { 2 }); + + // StepActionChanged replaces the row at a stable index; the shift must not touch the cache. + // The live re-selection is governed by OnActionChanged -> RequestSelection, not the shift. + _fixture.Coordinator.ChangeStepAction(2, RecipeTestDriver.PauseActionId); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(2); + } + + [AvaloniaFact] + public void InsertBeforeThenRemoveSelection_DeletesOriginallySelectedStep() + { + // Give the selected step a unique content signature so identity is provable by value. + _fixture.Coordinator.UpdateStepProperty(2, RecipeTestDriver.StepDurationColumn, "777"); + Dispatcher.UIThread.RunJobs(); + var originallySelectedStep = _fixture.Coordinator.CurrentRecipe.Steps[2]; + + _surface.UpdateSelection(new[] { 2 }); + + // Inserting before the selection shifts the cache from [2] to [3]. + _fixture.Coordinator.InsertStep(0, RecipeTestDriver.WaitActionId); + Dispatcher.UIThread.RunJobs(); + _surface.SelectedStepIndices.Should().Equal(3); + + // Deleting via the shifted cache must remove the originally selected step, not a bystander. + _fixture.Coordinator.RemoveSteps(_surface.SelectedStepIndices); + Dispatcher.UIThread.RunJobs(); + + _fixture.Coordinator.CurrentRecipe.Steps.Should().NotContain(originallySelectedStep); + } + + [AvaloniaFact] + public void RemoveSelectedStep_LiveControl_CacheAgreesWithSelectionModel() + { + var stepListBox = ShowView(); + + Select(stepListBox, 0); + Select(stepListBox, 2); + Select(stepListBox, 4); + + // The remove drops a selected step, so the ListBox pushes SelectionChanged mid-mutation. + // The surface shift and the control's pushed indices must agree. + _fixture.Coordinator.RemoveStep(2); + Dispatcher.UIThread.RunJobs(); + + _surface.SelectedStepIndices.Should().Equal(0, 3); + _surface.SelectedStepIndices.Should().Equal(stepListBox.Selection.SelectedIndexes); + } + + private static void Select(ListBox stepListBox, int index) + { + stepListBox.SelectedItems!.Add(((TransposedRecipeGridSurface)stepListBox.DataContext!).StepColumns[index]); + Dispatcher.UIThread.RunJobs(); + } + + private ListBox ShowView() + { + var view = new TransposedRecipeGridView { DataContext = _surface }; + _window = new Window + { + Width = 1200, + Height = 800, + Content = view, + }; + + var stepListBox = view.FindControl("StepListBox"); + stepListBox.Should().NotBeNull(); + stepListBox!.DataContext = _surface; + stepListBox.UseTransposedColumnsPanel(); + + _window.Show(); + Dispatcher.UIThread.RunJobs(); + + return stepListBox; + } +} diff --git a/SemiStep/SemiStep.UI/RecipeGrid/RecipeGridSurfaceBase.cs b/SemiStep/SemiStep.UI/RecipeGrid/RecipeGridSurfaceBase.cs index 72c921f..50a144c 100644 --- a/SemiStep/SemiStep.UI/RecipeGrid/RecipeGridSurfaceBase.cs +++ b/SemiStep/SemiStep.UI/RecipeGrid/RecipeGridSurfaceBase.cs @@ -137,6 +137,7 @@ public void OnMutation(MutationSignal signal) { Dispatcher.UIThread.VerifyAccess(); + var selectionSnapshot = SelectedStepIndices; var recipe = _coordinator.CurrentRecipe; _logger.LogInformation( @@ -191,11 +192,58 @@ public void OnMutation(MutationSignal signal) return; } - ReconcileSelectionWithItems(); + switch (signal) + { + case MutationSignal.StepsInserted: + case MutationSignal.StepRemoved: + case MutationSignal.StepsRemoved: + case MutationSignal.RecipeReplaced: + { + var shifted = ShiftSelection(selectionSnapshot, signal); + if (!SelectedStepIndices.SequenceEqual(shifted)) + { + SelectedStepIndices = shifted; + } + + break; + } + } + RefreshStepStartTimes(RefreshStartIndexFor(signal)); RefreshLoopDepths(); } + // Pure projection of the selection through the same index transaction the switch applied to Items. + private static IReadOnlyList ShiftSelection(IReadOnlyList selection, MutationSignal signal) + { + switch (signal) + { + case MutationSignal.StepsInserted(var startIndex, var count): + return selection + .Select(index => index >= startIndex ? index + count : index) + .ToList(); + + case MutationSignal.StepRemoved(var removedIndex): + return selection + .Where(index => index != removedIndex) + .Select(index => index > removedIndex ? index - 1 : index) + .ToList(); + + case MutationSignal.StepsRemoved(var removedIndices): + var removedSet = removedIndices.ToHashSet(); + return selection + .Where(index => !removedSet.Contains(index)) + .Select(index => index - removedIndices.Count(removed => removed < index)) + .ToList(); + + case MutationSignal.RecipeReplaced: + return []; + + default: + return selection; + } + } + // start-time[i] depends only on steps 0..i-1, so refreshing from the mutation index is safe; loop-depth can change earlier rows, so it stays a full scan. private static int RefreshStartIndexFor(MutationSignal signal) { @@ -213,23 +261,6 @@ private static int RefreshStartIndexFor(MutationSignal signal) return Math.Max(fromIndex, 0); } - private void ReconcileSelectionWithItems() - { - var currentSelection = SelectedStepIndices; - if (currentSelection.Count == 0) - { - return; - } - - var itemCount = Items.Count; - if (currentSelection.All(index => index < itemCount)) - { - return; - } - - SelectedStepIndices = currentSelection.Where(index => index < itemCount).ToList(); - } - public void RequestSelection(int? stepIndex) { if (_disposables.IsDisposed)