Tweak keyboard descriptions #5338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As reported in https://bugzilla.redhat.com/show_bug.cgi?id=2250388 we often prepend an apparently-random language to the keyboard layout+variant description we get from evdev, with sometimes bizarre results. This happens when a layout+variant is associated with more than one language: we effectively pick just one of those languages at random to "care about", and if the evdev description for the layout+variant doesn't start with the name of that language, we prepend it. This gives bizarre results like the one reported in the bug - "French (English (intl., with AltGr dead keys))". To avoid this, let's just keep track of *all* the languages a layout+variant is associated with, and if there's more than one, we'll not try to do anything clever, we'll just use the evdev description. We'll only ever prepend a language or country name if a layout+variant is associated with only one language, or no languages and at least one country. Resolves: rhbz#2250388 Signed-off-by: Adam Williamson <[email protected]>
AdamWill
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
|
I'll flag up one possibly-controversial change this causes: it changes the description for the We should be careful any time we touch a string like that, but I don't think this change is actually too provocative, because we're not making any claims or characterizations anyone would disagree with, I don't think. I think it's uncontroversial to refer to the keyboard layout as "Taiwanese", which is all we're doing here. The text description here is not a description of a place or anything potentially divisive like that, just a keyboard layout. I also hacked this logic into a test script which just dumps out the full list of layouts and descriptions so you can see the effects of the changes. Here are the outputs at various stages:
More details on the 'more radical' one - the difference there is that when we encounter a layout only by country, we store it with an empty I definitely feel like the "both changes" output is the best, which is why I proposed the PR this way, but wanted to provide the data for people to look through and spot any potential issues in. |
|
For the record, the full diff from original code to this PR is as follows. In case it's not obvious, the format of the lines is Note, there are some cases where the new string seems "less descriptive" - e.g. we go from "Maithili (Indian)" to just "Indian". But on further inspection I think these are all correct. Those are all cases where the layout is associated with multiple languages, often six or even more. It doesn't make sense to just pick one of them apparently at random to prepend, and prepending all of them would just look rather unwieldy - the non-variant |
Checking whether the layout+variant description from evdev starts with the exact ISO-ified name of the language or country with which it's associated is rather too strict. See things like the official ISO name of Iran - "Iran, Islamic Republic of" - or the ISO name of modern Greek - "Greek, Modern (1453-)" - or Berber - "Berber languages". These cases mean our layout list winds up with entries like "Greek, Modern (1453-) (Greek)" in it. Avoid that with a more liberal check. Once we have our 'lang' string, take the first word of it, strip any trailing commas and semi-colons, then check if that word is in the evdev description, and if so, don't prepend the lang string. Also ignore case - I noticed that in French all translated language names are not title-cased, so we wind up with entries like "anglais (Anglais (US))". Signed-off-by: Adam Williamson <[email protected]>
AdamWill
force-pushed
the
tweak-keyboard-descriptions
branch
from
3b6be72
to
c734dc5
Compare
AdamWill
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
|
fun, so I noticed the language/country names and layout descriptions themselves can be translated, so there's just a huge space here. It's a bit impractical to check the output in every single language anaconda offers, I think, but I checked a few and added a tweak for French (make the comparison non-case-sensitive as for some reason all ISO language names are lower-case in French; without this we have entries like "anglais (Anglais)", both with and without my changes). There are still some questionable ones in French even now (e.g. the evdev strings call Spanish "Espagnol" but the French ISO name for Spanish is "castillan" so we get "castillan (Espagnol)"), but at a quick look it does seem better than before. I also looked at Japanese and it seems OK as best I can tell, again with some dumb cases like the ISO names of languages (like Greek) not quite matching up with the ones used in the evdev name translations so we prepend unnecessarily, but again nothing in this PR makes things worse AFAICS. |
|
If you want to play around with this live in the installer in different languages to see the effects, https:///www.happyassassin.net/updates/2250388.img is an updates image with both changes in it. |
|
Hi @AdamWill thanks for the contribution. I just wonder if you want to spent time on this when we are planning to completely redesign the code because of libxklavier deprecation https://bugzilla.redhat.com/show_bug.cgi?id=1955025 . Seems that this have to happen on F40 or F41 anyhow. In general we would have to completely redesign solution of the keyboard switching. I'm fine with merging this now just not sure if it is beneficial for you from the testing perpective. |
|
it shouldn't affect testing at all (we use the default keyboard layout for the language in all existing automated tests). I already did all the work on it, so I think it would be a good idea to merge it - we don't know if the redesign will be done in time for F40, and if not, it would be nice to have this fixed. |
|
Most probably not for F40 but F41. |
jkonecny12
approved these changes
Dec 5, 2023
|
/kickstart-test --testtype smoke |
|
The failed test is not related to this change. Going with merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two changes with the effect that we prepend a language or country string to the evdev layout+variant description much less often. A lot of the cases where we do this are bogus and lead to confusing or redundant strings.