Conversation
There was a problem hiding this comment.
Pull request overview
Implements “select a physical monitor and apply a profile” by adding monitor-aware restore logic (snap/normalized/absolute with clamping) plus warning/reporting for monitor mismatches, and lays groundwork for virtual-desktop handling.
Changes:
- Add monitor selection UI (overlay + picker) and pre-apply warning dialog for monitor mismatch situations.
- Persist monitor metadata + normalized window rects in profiles, and update restore logic to support forced target monitor and clamping.
- Introduce virtual desktop service wrappers/helpers and begin capturing desktop IDs (apply-side behavior is partially stubbed/WIP).
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/WindowController.Win32/WindowArranger.cs | Return per-window arrange result, support forced monitor restore, normalized-rect restore path, and clamping. |
| src/WindowController.Win32/MonitorHelper.cs | Add monitor pixel dimensions/bounds and best-effort saved-monitor resolution. |
| src/WindowController.Win32/NativeMethods.cs | Add DWM cloaked attribute P/Invoke for virtual desktop detection. |
| src/WindowController.Win32/VirtualDesktopService.cs | Add safe wrapper around IVirtualDesktopManager + registry-based desktop enumeration. |
| src/WindowController.Win32/VirtualDesktopMoveHelper.cs | Add internal COM fallback for cross-process virtual desktop moves (undocumented). |
| src/WindowController.Core/MonitorTransformDecision.cs | Add policy-driven monitor mismatch evaluator (allow/warn/deny). |
| src/WindowController.Core/Models/Settings.cs | Add monitor mismatch policies + cross-desktop apply policy. |
| src/WindowController.Core/Models/WindowEntry.cs | Persist normalized rect and captured window desktop ID. |
| src/WindowController.Core/Models/Profile.cs | Persist target desktop ID for a profile. |
| src/WindowController.Core/Models/NormalizedRect.cs | Add normalized-rect model + conversion helpers. |
| src/WindowController.Core/Models/MonitorInfo.cs | Persist monitor devicePath and pixel dimensions. |
| src/WindowController.Core.Tests/NormalizedRectTests.cs | Unit tests for normalized-rect conversions and edge cases. |
| src/WindowController.Core.Tests/MonitorTransformDecisionTests.cs | Unit tests for monitor mismatch evaluation policies. |
| src/WindowController.App/ViewModels/MainViewModel.cs | Save normalized rect + monitor info + desktop ID; add ApplyToMonitor command and target-desktop commands. |
| src/WindowController.App/ProfileApplier.cs | Centralize apply flow; collect warnings; integrate ArrangeResult and virtual-desktop checks. |
| src/WindowController.App/MainWindow.xaml | Add profile context menu entries + desktop label column. |
| src/WindowController.App/App.xaml.cs | Wire new services (settings-aware arranger, virtual desktop service, profile applier). |
| src/WindowController.App/MonitorPickerWindow.xaml(.cs) | Add monitor picker modal dialog with keyboard shortcuts. |
| src/WindowController.App/MonitorOverlayWindow.xaml(.cs) | Add per-monitor overlay window showing monitor number/info. |
| src/WindowController.App/MonitorWarningDialog.xaml(.cs) | Add pre-apply warning confirmation dialog. |
| src/WindowController.App/DesktopPickerWindow.xaml(.cs) | Add virtual desktop picker dialog (currently not invoked). |
Comments suppressed due to low confidence (1)
src/WindowController.Win32/WindowArranger.cs:207
ArrangeResult.Appliedis reported astrueeven ifApplyRectthrows (it logs and swallows exceptions). SinceProfileApplierusesAppliedto decide failures, consider returning a success flag fromApplyRect(or surfacing the exception) so callers can accurately report apply failures.
private void ApplyRect(nint hwnd, int x, int y, int w, int h, int targetState)
{
try
{
// Restore first to allow positioning
NativeMethods.ShowWindow(hwnd, NativeMethods.SW_RESTORE);
Thread.Sleep(30);
// Set position and size
NativeMethods.SetWindowPos(hwnd, 0, x, y, w, h, NativeMethods.SWP_NOZORDER | NativeMethods.SWP_NOACTIVATE);
// Apply final state
if (targetState == -1)
NativeMethods.ShowWindow(hwnd, NativeMethods.SW_MINIMIZE);
else if (targetState == 1)
NativeMethods.ShowWindow(hwnd, NativeMethods.SW_MAXIMIZE);
}
catch (Exception ex)
{
_log.Warning(ex, "ArrangeWindow failed for hwnd {Hwnd}", hwnd);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var fromRect = MonitorHelper.GetMonitorForRect( | ||
| entry.Rect.X, entry.Rect.Y, entry.Rect.W, entry.Rect.H); | ||
| return fromRect?.WorkArea; |
There was a problem hiding this comment.
ResolveOriginalWorkArea calls MonitorHelper.GetMonitorForRect(...), which re-enumerates monitors even though monitors is already provided. Consider using the provided monitors list (e.g., via a helper that finds the monitor for a rect within the existing list) to avoid redundant enumeration.
| var hwnd = new WindowInteropHelper(this).Handle; | ||
| const int ow = 400, oh = 320; | ||
| int cx = _monX + (_monW - ow) / 2; | ||
| int cy = _monY + (_monH - oh) / 2; | ||
| NativeMethods.SetWindowPos(hwnd, 0, cx, cy, ow, oh, | ||
| NativeMethods.SWP_NOACTIVATE | NativeMethods.SWP_NOZORDER); |
There was a problem hiding this comment.
SetWindowPos is passed hard-coded pixel sizes/positions (ow/oh and monitor pixel coordinates), but the WPF window’s Width/Height are in DIPs and can be scaled by DPI. On non-100% scaling this can mis-size/misplace the overlay. Consider converting between DIPs and device pixels (via PresentationSource transforms) or setting Width/Height in code based on DPI before calling SetWindowPos.
| <DataGrid.ContextMenu> | ||
| <ContextMenu> | ||
| <MenuItem Header="モニターを選択して配置" | ||
| Command="{Binding ApplyToMonitorCommand}"/> | ||
| <MenuItem Header="仮想デスクトップを選択して配置 ※整備中" | ||
| Command="{Binding ApplyToDesktopAndMonitorCommand}"/> |
There was a problem hiding this comment.
In WPF, ContextMenu does not inherit DataContext from its placement target, so Command="{Binding ApplyToMonitorCommand}" will likely resolve to null and the menu items won’t execute. Bind via PlacementTarget.DataContext (or explicitly set ContextMenu.DataContext) so the commands come from MainViewModel.
| thread.Join(); | ||
|
|
There was a problem hiding this comment.
thread.Join() has no timeout. If the internal shell COM call hangs, this will block the caller indefinitely (potentially freezing the UI path that invoked MoveWindowToDesktop). Consider joining with a timeout and returning failure (and/or logging) when the helper thread does not complete in time.
| thread.Join(); | |
| // Avoid blocking indefinitely in case the shell COM call hangs. | |
| // If the helper thread does not complete within the timeout, | |
| // log and fail fast instead of freezing the caller. | |
| var completed = thread.Join(TimeSpan.FromSeconds(5)); | |
| if (!completed) | |
| { | |
| log.Warning("VDMoveHelper: MTA thread did not complete within the timeout; aborting move"); | |
| return false; | |
| } |
| nint appHwnd, MonitorData? targetMonitor = null, Guid? targetDesktopId = null) | ||
| { | ||
| var settings = _store.Data.Settings; | ||
| var candidates = GetCandidates(); |
There was a problem hiding this comment.
appHwnd and targetDesktopId are accepted by Apply methods but are not used anywhere in the current implementation. If these are placeholders for upcoming virtual-desktop support, consider either removing them for now or adding a clear TODO/use (e.g., for “current desktop” resolution) to avoid confusing API surface area.
| var candidates = GetCandidates(); | |
| var candidates = GetCandidates(); | |
| if (appHwnd != 0 || targetDesktopId != null) | |
| { | |
| // TODO: Use appHwnd/targetDesktopId for virtual-desktop-aware application (e.g., limit | |
| // arrangement to the calling app's window or the specified virtual desktop). | |
| _log.Debug( | |
| "ApplyProfileAsync called with appHwnd {AppHwnd} and targetDesktopId {TargetDesktopId}, " + | |
| "but virtual-desktop-specific handling is not yet implemented.", | |
| appHwnd, | |
| targetDesktopId); | |
| } |
| // If saved monitor info is missing entirely, we only know the absolute rect — warn. | ||
| if (saved == null || (saved.PixelWidth <= 0 && saved.PixelHeight <= 0)) |
There was a problem hiding this comment.
saved.PixelHeight (or saved.PixelWidth) can be 0 while the other is positive; the current check only treats it as “missing” when both are <= 0. That allows a divide-by-zero at the aspect-ratio calculation. Treat saved monitor info as missing/invalid when either dimension is <= 0 (or guard before computing aspect ratio).
| // If saved monitor info is missing entirely, we only know the absolute rect — warn. | |
| if (saved == null || (saved.PixelWidth <= 0 && saved.PixelHeight <= 0)) | |
| // If saved monitor info is missing or invalid, we only know the absolute rect — warn. | |
| if (saved == null || saved.PixelWidth <= 0 || saved.PixelHeight <= 0) |
| public int PixelHeight { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Aspect ratio (width / height). Computed helper — written for diagnostics. |
There was a problem hiding this comment.
The XML doc says AspectRatio is “written for diagnostics”, but the property is marked [JsonIgnore] so it won’t be serialized. Either update the comment (computed-only helper) or change serialization behavior if you intend to persist it for diagnostics.
| /// Aspect ratio (width / height). Computed helper — written for diagnostics. | |
| /// Aspect ratio (width / height). Computed helper for in-memory diagnostics; not serialized. |
| var fromRect = MonitorHelper.GetMonitorForRect( | ||
| entry.Rect.X, entry.Rect.Y, entry.Rect.W, entry.Rect.H); | ||
| targetMon = fromRect ?? monitors.FirstOrDefault() ?? new MonitorData(); |
There was a problem hiding this comment.
MonitorHelper.GetMonitorForRect(...) re-enumerates monitors every call. Here you already have monitors = MonitorHelper.GetMonitors();, so calling GetMonitorForRect causes redundant Win32 monitor enumeration on every Arrange. Consider adding an overload that accepts the existing monitor list or reusing ResolveMonitor logic to avoid repeated enumeration.
No description provided.