Implement auto-switching to English when the option is enabled #3366
Conversation
Yusyuriv
requested review from
JohnTheGr8,
onesounds,
VictoriousRaptor,
jjw24,
taooceros and
Jack251970
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: onesounds, Jack251970 onesounds, Jack251970 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis pull request introduces a new helper class to manage keyboard layouts on Windows systems. The Changes
Sequence Diagram(s)sequenceDiagram
participant MV as MainViewModel
participant K as KeyboardLayoutHelper
participant API as Windows API
MV->>K: On Show() - if StartWithEnglishMode enabled
K->>API: Retrieve installed keyboard layouts
API-->>K: Return layout list
K->>API: Activate English layout & store current layout
MV->>K: On Hide() - if StartWithEnglishMode enabled
K->>API: Restore previous keyboard layout
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (6)
Flow.Launcher/Helper/KeyboardLayoutHelper.cs (4)
1-66: Good implementation of Windows APIs for keyboard layout managementThe implementation correctly uses P/Invoke to access the necessary Windows APIs for keyboard layout management. The
FindEnglishKeyboardLayoutmethod effectively retrieves and filters keyboard layouts to identify English ones.However, the current language detection method could be improved:
Consider using explicit English language IDs instead of string comparison for more robust language detection. The current approach using string contains "english" might not work reliably across all Windows versions and language configurations.
- private const string LAYOUT_ENGLISH_SEARCH = "english"; + // English language ID (0x0409 = US English, 0x0809 = UK English, etc.) + private static readonly uint[] ENGLISH_LANGUAGE_IDS = { 0x0409, 0x0809, 0x0c09, 0x1009, 0x1409, 0x1809, 0x1c09, 0x2009, 0x2409, 0x2809, 0x2c09, 0x3009, 0x3409 }; private static IntPtr FindEnglishKeyboardLayout() { // Get the number of keyboard layouts var count = GetKeyboardLayoutList(0, null); if (count <= 0) return IntPtr.Zero; // Get all keyboard layouts var keyboardLayouts = new IntPtr[count]; GetKeyboardLayoutList(count, keyboardLayouts); // Look for any English keyboard layout foreach (var layout in keyboardLayouts) { // The lower word contains the language identifier var langId = (uint)layout.ToInt32() & 0xFFFF; - - // Get language name for the layout - var sb = new StringBuilder(256); - GetLocaleInfoA(langId, LOCALE_SLANGUAGE, sb, sb.Capacity); - var langName = sb.ToString().ToLowerInvariant(); - - // Check if it's an English layout - if (langName.Contains(LAYOUT_ENGLISH_SEARCH)) + // Check if it's an English language ID + if (Array.IndexOf(ENGLISH_LANGUAGE_IDS, langId) >= 0) { return layout; } } return IntPtr.Zero; }
68-87: Consider adding thread safety to the layout switching mechanismThe
_previousLayoutstatic field is shared and could cause race conditions if layout switching is triggered from multiple threads simultaneously.Add thread synchronization to prevent potential race conditions:
- private static IntPtr _previousLayout; + private static IntPtr _previousLayout; + private static readonly object _layoutLock = new object(); public static void SetEnglishKeyboardLayout() { - // Find an installed English layout - var englishLayout = FindEnglishKeyboardLayout(); - - // No installed English layout found - if (englishLayout == IntPtr.Zero) return; - - var hwnd = GetForegroundWindow(); - var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero); - - // Store current keyboard layout - _previousLayout = GetKeyboardLayout(threadId) & 0xFFFF; - - // Switch to English layout - ActivateKeyboardLayout(englishLayout, 0); + lock (_layoutLock) + { + // Find an installed English layout + var englishLayout = FindEnglishKeyboardLayout(); + + // No installed English layout found + if (englishLayout == IntPtr.Zero) return; + + var hwnd = GetForegroundWindow(); + var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero); + + // Store current keyboard layout + _previousLayout = GetKeyboardLayout(threadId); + + // Switch to English layout + ActivateKeyboardLayout(englishLayout, 0); + } }Also, the bitwise AND operation with 0xFFFF might be unnecessary when storing the layout handle. This operation extracts only the language ID portion but discards the keyboard type information.
89-94: Apply thread safety to SetPreviousKeyboardLayout method as wellSimilar to the
SetEnglishKeyboardLayoutmethod, this method should also be thread-safe.public static void SetPreviousKeyboardLayout() { - if (_previousLayout == IntPtr.Zero) return; - ActivateKeyboardLayout(_previousLayout, 0); - _previousLayout = IntPtr.Zero; + lock (_layoutLock) + { + if (_previousLayout == IntPtr.Zero) return; + ActivateKeyboardLayout(_previousLayout, 0); + _previousLayout = IntPtr.Zero; + } }
7-95: Add error handling and logging for diagnosticsSince this is an experimental feature and involves Windows API calls that might fail, consider adding error handling and logging.
Add try-catch blocks and logging to help with debugging:
public static void SetEnglishKeyboardLayout() { + try + { // Find an installed English layout var englishLayout = FindEnglishKeyboardLayout(); // No installed English layout found if (englishLayout == IntPtr.Zero) return; var hwnd = GetForegroundWindow(); var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero); // Store current keyboard layout _previousLayout = GetKeyboardLayout(threadId) & 0xFFFF; // Switch to English layout ActivateKeyboardLayout(englishLayout, 0); + } + catch (Exception ex) + { + // Log the exception but don't crash the application + Log.Error("KeyboardLayoutHelper", $"Error switching to English layout: {ex.Message}", ex); + } } public static void SetPreviousKeyboardLayout() { + try + { if (_previousLayout == IntPtr.Zero) return; ActivateKeyboardLayout(_previousLayout, 0); _previousLayout = IntPtr.Zero; + } + catch (Exception ex) + { + // Log the exception but don't crash the application + Log.Error("KeyboardLayoutHelper", $"Error restoring previous layout: {ex.Message}", ex); + } }Also consider adding a simple way to detect if an English layout is already active to avoid unnecessary switching:
public static bool IsEnglishLayoutActive() { try { var hwnd = GetForegroundWindow(); var threadId = GetWindowThreadProcessId(hwnd, IntPtr.Zero); var currentLayout = GetKeyboardLayout(threadId); var langId = (uint)currentLayout.ToInt32() & 0xFFFF; // Check if it's already an English language ID return Array.IndexOf(ENGLISH_LANGUAGE_IDS, langId) >= 0; } catch (Exception ex) { Log.Error("KeyboardLayoutHelper", $"Error checking if English layout is active: {ex.Message}", ex); return false; } }Flow.Launcher/ViewModel/MainViewModel.cs (2)
1377-1382: Good integration of keyboard layout switching on window showThe implementation correctly calls
KeyboardLayoutHelper.SetEnglishKeyboardLayout()whenStartWithEnglishModeis true, which fulfills the PR objective.Consider adding a check to avoid unnecessary layout switching if English is already the active layout:
if (StartWithEnglishMode) { + // Only switch if not already using English layout + if (!KeyboardLayoutHelper.IsEnglishLayoutActive()) KeyboardLayoutHelper.SetEnglishKeyboardLayout(); }This would require adding the
IsEnglishLayoutActive()method toKeyboardLayoutHelperas suggested in the previous comment.
1377-1455: Consider adding exception handling for keyboard layout operationsSince keyboard layout operations involve native API calls that could potentially fail, adding try-catch blocks would make the code more robust.
Add try-catch blocks around keyboard layout operations to prevent exceptions from affecting the main application flow:
if (StartWithEnglishMode) { + try + { KeyboardLayoutHelper.SetEnglishKeyboardLayout(); + } + catch (Exception ex) + { + Log.Error("MainViewModel", $"Error setting English keyboard layout: {ex.Message}", ex); + // Continue execution despite the error + } }And similarly for the hide method:
if (StartWithEnglishMode) { + try + { KeyboardLayoutHelper.SetPreviousKeyboardLayout(); + } + catch (Exception ex) + { + Log.Error("MainViewModel", $"Error restoring previous keyboard layout: {ex.Message}", ex); + // Continue execution despite the error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/Helper/KeyboardLayoutHelper.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
1451-1455: Good implementation of layout restoration on window hideThe code appropriately restores the previous keyboard layout when hiding the window, fulfilling the PR objective.
17-17: Appropriate namespace inclusionThe using directive for
Flow.Launcher.Helperis correctly added to access the newKeyboardLayoutHelperclass.
… of relying on layout name as a string
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I have no idea how to test it. So could you please test it to check if PInvoke introduction has broken sth? |
Yes, with this version the keyboard layout switches to English, but seems to never switch back. |
|
To test it, change your keyboard layout to a non-English one, then open Flow. It should switch to English. Close Flow. It should switch to the orignial layout. |
|
Get it |
This comment has been minimized.
This comment has been minimized.
|
Fixed. Now it works well. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This reverts commit 4146f4d.
This comment has been minimized.
This comment has been minimized.
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 ...
|
Warning
DO NOT MERGE YET. Highly experimental, needs thorough testing with both IME and non-IME languages; also needs someone to confirm there are no memory leaks with using the native Windows APIs.
Fixes #2108, fixes #2569, fixes #3126.
What it does
When the
Always Start Typing in English Modeoption is enabled, when the Flow Launcher window opens, it remembers the current keyboard layout and then switches to English. When the window hides, it restores the remembered keyboard layout.Testing I've done
Known issues
Things to consider