From 4337f8e5ffedcd930c371a73df96eb0ab752bc3c Mon Sep 17 00:00:00 2001 From: Michael Jolley Date: Fri, 27 Mar 2026 12:54:58 -0500 Subject: [PATCH] CmdPal: Make settings and app state immutable (#46451) ## Summary This PR refactors CmdPal settings/state to be immutable end-to-end. ### Core changes - Convert model types to immutable records / init-only properties: - `SettingsModel` - `AppStateModel` - `ProviderSettings` - `DockSettings` - `RecentCommandsManager` - supporting settings types (fallback/hotkey/alias/top-level hotkey/history items, etc.) - Replace mutable collections with immutable equivalents where appropriate: - `ImmutableDictionary<,>` - `ImmutableList<>` - Move mutation flow to atomic service updates: - `ISettingsService.UpdateSettings(Func)` - `IAppStateService.UpdateState(Func)` - Update ViewModels/managers/services to use copy-on-write (`with`) patterns instead of in-place mutation. - Update serialization context + tests for immutable model graph compatibility. ## Why Issue #46437 is caused by mutable shared state being updated from different execution paths/threads, leading to race-prone behavior during persistence/serialization. By making settings/app state immutable and using atomic swap/update patterns, we remove in-place mutation and eliminate this class of concurrency bug. ## Validation - Built successfully: - `Microsoft.CmdPal.UI.ViewModels` - `Microsoft.CmdPal.UI` - `Microsoft.CmdPal.UI.ViewModels.UnitTests` - Updated unit tests for immutable update patterns. Fixes #46437 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AliasManager.cs | 69 ++++--- .../AppStateModel.cs | 10 +- .../AppearanceSettingsViewModel.cs | 51 +++-- .../CommandAlias.cs | 8 +- .../CommandProviderWrapper.cs | 129 ++++++++---- .../Commands/MainListPage.cs | 7 +- .../Dock/DockBandSettingsViewModel.cs | 96 +++++---- .../Dock/DockBandViewModel.cs | 52 ++++- .../Dock/DockViewModel.cs | 186 ++++++++++-------- .../DockAppearanceSettingsViewModel.cs | 43 ++-- .../FallbackSettings.cs | 6 +- .../FallbackSettingsViewModel.cs | 33 ++-- .../HistoryItem.cs | 4 +- .../HotkeyManager.cs | 30 ++- .../ProviderSettings.cs | 65 ++++-- .../ProviderSettingsViewModel.cs | 27 ++- .../RecentCommandsManager.cs | 100 +++++----- .../Services/AppStateService.cs | 25 ++- .../Services/IAppStateService.cs | 6 + .../Services/ISettingsService.cs | 6 + .../Services/SettingsService.cs | 41 ++-- .../Settings/DockSettings.cs | 98 ++++----- .../Settings/HotkeySettings.cs | 12 +- .../SettingsModel.cs | 107 +++++----- .../SettingsViewModel.cs | 89 ++++----- .../TopLevelHotkey.cs | 12 +- .../TopLevelViewModel.cs | 4 +- .../CommandPaletteContextMenuFactory.cs | 4 +- .../ShortcutControl/ShortcutControl.xaml.cs | 12 +- .../Microsoft.CmdPal.UI/MainWindow.xaml.cs | 3 +- .../Microsoft.CmdPal.UI/RunHistoryService.cs | 28 +-- .../AppStateServiceTests.cs | 46 ++++- .../RecentCommandsTests.cs | 10 +- .../SettingsServiceTests.cs | 50 ++++- 34 files changed, 891 insertions(+), 578 deletions(-) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AliasManager.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AliasManager.cs index c1d6e4053f..64e517d01d 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AliasManager.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AliasManager.cs @@ -2,6 +2,7 @@ // 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.Immutable; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Messaging; using Microsoft.CmdPal.UI.ViewModels.Messages; @@ -14,26 +15,32 @@ public partial class AliasManager : ObservableObject private readonly TopLevelCommandManager _topLevelCommandManager; private readonly ISettingsService _settingsService; - // REMEMBER, CommandAlias.SearchPrefix is what we use as keys - private readonly Dictionary _aliases; + private static readonly ImmutableList _defaultAliases = new List + { + new CommandAlias(":", "com.microsoft.cmdpal.registry", true), + new CommandAlias("$", "com.microsoft.cmdpal.windowsSettings", true), + new CommandAlias("=", "com.microsoft.cmdpal.calculator", true), + new CommandAlias(">", "com.microsoft.cmdpal.shell", true), + new CommandAlias("<", "com.microsoft.cmdpal.windowwalker", true), + new CommandAlias("??", "com.microsoft.cmdpal.websearch", true), + new CommandAlias("file", "com.microsoft.indexer.fileSearch", false), + new CommandAlias(")", "com.microsoft.cmdpal.timedate", true), + }.ToImmutableList(); public AliasManager(TopLevelCommandManager tlcManager, ISettingsService settingsService) { _topLevelCommandManager = tlcManager; _settingsService = settingsService; - _aliases = _settingsService.Settings.Aliases; - if (_aliases.Count == 0) + if (_settingsService.Settings.Aliases.Count == 0) { PopulateDefaultAliases(); } } - private void AddAlias(CommandAlias a) => _aliases.Add(a.SearchPrefix, a); - public bool CheckAlias(string searchText) { - if (_aliases.TryGetValue(searchText, out var alias)) + if (_settingsService.Settings.Aliases.TryGetValue(searchText, out var alias)) { try { @@ -56,19 +63,18 @@ public partial class AliasManager : ObservableObject private void PopulateDefaultAliases() { - this.AddAlias(new CommandAlias(":", "com.microsoft.cmdpal.registry", true)); - this.AddAlias(new CommandAlias("$", "com.microsoft.cmdpal.windowsSettings", true)); - this.AddAlias(new CommandAlias("=", "com.microsoft.cmdpal.calculator", true)); - this.AddAlias(new CommandAlias(">", "com.microsoft.cmdpal.shell", true)); - this.AddAlias(new CommandAlias("<", "com.microsoft.cmdpal.windowwalker", true)); - this.AddAlias(new CommandAlias("??", "com.microsoft.cmdpal.websearch", true)); - this.AddAlias(new CommandAlias("file", "com.microsoft.indexer.fileSearch", false)); - this.AddAlias(new CommandAlias(")", "com.microsoft.cmdpal.timedate", true)); + _settingsService.UpdateSettings( + s => s with + { + Aliases = s.Aliases + .AddRange(_defaultAliases.ToDictionary(a => a.SearchPrefix, a => a)), + }, + hotReload: false); } public string? KeysFromId(string commandId) { - return _aliases + return _settingsService.Settings.Aliases .Where(kv => kv.Value.CommandId == commandId) .Select(kv => kv.Value.Alias) .FirstOrDefault(); @@ -76,7 +82,7 @@ public partial class AliasManager : ObservableObject public CommandAlias? AliasFromId(string commandId) { - return _aliases + return _settingsService.Settings.Aliases .Where(kv => kv.Value.CommandId == commandId) .Select(kv => kv.Value) .FirstOrDefault(); @@ -90,9 +96,11 @@ public partial class AliasManager : ObservableObject return; } + var aliases = _settingsService.Settings.Aliases; + // If we already have _this exact alias_, do nothing if (newAlias is not null && - _aliases.TryGetValue(newAlias.SearchPrefix, out var existingAlias)) + aliases.TryGetValue(newAlias.SearchPrefix, out var existingAlias)) { if (existingAlias.CommandId == commandId) { @@ -100,19 +108,19 @@ public partial class AliasManager : ObservableObject } } - List toRemove = []; - foreach (var kv in _aliases) + var keysToRemove = new List(); + foreach (var kv in aliases) { // Look for the old aliases for the command, and remove it if (kv.Value.CommandId == commandId) { - toRemove.Add(kv.Value); + keysToRemove.Add(kv.Key); } // Look for the alias belonging to another command, and remove it if (newAlias is not null && kv.Value.Alias == newAlias.Alias && kv.Value.CommandId != commandId) { - toRemove.Add(kv.Value); + keysToRemove.Add(kv.Key); // Remove alias from other TopLevelViewModels it may be assigned to var topLevelCommand = _topLevelCommandManager.LookupCommand(kv.Value.CommandId); @@ -123,15 +131,16 @@ public partial class AliasManager : ObservableObject } } - foreach (var alias in toRemove) + _settingsService.UpdateSettings(s => { - // REMEMBER, SearchPrefix is what we use as keys - _aliases.Remove(alias.SearchPrefix); - } + var updatedAliases = s.Aliases.RemoveRange(keysToRemove); - if (newAlias is not null) - { - AddAlias(newAlias); - } + if (newAlias is not null) + { + updatedAliases = updatedAliases.Add(newAlias.SearchPrefix, newAlias); + } + + return s with { Aliases = updatedAliases }; + }); } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppStateModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppStateModel.cs index 200b7cf585..ae21c73eb5 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppStateModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppStateModel.cs @@ -2,20 +2,18 @@ // 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.Text.Json.Serialization; -using CommunityToolkit.Mvvm.ComponentModel; +using System.Collections.Immutable; namespace Microsoft.CmdPal.UI.ViewModels; -public partial class AppStateModel : ObservableObject +public record AppStateModel { /////////////////////////////////////////////////////////////////////////// // STATE HERE - // Make sure that you make the setters public (JsonSerializer.Deserialize will fail silently otherwise)! // Make sure that any new types you add are added to JsonSerializationContext! - public RecentCommandsManager RecentCommands { get; set; } = new(); + public RecentCommandsManager RecentCommands { get; init; } = new(); - public List RunHistory { get; set; } = []; + public ImmutableList RunHistory { get; init; } = ImmutableList.Empty; // END SETTINGS /////////////////////////////////////////////////////////////////////////// diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppearanceSettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppearanceSettingsViewModel.cs index 941c962b86..180c7e2cdc 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppearanceSettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AppearanceSettingsViewModel.cs @@ -112,10 +112,10 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.Theme != value) { - _settingsService.Settings.Theme = value; + _settingsService.UpdateSettings(s => s with { Theme = value }); OnPropertyChanged(); OnPropertyChanged(nameof(ThemeIndex)); - Save(); + DebouncedReapply(); } } } @@ -127,7 +127,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.ColorizationMode != value) { - _settingsService.Settings.ColorizationMode = value; + _settingsService.UpdateSettings(s => s with { ColorizationMode = value }); OnPropertyChanged(); OnPropertyChanged(nameof(ColorizationModeIndex)); OnPropertyChanged(nameof(IsCustomTintVisible)); @@ -146,7 +146,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis IsColorizationDetailsExpanded = value != ColorizationMode.None; - Save(); + DebouncedReapply(); } } } @@ -164,7 +164,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.CustomThemeColor != value) { - _settingsService.Settings.CustomThemeColor = value; + _settingsService.UpdateSettings(s => s with { CustomThemeColor = value }); OnPropertyChanged(); @@ -173,7 +173,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis ColorIntensity = 100; } - Save(); + DebouncedReapply(); } } } @@ -183,10 +183,10 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis get => _settingsService.Settings.CustomThemeColorIntensity; set { - _settingsService.Settings.CustomThemeColorIntensity = value; + _settingsService.UpdateSettings(s => s with { CustomThemeColorIntensity = value }); OnPropertyChanged(); OnPropertyChanged(nameof(EffectiveTintIntensity)); - Save(); + DebouncedReapply(); } } @@ -195,10 +195,10 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis get => _settingsService.Settings.BackgroundImageTintIntensity; set { - _settingsService.Settings.BackgroundImageTintIntensity = value; + _settingsService.UpdateSettings(s => s with { BackgroundImageTintIntensity = value }); OnPropertyChanged(); OnPropertyChanged(nameof(EffectiveTintIntensity)); - Save(); + DebouncedReapply(); } } @@ -209,7 +209,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.BackgroundImagePath != value) { - _settingsService.Settings.BackgroundImagePath = value; + _settingsService.UpdateSettings(s => s with { BackgroundImagePath = value }); OnPropertyChanged(); if (BackgroundImageOpacity == 0) @@ -217,7 +217,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis BackgroundImageOpacity = 100; } - Save(); + DebouncedReapply(); } } } @@ -229,9 +229,9 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.BackgroundImageOpacity != value) { - _settingsService.Settings.BackgroundImageOpacity = value; + _settingsService.UpdateSettings(s => s with { BackgroundImageOpacity = value }); OnPropertyChanged(); - Save(); + DebouncedReapply(); } } } @@ -243,9 +243,9 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.BackgroundImageBrightness != value) { - _settingsService.Settings.BackgroundImageBrightness = value; + _settingsService.UpdateSettings(s => s with { BackgroundImageBrightness = value }); OnPropertyChanged(); - Save(); + DebouncedReapply(); } } } @@ -257,9 +257,9 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.BackgroundImageBlurAmount != value) { - _settingsService.Settings.BackgroundImageBlurAmount = value; + _settingsService.UpdateSettings(s => s with { BackgroundImageBlurAmount = value }); OnPropertyChanged(); - Save(); + DebouncedReapply(); } } } @@ -271,10 +271,10 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.BackgroundImageFit != value) { - _settingsService.Settings.BackgroundImageFit = value; + _settingsService.UpdateSettings(s => s with { BackgroundImageFit = value }); OnPropertyChanged(); OnPropertyChanged(nameof(BackgroundImageFitIndex)); - Save(); + DebouncedReapply(); } } } @@ -305,11 +305,11 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis { if (_settingsService.Settings.BackdropOpacity != value) { - _settingsService.Settings.BackdropOpacity = value; + _settingsService.UpdateSettings(s => s with { BackdropOpacity = value }); OnPropertyChanged(); OnPropertyChanged(nameof(EffectiveBackdropStyle)); OnPropertyChanged(nameof(EffectiveImageOpacity)); - Save(); + DebouncedReapply(); } } } @@ -322,7 +322,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis var newStyle = (BackdropStyle)value; if (_settingsService.Settings.BackdropStyle != newStyle) { - _settingsService.Settings.BackdropStyle = newStyle; + _settingsService.UpdateSettings(s => s with { BackdropStyle = newStyle }); OnPropertyChanged(); OnPropertyChanged(nameof(IsBackdropOpacityVisible)); @@ -335,7 +335,7 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis IsColorizationDetailsExpanded = false; } - Save(); + DebouncedReapply(); } } } @@ -468,9 +468,8 @@ public sealed partial class AppearanceSettingsViewModel : ObservableObject, IDis _saveTimer.Debounce(Reapply, TimeSpan.FromMilliseconds(200)); } - private void Save() + private void DebouncedReapply() { - _settingsService.Save(); _saveTimer.Debounce(Reapply, TimeSpan.FromMilliseconds(200)); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandAlias.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandAlias.cs index 7179ac7660..6d104b0666 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandAlias.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandAlias.cs @@ -6,13 +6,13 @@ using System.Text.Json.Serialization; namespace Microsoft.CmdPal.UI.ViewModels; -public class CommandAlias +public record CommandAlias { - public string CommandId { get; set; } + public string CommandId { get; init; } - public string Alias { get; set; } + public string Alias { get; init; } - public bool IsDirect { get; set; } + public bool IsDirect { get; init; } [JsonIgnore] public string SearchPrefix => Alias + (IsDirect ? string.Empty : " "); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs index aa46853ae0..2cf4070ee6 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs @@ -127,7 +127,12 @@ public sealed class CommandProviderWrapper : ICommandProviderContext private ProviderSettings GetProviderSettings(SettingsModel settings) { - return settings.GetProviderSettings(this); + if (!settings.ProviderSettings.TryGetValue(ProviderId, out var ps)) + { + ps = new ProviderSettings(); + } + + return ps.WithConnection(this); } public async Task LoadTopLevelCommands(IServiceProvider serviceProvider) @@ -140,9 +145,26 @@ public sealed class CommandProviderWrapper : ICommandProviderContext } var settingsService = serviceProvider.GetRequiredService(); - var settings = settingsService.Settings; + var providerSettings = GetProviderSettings(settingsService.Settings); + + // Persist the connected provider settings (fallback commands, etc.) + settingsService.UpdateSettings( + s => + { + if (!s.ProviderSettings.TryGetValue(ProviderId, out var ps)) + { + ps = new ProviderSettings(); + } + + var newPs = ps.WithConnection(this); + + return s with + { + ProviderSettings = s.ProviderSettings.SetItem(ProviderId, newPs), + }; + }, + hotReload: false); - var providerSettings = GetProviderSettings(settings); IsActive = providerSettings.IsEnabled; if (!IsActive) { @@ -419,32 +441,59 @@ public sealed class CommandProviderWrapper : ICommandProviderContext public void PinCommand(string commandId, IServiceProvider serviceProvider) { var settingsService = serviceProvider.GetRequiredService(); - var settings = settingsService.Settings; - var providerSettings = GetProviderSettings(settings); + var providerSettings = GetProviderSettings(settingsService.Settings); if (!providerSettings.PinnedCommandIds.Contains(commandId)) { - providerSettings.PinnedCommandIds.Add(commandId); + settingsService.UpdateSettings( + s => + { + if (!s.ProviderSettings.TryGetValue(ProviderId, out var ps)) + { + ps = new ProviderSettings(); + } + + var providerSettings = ps.WithConnection(this); + var newPinned = providerSettings.PinnedCommandIds.Add(commandId); + var newPs = providerSettings with { PinnedCommandIds = newPinned }; + + return s with + { + ProviderSettings = s.ProviderSettings.SetItem(ProviderId, newPs), + }; + }, + hotReload: false); // Raise CommandsChanged so the TopLevelCommandManager reloads our commands this.CommandsChanged?.Invoke(this, new ItemsChangedEventArgs(-1)); - settingsService.Save(hotReload: false); } } public void UnpinCommand(string commandId, IServiceProvider serviceProvider) { var settingsService = serviceProvider.GetRequiredService(); - var settings = settingsService.Settings; - var providerSettings = GetProviderSettings(settings); - if (providerSettings.PinnedCommandIds.Remove(commandId)) - { - // Raise CommandsChanged so the TopLevelCommandManager reloads our commands - this.CommandsChanged?.Invoke(this, new ItemsChangedEventArgs(-1)); + settingsService.UpdateSettings( + s => + { + if (!s.ProviderSettings.TryGetValue(ProviderId, out var ps)) + { + ps = new ProviderSettings(); + } - settingsService.Save(hotReload: false); - } + var providerSettings = ps.WithConnection(this); + var newPinned = providerSettings.PinnedCommandIds.Remove(commandId); + var newPs = providerSettings with { PinnedCommandIds = newPinned }; + + return s with + { + ProviderSettings = s.ProviderSettings.SetItem(ProviderId, newPs), + }; + }, + hotReload: false); + + // Raise CommandsChanged so the TopLevelCommandManager reloads our commands + this.CommandsChanged?.Invoke(this, new ItemsChangedEventArgs(-1)); } public void PinDockBand(string commandId, IServiceProvider serviceProvider, Dock.DockPinSide side = Dock.DockPinSide.Start, bool? showTitles = null, bool? showSubtitles = null) @@ -470,37 +519,47 @@ public sealed class CommandProviderWrapper : ICommandProviderContext ShowSubtitles = showSubtitles, }; - switch (side) - { - case Dock.DockPinSide.Center: - settings.DockSettings.CenterBands.Add(bandSettings); - break; - case Dock.DockPinSide.End: - settings.DockSettings.EndBands.Add(bandSettings); - break; - case Dock.DockPinSide.Start: - default: - settings.DockSettings.StartBands.Add(bandSettings); - break; - } + settingsService.UpdateSettings( + s => + { + var dockSettings = s.DockSettings; + return s with + { + DockSettings = side switch + { + Dock.DockPinSide.Center => dockSettings with { CenterBands = dockSettings.CenterBands.Add(bandSettings) }, + Dock.DockPinSide.End => dockSettings with { EndBands = dockSettings.EndBands.Add(bandSettings) }, + _ => dockSettings with { StartBands = dockSettings.StartBands.Add(bandSettings) }, + }, + }; + }, + hotReload: false); // Raise CommandsChanged so the TopLevelCommandManager reloads our commands this.CommandsChanged?.Invoke(this, new ItemsChangedEventArgs(-1)); - - settingsService.Save(hotReload: false); } public void UnpinDockBand(string commandId, IServiceProvider serviceProvider) { var settingsService = serviceProvider.GetRequiredService(); - var settings = settingsService.Settings; - settings.DockSettings.StartBands.RemoveAll(b => b.CommandId == commandId && b.ProviderId == ProviderId); - settings.DockSettings.CenterBands.RemoveAll(b => b.CommandId == commandId && b.ProviderId == ProviderId); - settings.DockSettings.EndBands.RemoveAll(b => b.CommandId == commandId && b.ProviderId == ProviderId); + settingsService.UpdateSettings( + s => + { + var dockSettings = s.DockSettings; + return s with + { + DockSettings = dockSettings with + { + StartBands = dockSettings.StartBands.RemoveAll(b => b.CommandId == commandId && b.ProviderId == ProviderId), + CenterBands = dockSettings.CenterBands.RemoveAll(b => b.CommandId == commandId && b.ProviderId == ProviderId), + EndBands = dockSettings.EndBands.RemoveAll(b => b.CommandId == commandId && b.ProviderId == ProviderId), + }, + }; + }, + hotReload: false); // Raise CommandsChanged so the TopLevelCommandManager reloads our commands this.CommandsChanged?.Invoke(this, new ItemsChangedEventArgs(-1)); - settingsService.Save(hotReload: false); } public ICommandProviderContext GetProviderContext() => this; diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Commands/MainListPage.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Commands/MainListPage.cs index b5e32b70a1..8b2623d0cf 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Commands/MainListPage.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Commands/MainListPage.cs @@ -679,9 +679,10 @@ public sealed partial class MainListPage : DynamicListPage, public void UpdateHistory(IListItem topLevelOrAppItem) { var id = IdForTopLevelOrAppItem(topLevelOrAppItem); - var history = _appStateService.State.RecentCommands; - history.AddHistoryItem(id); - _appStateService.Save(); + _appStateService.UpdateState(state => state with + { + RecentCommands = state.RecentCommands.WithHistoryItem(id), + }); } private static string IdForTopLevelOrAppItem(IListItem topLevelOrAppItem) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandSettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandSettingsViewModel.cs index 580ba3c669..5022233919 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandSettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandSettingsViewModel.cs @@ -2,6 +2,7 @@ // 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.Immutable; using System.Globalization; using System.Text; using CommunityToolkit.Mvvm.ComponentModel; @@ -14,10 +15,11 @@ public partial class DockBandSettingsViewModel : ObservableObject { private static readonly CompositeFormat PluralItemsFormatString = CompositeFormat.Parse(Properties.Resources.dock_item_count_plural); private readonly ISettingsService _settingsService; - private readonly DockBandSettings _dockSettingsModel; private readonly TopLevelViewModel _adapter; private readonly DockBandViewModel? _bandViewModel; + private DockBandSettings _dockSettingsModel; + public string Title => _adapter.Title; public string Description @@ -54,14 +56,14 @@ public partial class DockBandSettingsViewModel : ObservableObject if (value != _showLabels) { _showLabels = value; - _dockSettingsModel.ShowLabels = value switch + var newShowTitles = value switch { - ShowLabelsOption.Default => null, + ShowLabelsOption.Default => (bool?)null, ShowLabelsOption.ShowLabels => true, ShowLabelsOption.HideLabels => false, _ => null, }; - Save(); + UpdateModel(_dockSettingsModel with { ShowTitles = newShowTitles }); } } } @@ -174,9 +176,38 @@ public partial class DockBandSettingsViewModel : ObservableObject return bandVm.Items.Count; } - private void Save() + private void UpdateModel(DockBandSettings newModel) { - _settingsService.Save(); + var commandId = _dockSettingsModel.CommandId; + _settingsService.UpdateSettings( + s => + { + var dockSettings = s.DockSettings; + return s with + { + DockSettings = dockSettings with + { + StartBands = ReplaceInList(dockSettings.StartBands, commandId, newModel), + CenterBands = ReplaceInList(dockSettings.CenterBands, commandId, newModel), + EndBands = ReplaceInList(dockSettings.EndBands, commandId, newModel), + }, + }; + }, + hotReload: false); + _dockSettingsModel = newModel; + } + + private static ImmutableList ReplaceInList(ImmutableList list, string commandId, DockBandSettings newModel) + { + for (var i = 0; i < list.Count; i++) + { + if (list[i].CommandId == commandId) + { + return list.SetItem(i, newModel); + } + } + + return list; } private void UpdatePinSide(DockPinSide value) @@ -189,44 +220,31 @@ public partial class DockBandSettingsViewModel : ObservableObject public void SetBandPosition(DockPinSide side, int? index) { - var dockSettings = _settingsService.Settings.DockSettings; + var commandId = _dockSettingsModel.CommandId; - // Remove from all sides first - dockSettings.StartBands.RemoveAll(b => b.CommandId == _dockSettingsModel.CommandId); - dockSettings.CenterBands.RemoveAll(b => b.CommandId == _dockSettingsModel.CommandId); - dockSettings.EndBands.RemoveAll(b => b.CommandId == _dockSettingsModel.CommandId); - - // Add to the selected side - switch (side) + _settingsService.UpdateSettings(s => { - case DockPinSide.Start: - { - var insertIndex = index ?? dockSettings.StartBands.Count; - dockSettings.StartBands.Insert(insertIndex, _dockSettingsModel); - break; - } + var dockSettings = s.DockSettings; - case DockPinSide.Center: - { - var insertIndex = index ?? dockSettings.CenterBands.Count; - dockSettings.CenterBands.Insert(insertIndex, _dockSettingsModel); - break; - } + // Remove from all sides first + var newDock = dockSettings with + { + StartBands = dockSettings.StartBands.RemoveAll(b => b.CommandId == commandId), + CenterBands = dockSettings.CenterBands.RemoveAll(b => b.CommandId == commandId), + EndBands = dockSettings.EndBands.RemoveAll(b => b.CommandId == commandId), + }; - case DockPinSide.End: - { - var insertIndex = index ?? dockSettings.EndBands.Count; - dockSettings.EndBands.Insert(insertIndex, _dockSettingsModel); - break; - } + // Add to the selected side + newDock = side switch + { + DockPinSide.Start => newDock with { StartBands = newDock.StartBands.Insert(index ?? newDock.StartBands.Count, _dockSettingsModel) }, + DockPinSide.Center => newDock with { CenterBands = newDock.CenterBands.Insert(index ?? newDock.CenterBands.Count, _dockSettingsModel) }, + DockPinSide.End => newDock with { EndBands = newDock.EndBands.Insert(index ?? newDock.EndBands.Count, _dockSettingsModel) }, + _ => newDock, + }; - case DockPinSide.None: - default: - // Do nothing - break; - } - - Save(); + return s with { DockSettings = newDock }; + }); } private void OnPinSideChanged(DockPinSide value) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs index f20031d80a..7969c581ee 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs @@ -2,8 +2,10 @@ // 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.Immutable; using System.Collections.ObjectModel; using Microsoft.CmdPal.UI.ViewModels.Models; +using Microsoft.CmdPal.UI.ViewModels.Services; using Microsoft.CmdPal.UI.ViewModels.Settings; using Microsoft.CommandPalette.Extensions; using Microsoft.CommandPalette.Extensions.Toolkit; @@ -15,11 +17,11 @@ namespace Microsoft.CmdPal.UI.ViewModels.Dock; public sealed partial class DockBandViewModel : ExtensionObjectViewModel { private readonly CommandItemViewModel _rootItem; - private readonly DockBandSettings _bandSettings; - private readonly DockSettings _dockSettings; - private readonly Action _saveSettings; + private readonly ISettingsService _settingsService; private readonly IContextMenuFactory _contextMenuFactory; + private DockBandSettings _bandSettings; + public ObservableCollection Items { get; } = new(); private bool _showTitles = true; @@ -103,8 +105,7 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel /// internal void SaveShowLabels() { - _bandSettings.ShowTitles = _showTitles; - _bandSettings.ShowSubtitles = _showSubtitles; + ReplaceBandInSettings(_bandSettings with { ShowTitles = _showTitles, ShowSubtitles = _showSubtitles }); _showTitlesSnapshot = null; _showSubtitlesSnapshot = null; } @@ -127,21 +128,54 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel } } + private void ReplaceBandInSettings(DockBandSettings newSettings) + { + var commandId = _bandSettings.CommandId; + _settingsService.UpdateSettings( + s => + { + var dockSettings = s.DockSettings; + return s with + { + DockSettings = dockSettings with + { + StartBands = ReplaceBandInList(dockSettings.StartBands, commandId, newSettings), + CenterBands = ReplaceBandInList(dockSettings.CenterBands, commandId, newSettings), + EndBands = ReplaceBandInList(dockSettings.EndBands, commandId, newSettings), + }, + }; + }, + false); + _bandSettings = newSettings; + } + + private static ImmutableList ReplaceBandInList(ImmutableList list, string commandId, DockBandSettings newSettings) + { + for (var i = 0; i < list.Count; i++) + { + if (list[i].CommandId == commandId) + { + return list.SetItem(i, newSettings); + } + } + + return list; + } + internal DockBandViewModel( CommandItemViewModel commandItemViewModel, WeakReference errorContext, DockBandSettings settings, - DockSettings dockSettings, - Action saveSettings, + ISettingsService settingsService, IContextMenuFactory contextMenuFactory) : base(errorContext) { _rootItem = commandItemViewModel; _bandSettings = settings; - _dockSettings = dockSettings; - _saveSettings = saveSettings; + _settingsService = settingsService; _contextMenuFactory = contextMenuFactory; + var dockSettings = settingsService.Settings.DockSettings; _showTitles = settings.ResolveShowTitles(dockSettings.ShowLabels); _showSubtitles = settings.ResolveShowSubtitles(dockSettings.ShowLabels); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockViewModel.cs index 2f72efc3c3..39d507b866 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockViewModel.cs @@ -2,6 +2,7 @@ // 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.Immutable; using System.Collections.ObjectModel; using CommunityToolkit.Mvvm.Messaging; using ManagedCommon; @@ -80,7 +81,7 @@ public sealed partial class DockViewModel } private void SetupBands( - List bands, + ImmutableList bands, ObservableCollection target) { List newBands = new(); @@ -148,7 +149,7 @@ public sealed partial class DockViewModel DockBandSettings bandSettings, CommandItemViewModel commandItem) { - DockBandViewModel band = new(commandItem, commandItem.PageContext, bandSettings, _settings, SaveSettings, _contextMenuFactory); + DockBandViewModel band = new(commandItem, commandItem.PageContext, bandSettings, _settingsService, _contextMenuFactory); // the band is NOT initialized here! return band; @@ -156,7 +157,7 @@ public sealed partial class DockViewModel private void SaveSettings() { - _settingsService.Save(); + _settingsService.UpdateSettings(s => s with { DockSettings = _settings }); } public DockBandViewModel? FindBandByTopLevel(TopLevelViewModel tlc) @@ -201,7 +202,7 @@ public sealed partial class DockViewModel public void SyncBandPosition(DockBandViewModel band, DockPinSide targetSide, int targetIndex) { var bandId = band.Id; - var dockSettings = _settingsService.Settings.DockSettings; + var dockSettings = _settings; var bandSettings = dockSettings.StartBands.FirstOrDefault(b => b.CommandId == bandId) ?? dockSettings.CenterBands.FirstOrDefault(b => b.CommandId == bandId) @@ -213,20 +214,30 @@ public sealed partial class DockViewModel } // Remove from all settings lists - dockSettings.StartBands.RemoveAll(b => b.CommandId == bandId); - dockSettings.CenterBands.RemoveAll(b => b.CommandId == bandId); - dockSettings.EndBands.RemoveAll(b => b.CommandId == bandId); + var newDock = dockSettings with + { + StartBands = dockSettings.StartBands.RemoveAll(b => b.CommandId == bandId), + CenterBands = dockSettings.CenterBands.RemoveAll(b => b.CommandId == bandId), + EndBands = dockSettings.EndBands.RemoveAll(b => b.CommandId == bandId), + }; // Add to target settings list at the correct index - var targetSettings = targetSide switch + var targetList = targetSide switch { - DockPinSide.Start => dockSettings.StartBands, - DockPinSide.Center => dockSettings.CenterBands, - DockPinSide.End => dockSettings.EndBands, - _ => dockSettings.StartBands, + DockPinSide.Start => newDock.StartBands, + DockPinSide.Center => newDock.CenterBands, + DockPinSide.End => newDock.EndBands, + _ => newDock.StartBands, }; - var insertIndex = Math.Min(targetIndex, targetSettings.Count); - targetSettings.Insert(insertIndex, bandSettings); + var insertIndex = Math.Min(targetIndex, targetList.Count); + newDock = targetSide switch + { + DockPinSide.Start => newDock with { StartBands = targetList.Insert(insertIndex, bandSettings) }, + DockPinSide.Center => newDock with { CenterBands = targetList.Insert(insertIndex, bandSettings) }, + DockPinSide.End => newDock with { EndBands = targetList.Insert(insertIndex, bandSettings) }, + _ => newDock with { StartBands = targetList.Insert(insertIndex, bandSettings) }, + }; + _settings = newDock; } /// @@ -236,7 +247,7 @@ public sealed partial class DockViewModel public void MoveBandWithoutSaving(DockBandViewModel band, DockPinSide targetSide, int targetIndex) { var bandId = band.Id; - var dockSettings = _settingsService.Settings.DockSettings; + var dockSettings = _settings; var bandSettings = dockSettings.StartBands.FirstOrDefault(b => b.CommandId == bandId) ?? dockSettings.CenterBands.FirstOrDefault(b => b.CommandId == bandId) @@ -248,10 +259,15 @@ public sealed partial class DockViewModel return; } - // Remove from all sides (settings and UI) - dockSettings.StartBands.RemoveAll(b => b.CommandId == bandId); - dockSettings.CenterBands.RemoveAll(b => b.CommandId == bandId); - dockSettings.EndBands.RemoveAll(b => b.CommandId == bandId); + // Remove from all sides (settings) + var newDock = dockSettings with + { + StartBands = dockSettings.StartBands.RemoveAll(b => b.CommandId == bandId), + CenterBands = dockSettings.CenterBands.RemoveAll(b => b.CommandId == bandId), + EndBands = dockSettings.EndBands.RemoveAll(b => b.CommandId == bandId), + }; + + // Remove from UI collections StartItems.Remove(band); CenterItems.Remove(band); EndItems.Remove(band); @@ -261,8 +277,8 @@ public sealed partial class DockViewModel { case DockPinSide.Start: { - var settingsIndex = Math.Min(targetIndex, dockSettings.StartBands.Count); - dockSettings.StartBands.Insert(settingsIndex, bandSettings); + var settingsIndex = Math.Min(targetIndex, newDock.StartBands.Count); + newDock = newDock with { StartBands = newDock.StartBands.Insert(settingsIndex, bandSettings) }; var uiIndex = Math.Min(targetIndex, StartItems.Count); StartItems.Insert(uiIndex, band); @@ -271,8 +287,8 @@ public sealed partial class DockViewModel case DockPinSide.Center: { - var settingsIndex = Math.Min(targetIndex, dockSettings.CenterBands.Count); - dockSettings.CenterBands.Insert(settingsIndex, bandSettings); + var settingsIndex = Math.Min(targetIndex, newDock.CenterBands.Count); + newDock = newDock with { CenterBands = newDock.CenterBands.Insert(settingsIndex, bandSettings) }; var uiIndex = Math.Min(targetIndex, CenterItems.Count); CenterItems.Insert(uiIndex, band); @@ -281,8 +297,8 @@ public sealed partial class DockViewModel case DockPinSide.End: { - var settingsIndex = Math.Min(targetIndex, dockSettings.EndBands.Count); - dockSettings.EndBands.Insert(settingsIndex, bandSettings); + var settingsIndex = Math.Min(targetIndex, newDock.EndBands.Count); + newDock = newDock with { EndBands = newDock.EndBands.Insert(settingsIndex, bandSettings) }; var uiIndex = Math.Min(targetIndex, EndItems.Count); EndItems.Insert(uiIndex, band); @@ -290,6 +306,8 @@ public sealed partial class DockViewModel } } + _settings = newDock; + Logger.LogDebug($"Moved band {bandId} to {targetSide} at index {targetIndex} (not saved yet)"); } @@ -305,22 +323,57 @@ public sealed partial class DockViewModel band.SaveShowLabels(); } - _snapshotStartBands = null; - _snapshotCenterBands = null; - _snapshotEndBands = null; + // Preserve any per-band label edits made while in edit mode. Those edits are + // saved independently of reorder, so merge the latest band settings back into + // the local reordered snapshot before we persist dock settings. + var latestBandSettings = BuildBandSettingsLookup(_settingsService.Settings.DockSettings); + _settings = _settings with + { + StartBands = MergeBandSettings(_settings.StartBands, latestBandSettings), + CenterBands = MergeBandSettings(_settings.CenterBands, latestBandSettings), + EndBands = MergeBandSettings(_settings.EndBands, latestBandSettings), + }; + + _snapshotDockSettings = null; _snapshotBandViewModels = null; // Save without hotReload to avoid triggering SettingsChanged → SetupBands, // which could race with stale DockBands_CollectionChanged work items and // re-add bands that were just unpinned. - _settingsService.Save(hotReload: false); + _settingsService.UpdateSettings(s => s with { DockSettings = _settings }, false); _isEditing = false; Logger.LogDebug("Saved band order to settings"); } - private List? _snapshotStartBands; - private List? _snapshotCenterBands; - private List? _snapshotEndBands; + private static Dictionary BuildBandSettingsLookup(DockSettings dockSettings) + { + var lookup = new Dictionary(StringComparer.Ordinal); + foreach (var band in dockSettings.StartBands.Concat(dockSettings.CenterBands).Concat(dockSettings.EndBands)) + { + lookup[band.CommandId] = band; + } + + return lookup; + } + + private static ImmutableList MergeBandSettings( + ImmutableList targetBands, + IReadOnlyDictionary latestBandSettings) + { + var merged = targetBands; + for (var i = 0; i < merged.Count; i++) + { + var commandId = merged[i].CommandId; + if (latestBandSettings.TryGetValue(commandId, out var latestSettings)) + { + merged = merged.SetItem(i, latestSettings); + } + } + + return merged; + } + + private DockSettings? _snapshotDockSettings; private Dictionary? _snapshotBandViewModels; /// @@ -332,12 +385,14 @@ public sealed partial class DockViewModel _isEditing = true; var dockSettings = _settingsService.Settings.DockSettings; - _snapshotStartBands = dockSettings.StartBands.Select(b => b.Clone()).ToList(); - _snapshotCenterBands = dockSettings.CenterBands.Select(b => b.Clone()).ToList(); - _snapshotEndBands = dockSettings.EndBands.Select(b => b.Clone()).ToList(); + + var snapshotStartBandsCount = dockSettings.StartBands.Count; + var snapshotCenterBandsCount = dockSettings.CenterBands.Count; + var snapshotEndBandsCount = dockSettings.EndBands.Count; // Snapshot band ViewModels so we can restore unpinned bands // Use a dictionary but handle potential duplicates gracefully + _snapshotDockSettings = dockSettings; _snapshotBandViewModels = new Dictionary(); foreach (var band in StartItems.Concat(CenterItems).Concat(EndItems)) { @@ -350,7 +405,7 @@ public sealed partial class DockViewModel band.SnapshotShowLabels(); } - Logger.LogDebug($"Snapshot taken: {_snapshotStartBands.Count} start bands, {_snapshotCenterBands.Count} center bands, {_snapshotEndBands.Count} end bands"); + Logger.LogDebug($"Snapshot taken: {snapshotStartBandsCount} start bands, {snapshotCenterBandsCount} center bands, {snapshotEndBandsCount} end bands"); } /// @@ -359,9 +414,7 @@ public sealed partial class DockViewModel /// public void RestoreBandOrder() { - if (_snapshotStartBands == null || - _snapshotCenterBands == null || - _snapshotEndBands == null || _snapshotBandViewModels == null) + if (_snapshotDockSettings == null || _snapshotBandViewModels == null) { Logger.LogWarning("No snapshot to restore from"); return; @@ -373,37 +426,13 @@ public sealed partial class DockViewModel band.RestoreShowLabels(); } - var dockSettings = _settingsService.Settings.DockSettings; - - // Restore settings from snapshot - dockSettings.StartBands.Clear(); - dockSettings.CenterBands.Clear(); - dockSettings.EndBands.Clear(); - - foreach (var bandSnapshot in _snapshotStartBands) - { - var bandSettings = bandSnapshot.Clone(); - dockSettings.StartBands.Add(bandSettings); - } - - foreach (var bandSnapshot in _snapshotCenterBands) - { - var bandSettings = bandSnapshot.Clone(); - dockSettings.CenterBands.Add(bandSettings); - } - - foreach (var bandSnapshot in _snapshotEndBands) - { - var bandSettings = bandSnapshot.Clone(); - dockSettings.EndBands.Add(bandSettings); - } + // Restore settings from snapshot (immutable = just assign back) + _settings = _snapshotDockSettings; // Rebuild UI collections from restored settings using the snapshotted ViewModels RebuildUICollectionsFromSnapshot(); - _snapshotStartBands = null; - _snapshotCenterBands = null; - _snapshotEndBands = null; + _snapshotDockSettings = null; _snapshotBandViewModels = null; _isEditing = false; Logger.LogDebug("Restored band order from snapshot"); @@ -416,7 +445,7 @@ public sealed partial class DockViewModel return; } - var dockSettings = _settingsService.Settings.DockSettings; + var dockSettings = _settings; StartItems.Clear(); CenterItems.Clear(); @@ -449,7 +478,7 @@ public sealed partial class DockViewModel private void RebuildUICollections() { - var dockSettings = _settingsService.Settings.DockSettings; + var dockSettings = _settings; // Create a lookup of all current band ViewModels var allBands = StartItems.Concat(CenterItems).Concat(EndItems).ToDictionary(b => b.Id); @@ -526,7 +555,7 @@ public sealed partial class DockViewModel // Create settings for the new band var bandSettings = new DockBandSettings { ProviderId = topLevel.CommandProviderId, CommandId = bandId, ShowLabels = null }; - var dockSettings = _settingsService.Settings.DockSettings; + var dockSettings = _settings; // Create the band view model var bandVm = CreateBandItem(bandSettings, topLevel.ItemViewModel); @@ -535,15 +564,15 @@ public sealed partial class DockViewModel switch (targetSide) { case DockPinSide.Start: - dockSettings.StartBands.Add(bandSettings); + _settings = dockSettings with { StartBands = dockSettings.StartBands.Add(bandSettings) }; StartItems.Add(bandVm); break; case DockPinSide.Center: - dockSettings.CenterBands.Add(bandSettings); + _settings = dockSettings with { CenterBands = dockSettings.CenterBands.Add(bandSettings) }; CenterItems.Add(bandVm); break; case DockPinSide.End: - dockSettings.EndBands.Add(bandSettings); + _settings = dockSettings with { EndBands = dockSettings.EndBands.Add(bandSettings) }; EndItems.Add(bandVm); break; } @@ -566,12 +595,15 @@ public sealed partial class DockViewModel public void UnpinBand(DockBandViewModel band) { var bandId = band.Id; - var dockSettings = _settingsService.Settings.DockSettings; + var dockSettings = _settings; // Remove from settings - dockSettings.StartBands.RemoveAll(b => b.CommandId == bandId); - dockSettings.CenterBands.RemoveAll(b => b.CommandId == bandId); - dockSettings.EndBands.RemoveAll(b => b.CommandId == bandId); + _settings = dockSettings with + { + StartBands = dockSettings.StartBands.RemoveAll(b => b.CommandId == bandId), + CenterBands = dockSettings.CenterBands.RemoveAll(b => b.CommandId == bandId), + EndBands = dockSettings.EndBands.RemoveAll(b => b.CommandId == bandId), + }; // Remove from UI collections StartItems.Remove(band); @@ -635,7 +667,7 @@ public sealed partial class DockViewModel var isDockEnabled = _settingsService.Settings.EnableDock; var dockSide = isDockEnabled ? _settings.Side.ToString().ToLowerInvariant() : "none"; - static string FormatBands(List bands) => + static string FormatBands(ImmutableList bands) => string.Join("\n", bands.Select(b => $"{b.ProviderId}/{b.CommandId}")); var startBands = isDockEnabled ? FormatBands(_settings.StartBands) : string.Empty; diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/DockAppearanceSettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/DockAppearanceSettingsViewModel.cs index e794805efe..d4f4823ef6 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/DockAppearanceSettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/DockAppearanceSettingsViewModel.cs @@ -47,10 +47,10 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.Theme != value) { - _settingsService.Settings.DockSettings.Theme = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { Theme = value } }); OnPropertyChanged(); OnPropertyChanged(nameof(ThemeIndex)); - Save(); + DebouncedReapply(); } } } @@ -68,10 +68,10 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.Backdrop != value) { - _settingsService.Settings.DockSettings.Backdrop = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { Backdrop = value } }); OnPropertyChanged(); OnPropertyChanged(nameof(BackdropIndex)); - Save(); + DebouncedReapply(); } } } @@ -83,7 +83,7 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.ColorizationMode != value) { - _settingsService.Settings.DockSettings.ColorizationMode = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { ColorizationMode = value } }); OnPropertyChanged(); OnPropertyChanged(nameof(ColorizationModeIndex)); OnPropertyChanged(nameof(IsCustomTintVisible)); @@ -99,7 +99,7 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, IsColorizationDetailsExpanded = value != ColorizationMode.None; - Save(); + DebouncedReapply(); } } } @@ -117,7 +117,7 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.CustomThemeColor != value) { - _settingsService.Settings.DockSettings.CustomThemeColor = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { CustomThemeColor = value } }); OnPropertyChanged(); @@ -126,7 +126,7 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, ColorIntensity = 100; } - Save(); + DebouncedReapply(); } } } @@ -136,9 +136,9 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, get => _settingsService.Settings.DockSettings.CustomThemeColorIntensity; set { - _settingsService.Settings.DockSettings.CustomThemeColorIntensity = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { CustomThemeColorIntensity = value } }); OnPropertyChanged(); - Save(); + DebouncedReapply(); } } @@ -149,7 +149,7 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.BackgroundImagePath != value) { - _settingsService.Settings.DockSettings.BackgroundImagePath = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { BackgroundImagePath = value } }); OnPropertyChanged(); if (BackgroundImageOpacity == 0) @@ -157,7 +157,7 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, BackgroundImageOpacity = 100; } - Save(); + DebouncedReapply(); } } } @@ -169,9 +169,9 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.BackgroundImageOpacity != value) { - _settingsService.Settings.DockSettings.BackgroundImageOpacity = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { BackgroundImageOpacity = value } }); OnPropertyChanged(); - Save(); + DebouncedReapply(); } } } @@ -183,9 +183,9 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.BackgroundImageBrightness != value) { - _settingsService.Settings.DockSettings.BackgroundImageBrightness = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { BackgroundImageBrightness = value } }); OnPropertyChanged(); - Save(); + DebouncedReapply(); } } } @@ -197,9 +197,9 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.BackgroundImageBlurAmount != value) { - _settingsService.Settings.DockSettings.BackgroundImageBlurAmount = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { BackgroundImageBlurAmount = value } }); OnPropertyChanged(); - Save(); + DebouncedReapply(); } } } @@ -211,10 +211,10 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, { if (_settingsService.Settings.DockSettings.BackgroundImageFit != value) { - _settingsService.Settings.DockSettings.BackgroundImageFit = value; + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { BackgroundImageFit = value } }); OnPropertyChanged(); OnPropertyChanged(nameof(BackgroundImageFitIndex)); - Save(); + DebouncedReapply(); } } } @@ -298,9 +298,8 @@ public sealed partial class DockAppearanceSettingsViewModel : ObservableObject, _saveTimer.Debounce(Reapply, TimeSpan.FromMilliseconds(200)); } - private void Save() + private void DebouncedReapply() { - _settingsService.Save(); _saveTimer.Debounce(Reapply, TimeSpan.FromMilliseconds(200)); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettings.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettings.cs index 38b76957c3..7cfd3818aa 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettings.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettings.cs @@ -6,11 +6,11 @@ using System.Text.Json.Serialization; namespace Microsoft.CmdPal.UI.ViewModels; -public class FallbackSettings +public record FallbackSettings { - public bool IsEnabled { get; set; } = true; + public bool IsEnabled { get; init; } = true; - public bool IncludeInGlobalResults { get; set; } + public bool IncludeInGlobalResults { get; init; } public FallbackSettings() { diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettingsViewModel.cs index de209e0822..f0523fb508 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/FallbackSettingsViewModel.cs @@ -12,7 +12,9 @@ namespace Microsoft.CmdPal.UI.ViewModels; public partial class FallbackSettingsViewModel : ObservableObject { private readonly ISettingsService _settingsService; - private readonly FallbackSettings _fallbackSettings; + private readonly ProviderSettingsViewModel _providerSettingsViewModel; + + private FallbackSettings _fallbackSettings; public string DisplayName { get; private set; } = string.Empty; @@ -27,15 +29,18 @@ public partial class FallbackSettingsViewModel : ObservableObject { if (value != _fallbackSettings.IsEnabled) { - _fallbackSettings.IsEnabled = value; + var newSettings = _fallbackSettings with { IsEnabled = value }; - if (!_fallbackSettings.IsEnabled) + if (!newSettings.IsEnabled) { - _fallbackSettings.IncludeInGlobalResults = false; + newSettings = newSettings with { IncludeInGlobalResults = false }; } - Save(); + _fallbackSettings = newSettings; + _providerSettingsViewModel.UpdateFallbackSettings(Id, _fallbackSettings); + OnPropertyChanged(nameof(IsEnabled)); + WeakReferenceMessenger.Default.Send(new()); } } } @@ -47,15 +52,18 @@ public partial class FallbackSettingsViewModel : ObservableObject { if (value != _fallbackSettings.IncludeInGlobalResults) { - _fallbackSettings.IncludeInGlobalResults = value; + var newSettings = _fallbackSettings with { IncludeInGlobalResults = value }; - if (!_fallbackSettings.IsEnabled) + if (!newSettings.IsEnabled) { - _fallbackSettings.IsEnabled = true; + newSettings = newSettings with { IsEnabled = true }; } - Save(); + _fallbackSettings = newSettings; + _providerSettingsViewModel.UpdateFallbackSettings(Id, _fallbackSettings); + OnPropertyChanged(nameof(IncludeInGlobalResults)); + WeakReferenceMessenger.Default.Send(new()); } } } @@ -67,6 +75,7 @@ public partial class FallbackSettingsViewModel : ObservableObject ISettingsService settingsService) { _settingsService = settingsService; + _providerSettingsViewModel = providerSettings; _fallbackSettings = fallbackSettings; Id = fallback.Id; @@ -77,10 +86,4 @@ public partial class FallbackSettingsViewModel : ObservableObject Icon = new(fallback.InitialIcon); Icon.InitializeProperties(); } - - private void Save() - { - _settingsService.Save(); - WeakReferenceMessenger.Default.Send(new()); - } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HistoryItem.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HistoryItem.cs index e6ad820cd2..0f8d90acea 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HistoryItem.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HistoryItem.cs @@ -8,7 +8,7 @@ namespace Microsoft.CmdPal.UI.ViewModels; public record HistoryItem { - public required string CommandId { get; set; } + public required string CommandId { get; init; } - public required int Uses { get; set; } + public required int Uses { get; init; } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HotkeyManager.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HotkeyManager.cs index 4694664ed0..6307cc5c9e 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HotkeyManager.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/HotkeyManager.cs @@ -11,36 +11,28 @@ namespace Microsoft.CmdPal.UI.ViewModels; public partial class HotkeyManager : ObservableObject { private readonly TopLevelCommandManager _topLevelCommandManager; - private readonly List _commandHotkeys; + private readonly ISettingsService _settingsService; public HotkeyManager(TopLevelCommandManager tlcManager, ISettingsService settingsService) { _topLevelCommandManager = tlcManager; - _commandHotkeys = settingsService.Settings.CommandHotkeys; + _settingsService = settingsService; } public void UpdateHotkey(string commandId, HotkeySettings? hotkey) { - // If any of the commands were already bound to this hotkey, remove that - foreach (var item in _commandHotkeys) + _settingsService.UpdateSettings(s => { - if (item.Hotkey == hotkey) + // Remove any command already bound to this hotkey, and remove old binding for this command + var hotkeys = s.CommandHotkeys + .RemoveAll(item => item.Hotkey == hotkey || item.CommandId == commandId); + + if (hotkey is not null) { - item.Hotkey = null; + hotkeys = hotkeys.Add(new(hotkey, commandId)); } - } - _commandHotkeys.RemoveAll(item => item.Hotkey is null); - - foreach (var item in _commandHotkeys) - { - if (item.CommandId == commandId) - { - _commandHotkeys.Remove(item); - break; - } - } - - _commandHotkeys.Add(new(hotkey, commandId)); + return s with { CommandHotkeys = hotkeys }; + }); } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettings.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettings.cs index a05cfdd3b8..da79a7fe0e 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettings.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettings.cs @@ -2,37 +2,39 @@ // 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.Immutable; using System.Text.Json.Serialization; namespace Microsoft.CmdPal.UI.ViewModels; -public class ProviderSettings +public record ProviderSettings { // List of built-in fallbacks that should not have global results enabled by default - private readonly string[] _excludedBuiltInFallbacks = [ + private static readonly string[] _excludedBuiltInFallbacks = [ "com.microsoft.cmdpal.builtin.indexer.fallback", "com.microsoft.cmdpal.builtin.calculator.fallback", "com.microsoft.cmdpal.builtin.remotedesktop.fallback", ]; - public bool IsEnabled { get; set; } = true; + public bool IsEnabled { get; init; } = true; - public Dictionary FallbackCommands { get; set; } = new(); + public ImmutableDictionary FallbackCommands { get; init; } + = ImmutableDictionary.Empty; - public List PinnedCommandIds { get; set; } = []; + public ImmutableList PinnedCommandIds { get; init; } + = ImmutableList.Empty; [JsonIgnore] - public string ProviderDisplayName { get; set; } = string.Empty; + public string ProviderId { get; init; } = string.Empty; [JsonIgnore] - public string ProviderId { get; private set; } = string.Empty; + public bool IsBuiltin { get; init; } [JsonIgnore] - public bool IsBuiltin { get; private set; } + public string ProviderDisplayName { get; init; } = string.Empty; - public ProviderSettings(CommandProviderWrapper wrapper) + public ProviderSettings() { - Connect(wrapper); } [JsonConstructor] @@ -41,28 +43,51 @@ public class ProviderSettings IsEnabled = isEnabled; } - public void Connect(CommandProviderWrapper wrapper) + /// + /// Returns a new ProviderSettings connected to the given wrapper. + /// Returns when the connection produces no changes. + /// Pure function — does not mutate this instance. + /// + public ProviderSettings WithConnection(CommandProviderWrapper wrapper) { - ProviderId = wrapper.ProviderId; - IsBuiltin = wrapper.Extension is null; - - ProviderDisplayName = wrapper.DisplayName; + if (string.IsNullOrWhiteSpace(wrapper.ProviderId)) + { + throw new ArgumentException("ProviderId must not be null, empty, or whitespace.", nameof(wrapper)); + } + var changed = false; + var builder = FallbackCommands.ToBuilder(); if (wrapper.FallbackItems.Length > 0) { foreach (var fallback in wrapper.FallbackItems) { - if (!FallbackCommands.ContainsKey(fallback.Id)) + if (!string.IsNullOrEmpty(fallback.Id) && !builder.ContainsKey(fallback.Id)) { - var enableGlobalResults = IsBuiltin && !_excludedBuiltInFallbacks.Contains(fallback.Id); - FallbackCommands[fallback.Id] = new FallbackSettings(enableGlobalResults); + var enableGlobalResults = (wrapper.Extension is null) + && !_excludedBuiltInFallbacks.Contains(fallback.Id); + builder[fallback.Id] = new FallbackSettings(enableGlobalResults); + changed = true; } } } - if (string.IsNullOrEmpty(ProviderId)) + var isBuiltin = wrapper.Extension is null; + + // If nothing changed, return the same instance to avoid unnecessary allocations and saves + if (!changed + && ProviderId == wrapper.ProviderId + && IsBuiltin == isBuiltin + && ProviderDisplayName == wrapper.DisplayName) { - throw new InvalidDataException("Did you add a built-in command and forget to set the Id? Make sure you do that!"); + return this; } + + return this with + { + ProviderId = wrapper.ProviderId, + IsBuiltin = isBuiltin, + ProviderDisplayName = wrapper.DisplayName, + FallbackCommands = changed ? builder.ToImmutable() : FallbackCommands, + }; } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs index 7bcb494dd0..31088b51c8 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs @@ -22,10 +22,11 @@ public partial class ProviderSettingsViewModel : ObservableObject private static readonly CompositeFormat ExtensionSubtextDisabledFormat = CompositeFormat.Parse(Resources.builtin_extension_subtext_disabled); private readonly CommandProviderWrapper _provider; - private readonly ProviderSettings _providerSettings; private readonly ISettingsService _settingsService; private readonly Lock _initializeSettingsLock = new(); + private ProviderSettings _providerSettings; + private Task? _initializeSettingsTask; public ProviderSettingsViewModel( @@ -71,8 +72,13 @@ public partial class ProviderSettingsViewModel : ObservableObject { if (value != _providerSettings.IsEnabled) { - _providerSettings.IsEnabled = value; - Save(); + var newSettings = _providerSettings with { IsEnabled = value }; + _settingsService.UpdateSettings(s => s with + { + + ProviderSettings = s.ProviderSettings.SetItem(_provider.ProviderId, newSettings), + }); + _providerSettings = newSettings; WeakReferenceMessenger.Default.Send(new()); OnPropertyChanged(nameof(IsEnabled)); OnPropertyChanged(nameof(ExtensionSubtext)); @@ -191,7 +197,20 @@ public partial class ProviderSettingsViewModel : ObservableObject FallbackCommands = fallbackViewModels; } - private void Save() => _settingsService.Save(); + internal void UpdateFallbackSettings(string id, FallbackSettings settings) + { + var newProviderSettings = _providerSettings with + { + FallbackCommands = _providerSettings.FallbackCommands.SetItem(id, settings), + }; + _providerSettings = newProviderSettings; + _settingsService.UpdateSettings( + s => s with + { + ProviderSettings = s.ProviderSettings.SetItem(_provider.ProviderId, newProviderSettings), + }, + hotReload: false); + } private void InitializeSettingsPage() { diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/RecentCommandsManager.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/RecentCommandsManager.cs index c12d189445..da7a5711a6 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/RecentCommandsManager.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/RecentCommandsManager.cs @@ -2,17 +2,15 @@ // 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.Immutable; using System.Text.Json.Serialization; -using CommunityToolkit.Mvvm.ComponentModel; namespace Microsoft.CmdPal.UI.ViewModels; -public partial class RecentCommandsManager : ObservableObject, IRecentCommandsManager +public record RecentCommandsManager : IRecentCommandsManager { [JsonInclude] - internal List History { get; set; } = []; - - private readonly Lock _lock = new(); + internal ImmutableList History { get; init; } = ImmutableList.Empty; public RecentCommandsManager() { @@ -20,64 +18,64 @@ public partial class RecentCommandsManager : ObservableObject, IRecentCommandsMa public int GetCommandHistoryWeight(string commandId) { - lock (_lock) - { - var entry = History + var entry = History .Index() .Where(item => item.Item.CommandId == commandId) .FirstOrDefault(); - // These numbers are vaguely scaled so that "VS" will make "Visual Studio" the - // match after one use. - // Usually it has a weight of 84, compared to 109 for the VS cmd prompt - if (entry.Item is not null) + // These numbers are vaguely scaled so that "VS" will make "Visual Studio" the + // match after one use. + // Usually it has a weight of 84, compared to 109 for the VS cmd prompt + if (entry.Item is not null) + { + var index = entry.Index; + + // First, add some weight based on how early in the list this appears + var bucket = index switch { - var index = entry.Index; + _ when index <= 2 => 35, + _ when index <= 10 => 25, + _ when index <= 15 => 15, + _ when index <= 35 => 10, + _ => 5, + }; - // First, add some weight based on how early in the list this appears - var bucket = index switch - { - var i when index <= 2 => 35, - var i when index <= 10 => 25, - var i when index <= 15 => 15, - var i when index <= 35 => 10, - _ => 5, - }; + // Then, add weight for how often this is used, but cap the weight from usage. + var uses = Math.Min(entry.Item.Uses * 5, 35); - // Then, add weight for how often this is used, but cap the weight from usage. - var uses = Math.Min(entry.Item.Uses * 5, 35); - - return bucket + uses; - } - - return 0; + return bucket + uses; } + + return 0; } - public void AddHistoryItem(string commandId) + /// + /// Returns a new RecentCommandsManager with the given command added/promoted in history. + /// Pure function — does not mutate this instance. + /// + public RecentCommandsManager WithHistoryItem(string commandId) { - lock (_lock) - { - var entry = History - .Where(item => item.CommandId == commandId) - .FirstOrDefault(); - if (entry is null) - { - var newitem = new HistoryItem() { CommandId = commandId, Uses = 1 }; - History.Insert(0, newitem); - } - else - { - History.Remove(entry); - entry.Uses++; - History.Insert(0, entry); - } + var existing = History.FirstOrDefault(item => item.CommandId == commandId); + ImmutableList newHistory; - if (History.Count > 50) - { - History.RemoveRange(50, History.Count - 50); - } + if (existing is not null) + { + newHistory = History.Remove(existing); + var updated = existing with { Uses = existing.Uses + 1 }; + newHistory = newHistory.Insert(0, updated); } + else + { + var newItem = new HistoryItem { CommandId = commandId, Uses = 1 }; + newHistory = History.Insert(0, newItem); + } + + if (newHistory.Count > 50) + { + newHistory = newHistory.RemoveRange(50, newHistory.Count - 50); + } + + return this with { History = newHistory }; } } @@ -85,5 +83,5 @@ public interface IRecentCommandsManager { int GetCommandHistoryWeight(string commandId); - void AddHistoryItem(string commandId); + RecentCommandsManager WithHistoryItem(string commandId); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/AppStateService.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/AppStateService.cs index b0b250b41b..a0203e80bd 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/AppStateService.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/AppStateService.cs @@ -22,20 +22,35 @@ public sealed class AppStateService : IAppStateService _persistence = persistence; _appInfoService = appInfoService; _filePath = StateJsonPath(); - State = _persistence.Load(_filePath, JsonSerializationContext.Default.AppStateModel); + _state = _persistence.Load(_filePath, JsonSerializationContext.Default.AppStateModel); } + private AppStateModel _state; + /// - public AppStateModel State { get; private set; } + public AppStateModel State => Volatile.Read(ref _state); /// public event TypedEventHandler? StateChanged; /// - public void Save() + public void Save() => UpdateState(s => s); + + /// + public void UpdateState(Func transform) { - _persistence.Save(State, _filePath, JsonSerializationContext.Default.AppStateModel); - StateChanged?.Invoke(this, State); + AppStateModel snapshot; + AppStateModel updated; + do + { + snapshot = Volatile.Read(ref _state); + updated = transform(snapshot); + } + while (Interlocked.CompareExchange(ref _state, updated, snapshot) != snapshot); + + var newState = Volatile.Read(ref _state); + _persistence.Save(newState, _filePath, JsonSerializationContext.Default.AppStateModel); + StateChanged?.Invoke(this, newState); } private string StateJsonPath() diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/IAppStateService.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/IAppStateService.cs index 07c67be7ff..ea6c66e22b 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/IAppStateService.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/IAppStateService.cs @@ -21,6 +21,12 @@ public interface IAppStateService /// void Save(); + /// + /// Atomically applies a transformation to the current state, persists the result, + /// and raises . + /// + void UpdateState(Func transform); + /// /// Raised after state has been saved to disk. /// diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/ISettingsService.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/ISettingsService.cs index 383330caee..1f5c52f15c 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/ISettingsService.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/ISettingsService.cs @@ -22,6 +22,12 @@ public interface ISettingsService /// When , raises after saving. void Save(bool hotReload = true); + /// + /// Atomically applies a transformation to the current settings, persists the result, + /// and optionally raises . + /// + void UpdateSettings(Func transform, bool hotReload = true); + /// /// Raised after settings are saved with enabled, or after . /// diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/SettingsService.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/SettingsService.cs index 8f36d67a0f..84518a8fc0 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/SettingsService.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/SettingsService.cs @@ -29,27 +29,38 @@ public sealed class SettingsService : ISettingsService _persistence = persistence; _appInfoService = appInfoService; _filePath = SettingsJsonPath(); - Settings = _persistence.Load(_filePath, JsonSerializationContext.Default.SettingsModel); + _settings = _persistence.Load(_filePath, JsonSerializationContext.Default.SettingsModel); ApplyMigrations(); } + private SettingsModel _settings; + /// - public SettingsModel Settings { get; private set; } + public SettingsModel Settings => Volatile.Read(ref _settings); /// public event TypedEventHandler? SettingsChanged; /// - public void Save(bool hotReload = true) - { - _persistence.Save( - Settings, - _filePath, - JsonSerializationContext.Default.SettingsModel); + public void Save(bool hotReload = true) => UpdateSettings(s => s, hotReload); + /// + public void UpdateSettings(Func transform, bool hotReload = true) + { + SettingsModel snapshot; + SettingsModel updated; + do + { + snapshot = Volatile.Read(ref _settings); + updated = transform(snapshot); + } + while (Interlocked.CompareExchange(ref _settings, updated, snapshot) != snapshot); + + var newSettings = Volatile.Read(ref _settings); + _persistence.Save(newSettings, _filePath, JsonSerializationContext.Default.SettingsModel); if (hotReload) { - SettingsChanged?.Invoke(this, Settings); + SettingsChanged?.Invoke(this, newSettings); } } @@ -71,10 +82,10 @@ public sealed class SettingsService : ISettingsService migratedAny |= TryMigrate( "Migration #1: HotkeyGoesHome (bool) -> AutoGoHomeInterval (TimeSpan)", root, - Settings, + ref _settings, nameof(SettingsModel.AutoGoHomeInterval), DeprecatedHotkeyGoesHomeKey, - (model, goesHome) => model.AutoGoHomeInterval = goesHome ? TimeSpan.Zero : Timeout.InfiniteTimeSpan, + (ref SettingsModel model, bool goesHome) => model = model with { AutoGoHomeInterval = goesHome ? TimeSpan.Zero : Timeout.InfiniteTimeSpan }, JsonSerializationContext.Default.Boolean); } } @@ -89,13 +100,15 @@ public sealed class SettingsService : ISettingsService } } + private delegate void MigrationApply(ref SettingsModel model, T value); + private static bool TryMigrate( string migrationName, JsonObject root, - SettingsModel model, + ref SettingsModel model, string newKey, string oldKey, - Action apply, + MigrationApply apply, JsonTypeInfo jsonTypeInfo) { try @@ -108,7 +121,7 @@ public sealed class SettingsService : ISettingsService if (root.TryGetPropertyValue(oldKey, out var oldNode) && oldNode is not null) { var value = oldNode.Deserialize(jsonTypeInfo); - apply(model, value!); + apply(ref model, value!); return true; } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/DockSettings.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/DockSettings.cs index 5f27939c38..135115270e 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/DockSettings.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/DockSettings.cs @@ -2,6 +2,7 @@ // 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.Immutable; using System.Text.Json.Serialization; using Windows.UI; @@ -13,103 +14,93 @@ namespace Microsoft.CmdPal.UI.ViewModels.Settings; /// Settings for the Dock. These are settings for _the whole dock_. Band-specific /// settings are in . /// -public class DockSettings +public record DockSettings { - public DockSide Side { get; set; } = DockSide.Top; + public DockSide Side { get; init; } = DockSide.Top; - public DockSize DockSize { get; set; } = DockSize.Small; + public DockSize DockSize { get; init; } = DockSize.Small; - public DockSize DockIconsSize { get; set; } = DockSize.Small; + public DockSize DockIconsSize { get; init; } = DockSize.Small; // - public DockBackdrop Backdrop { get; set; } = DockBackdrop.Acrylic; + public DockBackdrop Backdrop { get; init; } = DockBackdrop.Acrylic; - public UserTheme Theme { get; set; } = UserTheme.Default; + public UserTheme Theme { get; init; } = UserTheme.Default; - public ColorizationMode ColorizationMode { get; set; } + public ColorizationMode ColorizationMode { get; init; } - public Color CustomThemeColor { get; set; } = new() { A = 0, R = 255, G = 255, B = 255 }; // Transparent — avoids WinUI3 COM dependency on Colors.Transparent and COM in class init + public Color CustomThemeColor { get; init; } = new() { A = 0, R = 255, G = 255, B = 255 }; // Transparent — avoids WinUI3 COM dependency on Colors.Transparent and COM in class init - public int CustomThemeColorIntensity { get; set; } = 100; + public int CustomThemeColorIntensity { get; init; } = 100; - public int BackgroundImageOpacity { get; set; } = 20; + public int BackgroundImageOpacity { get; init; } = 20; - public int BackgroundImageBlurAmount { get; set; } + public int BackgroundImageBlurAmount { get; init; } - public int BackgroundImageBrightness { get; set; } + public int BackgroundImageBrightness { get; init; } - public BackgroundImageFit BackgroundImageFit { get; set; } + public BackgroundImageFit BackgroundImageFit { get; init; } - public string? BackgroundImagePath { get; set; } + public string? BackgroundImagePath { get; init; } // - // public List PinnedCommands { get; set; } = []; - public List StartBands { get; set; } = []; - - public List CenterBands { get; set; } = []; - - public List EndBands { get; set; } = []; - - public bool ShowLabels { get; set; } = true; - - [JsonIgnore] - public IEnumerable<(string ProviderId, string CommandId)> AllPinnedCommands => - StartBands.Select(b => (b.ProviderId, b.CommandId)) - .Concat(CenterBands.Select(b => (b.ProviderId, b.CommandId))) - .Concat(EndBands.Select(b => (b.ProviderId, b.CommandId))); - - public DockSettings() - { - // Initialize with default values - // PinnedCommands = [ - // "com.microsoft.cmdpal.winget" - // ]; - StartBands.Add(new DockBandSettings + public ImmutableList StartBands { get; init; } = ImmutableList.Create( + new DockBandSettings { ProviderId = "com.microsoft.cmdpal.builtin.core", CommandId = "com.microsoft.cmdpal.home", - }); - StartBands.Add(new DockBandSettings + }, + new DockBandSettings { ProviderId = "WinGet", CommandId = "com.microsoft.cmdpal.winget", ShowLabels = false, }); - EndBands.Add(new DockBandSettings + public ImmutableList CenterBands { get; init; } = ImmutableList.Empty; + + public ImmutableList EndBands { get; init; } = ImmutableList.Create( + new DockBandSettings { ProviderId = "PerformanceMonitor", CommandId = "com.microsoft.cmdpal.performanceWidget", - }); - EndBands.Add(new DockBandSettings + }, + new DockBandSettings { ProviderId = "com.microsoft.cmdpal.builtin.datetime", CommandId = "com.microsoft.cmdpal.timedate.dockBand", }); - } + + public bool ShowLabels { get; init; } = true; + + [JsonIgnore] + public IEnumerable<(string ProviderId, string CommandId)> AllPinnedCommands => + StartBands.Select(b => (b.ProviderId, b.CommandId)) + .Concat(CenterBands.Select(b => (b.ProviderId, b.CommandId))) + .Concat(EndBands.Select(b => (b.ProviderId, b.CommandId))); } /// /// Settings for a specific dock band. These are per-band settings stored /// within the overall . /// -public class DockBandSettings +public record DockBandSettings { - public required string ProviderId { get; set; } + public required string ProviderId { get; init; } - public required string CommandId { get; set; } + public required string CommandId { get; init; } /// /// Gets or sets whether titles are shown for items in this band. /// If null, falls back to dock-wide ShowLabels setting. /// - public bool? ShowTitles { get; set; } + public bool? ShowTitles { get; init; } /// /// Gets or sets whether subtitles are shown for items in this band. /// If null, falls back to dock-wide ShowLabels setting. /// - public bool? ShowSubtitles { get; set; } + public bool? ShowSubtitles { get; init; } /// /// Gets or sets a value for backward compatibility. Maps to ShowTitles. @@ -118,7 +109,7 @@ public class DockBandSettings public bool? ShowLabels { get => ShowTitles; - set => ShowTitles = value; + init => ShowTitles = value; } /// @@ -134,17 +125,6 @@ public class DockBandSettings /// dock-wide setting (passed as ). /// public bool ResolveShowSubtitles(bool defaultValue) => ShowSubtitles ?? defaultValue; - - public DockBandSettings Clone() - { - return new() - { - ProviderId = this.ProviderId, - CommandId = this.CommandId, - ShowTitles = this.ShowTitles, - ShowSubtitles = this.ShowSubtitles, - }; - } } public enum DockSide diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/HotkeySettings.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/HotkeySettings.cs index c3c3a59e65..aac87bd8c4 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/HotkeySettings.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/HotkeySettings.cs @@ -39,24 +39,24 @@ public record HotkeySettings// : ICmdLineRepresentable } [JsonPropertyName("win")] - public bool Win { get; set; } + public bool Win { get; init; } [JsonPropertyName("ctrl")] - public bool Ctrl { get; set; } + public bool Ctrl { get; init; } [JsonPropertyName("alt")] - public bool Alt { get; set; } + public bool Alt { get; init; } [JsonPropertyName("shift")] - public bool Shift { get; set; } + public bool Shift { get; init; } [JsonPropertyName("code")] - public int Code { get; set; } + public int Code { get; init; } // This is currently needed for FancyZones, we need to unify these two objects // see src\common\settings_objects.h [JsonPropertyName("key")] - public string Key { get; set; } = string.Empty; + public string Key { get; init; } = string.Empty; public override string ToString() { diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsModel.cs index bc6c70b55a..cb08a22dee 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsModel.cs @@ -2,106 +2,114 @@ // 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.Immutable; using System.Text.Json.Serialization; -using CommunityToolkit.Mvvm.ComponentModel; using Microsoft.CmdPal.UI.ViewModels.Settings; using Windows.UI; namespace Microsoft.CmdPal.UI.ViewModels; -public partial class SettingsModel : ObservableObject +public record SettingsModel { /////////////////////////////////////////////////////////////////////////// // SETTINGS HERE public static HotkeySettings DefaultActivationShortcut { get; } = new HotkeySettings(true, false, true, false, 0x20); // win+alt+space - public HotkeySettings? Hotkey { get; set; } = DefaultActivationShortcut; + public HotkeySettings? Hotkey { get; init; } = DefaultActivationShortcut; - public bool UseLowLevelGlobalHotkey { get; set; } + public bool UseLowLevelGlobalHotkey { get; init; } - public bool ShowAppDetails { get; set; } + public bool ShowAppDetails { get; init; } - public bool BackspaceGoesBack { get; set; } + public bool BackspaceGoesBack { get; init; } - public bool SingleClickActivates { get; set; } + public bool SingleClickActivates { get; init; } - public bool HighlightSearchOnActivate { get; set; } = true; + public bool HighlightSearchOnActivate { get; init; } = true; - public bool KeepPreviousQuery { get; set; } + public bool KeepPreviousQuery { get; init; } - public bool ShowSystemTrayIcon { get; set; } = true; + public bool ShowSystemTrayIcon { get; init; } = true; - public bool IgnoreShortcutWhenFullscreen { get; set; } + public bool IgnoreShortcutWhenFullscreen { get; init; } - public bool AllowExternalReload { get; set; } + public bool AllowExternalReload { get; init; } - public Dictionary ProviderSettings { get; set; } = []; + public ImmutableDictionary ProviderSettings { get; init; } + = ImmutableDictionary.Empty; - public string[] FallbackRanks { get; set; } = []; + public string[] FallbackRanks { get; init; } = []; - public Dictionary Aliases { get; set; } = []; + public ImmutableDictionary Aliases { get; init; } + = ImmutableDictionary.Empty; - public List CommandHotkeys { get; set; } = []; + public ImmutableList CommandHotkeys { get; init; } + = ImmutableList.Empty; - public MonitorBehavior SummonOn { get; set; } = MonitorBehavior.ToMouse; + public MonitorBehavior SummonOn { get; init; } = MonitorBehavior.ToMouse; - public bool DisableAnimations { get; set; } = true; + public bool DisableAnimations { get; init; } = true; - public WindowPosition? LastWindowPosition { get; set; } + public WindowPosition? LastWindowPosition { get; init; } - public TimeSpan AutoGoHomeInterval { get; set; } = Timeout.InfiniteTimeSpan; + public TimeSpan AutoGoHomeInterval { get; init; } = Timeout.InfiniteTimeSpan; - public EscapeKeyBehavior EscapeKeyBehaviorSetting { get; set; } = EscapeKeyBehavior.ClearSearchFirstThenGoBack; + public EscapeKeyBehavior EscapeKeyBehaviorSetting { get; init; } = EscapeKeyBehavior.ClearSearchFirstThenGoBack; - public bool EnableDock { get; set; } + public bool EnableDock { get; init; } - public DockSettings DockSettings { get; set; } = new(); + public DockSettings DockSettings { get; init; } = new(); // Theme settings - public UserTheme Theme { get; set; } = UserTheme.Default; + public UserTheme Theme { get; init; } = UserTheme.Default; - public ColorizationMode ColorizationMode { get; set; } + public ColorizationMode ColorizationMode { get; init; } - public Color CustomThemeColor { get; set; } = new() { A = 0, R = 255, G = 255, B = 255 }; // Transparent — avoids WinUI3 COM dependency on Colors.Transparent + public Color CustomThemeColor { get; init; } = new() { A = 0, R = 255, G = 255, B = 255 }; // Transparent — avoids WinUI3 COM dependency on Colors.Transparent - public int CustomThemeColorIntensity { get; set; } = 100; + public int CustomThemeColorIntensity { get; init; } = 100; - public int BackgroundImageTintIntensity { get; set; } + public int BackgroundImageTintIntensity { get; init; } - public int BackgroundImageOpacity { get; set; } = 20; + public int BackgroundImageOpacity { get; init; } = 20; - public int BackgroundImageBlurAmount { get; set; } + public int BackgroundImageBlurAmount { get; init; } - public int BackgroundImageBrightness { get; set; } + public int BackgroundImageBrightness { get; init; } - public BackgroundImageFit BackgroundImageFit { get; set; } + public BackgroundImageFit BackgroundImageFit { get; init; } - public string? BackgroundImagePath { get; set; } + public string? BackgroundImagePath { get; init; } - public BackdropStyle BackdropStyle { get; set; } + public BackdropStyle BackdropStyle { get; init; } - public int BackdropOpacity { get; set; } = 100; + public int BackdropOpacity { get; init; } = 100; // // END SETTINGS /////////////////////////////////////////////////////////////////////////// - public ProviderSettings GetProviderSettings(CommandProviderWrapper provider) + public (SettingsModel Model, ProviderSettings Settings) GetProviderSettings(CommandProviderWrapper provider) { - ProviderSettings? settings; - if (!ProviderSettings.TryGetValue(provider.ProviderId, out settings)) + if (!ProviderSettings.TryGetValue(provider.ProviderId, out var settings)) { - settings = new ProviderSettings(provider); - settings.Connect(provider); - ProviderSettings[provider.ProviderId] = settings; - } - else - { - settings.Connect(provider); + settings = new ProviderSettings(); } - return settings; + var connected = settings.WithConnection(provider); + + // If WithConnection returned the same instance, nothing changed — skip SetItem + if (ReferenceEquals(connected, settings)) + { + return (this, connected); + } + + var newModel = this with + { + ProviderSettings = ProviderSettings.SetItem(provider.ProviderId, connected), + }; + return (newModel, connected); } public string[] GetGlobalFallbacks() @@ -150,6 +158,13 @@ public partial class SettingsModel : ObservableObject [JsonSerializable(typeof(RecentCommandsManager))] [JsonSerializable(typeof(List), TypeInfoPropertyName = "StringList")] [JsonSerializable(typeof(List), TypeInfoPropertyName = "HistoryList")] +[JsonSerializable(typeof(ImmutableList), TypeInfoPropertyName = "ImmutableHistoryList")] +[JsonSerializable(typeof(ImmutableDictionary), TypeInfoPropertyName = "ImmutableFallbackDictionary")] +[JsonSerializable(typeof(ImmutableList), TypeInfoPropertyName = "ImmutableStringList")] +[JsonSerializable(typeof(ImmutableList), TypeInfoPropertyName = "ImmutableDockBandSettingsList")] +[JsonSerializable(typeof(ImmutableDictionary), TypeInfoPropertyName = "ImmutableProviderSettingsDictionary")] +[JsonSerializable(typeof(ImmutableDictionary), TypeInfoPropertyName = "ImmutableAliasDictionary")] +[JsonSerializable(typeof(ImmutableList), TypeInfoPropertyName = "ImmutableTopLevelHotkeyList")] [JsonSerializable(typeof(Dictionary), TypeInfoPropertyName = "Dictionary")] [JsonSourceGenerationOptions(UseStringEnumConverter = true, WriteIndented = true, IncludeFields = true, PropertyNameCaseInsensitive = true, AllowTrailingCommas = true)] [System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.MaintainabilityRules", "SA1402:File may only contain a single type", Justification = "Just used here")] diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsViewModel.cs index 5310c9f9aa..60baa76750 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/SettingsViewModel.cs @@ -41,9 +41,8 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.Hotkey; set { - _settingsService.Settings.Hotkey = value ?? SettingsModel.DefaultActivationShortcut; + _settingsService.UpdateSettings(s => s with { Hotkey = value ?? SettingsModel.DefaultActivationShortcut }); PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Hotkey))); - Save(); } } @@ -52,9 +51,8 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.UseLowLevelGlobalHotkey; set { - _settingsService.Settings.UseLowLevelGlobalHotkey = value; + _settingsService.UpdateSettings(s => s with { UseLowLevelGlobalHotkey = value }); PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Hotkey))); - Save(); } } @@ -63,8 +61,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.AllowExternalReload; set { - _settingsService.Settings.AllowExternalReload = value; - Save(); + _settingsService.UpdateSettings(s => s with { AllowExternalReload = value }); } } @@ -73,8 +70,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.ShowAppDetails; set { - _settingsService.Settings.ShowAppDetails = value; - Save(); + _settingsService.UpdateSettings(s => s with { ShowAppDetails = value }); } } @@ -83,8 +79,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.BackspaceGoesBack; set { - _settingsService.Settings.BackspaceGoesBack = value; - Save(); + _settingsService.UpdateSettings(s => s with { BackspaceGoesBack = value }); } } @@ -93,8 +88,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.SingleClickActivates; set { - _settingsService.Settings.SingleClickActivates = value; - Save(); + _settingsService.UpdateSettings(s => s with { SingleClickActivates = value }); } } @@ -103,8 +97,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.HighlightSearchOnActivate; set { - _settingsService.Settings.HighlightSearchOnActivate = value; - Save(); + _settingsService.UpdateSettings(s => s with { HighlightSearchOnActivate = value }); } } @@ -113,8 +106,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.KeepPreviousQuery; set { - _settingsService.Settings.KeepPreviousQuery = value; - Save(); + _settingsService.UpdateSettings(s => s with { KeepPreviousQuery = value }); } } @@ -123,8 +115,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => (int)_settingsService.Settings.SummonOn; set { - _settingsService.Settings.SummonOn = (MonitorBehavior)value; - Save(); + _settingsService.UpdateSettings(s => s with { SummonOn = (MonitorBehavior)value }); } } @@ -133,8 +124,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.ShowSystemTrayIcon; set { - _settingsService.Settings.ShowSystemTrayIcon = value; - Save(); + _settingsService.UpdateSettings(s => s with { ShowSystemTrayIcon = value }); } } @@ -143,8 +133,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.IgnoreShortcutWhenFullscreen; set { - _settingsService.Settings.IgnoreShortcutWhenFullscreen = value; - Save(); + _settingsService.UpdateSettings(s => s with { IgnoreShortcutWhenFullscreen = value }); } } @@ -153,8 +142,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.DisableAnimations; set { - _settingsService.Settings.DisableAnimations = value; - Save(); + _settingsService.UpdateSettings(s => s with { DisableAnimations = value }); } } @@ -170,10 +158,8 @@ public partial class SettingsViewModel : INotifyPropertyChanged { if (value >= 0 && value < AutoGoHomeIntervals.Count) { - _settingsService.Settings.AutoGoHomeInterval = AutoGoHomeIntervals[value]; + _settingsService.UpdateSettings(s => s with { AutoGoHomeInterval = AutoGoHomeIntervals[value] }); } - - Save(); } } @@ -182,8 +168,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => (int)_settingsService.Settings.EscapeKeyBehaviorSetting; set { - _settingsService.Settings.EscapeKeyBehaviorSetting = (EscapeKeyBehavior)value; - Save(); + _settingsService.UpdateSettings(s => s with { EscapeKeyBehaviorSetting = (EscapeKeyBehavior)value }); } } @@ -192,8 +177,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.DockSettings.Side; set { - _settingsService.Settings.DockSettings.Side = value; - Save(); + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { Side = value } }); } } @@ -202,8 +186,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.DockSettings.DockSize; set { - _settingsService.Settings.DockSettings.DockSize = value; - Save(); + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { DockSize = value } }); } } @@ -212,8 +195,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.DockSettings.Backdrop; set { - _settingsService.Settings.DockSettings.Backdrop = value; - Save(); + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { Backdrop = value } }); } } @@ -222,8 +204,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.DockSettings.ShowLabels; set { - _settingsService.Settings.DockSettings.ShowLabels = value; - Save(); + _settingsService.UpdateSettings(s => s with { DockSettings = s.DockSettings with { ShowLabels = value } }); } } @@ -232,8 +213,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged get => _settingsService.Settings.EnableDock; set { - _settingsService.Settings.EnableDock = value; - Save(); + _settingsService.UpdateSettings(s => s with { EnableDock = value }); WeakReferenceMessenger.Default.Send(new ShowHideDockMessage(value)); WeakReferenceMessenger.Default.Send(new ReloadCommandsMessage()); // TODO! we need to update the MoreCommands of all top level items, but we don't _really_ want to reload } @@ -245,7 +225,11 @@ public partial class SettingsViewModel : INotifyPropertyChanged public SettingsExtensionsViewModel Extensions { get; } - public SettingsViewModel(TopLevelCommandManager topLevelCommandManager, TaskScheduler scheduler, IThemeService themeService, ISettingsService settingsService) + public SettingsViewModel( + TopLevelCommandManager topLevelCommandManager, + TaskScheduler scheduler, + IThemeService themeService, + ISettingsService settingsService) { _settingsService = settingsService; _topLevelCommandManager = topLevelCommandManager; @@ -259,15 +243,27 @@ public partial class SettingsViewModel : INotifyPropertyChanged var fallbacks = new List(); var currentRankings = _settingsService.Settings.FallbackRanks; var needsSave = false; + var currentSettingsModel = _settingsService.Settings; foreach (var item in activeProviders) { - var providerSettings = _settingsService.Settings.GetProviderSettings(item); + var (newModel, providerSettings) = currentSettingsModel.GetProviderSettings(item); + currentSettingsModel = newModel; - var settingsModel = new ProviderSettingsViewModel(item, providerSettings, settingsService); - CommandProviders.Add(settingsModel); + var providerSettingsModel = new ProviderSettingsViewModel(item, providerSettings, settingsService); + CommandProviders.Add(providerSettingsModel); - fallbacks.AddRange(settingsModel.FallbackCommands); + fallbacks.AddRange(providerSettingsModel.FallbackCommands); + } + + // Only persist if provider enumeration actually changed the model + // Smelly? Yes, but it avoids an unnecessary write to disk. + // I don't love it, but it seems better than the alternatives. + // Open to suggestions. + if (!ReferenceEquals(currentSettingsModel, _settingsService.Settings)) + { + var finalModel = currentSettingsModel; + _settingsService.UpdateSettings(_ => finalModel, hotReload: false); } var fallbackRankings = new List>(fallbacks.Count); @@ -306,10 +302,7 @@ public partial class SettingsViewModel : INotifyPropertyChanged public void ApplyFallbackSort() { - _settingsService.Settings.FallbackRanks = FallbackRankings.Select(s => s.Id).ToArray(); - Save(); + _settingsService.UpdateSettings(s => s with { FallbackRanks = FallbackRankings.Select(s2 => s2.Id).ToArray() }); PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(FallbackRankings))); } - - private void Save() => _settingsService.Save(); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelHotkey.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelHotkey.cs index 64ffbdb461..18c1301df9 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelHotkey.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelHotkey.cs @@ -7,9 +7,15 @@ using Microsoft.CmdPal.UI.ViewModels.Settings; namespace Microsoft.CmdPal.UI.ViewModels; -public class TopLevelHotkey(HotkeySettings? hotkey, string commandId) +public record TopLevelHotkey { - public string CommandId { get; set; } = commandId; + public string CommandId { get; init; } - public HotkeySettings? Hotkey { get; set; } = hotkey; + public HotkeySettings? Hotkey { get; init; } + + public TopLevelHotkey(HotkeySettings? hotkey, string commandId) + { + Hotkey = hotkey; + CommandId = commandId; + } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs index 0ca7cbbfe1..e3a3b48f41 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs @@ -121,7 +121,7 @@ public sealed partial class TopLevelViewModel : ObservableObject, IListItem, IEx { if (Alias is CommandAlias a) { - a.Alias = value; + Alias = a with { Alias = value }; } else { @@ -146,7 +146,7 @@ public sealed partial class TopLevelViewModel : ObservableObject, IListItem, IEx { if (Alias is CommandAlias a) { - a.IsDirect = value; + Alias = a with { IsDirect = value }; } HandleChangeAlias(); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs index 108bf2b676..bb3bb6cb4e 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs @@ -92,7 +92,7 @@ internal sealed partial class CommandPaletteContextMenuFactory : IContextMenuFac var providerId = providerContext.ProviderId; if (_topLevelCommandManager.LookupProvider(providerId) is CommandProviderWrapper provider) { - var providerSettings = _settingsService.Settings.GetProviderSettings(provider); + var (_, providerSettings) = _settingsService.Settings.GetProviderSettings(provider); var alreadyPinnedToTopLevel = providerSettings.PinnedCommandIds.Contains(itemId); @@ -159,7 +159,7 @@ internal sealed partial class CommandPaletteContextMenuFactory : IContextMenuFac var providerId = providerContext.ProviderId; if (_topLevelCommandManager.LookupProvider(providerId) is CommandProviderWrapper provider) { - var providerSettings = _settingsService.Settings.GetProviderSettings(provider); + var (_, providerSettings) = _settingsService.Settings.GetProviderSettings(provider); var isPinnedSubCommand = providerSettings.PinnedCommandIds.Contains(itemId); if (isPinnedSubCommand) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml.cs index b8aad6c323..13d0588216 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml.cs @@ -186,7 +186,7 @@ public sealed partial class ShortcutControl : UserControl, IDisposable, IRecipie _ = _modifierKeysOnEntering.Remove(virtualKey); } - internalSettings.Win = matchValue; + internalSettings = internalSettings with { Win = matchValue }; break; case VirtualKey.Control: case VirtualKey.LeftControl: @@ -197,7 +197,7 @@ public sealed partial class ShortcutControl : UserControl, IDisposable, IRecipie _ = _modifierKeysOnEntering.Remove(VirtualKey.Control); } - internalSettings.Ctrl = matchValue; + internalSettings = internalSettings with { Ctrl = matchValue }; break; case VirtualKey.Menu: case VirtualKey.LeftMenu: @@ -208,7 +208,7 @@ public sealed partial class ShortcutControl : UserControl, IDisposable, IRecipie _ = _modifierKeysOnEntering.Remove(VirtualKey.Menu); } - internalSettings.Alt = matchValue; + internalSettings = internalSettings with { Alt = matchValue }; break; case VirtualKey.Shift: case VirtualKey.LeftShift: @@ -219,14 +219,14 @@ public sealed partial class ShortcutControl : UserControl, IDisposable, IRecipie _ = _modifierKeysOnEntering.Remove(VirtualKey.Shift); } - internalSettings.Shift = matchValue; + internalSettings = internalSettings with { Shift = matchValue }; break; case VirtualKey.Escape: internalSettings = new HotkeySettings(); shortcutDialog.IsPrimaryButtonEnabled = false; return; default: - internalSettings.Code = matchValueCode; + internalSettings = internalSettings with { Code = matchValueCode }; break; } } @@ -276,7 +276,7 @@ public sealed partial class ShortcutControl : UserControl, IDisposable, IRecipie else if (internalSettings.Shift && !_modifierKeysOnEntering.Contains(VirtualKey.Shift) && !internalSettings.Win && !internalSettings.Alt && !internalSettings.Ctrl) { // This is to reset the shift key press within the control as it was not used within the control but rather was used to leave the hotkey. - internalSettings.Shift = false; + internalSettings = internalSettings with { Shift = false }; SendSingleKeyboardInput((short)VirtualKey.Shift, (uint)NativeKeyboardHelper.KeyEventF.KeyDown); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs index 5389c5875c..780c6ca846 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs @@ -910,8 +910,7 @@ public sealed partial class MainWindow : WindowEx, // the last non-dock placement because dock sessions intentionally skip updates. if (_currentWindowPosition.IsSizeValid) { - settings.LastWindowPosition = _currentWindowPosition; - settingsService.Save(); + settingsService.UpdateSettings(s => s with { LastWindowPosition = _currentWindowPosition }); } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/RunHistoryService.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/RunHistoryService.cs index 6d7a4d628f..867ec41890 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/RunHistoryService.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/RunHistoryService.cs @@ -2,6 +2,7 @@ // 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.Immutable; using Microsoft.CmdPal.Common.Services; using Microsoft.CmdPal.UI.ViewModels; using Microsoft.CmdPal.UI.ViewModels.Services; @@ -19,10 +20,13 @@ internal sealed class RunHistoryService : IRunHistoryService public IReadOnlyList GetRunHistory() { - if (_appStateService.State.RunHistory.Count == 0) + if (_appStateService.State.RunHistory.IsEmpty) { var history = Microsoft.Terminal.UI.RunHistory.CreateRunHistory(); - _appStateService.State.RunHistory.AddRange(history); + _appStateService.UpdateState(state => state with + { + RunHistory = history.ToImmutableList(), + }); } return _appStateService.State.RunHistory; @@ -30,22 +34,24 @@ internal sealed class RunHistoryService : IRunHistoryService public void ClearRunHistory() { - _appStateService.State.RunHistory.Clear(); + _appStateService.UpdateState(state => state with + { + RunHistory = ImmutableList.Empty, + }); } public void AddRunHistoryItem(string item) { - // insert at the beginning of the list if (string.IsNullOrWhiteSpace(item)) { - return; // Do not add empty or whitespace items + return; } - _appStateService.State.RunHistory.Remove(item); - - // Add the item to the front of the history - _appStateService.State.RunHistory.Insert(0, item); - - _appStateService.Save(); + _appStateService.UpdateState(state => state with + { + RunHistory = state.RunHistory + .Remove(item) + .Insert(0, item), + }); } } diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/AppStateServiceTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/AppStateServiceTests.cs index 6224eb360f..5298ccbf77 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/AppStateServiceTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/AppStateServiceTests.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.IO; using Microsoft.CmdPal.Common.Services; using Microsoft.CmdPal.UI.ViewModels.Services; @@ -41,7 +42,7 @@ public class AppStateServiceTests // Arrange var expectedState = new AppStateModel { - RunHistory = new List { "command1", "command2" }, + RunHistory = ImmutableList.Create("command1", "command2"), }; _mockPersistence .Setup(p => p.Load( @@ -86,7 +87,8 @@ public class AppStateServiceTests { // Arrange var service = new AppStateService(_mockPersistence.Object, _mockAppInfo.Object); - service.State.RunHistory.Add("test-command"); + service.UpdateState(s => s with { RunHistory = s.RunHistory.Add("test-command") }); + _mockPersistence.Invocations.Clear(); // Reset after Arrange — UpdateState also persists // Act service.Save(); @@ -160,4 +162,44 @@ public class AppStateServiceTests // Assert Assert.AreEqual(2, eventCount); } + + [TestMethod] + public void UpdateState_ConcurrentUpdates_NoLostUpdates() + { + // Arrange — two threads each add items to RunHistory concurrently. + // With the CAS loop, every add must land (no lost updates). + var service = new AppStateService(_mockPersistence.Object, _mockAppInfo.Object); + const int iterations = 50; + var barrier = new System.Threading.Barrier(2); + + // Act + var t1 = System.Threading.Tasks.Task.Run(() => + { + barrier.SignalAndWait(); + for (var i = 0; i < iterations; i++) + { + service.UpdateState(s => s with { RunHistory = s.RunHistory.Add($"t1-{i}") }); + } + }); + + var t2 = System.Threading.Tasks.Task.Run(() => + { + barrier.SignalAndWait(); + for (var i = 0; i < iterations; i++) + { + service.UpdateState(s => s with { RunHistory = s.RunHistory.Add($"t2-{i}") }); + } + }); + + System.Threading.Tasks.Task.WaitAll(t1, t2); + + // Assert — all 100 items must be present (no lost updates) + Assert.AreEqual(iterations * 2, service.State.RunHistory.Count, "All concurrent updates should be preserved"); + + for (var i = 0; i < iterations; i++) + { + Assert.IsTrue(service.State.RunHistory.Contains($"t1-{i}"), $"Missing t1-{i}"); + Assert.IsTrue(service.State.RunHistory.Contains($"t2-{i}"), $"Missing t2-{i}"); + } + } } diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/RecentCommandsTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/RecentCommandsTests.cs index d603c4a92f..8c7eab0a2a 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/RecentCommandsTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/RecentCommandsTests.cs @@ -26,7 +26,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase { foreach (var item in commandIds) { - history.AddHistoryItem(item); + history = history.WithHistoryItem(item); } } @@ -54,7 +54,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase var history = CreateHistory(); // Act - history.AddHistoryItem("com.microsoft.cmdpal.shell"); + history = history.WithHistoryItem("com.microsoft.cmdpal.shell"); // Assert Assert.IsTrue(history.GetCommandHistoryWeight("com.microsoft.cmdpal.shell") > 0); @@ -121,7 +121,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase var history = new RecentCommandsManager(); foreach (var item in items) { - history.AddHistoryItem(item.Id); + history = history.WithHistoryItem(item.Id); } return history; @@ -417,7 +417,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase // Add extra uses of VS Code to try and push it above Terminal for (var i = 0; i < 10; i++) { - history.AddHistoryItem(items[1].Id); + history = history.WithHistoryItem(items[1].Id); } var weightedScores = items.Select(item => MainListPage.ScoreTopLevelItem(q, item, history, fuzzyMatcher)).ToList(); @@ -446,7 +446,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase var vsCodeId = items[1].Id; for (var i = 0; i < 10; i++) { - history.AddHistoryItem(vsCodeId); + history = history.WithHistoryItem(vsCodeId); var weightedScores = items.Select(item => MainListPage.ScoreTopLevelItem(q, item, history, fuzzyMatcher)).ToList(); var weightedMatches = GetMatches(items, weightedScores).ToList(); diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/SettingsServiceTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/SettingsServiceTests.cs index bade964e94..7961be82cb 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/SettingsServiceTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/SettingsServiceTests.cs @@ -75,7 +75,14 @@ public class SettingsServiceTests public void Settings_ReturnsLoadedModel() { // Arrange - _testSettings.ShowAppDetails = true; + _testSettings = _testSettings with { ShowAppDetails = true }; + + // Reset mock to return updated settings + _mockPersistence + .Setup(p => p.Load( + It.IsAny(), + It.IsAny>())) + .Returns(_testSettings); // Act var service = new SettingsService(_mockPersistence.Object, _mockAppInfo.Object); @@ -89,7 +96,10 @@ public class SettingsServiceTests { // Arrange var service = new SettingsService(_mockPersistence.Object, _mockAppInfo.Object); - service.Settings.SingleClickActivates = true; + service.UpdateSettings( + s => s with { SingleClickActivates = true }, + hotReload: false); + _mockPersistence.Invocations.Clear(); // Reset after Arrange — UpdateSettings also persists // Act service.Save(hotReload: false); @@ -178,4 +188,40 @@ public class SettingsServiceTests Assert.AreSame(service, receivedSender); Assert.AreSame(service.Settings, receivedSettings); } + + [TestMethod] + public void UpdateSettings_ConcurrentUpdates_NoLostUpdates() + { + // Arrange — two threads each set a different property to true, 100 times. + // Without a CAS loop, one thread's Exchange can overwrite the other's + // property back to false from a stale snapshot. With CAS, both survive. + var service = new SettingsService(_mockPersistence.Object, _mockAppInfo.Object); + const int iterations = 100; + var barrier = new System.Threading.Barrier(2); + + // Act — t1 sets ShowAppDetails=true, t2 sets SingleClickActivates=true + var t1 = System.Threading.Tasks.Task.Run(() => + { + barrier.SignalAndWait(); + for (var i = 0; i < iterations; i++) + { + service.UpdateSettings(s => s with { ShowAppDetails = true }, hotReload: false); + } + }); + + var t2 = System.Threading.Tasks.Task.Run(() => + { + barrier.SignalAndWait(); + for (var i = 0; i < iterations; i++) + { + service.UpdateSettings(s => s with { SingleClickActivates = true }, hotReload: false); + } + }); + + System.Threading.Tasks.Task.WaitAll(t1, t2); + + // Assert — both properties must be true; neither should have been overwritten + Assert.IsTrue(service.Settings.ShowAppDetails, "ShowAppDetails lost — a stale snapshot overwrote it"); + Assert.IsTrue(service.Settings.SingleClickActivates, "SingleClickActivates lost — a stale snapshot overwrote it"); + } }