CmdPal: Fix SUI crash ; Allow extensions to be disabled (#38040)

Both `TopLevelCommandItemWrapper` and `TopLevelViewModel` were really the same thing. The latter was from an earlier prototype, and the former is a more correct, safer abstraction. We really should have only ever used the former, but alas, we only used it for the SUI, and it piggy-backed off the latter, and that meant the latter's bugs became the former's.


tldr: I made the icon access safe in the SUI. 

And while I was doing this, because we now have a cleaner VM abstraction here in the host, we can actually cleanly disable extensions, because the `CommandProviderWrapper` knows which `ViewModel`s it made. 

Closes https://github.com/zadjii-msft/PowerToys/issues/426
Closes https://github.com/zadjii-msft/PowerToys/issues/478
Closes https://github.com/zadjii-msft/PowerToys/issues/577

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandItemViewModel.cs

@@ -146,6 +146,10 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
         UpdateProperty(nameof(Title));
         UpdateProperty(nameof(Subtitle));
         UpdateProperty(nameof(Icon));
+
+        // Load-bearing: if you don't raise a IsInitialized here, then
+        // TopLevelViewModel will never know what the command's ID is, so it
+        // will never be able to load Hotkeys & aliases
         UpdateProperty(nameof(IsInitialized));
 
         Initialized |= InitializedState.Initialized;

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs

@@ -6,6 +6,8 @@ using ManagedCommon;
 using Microsoft.CmdPal.Common.Services;
 using Microsoft.CmdPal.UI.ViewModels.Models;
 using Microsoft.CommandPalette.Extensions;
+using Microsoft.Extensions.DependencyInjection;
+
 using Windows.Foundation;
 
 namespace Microsoft.CmdPal.UI.ViewModels;
@@ -20,9 +22,9 @@ public sealed class CommandProviderWrapper
 
     private readonly TaskScheduler _taskScheduler;
 
-    public ICommandItem[] TopLevelItems { get; private set; } = [];
+    public TopLevelViewModel[] TopLevelItems { get; private set; } = [];
 
-    public IFallbackCommandItem[] FallbackItems { get; private set; } = [];
+    public TopLevelViewModel[] FallbackItems { get; private set; } = [];
 
     public string DisplayName { get; private set; } = string.Empty;
 
@@ -38,7 +40,13 @@ public sealed class CommandProviderWrapper
 
     public CommandSettingsViewModel? Settings { get; private set; }
 
-    public string ProviderId => $"{Extension?.PackageFamilyName ?? string.Empty}/{Id}";
+    public string ProviderId
+    {
+        get
+        {
+            return string.IsNullOrEmpty(Extension?.ExtensionUniqueId) ? Id : Extension.ExtensionUniqueId;
+        }
+    }
 
     public CommandProviderWrapper(ICommandProvider provider, TaskScheduler mainThread)
     {
@@ -105,13 +113,25 @@ public sealed class CommandProviderWrapper
         isValid = true;
     }
 
-    public async Task LoadTopLevelCommands()
+    private ProviderSettings GetProviderSettings(SettingsModel settings)
+    {
+        return settings.GetProviderSettings(this);
+    }
+
+    public async Task LoadTopLevelCommands(IServiceProvider serviceProvider, WeakReference<IPageContext> pageContext)
     {
         if (!isValid)
         {
             return;
         }
 
+        var settings = serviceProvider.GetService<SettingsModel>()!;
+
+        if (!GetProviderSettings(settings).IsEnabled)
+        {
+            return;
+        }
+
         ICommandItem[]? commands = null;
         IFallbackCommandItem[]? fallbacks = null;
 
@@ -119,7 +139,7 @@ public sealed class CommandProviderWrapper
         {
             var model = _commandProvider.Unsafe!;
 
-            var t = new Task<ICommandItem[]>(model.TopLevelCommands);
+            Task<ICommandItem[]> t = new(model.TopLevelCommands);
             t.Start();
             commands = await t.ConfigureAwait(false);
 
@@ -134,6 +154,8 @@ public sealed class CommandProviderWrapper
             Settings = new(model.Settings, this, _taskScheduler);
             Settings.InitializeProperties();
 
+            InitializeCommands(commands, fallbacks, serviceProvider, pageContext);
+
             Logger.LogDebug($"Loaded commands from {DisplayName} ({ProviderId})");
         }
         catch (Exception e)
@@ -142,15 +164,33 @@ public sealed class CommandProviderWrapper
             Logger.LogError($"Extension was {Extension!.PackageFamilyName}");
             Logger.LogError(e.ToString());
         }
+    }
 
+    private void InitializeCommands(ICommandItem[] commands, IFallbackCommandItem[] fallbacks, IServiceProvider serviceProvider, WeakReference<IPageContext> pageContext)
+    {
+        var settings = serviceProvider.GetService<SettingsModel>()!;
+
+        Func<ICommandItem?, bool, TopLevelViewModel> makeAndAdd = (ICommandItem? i, bool fallback) =>
+        {
+            CommandItemViewModel commandItemViewModel = new(new(i), pageContext);
+            TopLevelViewModel topLevelViewModel = new(commandItemViewModel, fallback, ExtensionHost, ProviderId, settings, serviceProvider);
+
+            topLevelViewModel.ItemViewModel.SlowInitializeProperties();
+
+            return topLevelViewModel;
+        };
         if (commands != null)
         {
-            TopLevelItems = commands;
+            TopLevelItems = commands
+                .Select(c => makeAndAdd(c, false))
+                .ToArray();
         }
 
         if (fallbacks != null)
         {
-            FallbackItems = fallbacks;
+            FallbackItems = fallbacks
+                .Select(c => makeAndAdd(c, true))
+                .ToArray();
         }
     }
 

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandViewModel.cs

@@ -49,6 +49,7 @@ public partial class CommandViewModel : ExtensionObjectViewModel
             return;
         }
 
+        Id = model.Id ?? string.Empty;
         Name = model.Name ?? string.Empty;
         IsFastInitialized = true;
     }

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Commands/MainListPage.cs

@@ -73,7 +73,6 @@ public partial class MainListPage : DynamicListPage,
             {
                 return _tlcManager
                     .TopLevelCommands
-                    .Select(tlc => tlc)
                     .Where(tlc => !string.IsNullOrEmpty(tlc.Title))
                     .ToArray();
             }
@@ -167,16 +166,17 @@ public partial class MainListPage : DynamicListPage,
         var id = IdForTopLevelOrAppItem(topLevelOrAppItem);
 
         var extensionDisplayName = string.Empty;
-        if (topLevelOrAppItem is TopLevelCommandItemWrapper toplevel)
+        if (topLevelOrAppItem is TopLevelViewModel topLevel)
         {
-            isFallback = toplevel.IsFallback;
-            if (toplevel.Alias?.Alias is string alias)
+            isFallback = topLevel.IsFallback;
+            if (topLevel.HasAlias)
             {
+                var alias = topLevel.AliasText;
                 isAliasMatch = alias == query;
                 isAliasSubstringMatch = isAliasMatch || alias.StartsWith(query, StringComparison.CurrentCultureIgnoreCase);
             }
 
-            extensionDisplayName = toplevel.ExtensionHost?.Extension?.PackageDisplayName ?? string.Empty;
+            extensionDisplayName = topLevel.ExtensionHost?.Extension?.PackageDisplayName ?? string.Empty;
         }
 
         var nameMatch = StringMatcher.FuzzySearch(query, title);
@@ -221,9 +221,9 @@ public partial class MainListPage : DynamicListPage,
 
     private string IdForTopLevelOrAppItem(IListItem topLevelOrAppItem)
     {
-        if (topLevelOrAppItem is TopLevelCommandItemWrapper toplevel)
+        if (topLevelOrAppItem is TopLevelViewModel topLevel)
         {
-            return toplevel.Id;
+            return topLevel.Id;
         }
         else
         {

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/IconDataViewModel.cs

@@ -9,7 +9,7 @@ using Windows.Storage.Streams;
 
 namespace Microsoft.CmdPal.UI.ViewModels;
 
-public partial class IconDataViewModel : ObservableObject
+public partial class IconDataViewModel : ObservableObject, IIconData
 {
     private readonly ExtensionObject<IIconData> _model = new(null);
 
@@ -25,6 +25,8 @@ public partial class IconDataViewModel : ObservableObject
     // first. Hence why we're sticking this into an ExtensionObject
     public ExtensionObject<IRandomAccessStreamReference> Data { get; private set; } = new(null);
 
+    IRandomAccessStreamReference? IIconData.Data => Data.Unsafe;
+
     public IconDataViewModel(IIconData? icon)
     {
         _model = new(icon);

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/IconInfoViewModel.cs

@@ -8,7 +8,7 @@ using Microsoft.CommandPalette.Extensions;
 
 namespace Microsoft.CmdPal.UI.ViewModels;
 
-public partial class IconInfoViewModel : ObservableObject
+public partial class IconInfoViewModel : ObservableObject, IIconInfo
 {
     private readonly ExtensionObject<IIconInfo> _model = new(null);
 
@@ -28,6 +28,10 @@ public partial class IconInfoViewModel : ObservableObject
 
     public bool IsSet => _model.Unsafe != null;
 
+    IIconData? IIconInfo.Dark => Dark;
+
+    IIconData? IIconInfo.Light => Light;
+
     public IconInfoViewModel(IIconInfo? icon)
     {
         _model = new(icon);

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Messages/PerformCommandMessage.cs

@@ -18,16 +18,19 @@ public record PerformCommandMessage
 
     public bool WithAnimation { get; set; } = true;
 
+    public CommandPaletteHost? ExtensionHost { get; private set; }
+
     public PerformCommandMessage(ExtensionObject<ICommand> command)
     {
         Command = command;
         Context = null;
     }
 
-    public PerformCommandMessage(TopLevelCommandItemWrapper topLevelCommand)
+    public PerformCommandMessage(TopLevelViewModel topLevelCommand)
     {
-        Command = new(topLevelCommand.Command);
+        Command = topLevelCommand.CommandViewModel.Model;
         Context = null;
+        ExtensionHost = topLevelCommand.ExtensionHost;
     }
 
     public PerformCommandMessage(ExtensionObject<ICommand> command, ExtensionObject<IListItem> context)

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/ExtensionWrapper.cs

@@ -106,14 +106,14 @@ public class ExtensionWrapper : IExtensionWrapper
                 {
                     Logger.LogDebug($"Starting {ExtensionDisplayName} ({ExtensionClassId})");
 
-                    nint extensionPtr = nint.Zero;
+                    var extensionPtr = nint.Zero;
                     try
                     {
                         // -2147024809: E_INVALIDARG
                         // -2147467262: E_NOINTERFACE
                         // -2147024893: E_PATH_NOT_FOUND
-                        Guid guid = typeof(IExtension).GUID;
-                        global::Windows.Win32.Foundation.HRESULT hr = PInvoke.CoCreateInstance(Guid.Parse(ExtensionClassId), null, CLSCTX.CLSCTX_LOCAL_SERVER, guid, out object? extensionObj);
+                        var guid = typeof(IExtension).GUID;
+                        var hr = PInvoke.CoCreateInstance(Guid.Parse(ExtensionClassId), null, CLSCTX.CLSCTX_LOCAL_SERVER, guid, out var extensionObj);
 
                         if (hr.Value == -2147024893)
                         {
@@ -179,7 +179,7 @@ public class ExtensionWrapper : IExtensionWrapper
     {
         await StartExtensionAsync();
 
-        object? supportedProviders = GetExtensionObject()?.GetProvider(_providerTypeMap[typeof(T)]);
+        var supportedProviders = GetExtensionObject()?.GetProvider(_providerTypeMap[typeof(T)]);
         if (supportedProviders is IEnumerable<T> multipleProvidersSupported)
         {
             return multipleProvidersSupported;

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Properties/Resources.Designer.cs

@@ -303,6 +303,15 @@ namespace Microsoft.CmdPal.UI.ViewModels.Properties {
             }
         }
         
+        /// <summary>
+        ///   Looks up a localized string similar to Disabled.
+        /// </summary>
+        public static string builtin_disabled_extension {
+            get {
+                return ResourceManager.GetString("builtin_disabled_extension", resourceCulture);
+            }
+        }
+        
         /// <summary>
         ///   Looks up a localized string similar to Built-in commands.
         /// </summary>

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Properties/Resources.resx

@@ -236,4 +236,7 @@
   <data name="builtin_reload_display_title" xml:space="preserve">
     <value>Reload Command Palette extensions</value>
   </data>
+  <data name="builtin_disabled_extension" xml:space="preserve">
+    <value>Disabled</value>
+  </data>
 </root>

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettings.cs

@@ -10,23 +10,14 @@ public class ProviderSettings
 {
     public bool IsEnabled { get; set; } = true;
 
-    [JsonIgnore]
-    public string PackageFamilyName { get; set; } = string.Empty;
-
-    [JsonIgnore]
-    public string Id { get; set; } = string.Empty;
-
     [JsonIgnore]
     public string ProviderDisplayName { get; set; } = string.Empty;
 
-    // Originally, I wanted to do:
-    //    public string ProviderId => $"{PackageFamilyName}/{ProviderDisplayName}";
-    // but I think that's actually a bad idea, because the Display Name can be localized.
     [JsonIgnore]
-    public string ProviderId => $"{PackageFamilyName}/{Id}";
+    public string ProviderId { get; private set; } = string.Empty;
 
     [JsonIgnore]
-    public bool IsBuiltin => string.IsNullOrEmpty(PackageFamilyName);
+    public bool IsBuiltin { get; private set; }
 
     public ProviderSettings(CommandProviderWrapper wrapper)
     {
@@ -41,10 +32,12 @@ public class ProviderSettings
 
     public void Connect(CommandProviderWrapper wrapper)
     {
-        PackageFamilyName = wrapper.Extension?.PackageFamilyName ?? string.Empty;
-        Id = wrapper.DisplayName;
+        ProviderId = wrapper.ProviderId;
+        IsBuiltin = wrapper.Extension == null;
+
         ProviderDisplayName = wrapper.DisplayName;
-        if (ProviderId == "/")
+
+        if (string.IsNullOrEmpty(ProviderId))
         {
             throw new InvalidDataException("Did you add a built-in command and forget to set the Id? Make sure you do that!");
         }

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs

@@ -4,7 +4,10 @@
 
 using System.Diagnostics.CodeAnalysis;
 using CommunityToolkit.Mvvm.ComponentModel;
+using CommunityToolkit.Mvvm.Messaging;
 using Microsoft.CmdPal.Common.Services;
+using Microsoft.CmdPal.UI.ViewModels.Messages;
+using Microsoft.CmdPal.UI.ViewModels.Properties;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Microsoft.CmdPal.UI.ViewModels;
@@ -14,14 +17,13 @@ public partial class ProviderSettingsViewModel(
     ProviderSettings _providerSettings,
     IServiceProvider _serviceProvider) : ObservableObject
 {
-    private readonly TopLevelCommandManager _tlcManager = _serviceProvider.GetService<TopLevelCommandManager>()!;
     private readonly SettingsModel _settings = _serviceProvider.GetService<SettingsModel>()!;
 
     public string DisplayName => _provider.DisplayName;
 
     public string ExtensionName => _provider.Extension?.ExtensionDisplayName ?? "Built-in";
 
-    public string ExtensionSubtext => $"{ExtensionName}, {TopLevelCommands.Count} commands";
+    public string ExtensionSubtext => IsEnabled ? $"{ExtensionName}, {TopLevelCommands.Count} commands" : Resources.builtin_disabled_extension;
 
     [MemberNotNullWhen(true, nameof(Extension))]
     public bool IsFromExtension => _provider.Extension != null;
@@ -35,7 +37,29 @@ public partial class ProviderSettingsViewModel(
     public bool IsEnabled
     {
         get => _providerSettings.IsEnabled;
-        set => _providerSettings.IsEnabled = value;
+        set
+        {
+            if (value != _providerSettings.IsEnabled)
+            {
+                _providerSettings.IsEnabled = value;
+                Save();
+                WeakReferenceMessenger.Default.Send<ReloadCommandsMessage>(new());
+                OnPropertyChanged(nameof(IsEnabled));
+                OnPropertyChanged(nameof(ExtensionSubtext));
+            }
+
+            if (value == true)
+            {
+                _provider.CommandsChanged -= Provider_CommandsChanged;
+                _provider.CommandsChanged += Provider_CommandsChanged;
+            }
+        }
+    }
+
+    private void Provider_CommandsChanged(CommandProviderWrapper sender, CommandPalette.Extensions.IItemsChangedEventArgs args)
+    {
+        OnPropertyChanged(nameof(ExtensionSubtext));
+        OnPropertyChanged(nameof(TopLevelCommands));
     }
 
     public bool HasSettings => _provider.Settings != null && _provider.Settings.SettingsPage != null;
@@ -58,24 +82,12 @@ public partial class ProviderSettingsViewModel(
 
     private List<TopLevelViewModel> BuildTopLevelViewModels()
     {
-        var topLevelCommands = _tlcManager.TopLevelCommands;
         var thisProvider = _provider;
         var providersCommands = thisProvider.TopLevelItems;
-        List<TopLevelViewModel> results = [];
 
         // Remember! This comes in on the UI thread!
-        // TODO: GH #426
-        // Probably just do a background InitializeProperties
-        // Or better yet, merge TopLevelCommandWrapper and TopLevelViewModel
-        foreach (var command in providersCommands)
-        {
-            var match = topLevelCommands.Where(tlc => tlc.Model.Unsafe == command).FirstOrDefault();
-            if (match != null)
-            {
-                results.Add(new(match, _settings, _serviceProvider));
-            }
-        }
-
-        return results;
+        return [.. providersCommands];
     }
+
+    private void Save() => SettingsModel.SaveSettings(_settings);
 }

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsModel.cs

@@ -50,6 +50,23 @@ public partial class SettingsModel : ObservableObject
         FilePath = SettingsJsonPath();
     }
 
+    public ProviderSettings GetProviderSettings(CommandProviderWrapper provider)
+    {
+        ProviderSettings? settings;
+        if (!ProviderSettings.TryGetValue(provider.ProviderId, out settings))
+        {
+            settings = new ProviderSettings(provider);
+            settings.Connect(provider);
+            ProviderSettings[provider.ProviderId] = settings;
+        }
+        else
+        {
+            settings.Connect(provider);
+        }
+
+        return settings;
+    }
+
     public static SettingsModel LoadSettings()
     {
         if (string.IsNullOrEmpty(FilePath))

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsViewModel.cs

@@ -95,18 +95,7 @@ public partial class SettingsViewModel
 
         foreach (var item in activeProviders)
         {
-            if (!allProviderSettings.TryGetValue(item.ProviderId, out var value))
-            {
-                allProviderSettings[item.ProviderId] = new ProviderSettings(item);
-            }
-            else
-            {
-                value.Connect(item);
-            }
-
-            var providerSettings = allProviderSettings.TryGetValue(item.ProviderId, out var value2) ?
-                value2 :
-                new ProviderSettings(item);
+            var providerSettings = settings.GetProviderSettings(item);
 
             var settingsModel = new ProviderSettingsViewModel(item, providerSettings, _serviceProvider);
             CommandProviders.Add(settingsModel);

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandItemWrapper.cs

@@ -1,224 +0,0 @@
-// 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 CommunityToolkit.Mvvm.Messaging;
-using Microsoft.CmdPal.UI.ViewModels.Messages;
-using Microsoft.CmdPal.UI.ViewModels.Models;
-using Microsoft.CmdPal.UI.ViewModels.Settings;
-using Microsoft.CommandPalette.Extensions;
-using Microsoft.CommandPalette.Extensions.Toolkit;
-using Microsoft.Extensions.DependencyInjection;
-using WyHash;
-
-namespace Microsoft.CmdPal.UI.ViewModels;
-
-/// <summary>
-/// Abstraction of a top-level command. Currently owns just a live ICommandItem
-/// from an extension (or in-proc command provider), but in the future will
-/// also support stub top-level items.
-/// </summary>
-public partial class TopLevelCommandItemWrapper : ListItem
-{
-    private readonly IServiceProvider _serviceProvider;
-    private readonly string _commandProviderId;
-
-    public ExtensionObject<ICommandItem> Model { get; }
-
-    public bool IsFallback { get; private set; }
-
-    private readonly string _idFromModel = string.Empty;
-    private string _generatedId = string.Empty;
-
-    public string Id => string.IsNullOrEmpty(_idFromModel) ? _generatedId : _idFromModel;
-
-    private readonly TopLevelCommandWrapper _topLevelCommand;
-
-    public CommandAlias? Alias { get; private set; }
-
-    private HotkeySettings? _hotkey;
-
-    public HotkeySettings? Hotkey
-    {
-        get => _hotkey;
-        set
-        {
-            UpdateHotkey();
-            UpdateTags();
-        }
-    }
-
-    public CommandPaletteHost ExtensionHost { get => _topLevelCommand.ExtensionHost; }
-
-    public TopLevelCommandItemWrapper(
-        ExtensionObject<ICommandItem> commandItem,
-        bool isFallback,
-        CommandPaletteHost extensionHost,
-        string commandProviderId,
-        IServiceProvider serviceProvider)
-        : base(new TopLevelCommandWrapper(
-            commandItem.Unsafe?.Command ?? new NoOpCommand(),
-            extensionHost))
-    {
-        _serviceProvider = serviceProvider;
-        _topLevelCommand = (TopLevelCommandWrapper)this.Command!;
-        _commandProviderId = commandProviderId;
-
-        IsFallback = isFallback;
-
-        // TODO: In reality, we should do an async fetch when we're created
-        // from an extension object. Probably have an
-        // `static async Task<TopLevelCommandWrapper> FromExtension(ExtensionObject<ICommandItem>)`
-        // or a
-        // `async Task PromoteStub(ExtensionObject<ICommandItem>)`
-        Model = commandItem;
-        try
-        {
-            var model = Model.Unsafe;
-            if (model == null)
-            {
-                return;
-            }
-
-            _topLevelCommand.UnsafeInitializeProperties();
-
-            _idFromModel = _topLevelCommand.Id;
-
-            Title = model.Title;
-            Subtitle = model.Subtitle;
-            Icon = model.Icon;
-            MoreCommands = model.MoreCommands;
-
-            model.PropChanged += Model_PropChanged;
-            _topLevelCommand.PropChanged += Model_PropChanged;
-        }
-        catch (Exception ex)
-        {
-            System.Diagnostics.Debug.WriteLine(ex);
-        }
-
-        GenerateId();
-
-        UpdateAlias();
-        UpdateHotkey();
-        UpdateTags();
-    }
-
-    private void GenerateId()
-    {
-        // Use WyHash64 to generate stable ID hashes.
-        // manually seeding with 0, so that the hash is stable across launches
-        var result = WyHash64.ComputeHash64(_commandProviderId + Title + Subtitle, seed: 0);
-        _generatedId = $"{_commandProviderId}{result}";
-    }
-
-    public void UpdateAlias(CommandAlias? newAlias)
-    {
-        _serviceProvider.GetService<AliasManager>()!.UpdateAlias(Id, newAlias);
-        UpdateAlias();
-        UpdateTags();
-    }
-
-    private void UpdateAlias()
-    {
-        // Add tags for the alias, if we have one.
-        var aliases = _serviceProvider.GetService<AliasManager>();
-        if (aliases != null)
-        {
-            Alias = aliases.AliasFromId(Id);
-        }
-    }
-
-    private void UpdateHotkey()
-    {
-        var settings = _serviceProvider.GetService<SettingsModel>()!;
-        var hotkey = settings.CommandHotkeys.Where(hk => hk.CommandId == Id).FirstOrDefault();
-        if (hotkey != null)
-        {
-            _hotkey = hotkey.Hotkey;
-        }
-    }
-
-    private void UpdateTags()
-    {
-        var tags = new List<Tag>();
-
-        if (Hotkey != null)
-        {
-            tags.Add(new Tag() { Text = Hotkey.ToString() });
-        }
-
-        if (Alias != null)
-        {
-            tags.Add(new Tag() { Text = Alias.SearchPrefix });
-        }
-
-        this.Tags = tags.ToArray();
-    }
-
-    private void Model_PropChanged(object sender, IPropChangedEventArgs args)
-    {
-        try
-        {
-            var propertyName = args.PropertyName;
-            var model = Model.Unsafe;
-            if (model == null)
-            {
-                return; // throw?
-            }
-
-            switch (propertyName)
-            {
-                case nameof(_topLevelCommand.Name):
-                case nameof(Title):
-                    this.Title = model.Title;
-                    break;
-                case nameof(Subtitle):
-                    this.Subtitle = model.Subtitle;
-                    break;
-                case nameof(Icon):
-                    var listIcon = model.Icon;
-                    Icon = model.Icon;
-                    break;
-                case nameof(MoreCommands):
-                    this.MoreCommands = model.MoreCommands;
-                    break;
-                case nameof(Command):
-                    this.Command = model.Command;
-                    break;
-            }
-        }
-        catch
-        {
-        }
-    }
-
-    public void TryUpdateFallbackText(string newQuery)
-    {
-        if (!IsFallback)
-        {
-            return;
-        }
-
-        _ = Task.Run(() =>
-            {
-                try
-                {
-                    var model = Model.Unsafe;
-                    if (model is IFallbackCommandItem fallback)
-                    {
-                        var wasEmpty = string.IsNullOrEmpty(Title);
-                        fallback.FallbackHandler.UpdateQuery(newQuery);
-                        var isEmpty = string.IsNullOrEmpty(Title);
-                        if (wasEmpty != isEmpty)
-                        {
-                            WeakReferenceMessenger.Default.Send<UpdateFallbackItemsMessage>();
-                        }
-                    }
-                }
-                catch (Exception)
-                {
-                }
-            });
-    }
-}

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs

@@ -16,7 +16,8 @@ using Microsoft.Extensions.DependencyInjection;
 namespace Microsoft.CmdPal.UI.ViewModels;
 
 public partial class TopLevelCommandManager : ObservableObject,
-    IRecipient<ReloadCommandsMessage>
+    IRecipient<ReloadCommandsMessage>,
+    IPageContext
 {
     private readonly IServiceProvider _serviceProvider;
     private readonly TaskScheduler _taskScheduler;
@@ -24,6 +25,8 @@ public partial class TopLevelCommandManager : ObservableObject,
     private readonly List<CommandProviderWrapper> _builtInCommands = [];
     private readonly List<CommandProviderWrapper> _extensionCommandProviders = [];
 
+    TaskScheduler IPageContext.Scheduler => _taskScheduler;
+
     public TopLevelCommandManager(IServiceProvider serviceProvider)
     {
         _serviceProvider = serviceProvider;
@@ -31,7 +34,7 @@ public partial class TopLevelCommandManager : ObservableObject,
         WeakReferenceMessenger.Default.Register<ReloadCommandsMessage>(this);
     }
 
-    public ObservableCollection<TopLevelCommandItemWrapper> TopLevelCommands { get; set; } = [];
+    public ObservableCollection<TopLevelViewModel> TopLevelCommands { get; set; } = [];
 
     [ObservableProperty]
     public partial bool IsLoading { get; private set; } = true;
@@ -58,35 +61,43 @@ public partial class TopLevelCommandManager : ObservableObject,
     // May be called from a background thread
     private async Task LoadTopLevelCommandsFromProvider(CommandProviderWrapper commandProvider)
     {
-        await commandProvider.LoadTopLevelCommands();
+        WeakReference<IPageContext> weakSelf = new(this);
 
+        await commandProvider.LoadTopLevelCommands(_serviceProvider, weakSelf);
+
+        var settings = _serviceProvider.GetService<SettingsModel>()!;
         var makeAndAdd = (ICommandItem? i, bool fallback) =>
         {
-            TopLevelCommandItemWrapper wrapper = new(
-                new(i), fallback, commandProvider.ExtensionHost, commandProvider.ProviderId, _serviceProvider);
+            var commandItemViewModel = new CommandItemViewModel(new(i), weakSelf);
+            var topLevelViewModel = new TopLevelViewModel(commandItemViewModel, fallback, commandProvider.ExtensionHost, commandProvider.ProviderId, settings, _serviceProvider);
+
             lock (TopLevelCommands)
             {
-                TopLevelCommands.Add(wrapper);
+                TopLevelCommands.Add(topLevelViewModel);
             }
         };
 
         await Task.Factory.StartNew(
             () =>
             {
-                foreach (var i in commandProvider.TopLevelItems)
+                lock (TopLevelCommands)
                 {
-                    makeAndAdd(i, false);
-                }
+                    foreach (var item in commandProvider.TopLevelItems)
+                    {
+                        TopLevelCommands.Add(item);
+                    }
 
-                foreach (var i in commandProvider.FallbackItems)
-                {
-                    makeAndAdd(i, true);
+                    foreach (var item in commandProvider.FallbackItems)
+                    {
+                        TopLevelCommands.Add(item);
+                    }
                 }
             },
             CancellationToken.None,
             TaskCreationOptions.None,
             _taskScheduler);
 
+        commandProvider.CommandsChanged -= CommandProvider_CommandsChanged;
         commandProvider.CommandsChanged += CommandProvider_CommandsChanged;
     }
 
@@ -108,8 +119,8 @@ public partial class TopLevelCommandManager : ObservableObject,
     {
         // Work on a clone of the list, so that we can just do one atomic
         // update to the actual observable list at the end
-        List<TopLevelCommandItemWrapper> clone = [.. TopLevelCommands];
-        List<TopLevelCommandItemWrapper> newItems = [];
+        List<TopLevelViewModel> clone = [.. TopLevelCommands];
+        List<TopLevelViewModel> newItems = [];
         var startIndex = -1;
         var firstCommand = sender.TopLevelItems[0];
         var commandsToRemove = sender.TopLevelItems.Length + sender.FallbackItems.Length;
@@ -122,7 +133,8 @@ public partial class TopLevelCommandManager : ObservableObject,
             var wrapper = clone[i];
             try
             {
-                var thisCommand = wrapper.Model.Unsafe;
+                // TODO! this can be safer, we're not directly exposing ICommandItem's out of CPW anymore
+                var thisCommand = wrapper.ItemViewModel.Model.Unsafe;
                 if (thisCommand != null)
                 {
                     var isTheSame = thisCommand == firstCommand;
@@ -138,16 +150,21 @@ public partial class TopLevelCommandManager : ObservableObject,
             }
         }
 
+        WeakReference<IPageContext> weakSelf = new(this);
+
         // Fetch the new items
-        await sender.LoadTopLevelCommands();
+        await sender.LoadTopLevelCommands(_serviceProvider, weakSelf);
+
+        var settings = _serviceProvider.GetService<SettingsModel>()!;
+
         foreach (var i in sender.TopLevelItems)
         {
-            newItems.Add(new(new(i), false, sender.ExtensionHost, sender.ProviderId, _serviceProvider));
+            newItems.Add(i);
         }
 
         foreach (var i in sender.FallbackItems)
         {
-            newItems.Add(new(new(i), true, sender.ExtensionHost, sender.ProviderId, _serviceProvider));
+            newItems.Add(i);
         }
 
         // Slice out the old commands
@@ -253,7 +270,7 @@ public partial class TopLevelCommandManager : ObservableObject,
             async () =>
             {
                 // Then find all the top-level commands that belonged to that extension
-                List<TopLevelCommandItemWrapper> commandsToRemove = [];
+                List<TopLevelViewModel> commandsToRemove = [];
                 lock (TopLevelCommands)
                 {
                     foreach (var extension in extensions)
@@ -292,7 +309,7 @@ public partial class TopLevelCommandManager : ObservableObject,
             });
     }
 
-    public TopLevelCommandItemWrapper? LookupCommand(string id)
+    public TopLevelViewModel? LookupCommand(string id)
     {
         lock (TopLevelCommands)
         {
@@ -310,4 +327,10 @@ public partial class TopLevelCommandManager : ObservableObject,
 
     public void Receive(ReloadCommandsMessage message) =>
         ReloadAllCommandsAsync().ConfigureAwait(false);
+
+    void IPageContext.ShowException(Exception ex, string? extensionHint)
+    {
+        var errorMessage = $"A bug occurred in {$"the \"{extensionHint}\"" ?? "an unknown's"} extension's code:\n{ex.Message}\n{ex.Source}\n{ex.StackTrace}\n\n";
+        CommandPaletteHost.Instance.Log(errorMessage);
+    }
 }

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandWrapper.cs

@@ -1,74 +0,0 @@
-// 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 Microsoft.CmdPal.UI.ViewModels.Models;
-using Microsoft.CommandPalette.Extensions;
-using Microsoft.CommandPalette.Extensions.Toolkit;
-using Windows.Foundation;
-
-namespace Microsoft.CmdPal.UI.ViewModels;
-
-public partial class TopLevelCommandWrapper : ICommand
-{
-    private readonly ExtensionObject<ICommand> _command;
-
-    public event TypedEventHandler<object, IPropChangedEventArgs>? PropChanged;
-
-    public string Name { get; private set; } = string.Empty;
-
-    public string Id { get; private set; } = string.Empty;
-
-    public IIconInfo Icon { get; private set; } = new IconInfo(null);
-
-    public ICommand Command => _command.Unsafe!;
-
-    public CommandPaletteHost ExtensionHost { get; }
-
-    public TopLevelCommandWrapper(ICommand command, CommandPaletteHost extensionHost)
-    {
-        _command = new(command);
-        ExtensionHost = extensionHost;
-    }
-
-    public void UnsafeInitializeProperties()
-    {
-        var model = _command.Unsafe!;
-
-        Name = model.Name;
-        Id = model.Id;
-        Icon = model.Icon;
-
-        model.PropChanged += Model_PropChanged;
-        model.PropChanged += this.PropChanged;
-    }
-
-    private void Model_PropChanged(object sender, IPropChangedEventArgs args)
-    {
-        try
-        {
-            var propertyName = args.PropertyName;
-            var model = _command.Unsafe;
-            if (model == null)
-            {
-                return; // throw?
-            }
-
-            switch (propertyName)
-            {
-                case nameof(Name):
-                    this.Name = model.Name;
-                    break;
-                case nameof(Icon):
-                    var listIcon = model.Icon;
-                    Icon = model.Icon;
-                    break;
-            }
-
-            PropChanged?.Invoke(this, args);
-        }
-        catch
-        {
-        }
-    }
-}

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs

@@ -2,95 +2,267 @@
 // 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.Collections.ObjectModel;
 using CommunityToolkit.Mvvm.ComponentModel;
+using CommunityToolkit.Mvvm.Messaging;
+using Microsoft.CmdPal.UI.ViewModels.Messages;
 using Microsoft.CmdPal.UI.ViewModels.Settings;
+using Microsoft.CommandPalette.Extensions;
+using Microsoft.CommandPalette.Extensions.Toolkit;
 using Microsoft.Extensions.DependencyInjection;
+using Windows.Foundation;
+using WyHash;
 
 namespace Microsoft.CmdPal.UI.ViewModels;
 
-public sealed partial class TopLevelViewModel : ObservableObject
+public sealed partial class TopLevelViewModel : ObservableObject, IListItem
 {
     private readonly SettingsModel _settings;
     private readonly IServiceProvider _serviceProvider;
+    private readonly CommandItemViewModel _commandItemViewModel;
 
-    // TopLevelCommandItemWrapper is a ListItem, but it's in-memory for the app already.
-    // We construct it either from data that we pulled from the cache, or from the
-    // extension, but the data in it is all in our process now.
-    private readonly TopLevelCommandItemWrapper _item;
+    private readonly string _commandProviderId;
 
-    public IconInfoViewModel Icon { get; private set; }
+    private string IdFromModel => _commandItemViewModel.Command.Id;
 
-    public string Title => _item.Title;
+    private string _generatedId = string.Empty;
 
-    public string Subtitle => _item.Subtitle;
+    private HotkeySettings? _hotkey;
+
+    private CommandAlias? Alias { get; set; }
+
+    public bool IsFallback { get; private set; }
+
+    [ObservableProperty]
+    public partial ObservableCollection<Tag> Tags { get; set; } = [];
+
+    public string Id => string.IsNullOrEmpty(IdFromModel) ? _generatedId : IdFromModel;
+
+    public CommandPaletteHost ExtensionHost { get; private set; }
+
+    public CommandViewModel CommandViewModel => _commandItemViewModel.Command;
+
+    public CommandItemViewModel ItemViewModel => _commandItemViewModel;
+
+    ////// ICommandItem
+    public string Title => _commandItemViewModel.Title;
+
+    public string Subtitle => _commandItemViewModel.Subtitle;
+
+    public IIconInfo Icon => _commandItemViewModel.Icon;
+
+    ICommand? ICommandItem.Command => _commandItemViewModel.Command.Model.Unsafe;
+
+    IContextItem?[] ICommandItem.MoreCommands => _commandItemViewModel.MoreCommands.Select(i => i.Model.Unsafe).ToArray();
+
+    ////// IListItem
+    ITag[] IListItem.Tags => Tags.ToArray();
+
+    IDetails? IListItem.Details => null;
+
+    string IListItem.Section => string.Empty;
+
+    string IListItem.TextToSuggest => string.Empty;
+
+    ////// INotifyPropChanged
+    public event TypedEventHandler<object, IPropChangedEventArgs>? PropChanged;
 
     public HotkeySettings? Hotkey
     {
-        get => _item.Hotkey;
+        get => _hotkey;
         set
         {
-            _serviceProvider.GetService<HotkeyManager>()!.UpdateHotkey(_item.Id, value);
-            _item.Hotkey = value;
+            _serviceProvider.GetService<HotkeyManager>()!.UpdateHotkey(Id, value);
+            UpdateHotkey();
+            UpdateTags();
             Save();
         }
     }
 
-    private string _aliasText;
+    public bool HasAlias => !string.IsNullOrEmpty(AliasText);
 
     public string AliasText
     {
-        get => _aliasText;
+        get => Alias?.Alias ?? string.Empty;
         set
         {
-            if (SetProperty(ref _aliasText, value))
+            if (string.IsNullOrEmpty(value))
             {
-                UpdateAlias();
+                Alias = null;
             }
+            else
+            {
+                if (Alias is CommandAlias a)
+                {
+                    a.Alias = value;
+                }
+                else
+                {
+                    Alias = new CommandAlias(value, Id);
+                }
+            }
+
+            HandleChangeAlias();
         }
     }
 
-    private bool _isDirectAlias;
-
     public bool IsDirectAlias
     {
-        get => _isDirectAlias;
+        get => Alias?.IsDirect ?? false;
         set
         {
-            if (SetProperty(ref _isDirectAlias, value))
+            if (Alias is CommandAlias a)
             {
-                UpdateAlias();
+                a.IsDirect = value;
             }
+
+            HandleChangeAlias();
         }
     }
 
-    public TopLevelViewModel(TopLevelCommandItemWrapper item, SettingsModel settings, IServiceProvider serviceProvider)
+    public TopLevelViewModel(
+        CommandItemViewModel item,
+        bool isFallback,
+        CommandPaletteHost extensionHost,
+        string commandProviderId,
+        SettingsModel settings,
+        IServiceProvider serviceProvider)
     {
         _serviceProvider = serviceProvider;
         _settings = settings;
+        _commandProviderId = commandProviderId;
+        _commandItemViewModel = item;
 
-        _item = item;
-        Icon = new(item.Icon ?? item.Command?.Icon);
-        Icon.InitializeProperties();
+        IsFallback = isFallback;
+        ExtensionHost = extensionHost;
 
-        var aliases = _serviceProvider.GetService<AliasManager>()!;
-        _isDirectAlias = _item.Alias?.IsDirect ?? false;
-        _aliasText = _item.Alias?.Alias ?? string.Empty;
+        item.PropertyChanged += Item_PropertyChanged;
+
+        // UpdateAlias();
+        // UpdateHotkey();
+        // UpdateTags();
+    }
+
+    private void Item_PropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e)
+    {
+        if (!string.IsNullOrEmpty(e.PropertyName))
+        {
+            PropChanged?.Invoke(this, new PropChangedEventArgs(e.PropertyName));
+
+            if (e.PropertyName == "IsInitialized")
+            {
+                GenerateId();
+
+                UpdateAlias();
+                UpdateHotkey();
+                UpdateTags();
+            }
+        }
     }
 
     private void Save() => SettingsModel.SaveSettings(_settings);
 
-    private void UpdateAlias()
+    private void HandleChangeAlias()
     {
-        if (string.IsNullOrWhiteSpace(_aliasText))
-        {
-            _item.UpdateAlias(null);
-        }
-        else
-        {
-            var newAlias = new CommandAlias(_aliasText, _item.Id, _isDirectAlias);
-            _item.UpdateAlias(newAlias);
-        }
-
+        SetAlias(Alias);
         Save();
     }
+
+    public void SetAlias(CommandAlias? newAlias)
+    {
+        _serviceProvider.GetService<AliasManager>()!.UpdateAlias(Id, newAlias);
+        UpdateAlias();
+        UpdateTags();
+    }
+
+    private void UpdateAlias()
+    {
+        // Add tags for the alias, if we have one.
+        var aliases = _serviceProvider.GetService<AliasManager>();
+        if (aliases != null)
+        {
+            Alias = aliases.AliasFromId(Id);
+        }
+    }
+
+    private void UpdateHotkey()
+    {
+        var hotkey = _settings.CommandHotkeys.Where(hk => hk.CommandId == Id).FirstOrDefault();
+        if (hotkey != null)
+        {
+            _hotkey = hotkey.Hotkey;
+        }
+    }
+
+    private void UpdateTags()
+    {
+        List<Tag> tags = new();
+
+        if (Hotkey != null)
+        {
+            tags.Add(new Tag() { Text = Hotkey.ToString() });
+        }
+
+        if (Alias != null)
+        {
+            tags.Add(new Tag() { Text = Alias.SearchPrefix });
+        }
+
+        PropChanged?.Invoke(this, new PropChangedEventArgs(nameof(Tags)));
+
+        DoOnUiThread(
+            () =>
+            {
+                ListHelpers.InPlaceUpdateList(Tags, tags);
+            });
+    }
+
+    private void GenerateId()
+    {
+        // Use WyHash64 to generate stable ID hashes.
+        // manually seeding with 0, so that the hash is stable across launches
+        var result = WyHash64.ComputeHash64(_commandProviderId + Title + Subtitle, seed: 0);
+        _generatedId = $"{_commandProviderId}{result}";
+    }
+
+    private void DoOnUiThread(Action action)
+    {
+        if (_commandItemViewModel.PageContext.TryGetTarget(out var pageContext))
+        {
+            Task.Factory.StartNew(
+                action,
+                CancellationToken.None,
+                TaskCreationOptions.None,
+                pageContext.Scheduler);
+        }
+    }
+
+    public void TryUpdateFallbackText(string newQuery)
+    {
+        if (!IsFallback)
+        {
+            return;
+        }
+
+        _ = Task.Run(() =>
+        {
+            try
+            {
+                var model = _commandItemViewModel.Model.Unsafe;
+                if (model is IFallbackCommandItem fallback)
+                {
+                    var wasEmpty = string.IsNullOrEmpty(Title);
+                    fallback.FallbackHandler.UpdateQuery(newQuery);
+                    var isEmpty = string.IsNullOrEmpty(Title);
+                    if (wasEmpty != isEmpty)
+                    {
+                        WeakReferenceMessenger.Default.Send<UpdateFallbackItemsMessage>();
+                    }
+                }
+            }
+            catch (Exception)
+            {
+            }
+        });
+    }
 }