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)