-
-
Notifications
You must be signed in to change notification settings - Fork 352
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
base: master
Are you sure you want to change the base?
Conversation
@precondition what do you think? Looks like a good change overall. I'll check the code in more detail shortly. |
Sorry, I did not check all keys. These still cause a diff during import-export. Would also be cool if 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)", |
I think the problem might be that It however does hit Changing to qmk_configurator/src/store/keycodes.js Line 52 in cf968a0
|
Conclusion after previous comment is that we need to add more data into |
name, | ||
title: code | ||
})) | ||
]; |
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.
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.
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.
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):
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.
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?
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.
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.
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.
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.
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.
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)
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.
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.
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.
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.
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.
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.
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.
src/store/keycodes.js
Outdated
extra: Object.values(keymapExtras) | ||
.map(({ keycodeLUT, prefix }) => | ||
Object.entries(keycodeLUT).map(([code, { name, title }]) => ({ | ||
code: title?.split(' ')[0], // split removes ' (dead)' |
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.
Assuming that all title
s 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)
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.
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:
code: title?.split(' ')[0], // split removes ' (dead)' | |
code: title?.split(' ')[0], // best-effort parsing of what may be a keycode |
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.
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.
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.
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.
How about just showing the raw key codes if a match is not found? tenstad@3ec41bd |
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! |
The export format is based on the internal representation and is largely undocumented. It might be time to do that work. |
src/store/keycodes.js
Outdated
extra: Object.values(keymapExtras) | ||
.map(({ keycodeLUT, prefix }) => | ||
Object.entries(keycodeLUT).map(([code, { name, title }]) => ({ | ||
code: title?.split(' ')[0], // split removes ' (dead)' |
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.
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.
Co-authored-by: precondition <[email protected]>
Co-authored-by: precondition <[email protected]>
efe7db6
to
ce363a2
Compare
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)
andANY(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 withNO_
codes insideANY( )
.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.