From bc32d4216c35fe643e02c047f648d3650f40fbed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pol=C3=A1=C5=A1ek?= Date: Tue, 21 Jul 2026 17:25:09 +0200 Subject: [PATCH] CmdPal: Prevent selection from overriding ListView scrolling (#49354) ## Summary of the Pull Request This PR make ensuring selected item visibility on the list view optional and avoids it when user scrolls list view viewport manually (using scrollbar or mouse wheel), without touching selection. - Implicitly keep selection when using incrementel loading (incrementel loading) - Make ensuring the selected item is visible optional, and skip it when the user scrolls the ListView viewport using the scrollbar or mouse wheel ## PR Checklist - [x] Closes: #46592 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed --- .../ItemsUpdatedEventArgs.cs | 8 ++ .../ListViewModel.cs | 54 +++++++++---- .../ExtViews/ListItemsView.xaml.cs | 24 ++++-- .../ListViewModelTests.cs | 78 ++++++++++++++++++- 4 files changed, 138 insertions(+), 26 deletions(-) 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; + } + } }