Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a major UX improvement by adding configurable hotkey support and moving settings to a dedicated window. The changes enable users to customize keyboard shortcuts for showing the main GUI and for quickly applying individual profiles. The refactoring extracts settings-related code from the main window into a new SettingsWindow, improving code organization and maintainability.
Changes:
- Added comprehensive hotkey configuration system with support for GUI toggle and per-profile hotkeys
- Created new SettingsWindow with dedicated UI for managing all application settings
- Extracted profile application logic into a reusable ProfileApplier class for use by both UI and hotkeys
- Removed settings UI from MainWindow and relocated to SettingsWindow
- Added Win32 constants for additional modifier keys (Shift, Win)
- Implemented comprehensive tests for new model classes (HotkeyBinding, HotkeySettings, AppSettings)
- Improved UI styling by removing hardcoded colors in favor of theme defaults
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/WindowController.Win32/NativeMethods.cs | Added MOD_SHIFT and MOD_WIN constants, reordered hotkey modifiers alphabetically |
| src/WindowController.Core/Models/HotkeySettings.cs | New models for hotkey configuration with binding validation and serialization |
| src/WindowController.Core/Models/AppSettings.cs | Added Hotkeys property to store application-wide hotkey settings |
| src/WindowController.Core.Tests/AppSettingsTests.cs | Comprehensive tests for HotkeyBinding, HotkeySettings, and AppSettings serialization |
| src/WindowController.App/ViewModels/SettingsViewModel.cs | New ViewModel managing all settings including hotkey capture and validation |
| src/WindowController.App/ViewModels/MainViewModel.cs | Removed settings-related properties and commands, simplified to core window management |
| src/WindowController.App/SettingsWindow.xaml.cs | Code-behind for settings window with hotkey capture handling |
| src/WindowController.App/SettingsWindow.xaml | UI for settings including hotkey configuration with visual feedback |
| src/WindowController.App/ProfileApplier.cs | Extracted profile application logic for reuse by UI and hotkey handlers |
| src/WindowController.App/MainWindow.xaml.cs | Added SettingsRequested event for opening settings window |
| src/WindowController.App/MainWindow.xaml | Added settings button, removed settings section, improved button styling |
| src/WindowController.App/HotkeyManager.cs | Refactored to support multiple hotkeys with dynamic registration/unregistration |
| src/WindowController.App/App.xaml.cs | Integrated SettingsWindow, ProfileApplier, and enhanced hotkey registration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SyncManager syncManager, | ||
| ILogger log, | ||
| Action? refreshHotkeysCallback = null, | ||
| Func<string, bool, Task>? applyProfileCallback = null) |
There was a problem hiding this comment.
The applyProfileCallback parameter is stored in a field but never used anywhere in the SettingsViewModel class. This appears to be leftover code from initial development. Consider removing it to clean up the API, or if it's intended for future use, add a TODO comment explaining its purpose.
|
|
||
| public override bool Equals(object? obj) => obj is HotkeyBinding other && Equals(other); | ||
|
|
||
| public override int GetHashCode() => HashCode.Combine(Key?.ToUpperInvariant(), Ctrl, Alt, Shift, Win); |
There was a problem hiding this comment.
The GetHashCode implementation uses Key?.ToUpperInvariant() which properly handles null Key values. However, since Key is initialized to an empty string by default and the IsEmpty property checks for null or empty, it would be more consistent to ensure Key is never null. Consider adding a null check in the Key setter or documenting that Key should never be null.
| // Check for conflicts with already registered hotkeys | ||
| foreach (var (id, existingBinding) in _registeredBindings) | ||
| { | ||
| if (existingBinding.Equals(binding)) | ||
| { | ||
| return new HotkeyRegistrationResult(false, $"このホットキーは既に登録されています: {binding}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
TestHotkey checks for conflicts against _registeredBindings, but in a multi-threaded scenario, there could be a race condition where two calls to TestHotkey succeed concurrently before either is registered. Consider adding thread safety or documenting that TestHotkey and RegisterHotkey must be called on the same thread (which appears to be the UI thread based on the Dispatcher usage elsewhere).
| public static int GetVirtualKeyCode(string key) | ||
| { | ||
| if (string.IsNullOrEmpty(key)) return 0; | ||
|
|
||
| // Try parsing as a Key enum first | ||
| if (Enum.TryParse<Key>(key, ignoreCase: true, out var wpfKey)) | ||
| { | ||
| return KeyInterop.VirtualKeyFromKey(wpfKey); | ||
| } | ||
|
|
||
| // Handle single character keys | ||
| if (key.Length == 1) | ||
| { | ||
| var c = char.ToUpperInvariant(key[0]); | ||
| if (c >= 'A' && c <= 'Z') | ||
| return c; // VK_A to VK_Z are same as ASCII | ||
| if (c >= '0' && c <= '9') | ||
| return c; // VK_0 to VK_9 are same as ASCII | ||
| } | ||
|
|
||
| // Handle function keys | ||
| if (key.StartsWith("F", StringComparison.OrdinalIgnoreCase) && | ||
| int.TryParse(key.AsSpan(1), out var fNum) && fNum >= 1 && fNum <= 24) | ||
| { | ||
| return 0x70 + fNum - 1; // VK_F1 = 0x70 | ||
| } | ||
|
|
||
| // Handle special keys | ||
| return key.ToUpperInvariant() switch | ||
| { | ||
| "SPACE" => 0x20, | ||
| "TAB" => 0x09, | ||
| "ENTER" or "RETURN" => 0x0D, | ||
| "ESCAPE" or "ESC" => 0x1B, | ||
| "BACKSPACE" or "BACK" => 0x08, | ||
| "DELETE" or "DEL" => 0x2E, | ||
| "INSERT" or "INS" => 0x2D, | ||
| "HOME" => 0x24, | ||
| "END" => 0x23, | ||
| "PAGEUP" or "PGUP" => 0x21, | ||
| "PAGEDOWN" or "PGDN" => 0x22, | ||
| "UP" => 0x26, | ||
| "DOWN" => 0x28, | ||
| "LEFT" => 0x25, | ||
| "RIGHT" => 0x27, | ||
| "PRINTSCREEN" or "PRTSC" => 0x2C, | ||
| "PAUSE" => 0x13, | ||
| "NUMLOCK" => 0x90, | ||
| "SCROLLLOCK" => 0x91, | ||
| "CAPSLOCK" => 0x14, | ||
| _ => 0 | ||
| }; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Get the key string from a WPF Key value. | ||
| /// </summary> | ||
| public static string GetKeyString(Key key) | ||
| { | ||
| return key switch | ||
| { | ||
| Key.None => "", | ||
| >= Key.A and <= Key.Z => key.ToString(), | ||
| >= Key.D0 and <= Key.D9 => ((int)key - (int)Key.D0).ToString(), | ||
| >= Key.NumPad0 and <= Key.NumPad9 => "NumPad" + ((int)key - (int)Key.NumPad0), | ||
| >= Key.F1 and <= Key.F24 => key.ToString(), | ||
| Key.Space => "Space", | ||
| Key.Tab => "Tab", | ||
| Key.Enter => "Enter", | ||
| Key.Escape => "Escape", | ||
| Key.Back => "Backspace", | ||
| Key.Delete => "Delete", | ||
| Key.Insert => "Insert", | ||
| Key.Home => "Home", | ||
| Key.End => "End", | ||
| Key.PageUp => "PageUp", | ||
| Key.PageDown => "PageDown", | ||
| Key.Up => "Up", | ||
| Key.Down => "Down", | ||
| Key.Left => "Left", | ||
| Key.Right => "Right", | ||
| _ => key.ToString() | ||
| }; | ||
| } |
There was a problem hiding this comment.
The HotkeyManager class, which contains critical hotkey registration and management logic, lacks test coverage. While UI testing can be challenging, consider adding unit tests for the pure logic methods like GetVirtualKeyCode and GetKeyString to ensure correct key mapping, especially for edge cases like function keys F13-F24, numpad keys, and special characters.
| public async Task<ApplyResult> ApplyByIdAsync(string profileId, bool launchMissing) | ||
| { | ||
| var profile = _store.FindById(profileId); | ||
| if (profile == null) | ||
| { | ||
| return new ApplyResult(0, 0, new List<string> { "プロファイルが見つかりません" }); | ||
| } | ||
|
|
||
| return await ApplyProfileAsync(profile, launchMissing); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Apply a profile by name. | ||
| /// </summary> | ||
| public async Task<ApplyResult> ApplyByNameAsync(string profileName, bool launchMissing) | ||
| { | ||
| var profile = _store.FindByName(profileName); | ||
| if (profile == null) | ||
| { | ||
| return new ApplyResult(0, 0, new List<string> { "プロファイルが見つかりません" }); | ||
| } | ||
|
|
||
| return await ApplyProfileAsync(profile, launchMissing); | ||
| } |
There was a problem hiding this comment.
The ProfileApplier class lacks test coverage. This is a critical component that applies window layouts and can launch applications. Consider adding tests for ApplyByIdAsync and ApplyByNameAsync to verify profile lookup, window matching, and error handling for scenarios like missing profiles or invalid window handles.
1 participant
No description provided.