-
Notifications
You must be signed in to change notification settings - Fork 30
components: localization: enable more than keyboard selection in Workstation #1096
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
components: localization: enable more than keyboard selection in Workstation #1096
Conversation
Summary of ChangesHello @KKoukiou, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the localization capabilities in Workstation by allowing users to configure and utilize multiple keyboard layouts. This change removes a previous restriction that limited users to a single layout, which was particularly problematic for languages requiring diverse input methods. While acknowledging a potential edge case concerning the active layout during LUKS unlock, the benefits of supporting multiple layouts are deemed to outweigh this risk, with a visual indicator provided to inform users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly enables the selection of multiple keyboard layouts in the Workstation environment by removing the code that previously restricted it to a single layout. The change is straightforward, deleting a state, an effect hook, and the corresponding alert UI in Keyboard.jsx. The pull request description provides excellent context, clearly outlining the benefits and the potential edge case regarding LUKS password entry. My review is positive, with one minor suggestion for code cleanup that resulted from the removed logic.
I am having trouble creating individual review comments. Click here to see my feedback.
src/components/localization/Keyboard.jsx (303-309)
With the removal of this useEffect block, the plannedXlayouts variable, declared on line 301, is no longer used within the KeyboardGnome component. To improve code clarity and maintainability, this unused variable declaration should also be removed.
099ec93 to
3e6a34d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In
KeyboardGnome,plannedXlayoutsis now unused but still pulled from context and referenced in theuseEffectdependency array; consider removing the context read and using an empty dependency list (ordispatchonly) to avoid unnecessary re-subscriptions and lint warnings. isGnomeis no longer used in thesetIsKeyboardValidcalculation but is still included in the dependency array and component props; consider removing it from the dependency list and, if truly unused elsewhere, from the prop signature to keep the API minimal and avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `KeyboardGnome`, `plannedXlayouts` is now unused but still pulled from context and referenced in the `useEffect` dependency array; consider removing the context read and using an empty dependency list (or `dispatch` only) to avoid unnecessary re-subscriptions and lint warnings.
- `isGnome` is no longer used in the `setIsKeyboardValid` calculation but is still included in the dependency array and component props; consider removing it from the dependency list and, if truly unused elsewhere, from the prop signature to keep the API minimal and avoid confusion.
## Individual Comments
### Comment 1
<location> `src/components/localization/Keyboard.jsx:395` </location>
<code_context>
useEffect(() => {
- setIsKeyboardValid((plannedVconsole ?? "") !== "" && (!isGnome || plannedXlayouts?.length === 1));
+ setIsKeyboardValid((plannedVconsole ?? "") !== "" && (plannedXlayouts?.length > 0));
}, [plannedVconsole, plannedXlayouts, isGnome, setIsKeyboardValid]);
</code_context>
<issue_to_address>
**question (bug_risk):** Re-check whether non-GNOME flows should now require at least one X layout.
The previous condition made non-GNOME validity depend only on `plannedVconsole`, while GNOME additionally required exactly one X layout. The new condition now requires at least one X layout for non-GNOME as well and permits multiple layouts for GNOME. Please confirm that this stricter requirement for non-GNOME flows (failing when `plannedXlayouts` is empty or unset) is intentional.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| useEffect(() => { | ||
| setIsKeyboardValid((plannedVconsole ?? "") !== "" && (!isGnome || plannedXlayouts?.length === 1)); | ||
| setIsKeyboardValid((plannedVconsole ?? "") !== "" && (plannedXlayouts?.length > 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (bug_risk): Re-check whether non-GNOME flows should now require at least one X layout.
The previous condition made non-GNOME validity depend only on plannedVconsole, while GNOME additionally required exactly one X layout. The new condition now requires at least one X layout for non-GNOME as well and permits multiple layouts for GNOME. Please confirm that this stricter requirement for non-GNOME flows (failing when plannedXlayouts is empty or unset) is intentional.
rvykydal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise looks good to me.
From the user experience POV, I am reading the PR as there is no risk of any data loss, and the corner case could be resolved by user by redoing the installation with awareness of the issue cause found described somewhere. I.e. only new devices / data storage can be affected.
Also I am reading the commit comment as there is no sane way how to check ('at the right place', like before starting the installation, not sure) that the keyboard selection is the same as it was on the last update in the webui selection UI (possibly considering additional warning on that change detection).
And it seems there is an intention to add the needed support go gnome / mutter (https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/7761).
If my understanding of the situation is correct, the tradeoff seems acceptable to me.
…station GNOME Shell currently lacks support for localed [1], which prevents the installer from receiving signals about the compositor’s selected and active keyboard layouts. Currently, we read the compositor’s keyboard selection only when the Web UI localization page gains focus. This commit enables selecting multiple keyboard layouts, necessary for many languages, such as Russian with the Cyrillic alphabet. However, this introduces a problematic edge case: we cannot reliably determine the compositor’s active layout. Users may switch layouts with hotkeys while the Web UI is still focused, causing the compositor’s active layout to diverge from the one we will configure for the vconsole used during LUKS unlock. If those layouts do not match, the user will not be able to unlock their drive and we have no mechanism to warn the user against that. Despite this potential edge case, the benefits of supporting multiple layouts justify moving forward. Additionally, the vconsole layout used for the LUKS unlock screen is shown on the disk encryption page, giving users visibility into this potential issue. [1] https://bugzilla.redhat.com/show_bug.cgi?id=2346830
3e6a34d to
e65f45e
Compare
GNOME Shell currently lacks support for localed [1], which prevents the installer from receiving signals about the compositor’s selected and active keyboard layouts.
Currently, we read the compositor’s keyboard selection only when the Web UI localization page gains focus.
This commit enables selecting multiple keyboard layouts, necessary for many languages, such as Russian with the Cyrillic alphabet. However, this introduces a problematic edge case: we cannot reliably determine the compositor’s active layout. Users may switch layouts with hotkeys while the Web UI is still focused, causing the compositor’s active layout to diverge from the one we will configure for the vconsole used during LUKS unlock. If those layouts do not match, the user will not be able to unlock their drive and we have no mechanism to warn the user against that.
Despite this potential edge case, the benefits of supporting multiple layouts justify moving forward. Additionally, the vconsole layout used for the LUKS unlock screen is shown on the disk encryption page, giving users visibility into this potential issue.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=2346830