Add exception handling for register unregister hotkey #3263
Conversation
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
jjw24
force-pushed
the
add_exception_handling_register_unregister_hotkey
branch
from
deccf1a to
54a49d6
Compare
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThis change improves the hotkey management system by introducing error handling and logging in the hotkey registration and removal processes. The modifications include wrapping the hotkey-related methods with try-catch blocks, logging exceptions, and providing user feedback when errors occur. Additionally, new methods supporting the ChefKeys library have been added, and a new language resource for failed hotkey unregistration has been introduced in the application’s language file. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant HM as HotKeyMapper
participant CK as ChefKeys Library
participant Log as Logger
participant UI as UI Display
U->>HM: Request hotkey registration
HM->>CK: Register hotkey with ChefKeys
alt Registration successful
CK-->>HM: Hotkey registered
HM->>UI: Confirm registration
else Exception occurs
CK-->>HM: Exception thrown
HM->>Log: Log error details
HM->>UI: Display error message
end
sequenceDiagram
participant HM as HotKeyMapper
participant CK as ChefKeys Library
participant Log as Logger
participant UI as UI
HM->>HM: OnToggleHotkeyWithChefKeys() invoked
alt Hotkey active
HM->>HM: Call RemoveWithChefKeys
HM->>CK: Unregister hotkey
alt Removal successful
CK-->>HM: Hotkey unregistered
HM->>UI: Confirm removal
else Exception occurs
CK-->>HM: Exception thrown
HM->>Log: Log error details
HM->>UI: Display removal error
end
else
HM->>HM: Call SetWithChefKeys
HM->>CK: Register hotkey
alt Registration successful
CK-->>HM: Hotkey registered
HM->>UI: Confirm registration
else Exception occurs
CK-->>HM: Exception thrown
HM->>Log: Log error details
HM->>UI: Display registration error
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🥷 Code experts: Yusyuriv, onesounds Yusyuriv, jjw24 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🧹 Nitpick comments (5)
Flow.Launcher/Helper/HotKeyMapper.cs (1)
66-90: Reduce code duplication in error handling.The error handling code is duplicated between SetHotkey and SetWithChefKeys methods. Consider extracting the common error handling logic into a separate method.
+private static void LogHotkeyError(Exception e, string methodName, string hotkeyStr) +{ + Log.Error( + string.Format("|HotkeyMapper.{0}|Error registering hotkey {1}: {2} \nStackTrace:{3}", + methodName, + hotkeyStr, + e.Message, + e.StackTrace)); + string errorMsg = string.Format(InternationalizationManager.Instance.GetTranslation("registerHotkeyFailed"), hotkeyStr); + string errorMsgTitle = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); + MessageBoxEx.Show(errorMsg, errorMsgTitle); +} internal static void SetHotkey(HotkeyModel hotkey, EventHandler<HotkeyEventArgs> action) { string hotkeyStr = hotkey.ToString(); try { if (hotkeyStr == "LWin" || hotkeyStr == "RWin") { SetWithChefKeys(hotkeyStr); return; } HotkeyManager.Current.AddOrReplace(hotkeyStr, hotkey.CharKey, hotkey.ModifierKeys, action); } catch (Exception e) { - Log.Error( - string.Format("|HotkeyMapper.SetHotkey|Error registering hotkey {2}: {0} \nStackTrace:{1}", - e.Message, - e.StackTrace, - hotkeyStr)); - string errorMsg = string.Format(InternationalizationManager.Instance.GetTranslation("registerHotkeyFailed"), hotkeyStr); - string errorMsgTitle = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); - MessageBoxEx.Show(errorMsg, errorMsgTitle); + LogHotkeyError(e, "SetHotkey", hotkeyStr); } }Flow.Launcher/HotkeyControlDialog.xaml.cs (3)
37-37: Make isOpenFlowHotkey readonly and document temporary nature.The field should be marked as readonly since it's only set in the constructor. Additionally, the TODO comment should be expanded to explain why this is temporary.
-private static bool isOpenFlowHotkey; +/// <summary> +/// Temporary flag to enforce changing only the open flow hotkey to Win. +/// This will be removed by PR #3157 which implements a more permanent solution. +/// </summary> +private static readonly bool isOpenFlowHotkey; // In constructor: -// TODO: This is a temporary way to enforce changing only the open flow hotkey to Win, and will be removed by PR #3157 isOpenFlowHotkey = _hotkeySettings.RegisteredHotkeys .Any(x => x.DescriptionResourceKey == "flowlauncherHotkey" && x.Hotkey.ToString() == hotkey);Also applies to: 53-56
73-80: Extract and improve ChefKeys cleanup.The ChefKeys cleanup code is duplicated and lacks error handling. Extract it to a separate method with proper error handling.
+private void CleanupChefKeys() +{ + try + { + ChefKeysManager.StartMenuEnableBlocking = false; + ChefKeysManager.Stop(); + } + catch (Exception e) + { + Log.Error( + string.Format("|HotkeyControlDialog.CleanupChefKeys|Error: {0} \nStackTrace:{1}", + e.Message, + e.StackTrace)); + } +} private void Cancel(object sender, RoutedEventArgs routedEventArgs) { - ChefKeysManager.StartMenuEnableBlocking = false; - ChefKeysManager.Stop(); + CleanupChefKeys(); ResultType = EResultType.Cancel; Hide(); } private void Save(object sender, RoutedEventArgs routedEventArgs) { - ChefKeysManager.StartMenuEnableBlocking = false; - ChefKeysManager.Stop(); + CleanupChefKeys(); // ... rest of the method }Also applies to: 82-96
191-197: Document Windows key handling behavior.The special case handling for Windows keys should be documented to explain the behavior and its temporary nature.
+/// <summary> +/// Checks if a hotkey is available for use. +/// </summary> +/// <param name="hotkey">The hotkey to check.</param> +/// <param name="validateKeyGesture">Whether to validate the key gesture.</param> +/// <returns>True if the hotkey is available, false otherwise.</returns> +/// <remarks> +/// Temporarily allows Windows keys (LWin/RWin) for the open flow hotkey. +/// This special case will be removed by PR #3157. +/// </remarks> private static bool CheckHotkeyAvailability(HotkeyModel hotkey, bool validateKeyGesture) { if (isOpenFlowHotkey && (hotkey.ToString() == "LWin" || hotkey.ToString() == "RWin")) return true; return hotkey.Validate(validateKeyGesture) && HotKeyMapper.CheckAvailability(hotkey); }Flow.Launcher/HotkeyControl.xaml.cs (1)
153-182: Extract hotkey constants and improve documentation.The Windows key handling should be improved by:
- Extracting magic strings to constants
- Adding XML documentation
- Clarifying the temporary nature of the solution
+/// <summary> +/// Constants for special hotkeys that require ChefKeys handling. +/// </summary> +private static class SpecialHotkeys +{ + public const string LeftWindows = "LWin"; + public const string RightWindows = "RWin"; +} +/// <summary> +/// Sets a hotkey with optional validation. +/// </summary> +/// <param name="keyModel">The hotkey model to set.</param> +/// <param name="triggerValidate">Whether to validate the hotkey.</param> +/// <remarks> +/// Temporarily allows Windows keys (LWin/RWin) without validation. +/// This special case will be removed by PR #3157. +/// </remarks> private void SetHotkey(HotkeyModel keyModel, bool triggerValidate = true) { if (triggerValidate) { bool hotkeyAvailable = false; - // TODO: This is a temporary way to enforce changing only the open flow hotkey to Win, and will be removed by PR #3157 - if (keyModel.ToString() == "LWin" || keyModel.ToString() == "RWin") + string hotkeyStr = keyModel.ToString(); + if (hotkeyStr == SpecialHotkeys.LeftWindows || hotkeyStr == SpecialHotkeys.RightWindows) { hotkeyAvailable = true; } else { hotkeyAvailable = CheckHotkeyAvailability(keyModel, ValidateKeyGesture); } // ... rest of the method } }
🛑 Comments failed to post (5)
Flow.Launcher/Helper/HotKeyMapper.cs (4)
35-39: 🛠️ Refactor suggestion
Add null check for _mainViewModel.
The method should validate that _mainViewModel is not null before accessing it to prevent potential NullReferenceException.
internal static void OnToggleHotkeyWithChefKeys() { + if (_mainViewModel == null) + return; + if (!_mainViewModel.ShouldIgnoreHotkeys()) _mainViewModel.ToggleFlowLauncher(); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.internal static void OnToggleHotkeyWithChefKeys() { if (_mainViewModel == null) return; if (!_mainViewModel.ShouldIgnoreHotkeys()) _mainViewModel.ToggleFlowLauncher(); }
47-64: 🛠️ Refactor suggestion
Improve error handling in ChefKeys registration.
The current implementation has two issues:
- ChefKeysManager.Start() is called after registration, which could leave the system in an inconsistent state if Start() fails
- No cleanup is performed if registration fails
private static void SetWithChefKeys(string hotkeyStr) { try { + ChefKeysManager.Start(); ChefKeysManager.RegisterHotkey(hotkeyStr, hotkeyStr, OnToggleHotkeyWithChefKeys); - ChefKeysManager.Start(); } catch (Exception e) { + try + { + ChefKeysManager.UnregisterHotkey(hotkeyStr); + ChefKeysManager.Stop(); + } + catch (Exception cleanupEx) + { + Log.Error( + string.Format("|HotkeyMapper.SetWithChefKeys|Error during cleanup: {0} \nStackTrace:{1}", + cleanupEx.Message, + cleanupEx.StackTrace)); + } + Log.Error( string.Format("|HotkeyMapper.SetWithChefKeys|Error registering hotkey: {0} \nStackTrace:{1}", e.Message, e.StackTrace)); string errorMsg = string.Format(InternationalizationManager.Instance.GetTranslation("registerHotkeyFailed"), hotkeyStr); string errorMsgTitle = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); MessageBoxEx.Show(errorMsg, errorMsgTitle); } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private static void SetWithChefKeys(string hotkeyStr) { try { ChefKeysManager.Start(); ChefKeysManager.RegisterHotkey(hotkeyStr, hotkeyStr, OnToggleHotkeyWithChefKeys); } catch (Exception e) { try { ChefKeysManager.UnregisterHotkey(hotkeyStr); ChefKeysManager.Stop(); } catch (Exception cleanupEx) { Log.Error( string.Format("|HotkeyMapper.SetWithChefKeys|Error during cleanup: {0} \nStackTrace:{1}", cleanupEx.Message, cleanupEx.StackTrace)); } Log.Error( string.Format("|HotkeyMapper.SetWithChefKeys|Error registering hotkey: {0} \nStackTrace:{1}", e.Message, e.StackTrace)); string errorMsg = string.Format(InternationalizationManager.Instance.GetTranslation("registerHotkeyFailed"), hotkeyStr); string errorMsgTitle = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); MessageBoxEx.Show(errorMsg, errorMsgTitle); } }
92-115: 🛠️ Refactor suggestion
Add null check for hotkeyStr parameter.
The method should validate that hotkeyStr is not null before using it to prevent potential NullReferenceException.
internal static void RemoveHotkey(string hotkeyStr) { + if (string.IsNullOrEmpty(hotkeyStr)) + return; + try { if (hotkeyStr == "LWin" || hotkeyStr == "RWin") { RemoveWithChefKeys(hotkeyStr); return; } - if (!string.IsNullOrEmpty(hotkeyStr)) - HotkeyManager.Current.Remove(hotkeyStr); + HotkeyManager.Current.Remove(hotkeyStr); } catch (Exception e) { Log.Error( string.Format("|HotkeyMapper.RemoveHotkey|Error removing hotkey: {0} \nStackTrace:{1}", e.Message, e.StackTrace)); string errorMsg = string.Format(InternationalizationManager.Instance.GetTranslation("unregisterHotkeyFailed"), hotkeyStr); string errorMsgTitle = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); MessageBoxEx.Show(errorMsg, errorMsgTitle); } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.internal static void RemoveHotkey(string hotkeyStr) { if (string.IsNullOrEmpty(hotkeyStr)) return; try { if (hotkeyStr == "LWin" || hotkeyStr == "RWin") { RemoveWithChefKeys(hotkeyStr); return; } HotkeyManager.Current.Remove(hotkeyStr); } catch (Exception e) { Log.Error( string.Format("|HotkeyMapper.RemoveHotkey|Error removing hotkey: {0} \nStackTrace:{1}", e.Message, e.StackTrace)); string errorMsg = string.Format(InternationalizationManager.Instance.GetTranslation("unregisterHotkeyFailed"), hotkeyStr); string errorMsgTitle = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); MessageBoxEx.Show(errorMsg, errorMsgTitle); } }
117-121: 🛠️ Refactor suggestion
Add error handling for ChefKeys cleanup operations.
The method should handle potential exceptions from ChefKeys operations to ensure proper cleanup.
private static void RemoveWithChefKeys(string hotkeyStr) { - ChefKeysManager.UnregisterHotkey(hotkeyStr); - ChefKeysManager.Stop(); + try + { + ChefKeysManager.UnregisterHotkey(hotkeyStr); + } + catch (Exception e) + { + Log.Error( + string.Format("|HotkeyMapper.RemoveWithChefKeys|Error unregistering hotkey: {0} \nStackTrace:{1}", + e.Message, + e.StackTrace)); + throw; + } + finally + { + try + { + ChefKeysManager.Stop(); + } + catch (Exception e) + { + Log.Error( + string.Format("|HotkeyMapper.RemoveWithChefKeys|Error stopping ChefKeys: {0} \nStackTrace:{1}", + e.Message, + e.StackTrace)); + } + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private static void RemoveWithChefKeys(string hotkeyStr) { try { ChefKeysManager.UnregisterHotkey(hotkeyStr); } catch (Exception e) { Log.Error( string.Format("|HotkeyMapper.RemoveWithChefKeys|Error unregistering hotkey: {0} \nStackTrace:{1}", e.Message, e.StackTrace)); throw; } finally { try { ChefKeysManager.Stop(); } catch (Exception e) { Log.Error( string.Format("|HotkeyMapper.RemoveWithChefKeys|Error stopping ChefKeys: {0} \nStackTrace:{1}", e.Message, e.StackTrace)); } } }Flow.Launcher/HotkeyControlDialog.xaml.cs (1)
58-59: 🛠️ Refactor suggestion
Add error handling for ChefKeys initialization.
The ChefKeys setup operations should include error handling to provide user feedback if initialization fails.
-ChefKeysManager.StartMenuEnableBlocking = true; -ChefKeysManager.Start(); +try +{ + ChefKeysManager.StartMenuEnableBlocking = true; + ChefKeysManager.Start(); +} +catch (Exception e) +{ + Log.Error( + string.Format("|HotkeyControlDialog.ctor|Error initializing ChefKeys: {0} \nStackTrace:{1}", + e.Message, + e.StackTrace)); + string errorMsg = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); + MessageBoxEx.Show("Failed to initialize hotkey manager. Some hotkeys may not work.", errorMsg); +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { ChefKeysManager.StartMenuEnableBlocking = true; ChefKeysManager.Start(); } catch (Exception e) { Log.Error( string.Format("|HotkeyControlDialog.ctor|Error initializing ChefKeys: {0} \nStackTrace:{1}", e.Message, e.StackTrace)); string errorMsg = InternationalizationManager.Instance.GetTranslation("MessageBoxTitle"); MessageBoxEx.Show("Failed to initialize hotkey manager. Some hotkeys may not work.", errorMsg); }
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
Follows on from #3262, adding exception handling.