diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindow.xaml.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindow.xaml.cs index cd12b9fb03..41bb371b2e 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindow.xaml.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindow.xaml.cs @@ -614,6 +614,30 @@ public sealed partial class DockWindow : WindowEx, } } + internal void RefreshForMonitorChange() + { + if (_isDisposed) + { + return; + } + + RefreshTargetMonitor(); + + if (_appBarData.hWnd != IntPtr.Zero) + { + // The Shell caches the monitor coordinates from the original + // ABM_NEW registration, so after a topology change the stale + // AppBar rect cannot be repositioned correctly. Destroy and + // recreate to re-register with the new monitor geometry. + DestroyAppBar(_hwnd); + CreateAppBar(_hwnd); + } + else + { + UpdateWindowPosition(); + } + } + private void RefreshSideOverride() { if (_targetMonitor is null) @@ -1295,45 +1319,6 @@ public sealed partial class DockWindow : WindowEx, DispatcherQueue.TryEnqueue(HandleWorkAreaChanged); } } - else if (msg == PInvoke.WM_DISPLAYCHANGE) - { - Logger.LogDebug("WM_DISPLAYCHANGE"); - - // Invalidate the monitor cache so DockWindowManager can reconcile - _monitorService.NotifyMonitorsChanged(); - - // Use dispatcher to ensure we're on the UI thread. - // Refresh _targetMonitor before re-positioning: the MonitorInfo - // captured at construction is an immutable record, so its Bounds - // are stale after a topology change (e.g. an external display was - // disconnected, shifting our monitor's virtual-screen origin). - // Without this, UpdateAppBarDataForEdge would compute the AppBar - // rect against the old coordinates and produce a wildly incorrect - // size/position. - DispatcherQueue.TryEnqueue(() => - { - if (_isDisposed) - { - return; - } - - RefreshTargetMonitor(); - - if (_appBarData.hWnd != IntPtr.Zero) - { - // The Shell caches the monitor coordinates from the original - // ABM_NEW registration, so after a topology change the stale - // AppBar rect cannot be repositioned correctly. Destroy and - // recreate to re-register with the new monitor geometry. - DestroyAppBar(_hwnd); - CreateAppBar(_hwnd); - } - else - { - UpdateWindowPosition(); - } - }); - } else if (msg == PInvoke.WM_MOUSEMOVE) { HandleMouseMoveForAutoHide(); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindowManager.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindowManager.cs index f8c567132e..9893414b3a 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindowManager.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/Dock/DockWindowManager.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 CommunityToolkit.WinUI; using Microsoft.CmdPal.UI.ViewModels; using Microsoft.CmdPal.UI.ViewModels.Dock; using Microsoft.CmdPal.UI.ViewModels.Models; @@ -26,6 +27,15 @@ public sealed partial class DockWindowManager : IDisposable private bool _disposed; private int _syncing; + /// + /// Debounces rapid-fire monitor-change notifications (several WM_DISPLAYCHANGE messages + /// during a Win+P switch or dock/undock). Without it, each intermediate topology + /// snapshot gets reconciled and persisted, which can permanently corrupt dock configs + /// even though things settle fine on their own a moment later. + /// + private static readonly TimeSpan MonitorsChangedDebounceInterval = TimeSpan.FromMilliseconds(400); + private readonly DispatcherQueueTimer _monitorsChangedDebounceTimer; + private bool? _lastSyncedEnableDock; private DockSettings? _lastSyncedDockSettings; @@ -37,6 +47,7 @@ public sealed partial class DockWindowManager : IDisposable _monitorService = monitorService; _settingsService = settingsService; _dispatcherQueue = dispatcherQueue; + _monitorsChangedDebounceTimer = _dispatcherQueue.CreateTimer(); _monitorService.MonitorsChanged += OnMonitorsChanged; _settingsService.SettingsChanged += OnSettingsChanged; @@ -74,7 +85,7 @@ public sealed partial class DockWindowManager : IDisposable /// /// Synchronizes running dock windows to match the current settings and connected monitors. /// - public void SyncDocksToSettings() + public void SyncDocksToSettings(bool refreshDockWindows = false) { if (Interlocked.CompareExchange(ref _syncing, 1, 0) != 0) { @@ -83,7 +94,7 @@ public sealed partial class DockWindowManager : IDisposable try { - SyncDocksToSettingsCore(); + SyncDocksToSettingsCore(refreshDockWindows); } finally { @@ -91,7 +102,7 @@ public sealed partial class DockWindowManager : IDisposable } } - private void SyncDocksToSettingsCore() + private void SyncDocksToSettingsCore(bool refreshDockWindows) { var settings = _settingsService.Settings; if (!settings.EnableDock) @@ -167,6 +178,16 @@ public sealed partial class DockWindowManager : IDisposable dock.ViewModel.Dispose(); } } + + if (!refreshDockWindows) + { + return; + } + + foreach (var (_, (window, _)) in _docks) + { + window.RefreshForMonitorChange(); + } } public void Dispose() @@ -180,6 +201,8 @@ public sealed partial class DockWindowManager : IDisposable _monitorService.MonitorsChanged -= OnMonitorsChanged; _settingsService.SettingsChanged -= OnSettingsChanged; + _monitorsChangedDebounceTimer.Stop(); + HideDocks(); } @@ -211,10 +234,15 @@ public sealed partial class DockWindowManager : IDisposable { _dispatcherQueue.TryEnqueue(() => { - if (!_disposed) + if (_disposed) { - SyncDocksToSettings(); + return; } + + _monitorsChangedDebounceTimer.Debounce( + () => SyncDocksToSettings(refreshDockWindows: true), + interval: MonitorsChangedDebounceInterval, + immediate: false); }); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs index f6617e03b4..01ade0eac7 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/MainWindow.xaml.cs @@ -73,6 +73,7 @@ public sealed partial class MainWindow : WindowEx, private readonly LocalKeyboardListener _localKeyboardListener; private readonly HiddenOwnerWindowBehavior _hiddenOwnerBehavior = new(); private readonly ICmdPalProtocolActivation _protocolActivation; + private readonly ViewModels.Models.IMonitorService _monitorService; private readonly IThemeService _themeService; private readonly WindowThemeSynchronizer _windowThemeSynchronizer; private readonly List _breakthroughTimestamps = []; @@ -135,6 +136,7 @@ public sealed partial class MainWindow : WindowEx, public MainWindow() { _protocolActivation = App.Current.Services.GetRequiredService(); + _monitorService = App.Current.Services.GetRequiredService(); InitializeComponent(); @@ -1768,6 +1770,14 @@ public sealed partial class MainWindow : WindowEx, return (LRESULT)IntPtr.Zero; } + // Unlike DockWindow instances, MainWindow always exists, so it's the one + // reliable place to catch topology changes. Without this, the Settings page's + // monitor list goes stale whenever no dock window is around to see WM_DISPLAYCHANGE. + case PInvoke.WM_DISPLAYCHANGE: + Logger.LogDebug("MainWindow WM_DISPLAYCHANGE"); + _monitorService.NotifyMonitorsChanged(); + break; + default: if (uMsg == WM_TASKBAR_RESTART) { diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/Services/MonitorService.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/Services/MonitorService.cs index 366be25fb4..6a632e34b9 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/Services/MonitorService.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/Services/MonitorService.cs @@ -30,6 +30,20 @@ public sealed class MonitorService : IMonitorService /// public IReadOnlyList GetMonitors() { + // Check the cache first without paying for a retry-with-sleep under the lock. + lock (_lock) + { + if (_cachedSnapshot is not null) + { + return _cachedSnapshot; + } + } + + // BuildDisplayInfoMapWithRetry sleeps between attempts, so it runs unlocked. Another + // thread might race us and rebuild the map too, but that's cheaper than blocking + // every caller for up to 100ms. + var displayInfo = BuildDisplayInfoMapWithRetry(); + lock (_lock) { if (_cachedSnapshot is not null) @@ -37,7 +51,7 @@ public sealed class MonitorService : IMonitorService return _cachedSnapshot; } - _cachedMonitors = EnumerateMonitors(); + _cachedMonitors = EnumerateMonitors(displayInfo); _cachedSnapshot = _cachedMonitors.AsReadOnly(); return _cachedSnapshot; } @@ -104,10 +118,17 @@ public sealed class MonitorService : IMonitorService MonitorsChanged?.Invoke(this, EventArgs.Empty); } - private static unsafe List EnumerateMonitors() + /// + /// Number of immediate attempts to build the stable-ID display info map before giving up. + /// Right after WM_DISPLAYCHANGE, the Display Configuration API can transiently fail or + /// return an incomplete topology while Windows is still settling. Immediate retries + /// avoid blocking the UI thread while giving the API another chance to return stable data. + /// + private const int DisplayInfoMapRetryCount = 3; + + private static unsafe List EnumerateMonitors(Dictionary displayInfo) { var monitors = new List(); - var displayInfo = BuildDisplayInfoMap(); PInvoke.EnumDisplayMonitors( HDC.Null, @@ -173,14 +194,41 @@ public sealed class MonitorService : IMonitorService return monitors; } + /// + /// Calls , retrying a few times if it comes back + /// incomplete. Right after WM_DISPLAYCHANGE the API can transiently fail or only resolve + /// some active sources, and a partial map would leave those monitors on their volatile + /// GDI name, tricking into treating a + /// still-connected monitor as brand new. + /// + private static Dictionary BuildDisplayInfoMapWithRetry() + { + var map = new Dictionary(StringComparer.OrdinalIgnoreCase); + + for (var attempt = 0; attempt < DisplayInfoMapRetryCount; attempt++) + { + map = BuildDisplayInfoMap(out var expectedSourceCount); + if (map.Count >= expectedSourceCount && expectedSourceCount > 0) + { + return map; + } + } + + return map; + } + /// /// Builds a map from GDI device name (e.g. \\.\DISPLAY1) to display metadata /// (friendly name and stable device path) using the Display Configuration APIs. /// Returns an empty dictionary on failure so callers can fall back gracefully. + /// is the number of distinct GDI source device + /// names among the active paths, not the raw path count: in Duplicate/clone mode several + /// paths share one source, so comparing against the path count would never be satisfied. /// - private static unsafe Dictionary BuildDisplayInfoMap() + private static unsafe Dictionary BuildDisplayInfoMap(out uint expectedSourceCount) { var map = new Dictionary(StringComparer.OrdinalIgnoreCase); + expectedSourceCount = 0; try { @@ -209,9 +257,12 @@ public sealed class MonitorService : IMonitorService return map; } + var expectedSources = new HashSet<(LUID AdapterId, uint Id)>(); + for (var i = 0; i < pathCount; i++) { var path = paths[i]; + expectedSources.Add((path.sourceInfo.adapterId, path.sourceInfo.id)); // Get the GDI device name from the source info var sourceName = default(DISPLAYCONFIG_SOURCE_DEVICE_NAME); @@ -250,6 +301,8 @@ public sealed class MonitorService : IMonitorService map.TryAdd(gdiName, (friendly ?? string.Empty, devicePath ?? string.Empty)); } } + + expectedSourceCount = (uint)expectedSources.Count; } catch (Exception ex) when (ex is not OutOfMemoryException) { diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DockMultiMonitorTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DockMultiMonitorTests.cs index aa11b0d990..2f387859e4 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DockMultiMonitorTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DockMultiMonitorTests.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; using System.Text.Json; using Microsoft.CmdPal.UI.ViewModels.Dock; using Microsoft.CmdPal.UI.ViewModels.Models; @@ -271,27 +272,103 @@ public class DockMultiMonitorTests } [TestMethod] - public void Reconciler_FuzzyMatch_DoesNotMatchNonPrimaryMonitors() + public void Reconciler_UnmatchedSecondary_DoesNotConsumeExistingConfig() { - // Config has stale stable ID for a non-primary monitor + // A secondary ID change is ambiguous, so preserve the old config and create a + // separate default rather than moving the old settings onto the new monitor. var configs = ImmutableList.Create( new DockMonitorConfig { MonitorDeviceId = PrimaryMonitor.StableId, Enabled = true, IsPrimary = true }, - new DockMonitorConfig { MonitorDeviceId = @"\\?\DISPLAY#STALE#4&eee&0&UID333#{guidStale}", Enabled = true, IsPrimary = false, IsCustomized = true, LastSeen = DateTime.UtcNow }); + new DockMonitorConfig + { + MonitorDeviceId = @"\\?\DISPLAY#STALE#4&eee&0&UID333#{guidStale}", + Enabled = true, + IsPrimary = false, + IsCustomized = true, + StartBands = ImmutableList.Create(new DockBandSettings { ProviderId = "p", CommandId = "c" }), + LastSeen = DateTime.UtcNow, + }); - // Current monitors have primary + a different secondary + // Current monitors have primary + a secondary whose stable ID changed var monitors = new List { PrimaryMonitor, SecondaryMonitor }; var result = MonitorConfigReconciler.Reconcile(configs, monitors); - // Primary keeps its config, new secondary gets a fresh customized config, - // stale secondary is retained at end for future reconnection Assert.AreEqual(3, result.Count); - Assert.AreEqual(PrimaryMonitor.StableId, result[0].MonitorDeviceId); - Assert.AreEqual(SecondaryMonitor.StableId, result[1].MonitorDeviceId); - Assert.IsTrue(result[1].IsCustomized, "New secondary should get an empty-bands customized config."); - Assert.AreEqual(0, result[1].StartBands?.Count ?? 0, "New secondary should start with empty bands."); - Assert.AreEqual(@"\\?\DISPLAY#STALE#4&eee&0&UID333#{guidStale}", result[2].MonitorDeviceId, "Stale config should be preserved."); - Assert.IsTrue(result[2].IsCustomized, "Stale config should retain its customizations."); + var newConfig = result.FirstOrDefault(c => c.MonitorDeviceId == SecondaryMonitor.StableId); + var retainedConfig = result.FirstOrDefault(c => c.MonitorDeviceId.Contains("STALE", StringComparison.OrdinalIgnoreCase)); + Assert.IsNotNull(newConfig); + Assert.IsNotNull(retainedConfig); + Assert.IsFalse(newConfig!.Enabled, "An unmatched secondary monitor should get a disabled default."); + Assert.AreEqual(1, retainedConfig!.StartBands?.Count ?? 0, "The old config should remain available for reconnection."); + } + + [TestMethod] + public void Reconciler_MultipleUnmatchedSecondaryMonitors_PreservesConfigs() + { + // Two unmatched secondary monitors and one unmatched secondary config: ambiguous, + // so reconciliation must not guess and should fall back to creating fresh configs. + var thirdMonitor = SecondaryMonitor with + { + DeviceId = @"\\.\DISPLAY3", + StableId = @"\\?\DISPLAY#THIRD#4&ccc&0&UID333#{guid3}", + DisplayName = "Display 3", + }; + + var configs = ImmutableList.Create( + new DockMonitorConfig { MonitorDeviceId = PrimaryMonitor.StableId, Enabled = true, IsPrimary = true }, + new DockMonitorConfig + { + MonitorDeviceId = @"\\?\DISPLAY#STALE#4&eee&0&UID333#{guidStale}", + Enabled = true, + IsPrimary = false, + IsCustomized = true, + LastSeen = DateTime.UtcNow, + }); + + var monitors = new List { PrimaryMonitor, SecondaryMonitor, thirdMonitor }; + + var result = MonitorConfigReconciler.Reconcile(configs, monitors); + + // Both new secondary monitors get fresh configs; the stale one is retained for reconnection. + Assert.AreEqual(4, result.Count); + Assert.IsTrue(result.Any(c => c.MonitorDeviceId == SecondaryMonitor.StableId)); + Assert.IsTrue(result.Any(c => c.MonitorDeviceId == thirdMonitor.StableId)); + Assert.IsTrue(result.Any(c => c.MonitorDeviceId == @"\\?\DISPLAY#STALE#4&eee&0&UID333#{guidStale}")); + } + + [TestMethod] + public void Reconciler_TransientStableIdFallbackToDeviceId_PreservesExistingConfig() + { + // Simulates the StableId momentarily falling back to the volatile GDI DeviceId + // right after WM_DISPLAYCHANGE (Display Configuration API not yet settled). + // The fallback is ambiguous, so keep the stable-ID config instead of moving it. + var degradedSecondary = SecondaryMonitor with { StableId = SecondaryMonitor.DeviceId }; + + var configs = ImmutableList.Create( + new DockMonitorConfig { MonitorDeviceId = PrimaryMonitor.StableId, Enabled = true, IsPrimary = true }, + new DockMonitorConfig + { + MonitorDeviceId = SecondaryMonitor.StableId, + Enabled = true, + IsPrimary = false, + IsCustomized = true, + StartBands = ImmutableList.Create(new DockBandSettings { ProviderId = "p", CommandId = "c" }), + LastSeen = DateTime.UtcNow, + }); + + var monitors = new List { PrimaryMonitor, degradedSecondary }; + + var result = MonitorConfigReconciler.Reconcile(configs, monitors); + + Assert.AreEqual(3, result.Count); + var retainedConfig = result.FirstOrDefault(c => c.MonitorDeviceId == SecondaryMonitor.StableId); + var fallbackConfig = result.FirstOrDefault(c => c.MonitorDeviceId == degradedSecondary.StableId); + Assert.IsNotNull(retainedConfig, "The existing stable-ID config should remain available."); + Assert.IsNotNull(fallbackConfig, "The ambiguous fallback monitor should receive a default config."); + Assert.IsTrue(retainedConfig!.Enabled); + Assert.IsTrue(retainedConfig.IsCustomized); + Assert.AreEqual(1, retainedConfig.StartBands?.Count ?? 0, "Pinned bands should remain with the original config."); + Assert.IsFalse(fallbackConfig!.Enabled, "An unmatched secondary monitor should start disabled."); } // --- JSON serialization round-trip ---