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.

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

feat: support import/display/export of extra host keyboard layouts / languages #1374

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tenstad
Copy link

@tenstad tenstad commented Nov 11, 2024

Related #229

Enables uploading, displaying, and exporting keymaps of other languages.

Disclaimer: I do not at all know this codebase or the wider implications of this change. It works for me on my language/keymap and thus solve my problem. Hope some version of this can be implemented here as well.

Description

Tested with
tenstad/qmk_firmware//keyboards/handwired/tenstad/keymaps/default/keymap.json

Before

When importing a layout with NO_ codes, it is displayed as e.g. ANY(NO_T) and ANY(LSFT_T(NO_F)), and also exported as that. It is therefore very difficult to see the layout, and an import-export breaks the layout by wrapping all keys with NO_ codes inside ANY( ).

image

After

When selecting norwegian and importing a layout with NO_ codes, it is now displayed correctly, and exported correctly (as-is). One can therefore import, swap some keys, and export - and still have a valid layout.

image

@yanfali
Copy link
Collaborator

yanfali commented Nov 11, 2024

@precondition what do you think? Looks like a good change overall. I'll check the code in more detail shortly.

@tenstad
Copy link
Author

tenstad commented Nov 11, 2024

Sorry, I did not check all keys. These still cause a diff during import-export. Would also be cool if host_language automatically configured the Host Keyboard Layout in Settings, and was also exported.

Any idea how to support the rest of the codes?

-    "host_language": "norwegian",
-            "NO_LCBR",
-            "NO_RCBR",
+            "ANY(NO_LCBR)",
+            "ANY(NO_RCBR)",
-            "NO_AT",
+            "ANY(NO_AT)",
-            "NO_GRV",
-            "NO_GRV",
+            "ANY(NO_GRV)",
+            "ANY(NO_GRV)",
-            "NO_DLR",
+            "ANY(NO_DLR)",
-            "NO_LBRC",
-            "NO_RBRC",
-            "NO_CIRC",
-            "NO_TILD",
+            "ANY(NO_LBRC)",
+            "ANY(NO_RBRC)",
+            "ANY(NO_CIRC)",
+            "ANY(NO_TILD)",
-            "NO_RABK",
+            "ANY(NO_RABK)",

@tenstad
Copy link
Author

tenstad commented Nov 11, 2024

I think the problem might be that {"name":"{\n[","code":"KC_LBRC","keys":"["} will never hit 'ALGR(KC_8)': { name: '[', title: 'NO_LBRC' }, in the lookup (as KC_LBRC != ALGR(KC_8)').

It however does hit KC_LBRC: { name: 'Å', title: 'NO_ARNG' }, - meaning NO_LBRC is never in the state of known keys.

Changing to KC_LBRC: { name: '[', title: 'NO_LBRC' }, does indeed fix [, but then ofc also breaks Å.

if (keycodeLUT[keycodeObject.code]) {

@tenstad
Copy link
Author

tenstad commented Nov 11, 2024

Conclusion after previous comment is that we need to add more data into keycodesStore. To make the match simpler as well, I just added all the custom language codes to the store, swapping them so that the custom code (e.g. NO_) becomes the key. Added the KC_ to title, so that it can be used in the includes('KC') check.
Seems to work for all keycodes in my layout (see description).

name,
title: code
}))
];
Copy link
Author

Choose a reason for hiding this comment

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

We could in theory just add all languages like this, so that one can upload NO_, FR_ and everything else regardless of what language is configured in Settings. Would be easier to use, and I don't see any harm except maybe longer searches for match.

Copy link
Author

@tenstad tenstad Nov 12, 2024

Choose a reason for hiding this comment

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

Implemented in 5757e24 - let me know what you think.

Works for FR_CCED and IL_MUL at the same time as NO_ (not that you would have a keymap like that):
image image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what the value of that is, given that the interpretation is based on the OS setting. Don't you think this would be more confusing than helpful?

Copy link
Author

Choose a reason for hiding this comment

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

Can agree that it's a bit confusing when having multiple languages at the same time, but for that to happen, the user would have to configure it on their own before importing. It could also be useful for some to have multiple languages in different layers if they jump between computers with different host languages.

The nice thing here is that users do not need to remember nor know that they have to to configure host language before importing their NO_ or FR_ keyboard. As they use language-specific codes, they would probably never select the correct host language - because that just gives them the corresponding/mapped KC_ codes, not NO_ or FR? Thus I think it's nice that the tool actually displays the NO and _FR codes correctly with the default host language setting.

I think that is more valuable than potentially confusing those with multiple language codes in the same layout.

Copy link
Author

@tenstad tenstad Nov 13, 2024

Choose a reason for hiding this comment

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

We could put it behind a feature toggle in the settings. support extra language keycodes or something. However, I then against fear some will think the tool simply does not support it, as they try without going into settings. So would prefer not to.

Copy link
Contributor

@precondition precondition Nov 13, 2024

Choose a reason for hiding this comment

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

I think this will reinforce even more the incorrect preconception that keycodes like FR_CCED will invariably produce the "ç" keysym no matter the OS settings because that is what it says on the tin can after all.

It would be nice to properly support JSON files with mixed aliases but the displayed layout should obey the host OS keyboard layout setting or else the setting will lose meaning. So, I'd vote against the display of mixed aliases.

For a keymap.json like:

["FR_CCED", "IL_MUL", "LSFT_T(NO_T)", "NO_G"]

I'd expect these legends to look like this under "English (USA)":

(
9
RAlt

*
8
LSft

T
G

Although one problem with this approach is that there may exist multiple keymap_extras variants for the same language (e.g. keymap_french.h and keymap_french_afnor.h) and then it becomes ambiguous whether FR_CCED should translate to KC_9 or to ALGR(KC_C).

The host_language field in the JSON could potentially help to solve the ambiguity but as far as I can tell that only accepts a single host language and if we're dealing with JSON files mixing aliases from multiple keymap_extra aliases, who knows which host_language will be picked (it might be "norwegian" instead of either "french" or "french_afnor" in which case the translation becomes ambiguous)

Copy link
Author

@tenstad tenstad Nov 13, 2024

Choose a reason for hiding this comment

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

This is how it's looking in the Zsa configurator btw. First have to enable the language, and can then use it. By having to enable it before the relevant keys are possible to select, the information is at least somewhat conveyed to the user.

Screenshot_20241113_192022_Chrome.jpg

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still in the middle of moving to pinia so I'm not sure I want this disruption right now for what is a pretty confusing and niche use case.

Copy link
Author

Choose a reason for hiding this comment

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

Not completely comparable as the qmk configurator does not support setting the language-specific keycodes in the GUI. Here, it only applies to keymaps that already contain language-specific key codes on import.

I think the main thing I would like to accomplish here is 1. to not change the keycodes when importing and exporting a keymap if no modifications are made, and 2. ideally somewhat indicate what's actually in the JSON file for that key.

Other than that, I'm very open to making modifications.

Copy link
Contributor

@precondition precondition Nov 13, 2024

Choose a reason for hiding this comment

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

I can already imagine the case of the German programmer who hates having to use AltGr to access [] and {} and prefers the [ { and ] } keysyms found on US layouts but still wants access to the ß, ü, ä and ö keys she needs to type German. Mixing the US [ { keys with the German ß key sounds like the perfect solution except it won't work as she expects.

In fact, I remember multiple people on Discord tripping up over pretty much the same thing because they wanted the US shift pairs and thus used the default KC_ keycodes everywhere in their keymap and then imported a keymap_extra just "to get access" to a special key, unavailable in vanilla US, and were confused why the special key they added to their keymap was not producing the expected result.

EDIT: That said, it is not as if those "hardcoded" localized aliases are available in the GUI so maybe we can make the assumption that if someone includes those in a JSON file, (s)he must have inserted them manually on purpose and may know better what are the implications of doing that.

@tenstad tenstad changed the title feat: support import/render/export of selected host keyboard layout / language feat: support import/display/export of extra host keyboard layouts / languages Nov 12, 2024
extra: Object.values(keymapExtras)
.map(({ keycodeLUT, prefix }) =>
Object.entries(keycodeLUT).map(([code, { name, title }]) => ({
code: title?.split(' ')[0], // split removes ' (dead)'
Copy link
Contributor

@precondition precondition Nov 13, 2024

Choose a reason for hiding this comment

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

Assuming that all titles follow the pattern /\S+( \([^)]*\))?/ (i.e. a space-less token followed optionally by a parenthesized expression) is not a fair assumption to make.

Take for example the space cadet keycodes:

keymap_extras/keymap_french.js
173:  KC_COLN: { name: 'M', title: 'S(FR_M) (capital M)' },
174:  KC_PIPE: { name: 'µ', title: 'FR_MICR (capital µ)' },
176:  SC_LSPO: { name: 'LS / 9', title: 'Left Shift when held, 9 when tapped' },
177:  SC_RSPC: { name: 'RS / 0', title: 'Right Shift when held, 0 when tapped' },
178:  SC_LCPO: { name: 'LC / 9', title: 'Left Control when held, 9 when tapped' },
179:  SC_RCPC: { name: 'RC / 0', title: 'Right Control when held, 0 when tapped' },
180:  SC_LAPO: { name: 'LA / 9', title: 'Left Alt when held, 9 when tapped' },
181:  SC_RAPC: { name: 'RA / 0', title: 'Right Alt when held, 0 when tapped' },
183:  QK_GESC: {
184:    name: '²\nEsc',
185:    title: 'Esc normally, but ² when Shift or GUI is active'
186:  }

The [extra.code, extra.name] ends up looking like this:

Array [ "S(FR_SUP2)", "²" ]
Array [ "S(FR_M)", "M" ]
Array [ "FR_MICR", "µ" ]
Array [ "Left", "LS / 9" ]
Array [ "Right", "RS / 0" ]
Array [ "Left", "LC / 9" ]
Array [ "Right", "RC / 0" ]
Array [ "Left", "LA / 9" ]
Array [ "Right", "RA / 0" ]
Array [ "Esc", "²\nEsc" ]

You could argue that space cadet keycodes are out of scope so it does not matter what happens to them but the comment removes ' (dead)' can be misleading because (dead) is not the only parenthesized expression that can be found — consider also (soft hyphen), (capital Å), (dead European symbol key), (한 Han ↔ 영 Yeong) and more...

If the goal is to build a LUT that can translate a default US keycode alias to a localized alias, scraping the data from the title field is not the best way to go. If need be, the keymap_extras/keymap_###.js files can be easily regenerated with an extra field that can be dedicated to containing only the alias, without any extra information (like those seen in parentheses)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the clarification! As stated, I do not know this codebase, just thought it would be appreciated to create a MR with something that does solve a problem for me and is a registered issue.

Totally agree that we should have the correct data generated if that is an option, and that this is more correct:

Suggested change
code: title?.split(' ')[0], // split removes ' (dead)'
code: title?.split(' ')[0], // best-effort parsing of what may be a keycode

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing to look out for are collisions in aliases. The most egregious case concerns keymap_serbian and keymap_serbian_latin. Both use the RS_ prefix, however the former uses Cyrillic legends while the latter uses Latin legends. To illustrate, RS_I is displayed "И" if we assume "keymap_serbian" but the same keycode alias shows as "I" under "keymap_serbian_latin". It seems like the keymap_serbian data comes after the keymap_serbian_latin data in keycodeLUT so if a keymap.json file contains a RS_### keycode, it will always show the Cyrillic version.

The same problem also rears its head with the many Dvorak variations. DV_RPRN. In keymap_dvorak and keymap_spanish_dvorak, the expected legend is just ")" but in keymap_dvorak_fr, it should be "9 )" (i.e. pressing the key without modifiers produces ) but combining it with shift gives 9).

At the moment, I can't think of an elegant solution to this problem. For now, if the host_language field is present in the keymap.json, we can perhaps look to make sure that the data for that host_language is at the end of the keycodeLUT so that it takes priority over other possible matches.

Copy link
Member

Choose a reason for hiding this comment

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

The collisions are intentional, only one header can be included at a time and thus only one set of "alternative" keycode names other than KC_. There's not much point in enabling multiple as they all send the same values over the wire anyway. These headers are solely for making your keymap match your OS keyboard language; the use case of having multiple layers for various languages doesn't make sense since this is not synced to/from the OS.

Basically, if there are (basic set) keycodes in the keymap that aren't KC_ or can be found in the corresponding host_language header, this should be an error.

@tenstad
Copy link
Author

tenstad commented Nov 13, 2024

How about just showing the raw key codes if a match is not found? tenstad@3ec41bd

image

@tenstad
Copy link
Author

tenstad commented Nov 13, 2024

Would be nice to edit the raw codes as with the ANY-wrapped that we have today. So just removing ANY around the keycodes on export would go a long way!

@yanfali
Copy link
Collaborator

yanfali commented Nov 13, 2024

The export format is based on the internal representation and is largely undocumented. It might be time to do that work.

src/components/BaseKey.vue Outdated Show resolved Hide resolved
src/components/ContainerKey.vue Outdated Show resolved Hide resolved
extra: Object.values(keymapExtras)
.map(({ keycodeLUT, prefix }) =>
Object.entries(keycodeLUT).map(([code, { name, title }]) => ({
code: title?.split(' ')[0], // split removes ' (dead)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing to look out for are collisions in aliases. The most egregious case concerns keymap_serbian and keymap_serbian_latin. Both use the RS_ prefix, however the former uses Cyrillic legends while the latter uses Latin legends. To illustrate, RS_I is displayed "И" if we assume "keymap_serbian" but the same keycode alias shows as "I" under "keymap_serbian_latin". It seems like the keymap_serbian data comes after the keymap_serbian_latin data in keycodeLUT so if a keymap.json file contains a RS_### keycode, it will always show the Cyrillic version.

The same problem also rears its head with the many Dvorak variations. DV_RPRN. In keymap_dvorak and keymap_spanish_dvorak, the expected legend is just ")" but in keymap_dvorak_fr, it should be "9 )" (i.e. pressing the key without modifiers produces ) but combining it with shift gives 9).

At the moment, I can't think of an elegant solution to this problem. For now, if the host_language field is present in the keymap.json, we can perhaps look to make sure that the data for that host_language is at the end of the keycodeLUT so that it takes priority over other possible matches.

@tenstad
Copy link
Author

tenstad commented Nov 14, 2024

Now only for selected host language

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants