Enable the use of Win hotkey to trigger flow #3262
Conversation
jjw24
requested review from
Garulf,
Yusyuriv,
onesounds,
VictoriousRaptor,
deefrawley,
Zeroto521,
taooceros,
Jack251970,
z1nc0r3 and
cibere
jjw24
changed the title
|
🥷 Code experts: Yusyuriv, Jack251970 Jack251970 has 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:
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 |
|
This PR contains a TODO statement. Please check to see if they should be removed. |
jjw24
changed the title
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
@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 ...
|
📝 WalkthroughWalkthroughThe changes integrate a new dependency and extend hotkey management for specific Windows keys ("LWin" and "RWin"). A new package reference is added to the project file, and the hotkey handlers in the helper and UI classes are updated to register/unregister these keys using the ChefKeys library. Additionally, the hotkey availability logic in the UI components is refined, and the hotkey dialog now manages the blocking state of the start menu through ChefKeysManager. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HotkeyControl as HotkeyControl.xaml.cs
participant Mapper as HotKeyMapper
participant ChefKeys as ChefKeysManager
User->>HotkeyControl: SetHotkey(keyModel)
HotkeyControl->>Mapper: Validate hotkey (checks for "LWin"/"RWin")
alt If key is "LWin" or "RWin"
Mapper->>ChefKeys: Call SetWithChefKeys(key)
ChefKeys-->>Mapper: Hotkey registered
else
Mapper-->>HotkeyControl: Proceed with normal validation
end
sequenceDiagram
participant User
participant Dialog as HotkeyControlDialog.xaml.cs
participant ChefKeys as ChefKeysManager
User->>Dialog: Open hotkey dialog
Dialog->>Dialog: Check registered hotkeys (set isOpenFlowHotkey)
Dialog->>ChefKeys: Start() to enable start menu blocking
User->>Dialog: Cancel/Save dialog
Dialog->>ChefKeys: Stop() to disable start menu blocking
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Flow.Launcher/HotkeyControlDialog.xaml.cs (1)
38-60:⚠️ Potential issueEnsure ChefKeys cleanup in all exit paths.
The ChefKeys state is properly cleaned up in Cancel and Save methods, but not in case of exceptions. Consider using a try-finally block in the constructor.
public HotkeyControlDialog(string hotkey, string defaultHotkey, IHotkeySettings hotkeySettings, string windowTitle = "") { - WindowTitle = windowTitle switch - { - "" or null => InternationalizationManager.Instance.GetTranslation("hotkeyRegTitle"), - _ => windowTitle - }; - DefaultHotkey = defaultHotkey; - CurrentHotkey = new HotkeyModel(hotkey); - _hotkeySettings = hotkeySettings; - SetKeysToDisplay(CurrentHotkey); + try + { + WindowTitle = windowTitle switch + { + "" or null => InternationalizationManager.Instance.GetTranslation("hotkeyRegTitle"), + _ => windowTitle + }; + DefaultHotkey = defaultHotkey; + CurrentHotkey = new HotkeyModel(hotkey); + _hotkeySettings = hotkeySettings; + SetKeysToDisplay(CurrentHotkey); - InitializeComponent(); + InitializeComponent(); - isOpenFlowHotkey = _hotkeySettings.RegisteredHotkeys - .Any(x => x.DescriptionResourceKey == "flowlauncherHotkey" - && x.Hotkey.ToString() == hotkey); + isOpenFlowHotkey = _hotkeySettings.RegisteredHotkeys + .Any(x => x.DescriptionResourceKey == "flowlauncherHotkey" + && x.Hotkey.ToString() == hotkey); - ChefKeysManager.StartMenuEnableBlocking = true; - ChefKeysManager.Start(); + ChefKeysManager.StartMenuEnableBlocking = true; + ChefKeysManager.Start(); + } + catch + { + ChefKeysManager.StartMenuEnableBlocking = false; + ChefKeysManager.Stop(); + throw; + } }
🧹 Nitpick comments (2)
Flow.Launcher/Helper/HotKeyMapper.cs (1)
40-50: Refactor duplicate Windows key checks.The Windows key check logic is duplicated in both
SetHotkeymethods. Consider extracting this check into a private helper method.+ private static bool IsWindowsKey(string hotkeyStr) + { + return hotkeyStr == "LWin" || hotkeyStr == "RWin"; + } + private static void SetHotkey(string hotkeyStr, EventHandler<HotkeyEventArgs> action) { - if (hotkeyStr == "LWin" || hotkeyStr == "RWin") + if (IsWindowsKey(hotkeyStr)) { SetWithChefKeys(hotkeyStr); return; } // ... rest of the method } internal static void SetHotkey(HotkeyModel hotkey, EventHandler<HotkeyEventArgs> action) { string hotkeyStr = hotkey.ToString(); - if (hotkeyStr == "LWin" || hotkeyStr == "RWin") + if (IsWindowsKey(hotkeyStr)) { SetWithChefKeys(hotkeyStr); return; } // ... rest of the method }Also applies to: 58-67
Flow.Launcher/HotkeyControl.xaml.cs (1)
157-166: Simplify hotkey validation logic.The boolean logic for hotkey validation can be simplified for better readability.
- 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") - { - hotkeyAvailable = true; - } - else - { - hotkeyAvailable = CheckHotkeyAvailability(keyModel, ValidateKeyGesture); - } + // TODO: This is a temporary way to enforce changing only the open flow hotkey to Win, and will be removed by PR #3157 + bool hotkeyAvailable = keyModel.ToString() is "LWin" or "RWin" || + CheckHotkeyAvailability(keyModel, ValidateKeyGesture);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher/Flow.Launcher.csproj(1 hunks)Flow.Launcher/Helper/HotKeyMapper.cs(3 hunks)Flow.Launcher/HotkeyControl.xaml.cs(1 hunks)Flow.Launcher/HotkeyControlDialog.xaml.cs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher/HotkeyControlDialog.xaml.cs (1)
53-56:❓ Verification inconclusive
TODO comment indicates temporary implementation.
The comment suggests this is a temporary solution that will be replaced by PR #3157. Please ensure this is tracked and removed when PR #3157 is merged.
🏁 Script executed:
#!/bin/bash # Check if PR #3157 exists and its status gh pr view 3157 --json title,state,mergeStateStatusLength of output: 166
Temporary Workaround – Ensure Timely Removal
The TODO comment correctly notes that this is a temporary solution pending the changes proposed in PR #3157. Since PR #3157 is still open (currently in a BLOCKED state), please ensure that:
- The temporary code remains documented as a temporary workaround.
- The implementation is revisited and removed once PR Support the use of Windows key & four-combination hotkey #3157 becomes mergeable and is merged.
Flow.Launcher/Flow.Launcher.csproj (1)
86-86:✅ Verification successful
Verify ChefKeys package version.
The package version 0.1.2 seems to be an early release. Please verify if this is the latest stable version.
🌐 Web query:
What is the latest version of ChefKeys NuGet package?💡 Result:
The latest version of the ChefKeys NuGet package is 0.1.2, as indicated by its package summary on SOOS[7]. This version was reported to have no vulnerabilities at the time of documentation[7].
For reference, ChefKeys provides functionality related to handling Win key or keyboard combinations in applications[7].
Citations:
- 1: https://learn.microsoft.com/en-us/nuget/concepts/package-versioning
- 2: https://www.nuget.org/packages/NuGet.Versioning
- 3: https://docs.chef.io/release_notes_automate/
- 4: https://community.chocolatey.org/packages/nuget.commandline
- 5: https://community.chocolatey.org/packages?q=Nuget
- 6: https://help.cloudsmith.io/docs/nuget-feed
- 7: https://app.soos.io/research/packages/NuGet/-/ChefKeys
- 8: How to get the version list and latest version of NuGet package in VSCode NuGet/Home#8731
- 9: https://learn.microsoft.com/en-us/nuget/release-notes/
- 10: https://docs.chef.io/release_notes_client/
ChefKeys package version has been verified as the latest stable release.
The ChefKeys NuGet package is confirmed to be at version 0.1.2, which is indeed the latest stable version available. There is no need to update the version reference in
Flow.Launcher/Flow.Launcher.csproj.
|
Hi @Jack251970 have you had a look at this PR and is there anything I need to fix? |
This looks good to me and I don’t have any changes to suggest. I also tested it with the installer, and it works as expected.👍 |
Split from #3157, this PR focuses exclusively on letting the LWin/RWin to be used for opening flow. It is split so I can add this feature in while I work on the support for key combinations.
Source code for ChefKeys that handles the Win key operations in this change is available at https://github.com/jjw24/ChefKeys.
Most of this code is temporary and will be replaced by PR 3157.
Tested:
Close #662, #2709, #1358