Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Configure installer keymap #917

Merged
merged 14 commits into from
Dec 14, 2023
Merged

Configure installer keymap #917

merged 14 commits into from
Dec 14, 2023

Conversation

joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Dec 5, 2023 •
edited by lslezak
Loading

Problem

Solution

  • Display a keyboard switcher in the side bar, displayed only in local installation
  • Note: In the long term there should be some "welcome screen" displayed in the local installation which should contain settings for the language and the keyboard

Testing

  • Added a new unit test
  • Tested manually

Screenshots

Local connection1) Remote connection
keyboard_local2 keyboard_remote10

1) Local connection using the loopback device or or forced via LOCAL_CONNECTION=1 environment variable.

@coveralls
Copy link

coveralls commented Dec 6, 2023 •
edited
Loading

Coverage Status

coverage: 75.099% (+0.04%) from 75.055%
when pulling a0be520 on installer-keymap
into 3953469 on master.

@lslezak lslezak marked this pull request as ready for review December 12, 2023 13:56
joseivanlopez
joseivanlopez commented Dec 13, 2023
View reviewed changes
web/src/App.jsx Outdated Show resolved Hide resolved
joseivanlopez
joseivanlopez commented Dec 13, 2023
View reviewed changes
web/src/utils.js Outdated Show resolved Hide resolved
joseivanlopez
joseivanlopez commented Dec 13, 2023
View reviewed changes
web/src/utils.js Outdated Show resolved Hide resolved
joseivanlopez
joseivanlopez commented Dec 13, 2023
View reviewed changes
web/src/components/l10n/InstallerKeymapSwitcher.jsx
const sort = (keymaps) => {
// sort the keymap names using the current locale
const lang = cockpit.language || "en";
return keymaps.sort((k1, k2) => k1.name.localeCompare(k2.name, lang));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keymaps should come already translated from D-Bus.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you meant "sorted" here...

Yes, they are sorted. But are they sorted using the current locale? I mean in some languages the sorting might be a bit different than in English...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, why do we need #localeCompare? Isn't comparison of keymap.name values enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some languages have special rules for sorting, for example in Estonian the letter Z is not the last letter. It is between S and T. See https://en.wikipedia.org/wiki/Estonian_orthography

We even had some "fun" with that in [a-z] regular expressions, see bug #177560.

I'm not sure if localectl output is locale sorted or not, this is just to be really sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can even try that in browser console:
["t", "a", "z"].sort((k1, k2) => k1.localeCompare(k2, "et")) returns  ['a', 'z', 't']

joseivanlopez
joseivanlopez commented Dec 13, 2023
View reviewed changes
web/src/components/l10n/InstallerKeymapSwitcher.jsx Outdated
{options}
</FormSelect>) ||
// TRANSLATORS:
_("Keyboard layout cannot be changed in remote installation") }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested by @dgdavid during the review, I would show the current keymap as disabled for remote installation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested by @dgdavid during the review, I would show the current keymap as disabled for remote installation.

Just displaying it in plain text is enough along with the proper explanation in the popover @lslezak is working on. I mean, not needed to put a disabled selector there IMHO.

That said, at this moment I do not have an strong opinion for choosing between these two options.

Copy link
Contributor

@lslezak lslezak Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that and I didn't like it. There are several problems:

  • We would need to describe why the keyboard layout selector is disabled. It is not obvious and that could be frustrating for the users.
  • It does not display your current local keyboard layout, which will be very likely different than the "English US" default.
  • You do not care what's the keyboard layout on the remote machine, the current value is irrelevant, it does not provide any information for the remote user. It is actually the opposite, it creates some confusion.

See the screenshot:

keyboard_remote9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, one of the problem you found it's because using a disable selector for something that will be not be a selector. That's why I proposed using plain text and moving the explanation to a popover as we're doing now with the Language explanation.

But let's move on, we can re-evaluate at any time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understood it, the idea was to show the local keymap instead of the keymap in the remote system running Agama. But I guess that is not possible, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately that's not possible, you cannot get the local keyboard layout name in Javascript running in browser.

There is some experimental Keyboard API, but it does not provide the functionality we need.

You can get the name of a physical key to get a specified character, e.g. you can find which button produces letter Y. On the usual keyboard it is Y, but if you use QWERTZ layout you should get Z. (BTW I tested that with Chrome in KDE and it does not work properly with CZ QWERTZ layout.)

With that we could possibly only distinguish between QWERTY/QWERTZ/AZERTY variants, but we cannot get the full layout name like "Czech QWERTY".

@dgdavid dgdavid mentioned this pull request Dec 13, 2023
Merged
joseivanlopez
joseivanlopez commented Dec 13, 2023
View reviewed changes
Copy link
Contributor Author

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I cannot approve it.

@dgdavid
Copy link
Contributor

dgdavid commented Dec 13, 2023

LGTM, but I cannot approve it.

So, I'm going to approve it for you.

dgdavid
dgdavid approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving it based on #917 (review)

@lslezak lslezak merged commit bdb04d9 into master Dec 14, 2023
2 checks passed
@lslezak lslezak deleted the installer-keymap branch December 14, 2023 09:15
@imobachgs imobachgs mentioned this pull request Dec 21, 2023
Merged
imobachgs added a commit that referenced this pull request Dec 21, 2023
@imobachgs
Update to release 7 (#947)
a1c7f33 
* #915
* #916
* #917
* #919
* #920
* #921
* #922
* #923
* #924
* #925
* #926
* #927
* #928
* #929
* #930
* #931
* #933
* #934
* #935
* #936
* #937
* #938
* #939
* #940
* #941
* #942
* #943
* #944
* #945
* #946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lslezak lslezak lslezak left review comments

@dgdavid dgdavid dgdavid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joseivanlopez @coveralls @dgdavid @lslezak