mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-09-02 04:01:25 +02:00
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 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] Closes: #46592 <!-- - [ ] Closes: #yyy (add separate lines for additional resolved issues) --> - [ ] **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 <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed
This commit is contained in:
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -56,11 +56,12 @@ public partial class ListViewModel : PageViewModel, IDisposable
|
||||
[ThreadStatic]
|
||||
private static Dictionary<ListViewModel, int>? _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<ListViewModel, ItemsUpdatedEventArgs>? 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);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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).
|
||||
/// </param>
|
||||
private bool TrySetSelectionAfterUpdate(ListViewModel sender, long version, bool forceFirstItem)
|
||||
/// <param name="ensureSelectionVisible">
|
||||
/// When true, scroll a preserved selection into view after a soft refresh.
|
||||
/// </param>
|
||||
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);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -60,7 +60,23 @@ public partial class ListViewModelTests
|
||||
private static TaskCompletionSource<bool> 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<ItemsUpdatedEventArgs> ObserveNextItemsUpdateAsync(ListViewModel viewModel, Action action)
|
||||
{
|
||||
var updateObserved = new TaskCompletionSource<ItemsUpdatedEventArgs>(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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user