Skip to content

Lazy-fetch the settings for extensions #38844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 6, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,10 @@
DisplayName = provider.DisplayName;
Icon = new(provider.Icon);
Icon.InitializeProperties();

// Note: explicitly not InitializeProperties'ing the settings here. If

Check failure

Code scanning / check-spelling

Unrecognized Spelling

[Properties'ing](#security-tab) is not a recognized word. \(unrecognized-spelling\)
// we do that, then we'd regress GH #38321
Settings = new(provider.Settings, this, _taskScheduler);
Settings.InitializeProperties();

Logger.LogDebug($"Initialized command provider {ProviderId}");
}
Expand Down Expand Up @@ -151,9 +153,11 @@
Icon = new(model.Icon);
Icon.InitializeProperties();

// Note: explicitly not InitializeProperties'ing the settings here. If

Check failure

Code scanning / check-spelling

Unrecognized Spelling

[Properties'ing](#security-tab) is not a recognized word. \(unrecognized-spelling\)
// 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})");
Expand Down Expand Up @@ -194,21 +198,6 @@
}
}

/* 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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<ICommandSettings> _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)
Expand All @@ -27,4 +34,27 @@ public void InitializeProperties()
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public partial class ProviderSettingsViewModel(
IServiceProvider _serviceProvider) : ObservableObject
{
private readonly SettingsModel _settings = _serviceProvider.GetService<SettingsModel>()!;
private readonly Lock _initializeSettingsLock = new();

public string DisplayName => _provider.DisplayName;

Expand Down Expand Up @@ -62,9 +63,65 @@ private void Provider_CommandsChanged(CommandProviderWrapper sender, CommandPale
OnPropertyChanged(nameof(TopLevelCommands));
}

public bool HasSettings => _provider.Settings != null && _provider.Settings.SettingsPage != null;
/// <summary>
/// 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.
/// </summary>
public bool HasSettings
{
get
{
if (_provider.Settings == null)
{
return false;
}

if (_provider.Settings.Initialized)
{
return _provider.Settings.HasSettings;
}

// settings still need to be loaded.
return LoadingSettings;
}
}

/// <summary>
/// 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.
/// </summary>
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;
}
}

private Task? _initializeSettingsTask;

[ObservableProperty]
public partial bool LoadingSettings { get; set; } = _provider.Settings?.HasSettings ?? false;

[field: AllowNull]
public List<TopLevelViewModel> TopLevelCommands
Expand All @@ -90,4 +147,21 @@ private List<TopLevelViewModel> BuildTopLevelViewModels()
}

private void Save() => SettingsModel.SaveSettings(_settings);

private void InitializeSettingsPage()
{
if (_provider.Settings == null)
{
return;
}

_provider.Settings.SafeInitializeProperties();
_provider.Settings.DoOnUiThread(() =>
{
LoadingSettings = false;
OnPropertyChanged(nameof(HasSettings));
OnPropertyChanged(nameof(LoadingSettings));
OnPropertyChanged(nameof(SettingsPage));
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -44,6 +45,9 @@ public TopLevelCommandManager(IServiceProvider serviceProvider)

public async Task<bool> LoadBuiltinsAsync()
{
var s = new Stopwatch();
s.Start();

_builtInCommands.Clear();

// Load built-In commands first. These are all in-proc, and
Expand All @@ -56,6 +60,10 @@ public async Task<bool> LoadBuiltinsAsync()
await LoadTopLevelCommandsFromProvider(wrapper);
}

s.Stop();

Logger.LogDebug($"Loading built-ins took {s.ElapsedMilliseconds}ms");

return true;
}

Expand Down Expand Up @@ -239,6 +247,9 @@ private void ExtensionService_OnExtensionAdded(IExtensionService sender, IEnumer

private async Task StartExtensionsAndGetCommands(IEnumerable<IExtensionWrapper> extensions)
{
var s = new Stopwatch();
s.Start();

// TODO This most definitely needs a lock
foreach (var extension in extensions)
{
Expand All @@ -258,6 +269,10 @@ private async Task StartExtensionsAndGetCommands(IEnumerable<IExtensionWrapper>
Logger.LogError(ex.ToString());
}
}

s.Stop();

Logger.LogDebug($"Loading extensions took {s.ElapsedMilliseconds}ms");
}

private void ExtensionService_OnExtensionRemoved(IExtensionService sender, IEnumerable<IExtensionWrapper> extensions)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,24 @@
Visibility="{x:Bind ViewModel.HasSettings}" />

<Frame x:Name="SettingsFrame" Visibility="{x:Bind ViewModel.HasSettings}">
<cmdpalUI:ContentPage ViewModel="{x:Bind ViewModel.SettingsPage, Mode=OneWay}" />

<controls:SwitchPresenter
HorizontalAlignment="Stretch"
TargetType="x:Boolean"
Value="{x:Bind ViewModel.LoadingSettings, Mode=OneWay}">
<controls:Case Value="True">
<ProgressRing
Width="36"
Height="36"
HorizontalAlignment="Center"
VerticalAlignment="Center"
IsIndeterminate="True" />
</controls:Case>
<controls:Case Value="False">
<cmdpalUI:ContentPage ViewModel="{x:Bind ViewModel.SettingsPage, Mode=OneWay}" />
</controls:Case>
</controls:SwitchPresenter>

</Frame>

<TextBlock
Expand Down
Loading