From 466a94eb409d6cd764f0d8e542d1ca5bbc8638c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pol=C3=A1=C5=A1ek?= Date: Mon, 6 Oct 2025 16:45:10 +0200 Subject: [PATCH] CmdPal: Fix updating primary command and context menu and app icons (#42155) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request This PR fixes three issues in one go: - Restores missing icons in app context menus. - Fixes propagation of changes from a command item to the context menu item for the primary action. - Ensures the context menus stay in sync when underlying command items change. Details: - Correctly propagates updates of name, icon, and subtitle from a command item to its primary command (`CommandItemViewModel._defaultCommandContextItemViewModel`). - Correctly propagate updates of command's name to title (`CommandItem.ctor`). - Fixes icon loading for application items: `AppCommand` no longer loads an app icon by default but instead relies on the caller to provide one (since `AppListItem` also handles icon loading). - Adds a generic fallback icon for apps when an icon cannot be loaded. - Updates bindings on context menu items to `OneWay`, ensuring the UI properly reflects item changes. - Adds a sample that showcases dynamically updated commands (with cats and dolphins!) to _Samples โ†’ List Page Sample Command_. โš ๏ธ Toolkit changes: - `CommandItem` won't capture assigned Command's name as its `Title`. This will allow it to propagate future changes to `Command.Name`. Pictures? Moving ones! https://github.com/user-attachments/assets/1a482394-d222-4f7c-9922-bb67d47dc566 image ## PR Checklist - [x] Closes: #40946 - [x] Related: #40991 - [ ] **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 --- .../CommandItemViewModel.cs | 100 ++++++++++++------ .../Controls/ContextMenu.xaml | 20 ++-- .../Microsoft.CmdPal.Ext.Apps/AppCommand.cs | 17 ++- .../Microsoft.CmdPal.Ext.Apps/AppListItem.cs | 54 +++++++--- .../ext/Microsoft.CmdPal.Ext.Apps/Icons.cs | 14 +-- .../Pages/SampleListPage.cs | 55 +++++++++- .../CommandItem.cs | 9 +- 7 files changed, 187 insertions(+), 82 deletions(-) diff --git a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs index 77213f3f72..b1a977f0de 100644 --- a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs +++ b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs @@ -17,7 +17,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa public ExtensionObject Model => _commandItemModel; private readonly ExtensionObject _commandItemModel = new(null); - private CommandContextItemViewModel? _defaultCommandContextItem; + private CommandContextItemViewModel? _defaultCommandContextItemViewModel; internal InitializedState Initialized { get; private set; } = InitializedState.Uninitialized; @@ -43,9 +43,9 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa public string Subtitle { get; private set; } = string.Empty; - private IconInfoViewModel _listItemIcon = new(null); + private IconInfoViewModel _icon = new(null); - public IconInfoViewModel Icon => _listItemIcon.IsSet ? _listItemIcon : Command.Icon; + public IconInfoViewModel Icon => _icon.IsSet ? _icon : Command.Icon; public CommandViewModel Command { get; private set; } @@ -69,9 +69,9 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa { get { - List l = _defaultCommandContextItem is null ? + List l = _defaultCommandContextItemViewModel is null ? new() : - [_defaultCommandContextItem]; + [_defaultCommandContextItemViewModel]; l.AddRange(MoreCommands); return l; @@ -136,11 +136,11 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa Command.InitializeProperties(); - var listIcon = model.Icon; - if (listIcon is not null) + var icon = model.Icon; + if (icon is not null) { - _listItemIcon = new(listIcon); - _listItemIcon.InitializeProperties(); + _icon = new(icon); + _icon.InitializeProperties(); } // TODO: Do these need to go into FastInit? @@ -201,21 +201,19 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa if (!string.IsNullOrEmpty(model.Command?.Name)) { - _defaultCommandContextItem = new(new CommandContextItem(model.Command!), PageContext) + _defaultCommandContextItemViewModel = new CommandContextItemViewModel(new CommandContextItem(model.Command!), PageContext) { _itemTitle = Name, Subtitle = Subtitle, Command = Command, // TODO this probably should just be a CommandContextItemViewModel(CommandItemViewModel) ctor, or a copy ctor or whatever + // Anything we set manually here must stay in sync with the corresponding properties on CommandItemViewModel. }; // Only set the icon on the context item for us if our command didn't // have its own icon - if (!Command.HasIcon) - { - _defaultCommandContextItem._listItemIcon = _listItemIcon; - } + UpdateDefaultContextItemIcon(); } Initialized |= InitializedState.SelectionInitialized; @@ -238,7 +236,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa _itemTitle = "Error"; Subtitle = "Item failed to load"; MoreCommands = []; - _listItemIcon = _errorIcon; + _icon = _errorIcon; Initialized |= InitializedState.Error; } @@ -275,7 +273,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa _itemTitle = "Error"; Subtitle = "Item failed to load"; MoreCommands = []; - _listItemIcon = _errorIcon; + _icon = _errorIcon; Initialized |= InitializedState.Error; } @@ -305,17 +303,18 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa switch (propertyName) { case nameof(Command): - if (Command is not null) - { - Command.PropertyChanged -= Command_PropertyChanged; - } - + Command.PropertyChanged -= Command_PropertyChanged; Command = new(model.Command, PageContext); Command.InitializeProperties(); // Extensions based on Command Palette SDK < 0.3 CommandItem class won't notify when Title changes because Command // or Command.Name change. This is a workaround to ensure that the Title is always up-to-date for extensions with old SDK. _itemTitle = model.Title; + + _defaultCommandContextItemViewModel?.Command = Command; + _defaultCommandContextItemViewModel?.UpdateTitle(_itemTitle); + UpdateDefaultContextItemIcon(); + UpdateProperty(nameof(Name)); UpdateProperty(nameof(Title)); UpdateProperty(nameof(Icon)); @@ -326,12 +325,22 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa break; case nameof(Subtitle): - this.Subtitle = model.Subtitle; + var modelSubtitle = model.Subtitle; + this.Subtitle = modelSubtitle; + _defaultCommandContextItemViewModel?.Subtitle = modelSubtitle; break; case nameof(Icon): - _listItemIcon = new(model.Icon); - _listItemIcon.InitializeProperties(); + var oldIcon = _icon; + _icon = new(model.Icon); + _icon.InitializeProperties(); + if (oldIcon.IsSet || _icon.IsSet) + { + UpdateProperty(nameof(Icon)); + } + + UpdateDefaultContextItemIcon(); + break; case nameof(model.MoreCommands): @@ -378,26 +387,49 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa private void Command_PropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e) { var propertyName = e.PropertyName; + var model = _commandItemModel.Unsafe; + if (model is null) + { + return; + } + switch (propertyName) { case nameof(Command.Name): // Extensions based on Command Palette SDK < 0.3 CommandItem class won't notify when Title changes because Command // or Command.Name change. This is a workaround to ensure that the Title is always up-to-date for extensions with old SDK. - var model = _commandItemModel.Unsafe; - if (model is not null) - { - _itemTitle = model.Title; - } + _itemTitle = model.Title; + UpdateProperty(nameof(Title), nameof(Name)); - UpdateProperty(nameof(Title)); - UpdateProperty(nameof(Name)); + _defaultCommandContextItemViewModel?.UpdateTitle(model.Command.Name); break; + case nameof(Command.Icon): + UpdateDefaultContextItemIcon(); UpdateProperty(nameof(Icon)); break; } } + private void UpdateDefaultContextItemIcon() + { + // Command icon takes precedence over our icon on the primary command + _defaultCommandContextItemViewModel?.UpdateIcon(Command.Icon.IsSet ? Command.Icon : _icon); + } + + private void UpdateTitle(string? title) + { + _itemTitle = title ?? string.Empty; + UpdateProperty(nameof(Title)); + } + + private void UpdateIcon(IIconInfo? iconInfo) + { + _icon = new(iconInfo); + _icon.InitializeProperties(); + UpdateProperty(nameof(Icon)); + } + protected override void UnsafeCleanup() { base.UnsafeCleanup(); @@ -411,10 +443,10 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa } // _listItemIcon.SafeCleanup(); - _listItemIcon = new(null); // necessary? + _icon = new(null); // necessary? - _defaultCommandContextItem?.SafeCleanup(); - _defaultCommandContextItem = null; + _defaultCommandContextItemViewModel?.SafeCleanup(); + _defaultCommandContextItemViewModel = null; Command.PropertyChanged -= Command_PropertyChanged; Command.SafeCleanup(); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ContextMenu.xaml b/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ContextMenu.xaml index a64c6eae01..8e44d0f420 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ContextMenu.xaml +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ContextMenu.xaml @@ -31,7 +31,7 @@ - + @@ -42,7 +42,7 @@ Height="16" Margin="4,0,0,0" HorizontalAlignment="Left" - SourceKey="{x:Bind Icon}" + SourceKey="{x:Bind Icon, Mode=OneWay}" SourceRequested="{x:Bind help:IconCacheProvider.SourceRequested}" /> - + + Text="{x:Bind RequestedShortcut, Mode=OneWay, Converter={StaticResource KeyChordToStringConverter}}" /> - + @@ -83,7 +83,7 @@ Margin="4,0,0,0" HorizontalAlignment="Left" Foreground="{ThemeResource SystemFillColorCriticalBrush}" - SourceKey="{x:Bind Icon}" + SourceKey="{x:Bind Icon, Mode=OneWay}" SourceRequested="{x:Bind help:IconCacheProvider.SourceRequested}" /> - + + Text="{x:Bind RequestedShortcut, Mode=OneWay, Converter={StaticResource KeyChordToStringConverter}}" /> diff --git a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppCommand.cs b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppCommand.cs index 39e71f9a32..5fadf89bd6 100644 --- a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppCommand.cs +++ b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppCommand.cs @@ -16,24 +16,19 @@ using WyHash; namespace Microsoft.CmdPal.Ext.Apps; -public sealed partial class AppCommand : InvokableCommand +internal sealed partial class AppCommand : InvokableCommand { private readonly AppItem _app; public AppCommand(AppItem app) { _app = app; - - Name = Resources.run_command_action; + Name = Resources.run_command_action!; Id = GenerateId(); - - if (!string.IsNullOrEmpty(app.IcoPath)) - { - Icon = new(app.IcoPath); - } + Icon = Icons.GenericAppIcon; } - internal static async Task StartApp(string aumid) + private static async Task StartApp(string aumid) { await Task.Run(() => { @@ -58,7 +53,7 @@ public sealed partial class AppCommand : InvokableCommand }).ConfigureAwait(false); } - internal static async Task StartExe(string path) + private static async Task StartExe(string path) { await Task.Run(() => { @@ -73,7 +68,7 @@ public sealed partial class AppCommand : InvokableCommand }); } - internal async Task Launch() + private async Task Launch() { if (_app.IsPackaged) { diff --git a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppListItem.cs b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppListItem.cs index d91c195552..34f6c9c9c5 100644 --- a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppListItem.cs +++ b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/AppListItem.cs @@ -5,34 +5,51 @@ using System; using System.Collections.Generic; using System.Threading.Tasks; +using ManagedCommon; +using Microsoft.CmdPal.Core.Common.Helpers; using Microsoft.CmdPal.Ext.Apps.Commands; using Microsoft.CommandPalette.Extensions; using Microsoft.CommandPalette.Extensions.Toolkit; -using Windows.Storage.Streams; namespace Microsoft.CmdPal.Ext.Apps.Programs; internal sealed partial class AppListItem : ListItem { - private readonly AppItem _app; private static readonly Tag _appTag = new("App"); + private readonly AppCommand _appCommand; + private readonly AppItem _app; private readonly Lazy
_details; - private readonly Lazy _icon; + private readonly Lazy> _iconLoadTask; + + private InterlockedBoolean _isLoadingIcon; public override IDetails? Details { get => _details.Value; set => base.Details = value; } - public override IIconInfo? Icon { get => _icon.Value; set => base.Icon = value; } + public override IIconInfo? Icon + { + get + { + if (_isLoadingIcon.Set()) + { + _ = LoadIconAsync(); + } + + return base.Icon; + } + set => base.Icon = value; + } public string AppIdentifier => _app.AppIdentifier; public AppListItem(AppItem app, bool useThumbnails, bool isPinned) - : base(new AppCommand(app)) { + Command = _appCommand = new AppCommand(app); _app = app; Title = app.Name; Subtitle = app.Subtitle; Tags = [_appTag]; + Icon = Icons.GenericAppIcon; MoreCommands = AddPinCommands(_app.Commands!, isPinned); @@ -43,12 +60,19 @@ internal sealed partial class AppListItem : ListItem return t.Result; }); - _icon = new Lazy(() => + _iconLoadTask = new Lazy>(async () => await FetchIcon(useThumbnails)); + } + + private async Task LoadIconAsync() + { + try { - var t = FetchIcon(useThumbnails); - t.Wait(); - return t.Result; - }); + Icon = _appCommand.Icon = await _iconLoadTask.Value ?? Icons.GenericAppIcon; + } + catch (Exception ex) + { + Logger.LogWarning($"Failed to load icon for {AppIdentifier}\n{ex}"); + } } private async Task
BuildDetails() @@ -87,12 +111,12 @@ internal sealed partial class AppListItem : ListItem return new Details() { Title = this.Title, - HeroImage = heroImage ?? this.Icon ?? new IconInfo(string.Empty), + HeroImage = heroImage ?? this.Icon ?? Icons.GenericAppIcon, Metadata = metadata.ToArray(), }; } - public async Task FetchIcon(bool useThumbnails) + private async Task FetchIcon(bool useThumbnails) { IconInfo? icon = null; if (_app.IsPackaged) @@ -108,12 +132,12 @@ internal sealed partial class AppListItem : ListItem var stream = await ThumbnailHelper.GetThumbnail(_app.ExePath); if (stream is not null) { - var data = new IconData(RandomAccessStreamReference.CreateFromStream(stream)); - icon = new IconInfo(data, data); + icon = IconInfo.FromStream(stream); } } - catch + catch (Exception ex) { + Logger.LogDebug($"Failed to load icon for {AppIdentifier}:\n{ex}"); } icon = icon ?? new IconInfo(_app.IcoPath); diff --git a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/Icons.cs b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/Icons.cs index 5f4a3e7a92..47e012dcc2 100644 --- a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/Icons.cs +++ b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Apps/Icons.cs @@ -6,21 +6,23 @@ using Microsoft.CommandPalette.Extensions.Toolkit; namespace Microsoft.CmdPal.Ext.Apps; -internal sealed class Icons +internal static class Icons { - internal static IconInfo AllAppsIcon => IconHelpers.FromRelativePath("Assets\\AllApps.svg"); + internal static IconInfo AllAppsIcon { get; } = IconHelpers.FromRelativePath("Assets\\AllApps.svg"); - internal static IconInfo RunAsUserIcon => new("\uE7EE"); // OtherUser icon + internal static IconInfo RunAsUserIcon { get; } = new("\uE7EE"); // OtherUser icon - internal static IconInfo RunAsAdminIcon => new("\uE7EF"); // Admin icon + internal static IconInfo RunAsAdminIcon { get; } = new("\uE7EF"); // Admin icon - internal static IconInfo OpenPathIcon => new("\ue838"); // Folder Open icon + internal static IconInfo OpenPathIcon { get; } = new("\ue838"); // Folder Open icon - internal static IconInfo CopyIcon => new("\ue8c8"); // Copy icon + internal static IconInfo CopyIcon { get; } = new("\ue8c8"); // Copy icon public static IconInfo UnpinIcon { get; } = new("\uE77A"); // Unpin icon public static IconInfo PinIcon { get; } = new("\uE840"); // Pin icon public static IconInfo UninstallApplicationIcon { get; } = new("\uE74D"); // Uninstall icon + + public static IconInfo GenericAppIcon { get; } = new("\uE737"); // Favicon } diff --git a/src/modules/cmdpal/ext/SamplePagesExtension/Pages/SampleListPage.cs b/src/modules/cmdpal/ext/SamplePagesExtension/Pages/SampleListPage.cs index 85e9cc8083..95f5eb84c0 100644 --- a/src/modules/cmdpal/ext/SamplePagesExtension/Pages/SampleListPage.cs +++ b/src/modules/cmdpal/ext/SamplePagesExtension/Pages/SampleListPage.cs @@ -2,8 +2,9 @@ // 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.Collections.Generic; -using System.Diagnostics.CodeAnalysis; +using System.Threading; using Microsoft.CommandPalette.Extensions; using Microsoft.CommandPalette.Extensions.Toolkit; using Windows.Foundation; @@ -181,6 +182,15 @@ internal sealed partial class SampleListPage : ListPage { Title = "I also have properties", }, + new ListItem(new EverChangingCommand("Cat", "๐Ÿˆโ€โฌ›", "๐Ÿˆ")) + { + Title = "And I have a commands with changing name and icon", + MoreCommands = [ + new CommandContextItem(new EverChangingCommand("Water", "๐Ÿฌ", "๐Ÿณ", "๐ŸŸ", "๐Ÿฆˆ")), + new CommandContextItem(new EverChangingCommand("Faces", "๐Ÿ˜", "๐Ÿฅบ", "๐Ÿ˜")), + new CommandContextItem(new EverChangingCommand("Hearts", "โ™ฅ๏ธ", "๐Ÿ’š", "๐Ÿ’œ", "๐Ÿงก", "๐Ÿ’›", "๐Ÿ’™")), + ], + } ]; } @@ -229,4 +239,47 @@ internal sealed partial class SampleListPage : ListPage { "hmm?", null }, }; } + + internal sealed partial class EverChangingCommand : InvokableCommand, IDisposable + { + private readonly string[] _icons; + private readonly Timer _timer; + private readonly string _name; + private int _currentIndex; + + public EverChangingCommand(string name, params string[] icons) + { + _icons = icons ?? throw new ArgumentNullException(nameof(icons)); + if (_icons.Length == 0) + { + throw new ArgumentException("Icons array cannot be empty", nameof(icons)); + } + + _name = name; + Name = $"{_name} {DateTimeOffset.UtcNow:hh:mm:ss}"; + Icon = new IconInfo(_icons[_currentIndex]); + + // Start timer to change icon and name every 5 seconds + _timer = new Timer(OnTimerElapsed, null, TimeSpan.FromSeconds(5), TimeSpan.FromSeconds(5)); + } + + private void OnTimerElapsed(object state) + { + var nextIndex = (_currentIndex + 1) % _icons.Length; + if (nextIndex == _currentIndex && _icons.Length > 1) + { + nextIndex = (_currentIndex + 1) % _icons.Length; + } + + _currentIndex = nextIndex; + + Name = $"{_name} {DateTimeOffset.UtcNow:hh:mm:ss}"; + Icon = new IconInfo(_icons[_currentIndex]); + } + + public void Dispose() + { + _timer?.Dispose(); + } + } } diff --git a/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs b/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs index b1a0917260..cddb678fa3 100644 --- a/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs +++ b/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs @@ -63,16 +63,17 @@ public partial class CommandItem : BaseObservable, ICommandItem } OnPropertyChanged(nameof(Command)); - if (string.IsNullOrWhiteSpace(_title)) + if (string.IsNullOrEmpty(_title)) { OnPropertyChanged(nameof(Title)); } } } - private static void OnCommandPropertyChanged(CommandItem instance, object source, IPropChangedEventArgs args) + private void OnCommandPropertyChanged(CommandItem instance, object source, IPropChangedEventArgs args) { - if (args.PropertyName == nameof(ICommand.Name)) + // command's name affects Title only if Title wasn't explicitly set + if (args.PropertyName == nameof(ICommand.Name) && string.IsNullOrEmpty(_title)) { instance.OnPropertyChanged(nameof(Title)); } @@ -98,13 +99,11 @@ public partial class CommandItem : BaseObservable, ICommandItem public CommandItem(ICommand command) { Command = command; - Title = command.Name; } public CommandItem(ICommandItem other) { Command = other.Command; - Title = other.Title; Subtitle = other.Subtitle; Icon = (IconInfo?)other.Icon; MoreCommands = other.MoreCommands;