diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ItemsUpdatedEventArgs.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ItemsUpdatedEventArgs.cs index d9f3684ece..5db6bb9a3c 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ItemsUpdatedEventArgs.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ItemsUpdatedEventArgs.cs @@ -8,8 +8,16 @@ public sealed partial class ItemsUpdatedEventArgs : EventArgs { public bool ForceFirstItem { get; } + public bool EnsureSelectionVisible { get; } + public ItemsUpdatedEventArgs(bool forceFirstItem) + : this(forceFirstItem, ensureSelectionVisible: true) + { + } + + public ItemsUpdatedEventArgs(bool forceFirstItem, bool ensureSelectionVisible) { ForceFirstItem = forceFirstItem; + EnsureSelectionVisible = ensureSelectionVisible; } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListViewModel.cs index cfbb3a7e53..344c28dcf8 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListViewModel.cs @@ -56,11 +56,12 @@ public partial class ListViewModel : PageViewModel, IDisposable [ThreadStatic] private static Dictionary? _getItemsDepthByViewModel; - private InterlockedBoolean _isLoading; + private InterlockedBoolean _isLoadingMore; private int _activeFetchCount; private int _latestFetchGeneration; private bool _deferredFetchRequested; private bool _deferredFetchKeepSelection = true; + private bool _deferredFetchEnsureSelectionVisible; public event TypedEventHandler? ItemsUpdated; @@ -149,7 +150,14 @@ public partial class ListViewModel : PageViewModel, IDisposable private void Model_ItemsChanged(object sender, IItemsChangedEventArgs args) { - RequestFetch(args.TotalItems == IncrementalRefresh); + var isLoadingMore = _isLoadingMore.Value; + + // Perform a soft refresh when: + // - the caller explicitly requests it through a flag piggybacked on args.TotalItems; + // - incremental loading (LoadMore) is used, which implies a soft refresh by definition. + RequestFetch( + keepSelection: args.TotalItems == IncrementalRefresh || isLoadingMore, + ensureSelectionVisible: !isLoadingMore); } protected override void OnSearchTextBoxUpdated(string searchTextBox) @@ -199,9 +207,9 @@ public partial class ListViewModel : PageViewModel, IDisposable RunFilteredItemsUpdate(ApplyFilterUnderLock); } - ItemsUpdated?.Invoke(this, new ItemsUpdatedEventArgs(true)); + ItemsUpdated?.Invoke(this, new ItemsUpdatedEventArgs(forceFirstItem: true, ensureSelectionVisible: true)); UpdateEmptyContent(); - _isLoading.Clear(); + _isLoadingMore.Clear(); } } @@ -225,14 +233,16 @@ public partial class ListViewModel : PageViewModel, IDisposable }); } - private void RequestFetch(bool keepSelection) + private void RequestFetch(bool keepSelection, bool ensureSelectionVisible) { // Keep RPC GetItems work off the UI thread. If the provider raises // ItemsChanged while we're already on a background thread, stay on that // thread so same-thread reentrancy detection still works. if (IsCurrentThreadUiThread()) { - QueueObservedBackgroundFetch(() => RequestFetch(keepSelection), "Failed to request background fetch"); + QueueObservedBackgroundFetch( + () => RequestFetch(keepSelection, ensureSelectionVisible), + "Failed to request background fetch"); return; } @@ -242,29 +252,35 @@ public partial class ListViewModel : PageViewModel, IDisposable { _deferredFetchRequested = true; _deferredFetchKeepSelection &= keepSelection; + _deferredFetchEnsureSelectionVisible |= ensureSelectionVisible; } return; } - FetchItems(keepSelection); + FetchItems(keepSelection, ensureSelectionVisible); } private void QueueDeferredFetchIfNeeded() { bool deferredFetchRequested; bool keepSelection; + bool ensureSelectionVisible; lock (_fetchStateLock) { deferredFetchRequested = _deferredFetchRequested; keepSelection = _deferredFetchKeepSelection; + ensureSelectionVisible = _deferredFetchEnsureSelectionVisible; _deferredFetchRequested = false; _deferredFetchKeepSelection = true; + _deferredFetchEnsureSelectionVisible = false; } if (deferredFetchRequested) { - QueueObservedBackgroundFetch(() => FetchItems(keepSelection), "Failed to execute deferred fetch"); + QueueObservedBackgroundFetch( + () => FetchItems(keepSelection, ensureSelectionVisible), + "Failed to execute deferred fetch"); } } @@ -288,7 +304,7 @@ public partial class ListViewModel : PageViewModel, IDisposable } //// Run on background thread, from InitializeAsync or Model_ItemsChanged - private void FetchItems(bool keepSelection) + private void FetchItems(bool keepSelection, bool ensureSelectionVisible) { System.Diagnostics.Debug.Assert(!IsCurrentThreadUiThread(), "FetchItems should not run on the UI thread."); @@ -536,8 +552,12 @@ public partial class ListViewModel : PageViewModel, IDisposable var forceFirst = _forceFirstItemPending; _forceFirstItemPending = false; - ItemsUpdated?.Invoke(this, new ItemsUpdatedEventArgs(forceFirstItem: IsRootPage && forceFirst)); - _isLoading.Clear(); + ItemsUpdated?.Invoke( + this, + new ItemsUpdatedEventArgs( + forceFirstItem: IsRootPage && forceFirst, + ensureSelectionVisible: ensureSelectionVisible)); + _isLoadingMore.Clear(); } }); } @@ -964,7 +984,7 @@ public partial class ListViewModel : PageViewModel, IDisposable LoadExtendedAttributes(haveProperties.GetProperties().AsReadOnly()); } - FetchItems(true); + FetchItems(keepSelection: true, ensureSelectionVisible: true); model.ItemsChanged += Model_ItemsChanged; } @@ -998,7 +1018,7 @@ public partial class ListViewModel : PageViewModel, IDisposable return; } - if (!_isLoading.Set()) + if (!_isLoadingMore.Set()) { return; @@ -1016,17 +1036,17 @@ public partial class ListViewModel : PageViewModel, IDisposable { model.LoadMore(); - // _isLoading flag will be set as a result of LoadMore, - // which must raise ItemsChanged to end the loading. + // LoadMore must raise ItemsChanged; the resulting fetch clears + // _isLoadingMore when the updated items are published. } else { - _isLoading.Clear(); + _isLoadingMore.Clear(); } } catch (Exception ex) { - _isLoading.Clear(); + _isLoadingMore.Clear(); ShowException(ex, model.Name); } }); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/ExtViews/ListItemsView.xaml.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/ExtViews/ListItemsView.xaml.cs index 355854de70..d911119a41 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/ExtViews/ListItemsView.xaml.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/ExtViews/ListItemsView.xaml.cs @@ -680,12 +680,13 @@ public sealed partial class ListItemsView : UserControl, // Latch: once any update requests force-first, keep it until consumed. _forceFirstPending |= args.ForceFirstItem; var forceFirstItem = _forceFirstPending; + var ensureSelectionVisible = args.EnsureSelectionVisible; // Try to handle selection immediately — items should already be available // since FilteredItems is a direct ObservableCollection bound as ItemsSource. // TrySetSelectionAfterUpdate clears _forceFirstPending internally once // selection stabilizes (no repair needed), so we don't clear it here. - if (TrySetSelectionAfterUpdate(sender, version, forceFirstItem)) + if (TrySetSelectionAfterUpdate(sender, version, forceFirstItem, ensureSelectionVisible)) { return; } @@ -700,7 +701,7 @@ public sealed partial class ListItemsView : UserControl, return; } - TrySetSelectionAfterUpdate(sender, version, forceFirstItem); + TrySetSelectionAfterUpdate(sender, version, forceFirstItem, ensureSelectionVisible); }); } @@ -712,7 +713,10 @@ public sealed partial class ListItemsView : UserControl, /// When true, always select the first selectable item and scroll to top /// (used for filter changes and top-level fetches). /// - private bool TrySetSelectionAfterUpdate(ListViewModel sender, long version, bool forceFirstItem) + /// + /// When true, scroll a preserved selection into view after a soft refresh. + /// + private bool TrySetSelectionAfterUpdate(ListViewModel sender, long version, bool forceFirstItem, bool ensureSelectionVisible) { if (version != Volatile.Read(ref _itemsUpdatedVersion)) { @@ -819,7 +823,7 @@ public sealed partial class ListItemsView : UserControl, ItemView.UpdateLayout(); - if (stickyRestored is not null) + if (stickyRestored is not null && ensureSelectionVisible) { ScrollToItem(stickyRestored); } @@ -838,16 +842,20 @@ public sealed partial class ListItemsView : UserControl, else { // Selection is valid and unchanged: the force-first intent (if any) - // has been fully delivered and selection has stabilized. Safe to clear. + // has been fully delivered and selection has stabilized. _forceFirstPending = false; - // Just make sure the item is visible - if (_stickySelectedItem is ListItemViewModel li) + if (ensureSelectionVisible && _stickySelectedItem is ListItemViewModel selectedItem) { _ = DispatcherQueue.TryEnqueue(Microsoft.UI.Dispatching.DispatcherQueuePriority.Low, () => { + if (version != Volatile.Read(ref _itemsUpdatedVersion)) + { + return; + } + ItemView.UpdateLayout(); - ScrollToItem(li); + ScrollToItem(selectedItem); }); } } diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ListViewModelTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ListViewModelTests.cs index 5bb693c5bc..27d74d3c97 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ListViewModelTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ListViewModelTests.cs @@ -60,7 +60,23 @@ public partial class ListViewModelTests private static TaskCompletionSource NewDeferredFetchObserved() => new(TaskCreationOptions.RunContinuationsAsynchronously); } - private static ListViewModel CreateViewModel(RecursiveItemsChangedPage page) => + private sealed partial class IncrementalLoadingPage : ListPage + { + private IListItem[] _items = [new ListItem(new NoOpCommand() { Name = "Item 1" })]; + + public override IListItem[] GetItems() => _items; + + public override void LoadMore() + { + _items = [.. _items, new ListItem(new NoOpCommand() { Name = "Item 2" })]; + HasMoreItems = false; + RaiseItemsChanged(_items.Length); + } + + public void TriggerItemsChanged(int totalItems) => RaiseItemsChanged(totalItems); + } + + private static ListViewModel CreateViewModel(IListPage page) => new(page, TaskScheduler.Default, new TestAppExtensionHost(), CommandProviderContext.Empty, DefaultContextMenuFactory.Instance); [TestMethod] @@ -86,4 +102,64 @@ public partial class ListViewModelTests viewModel.SafeCleanup(); viewModel.Dispose(); } + + [TestMethod] + public async Task LoadMoreItemsChanged_PreservesSelectionImplicitly() + { + var page = new IncrementalLoadingPage + { + Id = "list.page", + Name = "List Page", + Title = "List Page", + HasMoreItems = true, + }; + + var viewModel = CreateViewModel(page); + try + { + var initialUpdate = await ObserveNextItemsUpdateAsync(viewModel, viewModel.InitializeProperties); + Assert.IsFalse(initialUpdate.ForceFirstItem); + Assert.IsTrue(initialUpdate.EnsureSelectionVisible); + + var regularUpdate = await ObserveNextItemsUpdateAsync(viewModel, () => page.TriggerItemsChanged(1)); + Assert.IsTrue(regularUpdate.ForceFirstItem); + Assert.IsTrue(regularUpdate.EnsureSelectionVisible); + + var explicitIncrementalUpdate = await ObserveNextItemsUpdateAsync( + viewModel, + () => page.TriggerItemsChanged(ListViewModel.IncrementalRefresh)); + Assert.IsFalse(explicitIncrementalUpdate.ForceFirstItem); + Assert.IsTrue(explicitIncrementalUpdate.EnsureSelectionVisible); + + var loadMoreUpdate = await ObserveNextItemsUpdateAsync(viewModel, viewModel.LoadMoreIfNeeded); + Assert.IsFalse(loadMoreUpdate.ForceFirstItem); + Assert.IsFalse(loadMoreUpdate.EnsureSelectionVisible); + } + finally + { + viewModel.SafeCleanup(); + viewModel.Dispose(); + } + } + + private static async Task ObserveNextItemsUpdateAsync(ListViewModel viewModel, Action action) + { + var updateObserved = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + void OnItemsUpdated(ListViewModel sender, ItemsUpdatedEventArgs args) => updateObserved.TrySetResult(args); + + viewModel.ItemsUpdated += OnItemsUpdated; + try + { + action(); + + var completed = await Task.WhenAny(updateObserved.Task, Task.Delay(TimeSpan.FromSeconds(2))); + Assert.AreSame(updateObserved.Task, completed); + return await updateObserved.Task; + } + finally + { + viewModel.ItemsUpdated -= OnItemsUpdated; + } + } }