Korean IME Infobar #3426
Korean IME Infobar #3426
Conversation
This comment has been minimized.
This comment has been minimized.
# Conflicts: # Flow.Launcher/Languages/en.xaml # Flow.Launcher/Resources/Dark.xaml
This comment has been minimized.
This comment has been minimized.
# Conflicts: # Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs
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.
- Adjust Margin - Cleanup comment
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Jack251970, Yusyuriv onesounds, Jack251970 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:
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 |
|
@coderabbitai Please review this PR, milady. |
📝 WalkthroughWalkthroughThis pull request introduces enhancements for Korean IME support, including new string resources for user guidance and a new InfoBar user control for displaying messages. It also updates the theme resource dictionaries with additional colors for the InfoBar states in both dark and light modes. Functionality is added to the settings view model for managing legacy Korean IME settings through registry interactions and commands to open the IME settings. The UI in the settings page is modified to conditionally display InfoBar warnings and toggle options for the Korean IME. Changes
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (12)
✨ Finishing Touches
🪧 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 (4)
Flow.Launcher/Resources/Controls/InfoBar.xaml.cs (3)
50-52: Translate Korean comments to English.There are Korean comments in the code that should be translated to English for consistency and maintainability.
- SetValue(MessageProperty, value); - UpdateMessageVisibility(); // Message 속성 변경 시 Visibility 업데이트 + SetValue(MessageProperty, value); + UpdateMessageVisibility(); // Update visibility when Message property changes
76-78: Translate Korean comments to English.There are Korean comments in the code that should be translated to English for consistency and maintainability.
- SetValue(TitleProperty, value); - UpdateTitleVisibility(); // Title 속성 변경 시 Visibility 업데이트 + SetValue(TitleProperty, value); + UpdateTitleVisibility(); // Update visibility when Title property changes
22-22: Remove commented code.This commented line should be removed if not needed, as it adds no value to the codebase.
- //this.DataContext = this;Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (1)
37-38: Consider adding comments for clarity.It would be helpful to add a brief comment explaining why
IsLegacyKoreanIMEEnabled()is called in the constructor.- IsLegacyKoreanIMEEnabled(); + // Initialize the Korean IME status by checking registry + IsLegacyKoreanIMEEnabled();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Flow.Launcher/Languages/en.xaml(3 hunks)Flow.Launcher/Resources/Controls/InfoBar.xaml(1 hunks)Flow.Launcher/Resources/Controls/InfoBar.xaml.cs(1 hunks)Flow.Launcher/Resources/Dark.xaml(1 hunks)Flow.Launcher/Resources/Light.xaml(1 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs(4 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (28)
Flow.Launcher/Resources/Controls/InfoBar.xaml (6)
1-13: Well-structured UserControl declaration with proper namespace and references.The UserControl declaration follows good XAML practices with appropriate namespaces and design-time attributes.
14-22: Good structure with nested Border for InfoBar styling.The outer Grid and Border implementation creates a visually appealing container with proper padding, background and corner radius. The BorderBrush is properly bound to a dynamic resource.
23-28: Well-designed Grid layout with appropriate column definitions.The three-column Grid layout appropriately allocates space for the icon, content, and close button. The Auto-*-Auto pattern is ideal for this kind of control layout.
29-46: Properly implemented icon section with named parts.The icon implementation with Border and FontIcon follows best practices:
- Named parts for template access (Part_IconBorder, PART_Icon)
- Proper alignment and dimensions
- Dynamic resource binding for foreground
48-64: Well-structured content area with proper text styling.The content area correctly implements:
- Proper binding to InfoBar properties
- Semantic styling (semibold for title)
- Text wrapping for the message
- Appropriate spacing between elements
66-76: Properly implemented close button with event handling.The close button is well implemented with:
- Appropriate sizing and alignment
- Named part for access in code-behind
- Event handler for click events
- Font icon for the close glyph
Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (5)
6-6: Properly added converter namespace.The namespace for converters is correctly added to access the BoolToVisibilityConverter.
18-20: Well-implemented converter resource.The BoolToVisibilityConverter is properly registered as a resource with an appropriate key.
224-228: Improved ordering of NumberBox attributes.The reordering of Minimum before Value ensures validation is properly applied when the control is initialized.
320-331: Well-implemented Korean IME warning InfoBar.The InfoBar is correctly:
- Bound for visibility to show only when needed
- Using localized resources for title and message
- Set as non-closable since it represents an important system setting
- Displaying warning state to notify users of potential issues
This implementation fulfills the PR objective of notifying users about Korean IME settings on Windows 11.
332-348: Properly implemented Korean IME settings controls.The controls for managing Korean IME settings are well implemented:
- Grouped logically in a CardGroup
- Show/hide based on registry key existence
- Include both toggle for enabling legacy IME and a button to open settings
- Use proper localization for all text elements
- Follow the application's UI pattern for settings
This implementation fulfills the PR objective of providing controls to enable legacy Korean IME.
Flow.Launcher/Resources/Light.xaml (1)
112-124: Well-designed InfoBar theme resources for light mode.The added resources provide a complete set of styles for the InfoBar control:
- Border color with appropriate transparency
- Icon colors for different states (info, warning, success, error)
- Background colors for different states with appropriate contrast
- Consistent naming pattern matching the existing resources
The InfoBar styles align well with the light theme of the application.
Flow.Launcher/Resources/Dark.xaml (1)
117-129: Good implementation of InfoBar theme resources.The addition of these custom InfoBar resources is well-structured and follows the existing styling patterns in the file. The color choices are appropriately differentiated for each InfoBar state (Info, Warning, Success, Error), providing clear visual distinction for users.
I like how you've set up the system accent color for the info state with proper opacity, while using custom colors for the other states to maintain visual hierarchy.
Flow.Launcher/Resources/Controls/InfoBar.xaml.cs (4)
5-24: Well-structured UserControl implementation.The InfoBar control is cleanly implemented with the proper initialization and event handling. The structure follows good WPF practices, utilizing the Loaded event to trigger initial UI updates.
25-40: Good use of dependency properties.The Type property is properly implemented as a dependency property with change notification. This follows WPF best practices and ensures the UI updates appropriately when the property changes.
154-179: Well-implemented style update mechanism.The UpdateStyle method efficiently handles the appearance changes based on the InfoBar type. It's a clean approach that encapsulates the styling logic in one place, making maintenance easier.
204-216: Clear enum definitions.The enum definitions are clear and concise, providing a good balance between simplicity and functionality. The InfoBarType enum values correspond well to the styles defined in the UpdateStyle method.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (5)
191-207: Well-implemented property with notification.The
LegacyKoreanIMEEnabledproperty correctly handles changes and notifies related properties. The debug logging is helpful for troubleshooting.
209-227: Good implementation of registry checking properties.The
KoreanIMERegistryKeyExistsandKoreanIMERegistryValueIsZeroproperties are well implemented with proper null handling and type checking. This defensive coding approach helps prevent potential exceptions.
250-277: Robust registry manipulation with proper error handling.The
SetLegacyKoreanIMEEnabledmethod properly handles registry operations with appropriate error checking and exception handling. The use of constants for registry paths and value names improves maintainability.
279-300: Safe registry reading implementation.The
GetLegacyKoreanIMERegistryValuemethod safely reads from the registry with proper error handling and resource cleanup. This implementation reduces the risk of resource leaks.
302-312: Simple and effective IME settings opener.The
OpenImeSettingsmethod uses the system settings URI scheme to directly open the relevant settings page. The try-catch block ensures that any errors are properly logged without crashing the application.Flow.Launcher/Languages/en.xaml (6)
113-113: LGTM: Clarified the tooltip text.The updated tooltip text more clearly explains that this setting refers to plugin default delay time, which improves user understanding.
114-118: LGTM: Added helpful search delay time options.These new string resources provide clear descriptions for the various search delay time options, making it more intuitive for users to select their preferred setting.
119-131: LGTM: Excellent Korean IME guidance.The added strings provide comprehensive guidance for Korean IME users on Windows 11, including:
- Clear indication of the potential issue
- Step-by-step instructions for enabling the legacy IME
- Navigation paths to find the relevant settings
- Text for UI buttons to directly access settings
This aligns perfectly with the PR objectives to assist users in managing Korean IME settings on Windows 11.
169-169: LGTM: Added default string resource.The new "Default" string resource will likely be used as a selection option in dropdown menus or settings, providing consistency across the application.
396-396: LGTM: Updated search delay time tips.The updated tip text now instructs users to select a search delay time rather than inputting it, which aligns with a UI change from text input to selection component. The text also clearly explains the function of the default option.
397-398: LGTM: Added current and new search delay time labels.These new string resources provide clear labels for displaying the current and new search delay time settings in the UI, improving user understanding when making changes.
🧩 Analysis chain🏁 Script executed: Length of output: 76733 🏁 Script executed: Length of output: 1538 🏁 Script executed: Length of output: 66489 @onesounds I've reviewed the Korean IME Infobar PR and it looks excellent. The implementation provides an elegant solution for Korean users on Windows 11 who are experiencing IME issues. Key strengths of this PR:
The PR is well-implemented and ready to merge. It will significantly improve the experience for Korean-language users of Flow Launcher on Windows 11. ✅ Actions performedReview triggered.
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (1)
229-233: 🛠️ Refactor suggestionMissing user feedback for failed launches.
The
OpenImeSettingsmethod delegates toWin32Helper.OpenImeSettings()which silently catches exceptions without notifying the user if the operation fails.[RelayCommand] private void OpenImeSettings() { - Win32Helper.OpenImeSettings(); + try + { + Process.Start(new ProcessStartInfo("ms-settings:regionlanguage") { UseShellExecute = true }); + } + catch (Exception e) + { + Debug.WriteLine($"Error opening IME settings: {e.Message}"); + App.API.ShowMsg("Failed to open IME settings", "Please try opening Windows Settings > Time & Language > Language & Region manually."); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/Win32Helper.cs(2 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (1)
Flow.Launcher.Infrastructure/Win32Helper.cs (6)
Win32Helper(20-602)IsLegacyKoreanIMEEnabled(529-543)SetLegacyKoreanIMEEnabled(545-566)IsKoreanIMEExist(524-527)GetLegacyKoreanIMERegistryValue(568-587)OpenImeSettings(589-599)
🔇 Additional comments (8)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (4)
184-190: Well-documented feature rationale.The comment clearly explains the problem with the Korean IME on Windows 11 and WPF applications, along with the solution approach. This documentation is valuable for future maintenance.
191-207: Property implementation handles error cases correctly.The
LegacyKoreanIMEEnabledproperty correctly notifies property changes and provides user feedback on registry update failures. The error message helps users understand what went wrong.
209-209: Good conditional visibility check.This property provides a clean way to conditionally display UI elements only when Korean IME is present on the system.
211-227: Robust value parsing with type checks.The implementation properly handles different potential data types from the registry and includes appropriate fallback parsing. The code handles the case where the registry value might be returned as a string instead of an integer.
Flow.Launcher.Infrastructure/Win32Helper.cs (4)
524-527: Simple and effective existence check.This method provides a clear way to determine if the Korean IME is installed on the system by checking the registry value existence.
529-543: Thorough type handling for registry value.The implementation properly handles both integer values and string values that need parsing, ensuring robust behavior regardless of how the registry stores the value.
545-566: Registry modification with proper error handling pattern.The method correctly handles both the creation of registry keys and setting values with appropriate error handling. The boolean return value allows callers to know whether the operation succeeded or failed.
568-587: Consistent registry access pattern.This method follows the same careful pattern for accessing registry values with proper error handling by returning null when the key doesn't exist or an exception occurs.
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.
There was a problem hiding this comment.
Copilot reviewed 3 out of 8 changed files in this pull request and generated 2 comments.
Files not reviewed (5)
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/Resources/Controls/InfoBar.xaml: Language not supported
- Flow.Launcher/Resources/Dark.xaml: Language not supported
- Flow.Launcher/Resources/Light.xaml: Language not supported
- Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml: Language not supported
Comments suppressed due to low confidence (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs:210
- [nitpick] The property name 'KoreanIMERegistryValueIsZero' may be unclear; consider renaming it to something like 'IsLegacyKoreanIMEDisabled' to more explicitly indicate that a value of 0 means the legacy IME is off.
public bool KoreanIMERegistryValueIsZero
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (2)
191-207: Potential UI desynchronization on failure.
IfSetLegacyKoreanIMEEnabled(value)fails, the UI will not roll back the property, which can confuse users.
240-244: Handle exceptions for failed IME settings launch.
Currently, this call relies on silent exception handling inWin32Helper.OpenImeSettings. Adding a user-facing error message can improve troubleshooting when the launch fails.Flow.Launcher.Infrastructure/Win32Helper.cs (1)
593-603: Provide caller feedback or return status on failure.
Silently ignoring exceptions hides potential issues from the user. Consider returning a boolean or rethrowing.
🧹 Nitpick comments (4)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (1)
222-238: Consider renaming for clarity.
KoreanIMERegistryValueIsZeromight be more descriptive asLegacyIMERegistryValueIsZeroorIsLegacyKoreanIMEDisabledto reflect intent more clearly.Flow.Launcher.Infrastructure/Win32Helper.cs (3)
528-531: Improve method naming.
IsKoreanIMEExistcould be more naturally namedIsKoreanIMEInstalledorDoesKoreanIMEExist.
549-570: Consider propagating exceptions to enable user feedback.
Silently catching exceptions can hinder debugging and prevent users from knowing why changes fail. Either return an error state or rethrow the exception to the caller.
572-591: Likewise, consider surfacing exceptions.
Swallowing registry access exceptions may obscure issues. Returning or logging error details could aid troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/Win32Helper.cs(3 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (1)
Flow.Launcher.Infrastructure/Win32Helper.cs (7)
Win32Helper(20-606)IsLegacyKoreanIMEEnabled(533-547)SetLegacyKoreanIMEEnabled(549-570)IsKoreanIMEExist(528-531)IsWindows11(24-27)GetLegacyKoreanIMERegistryValue(572-591)OpenImeSettings(593-603)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (7)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (4)
3-3: No issues with the added import.
This import is needed for theAny()usage.
10-10: No issues with the added import.
This import is needed for referencingWin32Helper.
184-190: Region commentary looks good.
No concerns with the added region and explanatory comment.
209-220: Confirm logic for Windows 10 scenarios.
If the user is on Windows 10 but the registry key is present, this property returns true, which may display the InfoBar unintentionally.Would you mind verifying behavior on Windows 10? For instance, confirm whether the InfoBar is incorrectly shown due to a leftover registry key. If necessary, we can refine this condition.
Flow.Launcher.Infrastructure/Win32Helper.cs (3)
3-3: No issues with the added import.
This import is necessary for launching processes and debug logging.
24-27: No concerns with IsWindows11 detection.
The straightforward version check suffices for now.
533-547: Logic is clear and concise.
No concerns with checking the registry value and parsing it.
🔄 Changes OverviewIssue
Current Behavior:
Updated Behavior:
Other Notes:
|
|
@coderabbitai review please. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@onesounds I'll review the latest changes to your Korean IME InfoBar PR. Let me check what's been updated since my last review. 🏁 Script executed: Length of output: 70555 🏁 Script executed: Length of output: 336 🏁 Script executed: Length of output: 14340 @onesounds I've reviewed the latest changes to your Korean IME InfoBar PR. This update adds an important enhancement by checking whether the Korean IME is actually installed on the user's system. The key improvement in this change is:
().Any(lang => lang.Culture.Name.StartsWith("ko")); // Return true if Windows 11 with Korean IME installed, or if the registry key exists #!/bin/bash Get the latest commit informationgit log -n 1 --oneline Check what files were changed in the most recent commitgit show --name-only HEAD Show the actual changes in the most recent commitgit show HEAD #!/bin/bash Let's get the specific changes related to checking if Korean IME is installed/in useFirst, let's check the commit message to ensure we're looking at the right commitgit log -n 1 --oneline Let's examine changes to the Win32Helper.cs file which likely contains IME detection logicgit diff HEAD~1 HEAD -- Flow.Launcher.Infrastructure/Win32Helper.cs Let's also check changes to the settings view model which would control visibilitygit diff HEAD~1 HEAD -- Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs And the XAML file that would use the new conditions for visibilitygit diff HEAD~1 HEAD -- Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml #!/bin/bash Since we're having issues with git diff, let's look at the relevant files directlyecho "Examining Win32Helper.cs for Korean IME detection logic" echo -e "\nChecking SettingsPaneGeneralViewModel.cs for Korean IME related properties" echo -e "\nExamining SettingsPaneGeneral.xaml for visibility conditions" Let's also get the current content of these files to better understand the implementationecho -e "\nContent of Win32Helper.cs (Korean IME related methods)" echo -e "\nContent of SettingsPaneGeneralViewModel.cs (Korean IME related properties)" echo -e "\nContent of the InfoBar.xaml.cs file"
This change is well-implemented and makes the feature even more polished. The logic is sound - checking both OS version and installed languages ensures the InfoBar is shown precisely to the target audience who would benefit from it. The PR continues to be ready for merging, with this latest enhancement further improving the user experience. --> |
@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 ...
|
There was a problem hiding this comment.
Copilot reviewed 3 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (5)
- Flow.Launcher/Languages/en.xaml: Language not supported
- Flow.Launcher/Resources/Controls/InfoBar.xaml: Language not supported
- Flow.Launcher/Resources/Dark.xaml: Language not supported
- Flow.Launcher/Resources/Light.xaml: Language not supported
- Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml: Language not supported
Comments suppressed due to low confidence (2)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs:222
- [nitpick] The property name 'KoreanIMERegistryValueIsZero' may be ambiguous as it returns true when the legacy IME setting is off (0). Consider renaming it to 'IsLegacyKoreanIMEDisabled' to more clearly reflect its purpose.
public bool KoreanIMERegistryValueIsZero
Flow.Launcher/Infrastructure/Win32Helper.cs:567
- The exception in the registry write operation is swallowed silently; consider logging the exception details to aid in troubleshooting potential registry access issues.
catch (System.Exception)
What's the PR
On Windows 11, the default Korean IME has issues with WPF.
(You can tell whether the issue is present by checking if an underline appears when typing in Korean.)
To make Korean input work properly in Flow, the "Use legacy IME" setting in the IME options must be enabled.
This issue is difficult to fix within WPF itself — it needs to be handled either by the user or by Microsoft.
On Windows 10, there is no new IME, so this issue does not occur.
InfoBar
Therefore, if this setting option exists, it means the user is using Korean + Windows 11.
These three elements are shown only when the setting exists and legacy IME is currently off.
If the setting doesn't exist, or legacy IME is already enabled, or was just switched on, the InfoBar is hidden.
And......
Add Infobar UserControl
Add a toggle button to enable or disable the legacy Korean IME registry setting
Add a button to open the relevant registry location directly