From 90131e35d936ef387e8416814a917e97ef7c7d7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pol=C3=A1=C5=A1ek?= Date: Mon, 9 Mar 2026 23:34:44 +0100 Subject: [PATCH] CmdPal: Prevent unsynchornized access to More commands (#46020) ## Summary of the Pull Request This PR fixes a crash caused by unsynchronized access to a context menu: - Fixes `System.ArgumentException: Destination array was not long enough` crash in `CommandItemViewModel.AllCommands` caused by `List.AddRange()` racing with background `BuildAndInitMoreCommands` mutations - Replaces mutable `List` public surfaces with immutable array snapshots protected by a `Lock`; writers hold the lock, mutate the backing list, then atomically publish new snapshots via `volatile` fields that readers access lock-free - Applies the same snapshot pattern to `ContentPageViewModel`, using a bundled `CommandSnapshot` object for atomic publication (since `PrimaryCommand` drives command invocation there, not just UI hints) - Narrows `IContextMenuContext.MoreCommands` and `AllCommands` from `List`/`IEnumerable` to `IReadOnlyList` to prevent consumers from casting back and mutating - Moves `SafeCleanup()` calls outside locks in cleanup paths to avoid holding the lock during cross-process RPC calls ## PR Checklist - [x] Closes: #45975 - [ ] **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 --- .../CommandBarViewModel.cs | 6 +- .../CommandItemViewModel.cs | 126 +++++++----- .../ContentPageViewModel.cs | 189 ++++++++++++------ .../ListItemViewModel.cs | 60 +++--- .../Messages/UpdateCommandBarMessage.cs | 4 +- .../CommandItemViewModelTests.cs | 79 ++++++++ .../ContentPageViewModelTests.cs | 104 ++++++++++ 7 files changed, 431 insertions(+), 137 deletions(-) create mode 100644 src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/CommandItemViewModelTests.cs create mode 100644 src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ContentPageViewModelTests.cs diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandBarViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandBarViewModel.cs index 2681718485..a5d34eea63 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandBarViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandBarViewModel.cs @@ -122,11 +122,9 @@ public sealed partial class CommandBarViewModel : ObservableObject, } SecondaryCommand = SelectedItem.SecondaryCommand; + var moreCommands = SelectedItem.MoreCommands; - var hasMoreThanOneContextItem = SelectedItem.MoreCommands.Count() > 1; - var hasMoreThanOneCommand = SelectedItem.MoreCommands.OfType().Any(); - - ShouldShowContextMenu = hasMoreThanOneContextItem && hasMoreThanOneCommand; + ShouldShowContextMenu = moreCommands.Count > 1 && SelectedItem.HasMoreCommands; OnPropertyChanged(nameof(HasSecondaryCommand)); OnPropertyChanged(nameof(SecondaryCommand)); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandItemViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandItemViewModel.cs index 0ba179f207..068748d585 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandItemViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandItemViewModel.cs @@ -21,6 +21,12 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa private readonly IContextMenuFactory? _contextMenuFactory; + private readonly Lock _moreCommandsLock = new(); + private readonly List _moreCommands = []; + private volatile CommandContextItemViewModel? _secondaryMoreCommand; + private volatile IContextItemViewModel[] _moreCommandsSnapshot = []; + private volatile IContextItemViewModel[] _allCommandsSnapshot = []; + private ExtensionObject? ExtendedAttributesProvider { get; set; } private readonly ExtensionObject _commandItemModel = new(null); @@ -63,33 +69,22 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa public CommandViewModel Command { get; private set; } - public List MoreCommands { get; private set; } = []; + // Reuse a cached read-only snapshot so repeated reads don't allocate. + public IReadOnlyList MoreCommands => _moreCommandsSnapshot; - IEnumerable IContextMenuContext.MoreCommands => MoreCommands; + IReadOnlyList IContextMenuContext.MoreCommands => _moreCommandsSnapshot; - private List ActualCommands => MoreCommands.OfType().ToList(); + protected Lock MoreCommandsLock => _moreCommandsLock; - public bool HasMoreCommands => ActualCommands.Count > 0; + protected List UnsafeMoreCommands => _moreCommands; - public string SecondaryCommandName => SecondaryCommand?.Name ?? string.Empty; + public bool HasMoreCommands => _secondaryMoreCommand is not null; + + public string SecondaryCommandName => _secondaryMoreCommand?.Name ?? string.Empty; public CommandItemViewModel? PrimaryCommand => this; - public CommandItemViewModel? SecondaryCommand - { - get - { - if (HasMoreCommands) - { - if (MoreCommands[0] is CommandContextItemViewModel command) - { - return command; - } - } - - return null; - } - } + public CommandItemViewModel? SecondaryCommand => _secondaryMoreCommand; public bool ShouldBeVisible => !string.IsNullOrEmpty(Name); @@ -101,18 +96,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa public DataPackageView? DataPackage { get; private set; } - public List AllCommands - { - get - { - List l = _defaultCommandContextItemViewModel is null ? - new() : - [_defaultCommandContextItemViewModel]; - - l.AddRange(MoreCommands); - return l; - } - } + public IReadOnlyList AllCommands => _allCommandsSnapshot; private static readonly IconInfoViewModel _errorIcon; @@ -246,6 +230,11 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa UpdateDefaultContextItemIcon(); } + lock (_moreCommandsLock) + { + RefreshMoreCommandStateUnsafe(); + } + Initialized |= InitializedState.SelectionInitialized; UpdateProperty(nameof(MoreCommands)); UpdateProperty(nameof(AllCommands)); @@ -265,7 +254,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa Command = new(null, PageContext); _itemTitle = "Error"; Subtitle = "Item failed to load"; - MoreCommands = []; + ClearMoreCommands(); _icon = _errorIcon; _titleCache.Invalidate(); _subtitleCache.Invalidate(); @@ -304,7 +293,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa Command = new(null, PageContext); _itemTitle = "Error"; Subtitle = "Item failed to load"; - MoreCommands = []; + ClearMoreCommands(); _icon = _errorIcon; _titleCache.Invalidate(); _subtitleCache.Invalidate(); @@ -385,9 +374,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa case nameof(model.MoreCommands): BuildAndInitMoreCommands(); - UpdateProperty(nameof(SecondaryCommand)); - UpdateProperty(nameof(SecondaryCommandName)); - UpdateProperty(nameof(HasMoreCommands)); + UpdateProperty(nameof(SecondaryCommand), nameof(SecondaryCommandName), nameof(HasMoreCommands), nameof(AllCommands)); break; case nameof(DataPackage): @@ -478,9 +465,10 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa var results = factory.UnsafeBuildAndInitMoreCommands(more, this); List? freedItems; - lock (MoreCommands) + lock (_moreCommandsLock) { - ListHelpers.InPlaceUpdateList(MoreCommands, results, out freedItems); + ListHelpers.InPlaceUpdateList(_moreCommands, results, out freedItems); + RefreshMoreCommandStateUnsafe(); } freedItems.OfType() @@ -516,20 +504,30 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa { base.UnsafeCleanup(); - lock (MoreCommands) + List freedItems; + CommandContextItemViewModel? freedDefault; + lock (_moreCommandsLock) { - MoreCommands.OfType() - .ToList() - .ForEach(c => c.SafeCleanup()); - MoreCommands.Clear(); + freedItems = [.. _moreCommands]; + _moreCommands.Clear(); + + // Null out here so the single RefreshMoreCommandStateUnsafe call + // produces an _allCommandsSnapshot that excludes the default command. + freedDefault = _defaultCommandContextItemViewModel; + _defaultCommandContextItemViewModel = null; + + RefreshMoreCommandStateUnsafe(); } + // Cleanup outside lock to avoid holding it during RPC calls + freedItems.OfType() + .ToList() + .ForEach(c => c.SafeCleanup()); + freedDefault?.SafeCleanup(); + // _listItemIcon.SafeCleanup(); _icon = new(null); // necessary? - _defaultCommandContextItemViewModel?.SafeCleanup(); - _defaultCommandContextItemViewModel = null; - Command.PropertyChanged -= Command_PropertyChanged; Command.SafeCleanup(); @@ -545,6 +543,40 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa base.SafeCleanup(); Initialized |= InitializedState.CleanedUp; } + + protected void RefreshMoreCommandStateUnsafe() + { + _moreCommandsSnapshot = [.. _moreCommands]; + + _secondaryMoreCommand = null; + foreach (var item in _moreCommands) + { + if (item is CommandContextItemViewModel command) + { + _secondaryMoreCommand = command; + break; + } + } + + _allCommandsSnapshot = _defaultCommandContextItemViewModel is null ? + _moreCommandsSnapshot : + [_defaultCommandContextItemViewModel, .. _moreCommandsSnapshot]; + } + + private void ClearMoreCommands() + { + List freedItems; + lock (_moreCommandsLock) + { + freedItems = [.. _moreCommands]; + _moreCommands.Clear(); + RefreshMoreCommandStateUnsafe(); + } + + freedItems.OfType() + .ToList() + .ForEach(c => c.SafeCleanup()); + } } [Flags] diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ContentPageViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ContentPageViewModel.cs index da34915712..41c9735808 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ContentPageViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ContentPageViewModel.cs @@ -17,13 +17,15 @@ namespace Microsoft.CmdPal.UI.ViewModels; public partial class ContentPageViewModel : PageViewModel, ICommandBarContext { private readonly ExtensionObject _model; + private readonly Lock _commandsLock = new(); + private volatile CommandSnapshot _snapshot = CommandSnapshot.Empty; [ObservableProperty] public partial ObservableCollection Content { get; set; } = []; - public List Commands { get; private set; } = []; + private List Commands { get; } = []; - public bool HasCommands => ActualCommands.Count > 0; + public bool HasCommands => _snapshot.PrimaryCommand is not null; public DetailsViewModel? Details { get; private set; } @@ -31,19 +33,17 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext public bool HasDetails => Details is not null; /////// ICommandBarContext /////// - public IEnumerable MoreCommands => Commands.Skip(1); + public IReadOnlyList MoreCommands => _snapshot.MoreCommands; - private List ActualCommands => Commands.OfType().ToList(); + public bool HasMoreCommands => _snapshot.SecondaryCommand is not null; - public bool HasMoreCommands => ActualCommands.Count > 1; + public string SecondaryCommandName => _snapshot.SecondaryCommand?.Name ?? string.Empty; - public string SecondaryCommandName => SecondaryCommand?.Name ?? string.Empty; + public CommandItemViewModel? PrimaryCommand => _snapshot.PrimaryCommand; - public CommandItemViewModel? PrimaryCommand => HasCommands ? ActualCommands[0] : null; + public CommandItemViewModel? SecondaryCommand => _snapshot.SecondaryCommand; - public CommandItemViewModel? SecondaryCommand => HasMoreCommands ? ActualCommands[1] : null; - - public List AllCommands => Commands; + public IReadOnlyList AllCommands => _snapshot.AllCommands; // Remember - "observable" properties from the model (via PropChanged) // cannot be marked [ObservableProperty] @@ -109,28 +109,14 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext return; // throw? } - Commands = model.Commands - .ToList() - .Select(item => - { - if (item is ICommandContextItem contextItem) - { - return new CommandContextItemViewModel(contextItem, PageContext); - } - else - { - return new SeparatorViewModel(); - } - }) - .ToList(); + var commands = BuildCommandViewModels(model.Commands); + InitializeCommandViewModels(commands, static contextItem => contextItem.InitializeProperties()); - Commands - .OfType() - .ToList() - .ForEach(contextItem => - { - contextItem.InitializeProperties(); - }); + lock (_commandsLock) + { + ListHelpers.InPlaceUpdateList(Commands, commands); + RefreshCommandSnapshotsUnsafe(); + } var extensionDetails = model.Details; if (extensionDetails is not null) @@ -168,37 +154,29 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext var more = model.Commands; if (more is not null) { - var newContextMenu = more - .ToList() - .Select(item => - { - if (item is ICommandContextItem contextItem) - { - return new CommandContextItemViewModel(contextItem, PageContext) as IContextItemViewModel; - } - else - { - return new SeparatorViewModel(); - } - }) - .ToList(); + var newContextMenu = BuildCommandViewModels(more); + InitializeCommandViewModels(newContextMenu, static contextItem => contextItem.SlowInitializeProperties()); - lock (Commands) + List removedItems; + lock (_commandsLock) { - ListHelpers.InPlaceUpdateList(Commands, newContextMenu); + ListHelpers.InPlaceUpdateList(Commands, newContextMenu, out removedItems); + RefreshCommandSnapshotsUnsafe(); } - Commands - .OfType() - .ToList() - .ForEach(contextItem => - { - contextItem.SlowInitializeProperties(); - }); + CleanupCommandViewModels(removedItems); } else { - Commands.Clear(); + List removedItems; + lock (_commandsLock) + { + removedItems = [.. Commands]; + Commands.Clear(); + RefreshCommandSnapshotsUnsafe(); + } + + CleanupCommandViewModels(removedItems); } UpdateProperty(nameof(PrimaryCommand)); @@ -206,6 +184,7 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext UpdateProperty(nameof(SecondaryCommandName)); UpdateProperty(nameof(HasCommands)); UpdateProperty(nameof(HasMoreCommands)); + UpdateProperty(nameof(MoreCommands)); UpdateProperty(nameof(AllCommands)); DoOnUiThread( () => @@ -243,6 +222,72 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext }); } + private List BuildCommandViewModels(IContextItem[]? items) + { + if (items is null) + { + return []; + } + + return items + .Select(item => + { + if (item is ICommandContextItem contextItem) + { + return new CommandContextItemViewModel(contextItem, PageContext); + } + + return new SeparatorViewModel(); + }) + .ToList(); + } + + private static void InitializeCommandViewModels(IEnumerable commands, Action initialize) + { + foreach (var contextItem in commands.OfType()) + { + initialize(contextItem); + } + } + + private static void CleanupCommandViewModels(IEnumerable commands) + { + foreach (var contextItem in commands.OfType()) + { + contextItem.SafeCleanup(); + } + } + + private void RefreshCommandSnapshotsUnsafe() + { + var allCommands = (IContextItemViewModel[])[.. Commands]; + var moreCommands = allCommands.Length > 1 + ? allCommands[1..] + : []; + + CommandContextItemViewModel? primary = null; + CommandContextItemViewModel? secondary = null; + foreach (var item in allCommands) + { + if (item is not CommandContextItemViewModel command) + { + continue; + } + + if (primary is null) + { + primary = command; + } + else if (secondary is null) + { + secondary = command; + break; + } + } + + _snapshot = new(allCommands, moreCommands, primary, secondary); + } + // InvokeItemCommand is what this will be in Xaml due to source generator // this comes in on Enter keypresses in the SearchBox [RelayCommand] @@ -270,12 +315,15 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext Details?.SafeCleanup(); - Commands - .OfType() - .ToList() - .ForEach(item => item.SafeCleanup()); + List removedItems; + lock (_commandsLock) + { + removedItems = [.. Commands]; + Commands.Clear(); + RefreshCommandSnapshotsUnsafe(); + } - Commands.Clear(); + CleanupCommandViewModels(removedItems); foreach (var item in Content) { @@ -290,4 +338,25 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext model.ItemsChanged -= Model_ItemsChanged; } } + + /// + /// Immutable bundle of derived command state, published atomically via a + /// single volatile write so readers never see a torn snapshot. + /// + private sealed class CommandSnapshot( + IContextItemViewModel[] allCommands, + IContextItemViewModel[] moreCommands, + CommandContextItemViewModel? primaryCommand, + CommandContextItemViewModel? secondaryCommand) + { + public static CommandSnapshot Empty { get; } = new([], [], null, null); + + public IContextItemViewModel[] AllCommands { get; } = allCommands; + + public IContextItemViewModel[] MoreCommands { get; } = moreCommands; + + public CommandContextItemViewModel? PrimaryCommand { get; } = primaryCommand; + + public CommandContextItemViewModel? SecondaryCommand { get; } = secondaryCommand; + } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListItemViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListItemViewModel.cs index 21cdba9797..0cda382191 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListItemViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListItemViewModel.cs @@ -159,7 +159,6 @@ public partial class ListItemViewModel : CommandItemViewModel UpdateShowDetailsCommand(); break; case nameof(model.MoreCommands): - UpdateProperty(nameof(MoreCommands)); AddShowDetailsCommands(); break; case nameof(model.Title): @@ -195,19 +194,27 @@ public partial class ListItemViewModel : CommandItemViewModel pageContext is ListViewModel listViewModel && !listViewModel.ShowDetails) { - // Check if "Show Details" action already exists to prevent duplicates - if (!MoreCommands.Any(cmd => cmd is CommandContextItemViewModel contextItemViewModel && - contextItemViewModel.Command.Id == ShowDetailsCommand.ShowDetailsCommandId)) + var addedCommand = false; + lock (MoreCommandsLock) { - // Create the view model for the show details command - var showDetailsCommand = new ShowDetailsCommand(Details); - var showDetailsContextItem = new CommandContextItem(showDetailsCommand); - var showDetailsContextItemViewModel = new CommandContextItemViewModel(showDetailsContextItem, PageContext); - showDetailsContextItemViewModel.SlowInitializeProperties(); - MoreCommands.Add(showDetailsContextItemViewModel); + // Check if "Show Details" action already exists to prevent duplicates + if (!UnsafeMoreCommands.Any(cmd => cmd is CommandContextItemViewModel contextItemViewModel && + contextItemViewModel.Command.Id == ShowDetailsCommand.ShowDetailsCommandId)) + { + var showDetailsCommand = new ShowDetailsCommand(Details); + var showDetailsContextItem = new CommandContextItem(showDetailsCommand); + var showDetailsContextItemViewModel = new CommandContextItemViewModel(showDetailsContextItem, PageContext); + showDetailsContextItemViewModel.SlowInitializeProperties(); + UnsafeMoreCommands.Add(showDetailsContextItemViewModel); + RefreshMoreCommandStateUnsafe(); + addedCommand = true; + } } - UpdateProperty(nameof(MoreCommands), nameof(AllCommands)); + if (addedCommand) + { + UpdateProperty(nameof(MoreCommands), nameof(AllCommands)); + } } } @@ -222,22 +229,27 @@ public partial class ListItemViewModel : CommandItemViewModel pageContext is ListViewModel listViewModel && !listViewModel.ShowDetails) { - var existingCommand = MoreCommands.FirstOrDefault(cmd => - cmd is CommandContextItemViewModel contextItemViewModel && - contextItemViewModel.Command.Id == ShowDetailsCommand.ShowDetailsCommandId); - - // If the command already exists, remove it to update with the new details - if (existingCommand is not null) + CommandContextItemViewModel? oldCommand = null; + lock (MoreCommandsLock) { - MoreCommands.Remove(existingCommand); + oldCommand = UnsafeMoreCommands + .OfType() + .FirstOrDefault(contextItemViewModel => contextItemViewModel.Command.Id == ShowDetailsCommand.ShowDetailsCommandId); + + if (oldCommand is not null) + { + UnsafeMoreCommands.Remove(oldCommand); + } + + var showDetailsCommand = new ShowDetailsCommand(Details); + var showDetailsContextItem = new CommandContextItem(showDetailsCommand); + var showDetailsContextItemViewModel = new CommandContextItemViewModel(showDetailsContextItem, PageContext); + showDetailsContextItemViewModel.SlowInitializeProperties(); + UnsafeMoreCommands.Add(showDetailsContextItemViewModel); + RefreshMoreCommandStateUnsafe(); } - // Create the view model for the show details command - var showDetailsCommand = new ShowDetailsCommand(Details); - var showDetailsContextItem = new CommandContextItem(showDetailsCommand); - var showDetailsContextItemViewModel = new CommandContextItemViewModel(showDetailsContextItem, PageContext); - showDetailsContextItemViewModel.SlowInitializeProperties(); - MoreCommands.Add(showDetailsContextItemViewModel); + oldCommand?.SafeCleanup(); UpdateProperty(nameof(MoreCommands), nameof(AllCommands)); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Messages/UpdateCommandBarMessage.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Messages/UpdateCommandBarMessage.cs index ab97a178fa..cb3f2e68f7 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Messages/UpdateCommandBarMessage.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Messages/UpdateCommandBarMessage.cs @@ -18,11 +18,11 @@ public record UpdateCommandBarMessage(ICommandBarContext? ViewModel) public interface IContextMenuContext : INotifyPropertyChanged { - public IEnumerable MoreCommands { get; } + public IReadOnlyList MoreCommands { get; } public bool HasMoreCommands { get; } - public List AllCommands { get; } + public IReadOnlyList AllCommands { get; } /// /// Generates a mapping of key -> command item for this particular item's diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/CommandItemViewModelTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/CommandItemViewModelTests.cs new file mode 100644 index 0000000000..e4c65ecfaf --- /dev/null +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/CommandItemViewModelTests.cs @@ -0,0 +1,79 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Threading.Tasks; +using Microsoft.CmdPal.UI.ViewModels.Models; +using Microsoft.CommandPalette.Extensions.Toolkit; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.CmdPal.UI.ViewModels.UnitTests; + +[TestClass] +public class CommandItemViewModelTests +{ + private sealed class TestPageContext : IPageContext + { + public TaskScheduler Scheduler => TaskScheduler.Default; + + public ICommandProviderContext ProviderContext => CommandProviderContext.Empty; + + public void ShowException(Exception ex, string? extensionHint = null) + { + throw new AssertFailedException($"Unexpected exception from view model: {ex}"); + } + } + + [TestMethod] + public void MoreCommandsAndAllCommands_ReturnSnapshots() + { + // The public getters should return cached read-only snapshots, so + // repeated reads don't allocate a new list when the backing data hasn't + // changed. + var pageContext = new TestPageContext(); + var item = new CommandItem(new NoOpCommand { Name = "Primary" }) + { + Title = "Primary", + MoreCommands = + [ + new CommandContextItem(new NoOpCommand { Name = "Secondary" }), + ], + }; + + var viewModel = new CommandItemViewModel(new(item), new(pageContext), DefaultContextMenuFactory.Instance); + viewModel.SlowInitializeProperties(); + + var moreCommands = viewModel.MoreCommands; + var allCommands = viewModel.AllCommands; + + Assert.AreSame(moreCommands, viewModel.MoreCommands); + Assert.AreSame(allCommands, viewModel.AllCommands); + Assert.AreEqual(1, moreCommands.Count); + Assert.AreEqual(2, allCommands.Count); + } + + [TestMethod] + public void SecondaryCommand_IgnoresLeadingSeparators() + { + // SecondaryCommand/HasMoreCommands should be derived from the first actual command item, + // not from the raw first entry in MoreCommands. + var pageContext = new TestPageContext(); + var item = new CommandItem(new NoOpCommand { Name = "Primary" }) + { + Title = "Primary", + MoreCommands = + [ + new Separator("Group"), + new CommandContextItem(new NoOpCommand { Name = "Secondary" }), + ], + }; + + var viewModel = new CommandItemViewModel(new(item), new(pageContext), DefaultContextMenuFactory.Instance); + viewModel.SlowInitializeProperties(); + + Assert.IsTrue(viewModel.HasMoreCommands); + Assert.IsNotNull(viewModel.SecondaryCommand); + Assert.AreEqual("Secondary", viewModel.SecondaryCommand.Name); + } +} diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ContentPageViewModelTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ContentPageViewModelTests.cs new file mode 100644 index 0000000000..07d5c346db --- /dev/null +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/ContentPageViewModelTests.cs @@ -0,0 +1,104 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Threading.Tasks; +using Microsoft.CommandPalette.Extensions; +using Microsoft.CommandPalette.Extensions.Toolkit; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.CmdPal.UI.ViewModels.UnitTests; + +[TestClass] +public partial class ContentPageViewModelTests +{ + private sealed partial class TestAppExtensionHost : AppExtensionHost + { + public override string? GetExtensionDisplayName() => "Test Host"; + } + + private sealed partial class TestContentPage : ContentPage + { + public override IContent[] GetContent() => []; + } + + private static CommandContextItem Command(string name) => new(new NoOpCommand { Name = name }); + + private static ContentPageViewModel CreateViewModel(TestContentPage page) => + new(page, TaskScheduler.Default, new TestAppExtensionHost(), CommandProviderContext.Empty); + + [TestMethod] + public void AllCommandsAndMoreCommands_ReturnCachedSnapshots() + { + // Content pages should expose stable snapshots, not the live Commands + // list, so repeated reads don't allocate and callers can't observe + // in-place list mutations. + var page = new TestContentPage + { + Id = "content.page", + Name = "Content Page", + Title = "Content Page", + Commands = + [ + Command("Primary"), + Command("Secondary"), + ], + }; + + var viewModel = CreateViewModel(page); + viewModel.InitializeProperties(); + + var allCommands = viewModel.AllCommands; + var moreCommands = viewModel.MoreCommands; + + Assert.AreSame(allCommands, viewModel.AllCommands); + Assert.AreSame(moreCommands, viewModel.MoreCommands); + Assert.AreEqual(2, allCommands.Count); + Assert.AreEqual(1, moreCommands.Count); + Assert.AreEqual("Primary", viewModel.PrimaryCommand?.Name); + Assert.AreEqual("Secondary", viewModel.SecondaryCommand?.Name); + } + + [TestMethod] + public void CommandsUpdate_RefreshesSnapshotsConsistently() + { + // Updating the model commands should swap in a new coherent snapshot. + // The old snapshots stay intact, and the new cached values agree on + // counts, primary/secondary commands, and "has more" state. + var page = new TestContentPage + { + Id = "content.page", + Name = "Content Page", + Title = "Content Page", + Commands = + [ + Command("Primary"), + Command("Secondary"), + ], + }; + + var viewModel = CreateViewModel(page); + viewModel.InitializeProperties(); + + var oldAllCommands = viewModel.AllCommands; + var oldMoreCommands = viewModel.MoreCommands; + + page.Commands = + [ + Command("Updated Primary"), + new Separator("Group"), + Command("Updated Secondary"), + ]; + + Assert.AreEqual(2, oldAllCommands.Count); + Assert.AreEqual(1, oldMoreCommands.Count); + + Assert.AreEqual(3, viewModel.AllCommands.Count); + Assert.AreEqual(2, viewModel.MoreCommands.Count); + Assert.IsTrue(viewModel.HasCommands); + Assert.IsTrue(viewModel.HasMoreCommands); + Assert.AreEqual("Updated Primary", viewModel.PrimaryCommand?.Name); + Assert.AreEqual("Updated Secondary", viewModel.SecondaryCommand?.Name); + Assert.AreEqual("Updated Secondary", viewModel.SecondaryCommandName); + } +}