-
Notifications
You must be signed in to change notification settings - Fork 356
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
Tweak keyboard descriptions #5338
Tweak keyboard descriptions #5338
Conversation
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]>
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]>
3b6be72
to
c734dc5
Compare
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. |
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.
Looks good to me from code perspective.
/kickstart-test --testtype smoke |
The failed test is not related to this change. Going with merge. |
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.