From acbd438426dd2c658ea765ca0ade0dad3ae59aee Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Sun, 4 May 2025 06:09:09 -0500 Subject: [PATCH] PRE-MERGE #38844 --- .../CommandProviderWrapper.cs | 23 ++--- .../CommandSettingsViewModel.cs | 34 +++++++- .../ProviderSettingsViewModel.cs | 86 +++++++++++++++++-- .../TopLevelCommandManager.cs | 15 ++++ .../Settings/ExtensionPage.xaml | 19 +++- 5 files changed, 152 insertions(+), 25 deletions(-) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs index f1e09f6728..f0044a33d6 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs @@ -66,8 +66,10 @@ public sealed class CommandProviderWrapper DisplayName = provider.DisplayName; Icon = new(provider.Icon); Icon.InitializeProperties(); + + // Note: explicitly not InitializeProperties()ing the settings here. If + // we do that, then we'd regress GH #38321 Settings = new(provider.Settings, this, _taskScheduler); - Settings.InitializeProperties(); Logger.LogDebug($"Initialized command provider {ProviderId}"); } @@ -151,9 +153,11 @@ public sealed class CommandProviderWrapper Icon = new(model.Icon); Icon.InitializeProperties(); + // Note: explicitly not InitializeProperties()ing the settings here. If + // we do that, then we'd regress GH #38321 Settings = new(model.Settings, this, _taskScheduler); - Settings.InitializeProperties(); + // We do need to explicitly initialize commands though InitializeCommands(commands, fallbacks, serviceProvider, pageContext); Logger.LogDebug($"Loaded commands from {DisplayName} ({ProviderId})"); @@ -194,21 +198,6 @@ public sealed class CommandProviderWrapper } } - /* This is a View/ExtensionHost piece - * public void AllowSetForeground(bool allow) - { - if (!IsExtension) - { - return; - } - - var iextn = extensionWrapper?.GetExtensionObject(); - unsafe - { - PInvoke.CoAllowSetForegroundWindow(iextn); - } - }*/ - public override bool Equals(object? obj) => obj is CommandProviderWrapper wrapper && isValid == wrapper.isValid; public override int GetHashCode() => _commandProvider.GetHashCode(); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandSettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandSettingsViewModel.cs index 0853dbd932..f6cc2cd5a9 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandSettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandSettingsViewModel.cs @@ -2,18 +2,25 @@ // The Microsoft Corporation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using ManagedCommon; using Microsoft.CmdPal.UI.ViewModels.Models; using Microsoft.CommandPalette.Extensions; namespace Microsoft.CmdPal.UI.ViewModels; -public partial class CommandSettingsViewModel(ICommandSettings _unsafeSettings, CommandProviderWrapper provider, TaskScheduler mainThread) +public partial class CommandSettingsViewModel(ICommandSettings? _unsafeSettings, CommandProviderWrapper provider, TaskScheduler mainThread) { private readonly ExtensionObject _model = new(_unsafeSettings); public ContentPageViewModel? SettingsPage { get; private set; } - public void InitializeProperties() + public bool Initialized { get; private set; } + + public bool HasSettings => + _model.Unsafe != null && // We have a settings model AND + (!Initialized || SettingsPage != null); // we weren't initialized, OR we were, and we do have a settings page + + private void UnsafeInitializeProperties() { var model = _model.Unsafe; if (model == null) @@ -27,4 +34,27 @@ public partial class CommandSettingsViewModel(ICommandSettings _unsafeSettings, SettingsPage.InitializeProperties(); } } + + public void SafeInitializeProperties() + { + try + { + UnsafeInitializeProperties(); + } + catch (Exception ex) + { + Logger.LogError($"Failed to load settings page", ex: ex); + } + + Initialized = true; + } + + public void DoOnUiThread(Action action) + { + Task.Factory.StartNew( + action, + CancellationToken.None, + TaskCreationOptions.None, + mainThread); + } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs index 180d77e629..1a8a763868 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs @@ -18,6 +18,8 @@ public partial class ProviderSettingsViewModel( IServiceProvider _serviceProvider) : ObservableObject { private readonly SettingsModel _settings = _serviceProvider.GetService()!; + private readonly Lock _initializeSettingsLock = new(); + private Task? _initializeSettingsTask; public string DisplayName => _provider.DisplayName; @@ -34,6 +36,9 @@ public partial class ProviderSettingsViewModel( public IconInfoViewModel Icon => _provider.Icon; + [ObservableProperty] + public partial bool LoadingSettings { get; set; } = _provider.Settings?.HasSettings ?? false; + public bool IsEnabled { get => _providerSettings.IsEnabled; @@ -56,15 +61,60 @@ public partial class ProviderSettingsViewModel( } } - private void Provider_CommandsChanged(CommandProviderWrapper sender, CommandPalette.Extensions.IItemsChangedEventArgs args) + /// + /// Gets a value indicating whether returns true if we have a settings page + /// that's initialized, or we are still working on initializing that + /// settings page. If we don't have a settings object, or that settings + /// object doesn't have a settings page, then we'll return false. + /// + public bool HasSettings { - OnPropertyChanged(nameof(ExtensionSubtext)); - OnPropertyChanged(nameof(TopLevelCommands)); + get + { + if (_provider.Settings == null) + { + return false; + } + + if (_provider.Settings.Initialized) + { + return _provider.Settings.HasSettings; + } + + // settings still need to be loaded. + return LoadingSettings; + } } - public bool HasSettings => _provider.Settings != null && _provider.Settings.SettingsPage != null; + /// + /// Gets will return the settings page, if we have one, and have initialized it. + /// If we haven't initialized it, this will kick off a thread to start + /// initializing it. + /// + public ContentPageViewModel? SettingsPage + { + get + { + if (_provider.Settings == null) + { + return null; + } - public ContentPageViewModel? SettingsPage => HasSettings ? _provider?.Settings?.SettingsPage : null; + if (_provider.Settings.Initialized) + { + LoadingSettings = false; + return _provider.Settings.SettingsPage; + } + + // Don't load the settings if we're already working on it + lock (_initializeSettingsLock) + { + _initializeSettingsTask ??= Task.Run(InitializeSettingsPage); + } + + return null; + } + } [field: AllowNull] public List TopLevelCommands @@ -90,4 +140,30 @@ public partial class ProviderSettingsViewModel( } private void Save() => SettingsModel.SaveSettings(_settings); + + private void InitializeSettingsPage() + { + if (_provider.Settings == null) + { + return; + } + + _provider.Settings.SafeInitializeProperties(); + _provider.Settings.DoOnUiThread(() => + { + // Changing these properties will try to update XAML, and that has + // to be handled on the UI thread, so we need to raise them on the + // UI thread + LoadingSettings = false; + OnPropertyChanged(nameof(HasSettings)); + OnPropertyChanged(nameof(LoadingSettings)); + OnPropertyChanged(nameof(SettingsPage)); + }); + } + + private void Provider_CommandsChanged(CommandProviderWrapper sender, CommandPalette.Extensions.IItemsChangedEventArgs args) + { + OnPropertyChanged(nameof(ExtensionSubtext)); + OnPropertyChanged(nameof(TopLevelCommands)); + } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs index b33a515b7a..3a94777aca 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs @@ -4,6 +4,7 @@ using System.Collections.Immutable; using System.Collections.ObjectModel; +using System.Diagnostics; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using CommunityToolkit.Mvvm.Messaging; @@ -44,6 +45,9 @@ public partial class TopLevelCommandManager : ObservableObject, public async Task LoadBuiltinsAsync() { + var s = new Stopwatch(); + s.Start(); + _builtInCommands.Clear(); // Load built-In commands first. These are all in-proc, and @@ -56,6 +60,10 @@ public partial class TopLevelCommandManager : ObservableObject, await LoadTopLevelCommandsFromProvider(wrapper); } + s.Stop(); + + Logger.LogDebug($"Loading built-ins took {s.ElapsedMilliseconds}ms"); + return true; } @@ -239,6 +247,9 @@ public partial class TopLevelCommandManager : ObservableObject, private async Task StartExtensionsAndGetCommands(IEnumerable extensions) { + var s = new Stopwatch(); + s.Start(); + // TODO This most definitely needs a lock foreach (var extension in extensions) { @@ -258,6 +269,10 @@ public partial class TopLevelCommandManager : ObservableObject, Logger.LogError(ex.ToString()); } } + + s.Stop(); + + Logger.LogDebug($"Loading extensions took {s.ElapsedMilliseconds}ms"); } private void ExtensionService_OnExtensionRemoved(IExtensionService sender, IEnumerable extensions) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/Settings/ExtensionPage.xaml b/src/modules/cmdpal/Microsoft.CmdPal.UI/Settings/ExtensionPage.xaml index 53168961ce..dc297df841 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/Settings/ExtensionPage.xaml +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/Settings/ExtensionPage.xaml @@ -113,7 +113,24 @@ Visibility="{x:Bind ViewModel.HasSettings}" /> - + + + + + + + + + +