Skip to content

Commit c41f3dc

Browse files
Address PR #46915 review feedback from zadjii-msft
- Null-coalesce MonitorConfigs in PopulateMonitorConfigs for SequenceEqual safety - Remove over-qualified Dock.DockWindowManager in ShellPage.xaml.cs - Disambiguate MonitorInfo via using alias in DockWindow.xaml.cs - Add debug log in MonitorService.NotifyMonitorsChanged - Move side resolution to DockSettings.GetSideForMonitor() helper - Merge _dockWindows/_dockViewModels into single _docks dictionary - Make _syncing atomic with Interlocked.CompareExchange - Reconciler: use DeviceId dictionary for O(1) lookups, remove mutable copy, clarify Phase 2 comment - Simplify bands resolution in CommandProviderWrapper - Expand activeSt/activeCt abbreviations in DockViewModel - Remove ShowLabels migration code and tests (separate PR) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 58f1cbd commit c41f3dc

11 files changed

Lines changed: 100 additions & 573 deletions

File tree

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

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -505,35 +505,26 @@ public void PinDockBand(string commandId, IServiceProvider serviceProvider, Dock
505505
// Prevent duplicate pins — check the target destination's bands.
506506
// When pinning to a specific monitor, check that monitor's resolved bands
507507
// (which include forked-from-global bands). Otherwise, check global bands.
508-
bool alreadyPinned;
508+
DockMonitorConfig? targetConfig = null;
509509
if (monitorDeviceId is not null)
510510
{
511-
var configs = dockSettings.MonitorConfigs ?? System.Collections.Immutable.ImmutableList<DockMonitorConfig>.Empty;
512-
DockMonitorConfig? targetConfig = null;
513-
foreach (var cfg in configs)
511+
foreach (var cfg in dockSettings.MonitorConfigs)
514512
{
515-
if (string.Equals(cfg.MonitorDeviceId, monitorDeviceId, System.StringComparison.OrdinalIgnoreCase))
513+
if (string.Equals(cfg.MonitorDeviceId, monitorDeviceId, StringComparison.OrdinalIgnoreCase))
516514
{
517515
targetConfig = cfg;
518516
break;
519517
}
520518
}
519+
}
521520

522-
// Resolve the bands for the target monitor (per-monitor if customized, else global)
523-
var resolvedStart = targetConfig?.ResolveStartBands(dockSettings.StartBands) ?? dockSettings.StartBands;
524-
var resolvedCenter = targetConfig?.ResolveCenterBands(dockSettings.CenterBands) ?? dockSettings.CenterBands;
525-
var resolvedEnd = targetConfig?.ResolveEndBands(dockSettings.EndBands) ?? dockSettings.EndBands;
521+
var resolvedStart = targetConfig?.ResolveStartBands(dockSettings.StartBands) ?? dockSettings.StartBands;
522+
var resolvedCenter = targetConfig?.ResolveCenterBands(dockSettings.CenterBands) ?? dockSettings.CenterBands;
523+
var resolvedEnd = targetConfig?.ResolveEndBands(dockSettings.EndBands) ?? dockSettings.EndBands;
526524

527-
alreadyPinned = resolvedStart.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
525+
var alreadyPinned = resolvedStart.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
528526
resolvedCenter.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
529527
resolvedEnd.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId);
530-
}
531-
else
532-
{
533-
alreadyPinned = dockSettings.StartBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
534-
dockSettings.CenterBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
535-
dockSettings.EndBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId);
536-
}
537528

538529
if (alreadyPinned)
539530
{

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockViewModel.cs

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -368,10 +368,10 @@ private void SaveSettings()
368368
public void SyncBandPosition(DockBandViewModel band, DockPinSide targetSide, int targetIndex)
369369
{
370370
var bandId = band.Id;
371-
var (activeSt, activeCt, activeEnd) = GetActiveBands();
371+
var (activeStart, activeCenter, activeEnd) = GetActiveBands();
372372

373-
var bandSettings = activeSt.FirstOrDefault(b => b.CommandId == bandId)
374-
?? activeCt.FirstOrDefault(b => b.CommandId == bandId)
373+
var bandSettings = activeStart.FirstOrDefault(b => b.CommandId == bandId)
374+
?? activeCenter.FirstOrDefault(b => b.CommandId == bandId)
375375
?? activeEnd.FirstOrDefault(b => b.CommandId == bandId);
376376

377377
if (bandSettings == null)
@@ -380,8 +380,8 @@ public void SyncBandPosition(DockBandViewModel band, DockPinSide targetSide, int
380380
}
381381

382382
// Remove from all active band lists
383-
var newStart = activeSt.RemoveAll(b => b.CommandId == bandId);
384-
var newCenter = activeCt.RemoveAll(b => b.CommandId == bandId);
383+
var newStart = activeStart.RemoveAll(b => b.CommandId == bandId);
384+
var newCenter = activeCenter.RemoveAll(b => b.CommandId == bandId);
385385
var newEnd = activeEnd.RemoveAll(b => b.CommandId == bandId);
386386

387387
// Add to target list at the correct index
@@ -417,10 +417,10 @@ public void SyncBandPosition(DockBandViewModel band, DockPinSide targetSide, int
417417
public void MoveBandWithoutSaving(DockBandViewModel band, DockPinSide targetSide, int targetIndex)
418418
{
419419
var bandId = band.Id;
420-
var (activeSt, activeCt, activeEnd) = GetActiveBands();
420+
var (activeStart, activeCenter, activeEnd) = GetActiveBands();
421421

422-
var bandSettings = activeSt.FirstOrDefault(b => b.CommandId == bandId)
423-
?? activeCt.FirstOrDefault(b => b.CommandId == bandId)
422+
var bandSettings = activeStart.FirstOrDefault(b => b.CommandId == bandId)
423+
?? activeCenter.FirstOrDefault(b => b.CommandId == bandId)
424424
?? activeEnd.FirstOrDefault(b => b.CommandId == bandId);
425425

426426
if (bandSettings == null)
@@ -430,8 +430,8 @@ public void MoveBandWithoutSaving(DockBandViewModel band, DockPinSide targetSide
430430
}
431431

432432
// Remove from all sides (settings)
433-
var newStart = activeSt.RemoveAll(b => b.CommandId == bandId);
434-
var newCenter = activeCt.RemoveAll(b => b.CommandId == bandId);
433+
var newStart = activeStart.RemoveAll(b => b.CommandId == bandId);
434+
var newCenter = activeCenter.RemoveAll(b => b.CommandId == bandId);
435435
var newEnd = activeEnd.RemoveAll(b => b.CommandId == bandId);
436436

437437
// Remove from UI collections
@@ -678,21 +678,21 @@ private void RebuildUICollectionsFromSnapshot()
678678
return;
679679
}
680680

681-
var (activeSt, activeCt, activeEnd) = GetActiveBands();
681+
var (activeStart, activeCenter, activeEnd) = GetActiveBands();
682682

683683
StartItems.Clear();
684684
CenterItems.Clear();
685685
EndItems.Clear();
686686

687-
foreach (var bandSettings in activeSt)
687+
foreach (var bandSettings in activeStart)
688688
{
689689
if (_snapshotBandViewModels.TryGetValue(bandSettings.CommandId, out var bandVM))
690690
{
691691
StartItems.Add(bandVM);
692692
}
693693
}
694694

695-
foreach (var bandSettings in activeCt)
695+
foreach (var bandSettings in activeCenter)
696696
{
697697
if (_snapshotBandViewModels.TryGetValue(bandSettings.CommandId, out var bandVM))
698698
{
@@ -711,7 +711,7 @@ private void RebuildUICollectionsFromSnapshot()
711711

712712
private void RebuildUICollections()
713713
{
714-
var (activeSt, activeCt, activeEnd) = GetActiveBands();
714+
var (activeStart, activeCenter, activeEnd) = GetActiveBands();
715715

716716
// Create a lookup of all current band ViewModels
717717
var allBands = StartItems.Concat(CenterItems).Concat(EndItems).ToDictionary(b => b.Id);
@@ -720,15 +720,15 @@ private void RebuildUICollections()
720720
CenterItems.Clear();
721721
EndItems.Clear();
722722

723-
foreach (var bandSettings in activeSt)
723+
foreach (var bandSettings in activeStart)
724724
{
725725
if (allBands.TryGetValue(bandSettings.CommandId, out var bandVM))
726726
{
727727
StartItems.Add(bandVM);
728728
}
729729
}
730730

731-
foreach (var bandSettings in activeCt)
731+
foreach (var bandSettings in activeCenter)
732732
{
733733
if (allBands.TryGetValue(bandSettings.CommandId, out var bandVM))
734734
{
@@ -789,7 +789,7 @@ public void AddBandToSection(TopLevelViewModel topLevel, DockPinSide targetSide)
789789
// Create settings for the new band
790790
var bandSettings = new DockBandSettings { ProviderId = topLevel.CommandProviderId, CommandId = bandId };
791791
var dockSettings = _settings;
792-
var (activeSt, activeCt, activeEnd) = GetActiveBands();
792+
var (activeStart, activeCenter, activeEnd) = GetActiveBands();
793793

794794
// Create the band view model
795795
var bandVm = CreateBandItem(bandSettings, topLevel.ItemViewModel);
@@ -798,15 +798,15 @@ public void AddBandToSection(TopLevelViewModel topLevel, DockPinSide targetSide)
798798
switch (targetSide)
799799
{
800800
case DockPinSide.Start:
801-
_settings = WithActiveBands(activeSt.Add(bandSettings), activeCt, activeEnd);
801+
_settings = WithActiveBands(activeStart.Add(bandSettings), activeCenter, activeEnd);
802802
StartItems.Add(bandVm);
803803
break;
804804
case DockPinSide.Center:
805-
_settings = WithActiveBands(activeSt, activeCt.Add(bandSettings), activeEnd);
805+
_settings = WithActiveBands(activeStart, activeCenter.Add(bandSettings), activeEnd);
806806
CenterItems.Add(bandVm);
807807
break;
808808
case DockPinSide.End:
809-
_settings = WithActiveBands(activeSt, activeCt, activeEnd.Add(bandSettings));
809+
_settings = WithActiveBands(activeStart, activeCenter, activeEnd.Add(bandSettings));
810810
EndItems.Add(bandVm);
811811
break;
812812
}
@@ -829,12 +829,12 @@ public void AddBandToSection(TopLevelViewModel topLevel, DockPinSide targetSide)
829829
public void UnpinBand(DockBandViewModel band)
830830
{
831831
var bandId = band.Id;
832-
var (activeSt, activeCt, activeEnd) = GetActiveBands();
832+
var (activeStart, activeCenter, activeEnd) = GetActiveBands();
833833

834834
// Remove from settings
835835
_settings = WithActiveBands(
836-
activeSt.RemoveAll(b => b.CommandId == bandId),
837-
activeCt.RemoveAll(b => b.CommandId == bandId),
836+
activeStart.RemoveAll(b => b.CommandId == bandId),
837+
activeCenter.RemoveAll(b => b.CommandId == bandId),
838838
activeEnd.RemoveAll(b => b.CommandId == bandId));
839839

840840
// Remove from UI collections
@@ -899,13 +899,13 @@ private void EmitDockConfiguration()
899899
var isDockEnabled = _settingsService.Settings.EnableDock;
900900
var dockSide = isDockEnabled ? GetEffectiveSide().ToString().ToLowerInvariant() : "none";
901901

902-
var (activeSt, activeCt, activeEnd) = GetActiveBands();
902+
var (activeStart, activeCenter, activeEnd) = GetActiveBands();
903903

904904
static string FormatBands(ImmutableList<DockBandSettings> bands) =>
905905
string.Join("\n", bands.Select(b => $"{b.ProviderId}/{b.CommandId}"));
906906

907-
var startBands = isDockEnabled ? FormatBands(activeSt) : string.Empty;
908-
var centerBands = isDockEnabled ? FormatBands(activeCt) : string.Empty;
907+
var startBands = isDockEnabled ? FormatBands(activeStart) : string.Empty;
908+
var centerBands = isDockEnabled ? FormatBands(activeCenter) : string.Empty;
909909
var endBands = isDockEnabled ? FormatBands(activeEnd) : string.Empty;
910910

911911
WeakReferenceMessenger.Default.Send(new TelemetryDockConfigurationMessage(

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Services/SettingsService.cs

Lines changed: 0 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,6 @@ private void ApplyMigrations()
8989
DeprecatedHotkeyGoesHomeKey,
9090
(ref SettingsModel model, bool goesHome) => model = model with { AutoGoHomeInterval = goesHome ? TimeSpan.Zero : Timeout.InfiniteTimeSpan },
9191
JsonSerializationContext.Default.Boolean);
92-
93-
migratedAny |= TryMigrateBandShowLabels(root, ref _settings);
9492
}
9593
}
9694
catch (Exception ex)
@@ -136,106 +134,4 @@ private static bool TryMigrate<T>(
136134

137135
return false;
138136
}
139-
140-
/// <summary>
141-
/// Migrates per-band <c>ShowLabels</c> to <c>ShowTitles</c> and <c>ShowSubtitles</c>.
142-
/// The old <c>ShowLabels</c> property on <see cref="DockBandSettings"/> was renamed to
143-
/// <c>ShowTitles</c> (with <c>ShowSubtitles</c> added). Because the legacy property is
144-
/// <c>[JsonIgnore]</c>, old JSON values are lost during deserialization. This migration
145-
/// reads the raw JSON to recover them.
146-
/// </summary>
147-
private static bool TryMigrateBandShowLabels(JsonObject root, ref SettingsModel model)
148-
{
149-
try
150-
{
151-
if (root[nameof(SettingsModel.DockSettings)] is not JsonObject dockSettingsNode)
152-
{
153-
return false;
154-
}
155-
156-
var migrated = false;
157-
var ds = model.DockSettings;
158-
159-
var newStart = MigrateBandList(dockSettingsNode, nameof(DockSettings.StartBands), ds.StartBands, ref migrated);
160-
var newCenter = MigrateBandList(dockSettingsNode, nameof(DockSettings.CenterBands), ds.CenterBands, ref migrated);
161-
var newEnd = MigrateBandList(dockSettingsNode, nameof(DockSettings.EndBands), ds.EndBands, ref migrated);
162-
163-
if (migrated)
164-
{
165-
model = model with
166-
{
167-
DockSettings = ds with
168-
{
169-
StartBands = newStart,
170-
CenterBands = newCenter,
171-
EndBands = newEnd,
172-
},
173-
};
174-
}
175-
176-
return migrated;
177-
}
178-
catch (Exception ex)
179-
{
180-
Logger.LogError("Error during band ShowLabels migration.", ex);
181-
return false;
182-
}
183-
}
184-
185-
/// <summary>
186-
/// Scans a single band array in the raw JSON for <c>ShowLabels</c> entries that
187-
/// need migrating to <c>ShowTitles</c> / <c>ShowSubtitles</c>.
188-
/// </summary>
189-
private static ImmutableList<DockBandSettings> MigrateBandList(
190-
JsonObject dockSettingsNode,
191-
string bandKey,
192-
ImmutableList<DockBandSettings> bands,
193-
ref bool anyMigrated)
194-
{
195-
if (dockSettingsNode[bandKey] is not JsonArray jsonBands)
196-
{
197-
return bands;
198-
}
199-
200-
var builder = bands.ToBuilder();
201-
var listChanged = false;
202-
203-
for (var i = 0; i < builder.Count && i < jsonBands.Count; i++)
204-
{
205-
if (jsonBands[i] is not JsonObject jsonBand)
206-
{
207-
continue;
208-
}
209-
210-
// Only migrate if old key exists and new key does not
211-
if (!jsonBand.ContainsKey("ShowLabels") || jsonBand.ContainsKey("ShowTitles"))
212-
{
213-
continue;
214-
}
215-
216-
var showLabelsNode = jsonBand["ShowLabels"];
217-
if (showLabelsNode is null)
218-
{
219-
continue;
220-
}
221-
222-
var showLabels = showLabelsNode.GetValue<bool>();
223-
var band = builder[i];
224-
band = band with
225-
{
226-
ShowTitles = band.ShowTitles ?? showLabels,
227-
ShowSubtitles = band.ShowSubtitles ?? showLabels,
228-
};
229-
builder[i] = band;
230-
listChanged = true;
231-
}
232-
233-
if (listChanged)
234-
{
235-
anyMigrated = true;
236-
return builder.ToImmutable();
237-
}
238-
239-
return bands;
240-
}
241137
}

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Settings/DockSettings.cs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,23 @@ public ImmutableList<DockMonitorConfig> MonitorConfigs
105105
init => _monitorConfigs = value ?? ImmutableList<DockMonitorConfig>.Empty;
106106
}
107107

108+
/// <summary>
109+
/// Gets the dock side override for a specific monitor, or <c>null</c> if the
110+
/// monitor has no override (inherits global <see cref="Side"/>).
111+
/// </summary>
112+
public DockSide? GetSideForMonitor(string deviceId)
113+
{
114+
foreach (var cfg in MonitorConfigs)
115+
{
116+
if (string.Equals(cfg.MonitorDeviceId, deviceId, StringComparison.OrdinalIgnoreCase))
117+
{
118+
return cfg.Side;
119+
}
120+
}
121+
122+
return null;
123+
}
124+
108125
[JsonIgnore]
109126
public IEnumerable<(string ProviderId, string CommandId)> AllPinnedCommands
110127
{

0 commit comments

Comments
 (0)