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

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
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f526ee0
feat: support import/render/export of other keyboard layouts
tenstad Nov 11, 2024
5299850
fix: dead keys
tenstad Nov 11, 2024
ac2f809
fix: simply add custom language codes to lookup
tenstad Nov 11, 2024
5757e24
feat: support all languages - not just selected
tenstad Nov 12, 2024
db616fc
feat: display language codes
tenstad Nov 13, 2024
e8a778f
fix: only show language when showing keymap
tenstad Nov 14, 2024
67af324
fix: only show language when showing keymap
tenstad Nov 14, 2024
71654df
fix: improved keycode extraction from title
tenstad Nov 14, 2024
ce363a2
fix: only display language-specific codes for chosen host language
tenstad Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/components/BaseKey.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
@dragover.prevent="dragover" @dragenter.prevent="dragenter">{{ displayName }}<font-awesome-icon v-if="icon"
size="2x" :icon="icon" /><template v-if="visible">
<div v-if="visible" class="remove" @click.stop="remove">x</div>
<span v-if="visible" class="language">{{ displayLanguage }}</span>
</template></div>
</template>
<script>
Expand Down Expand Up @@ -118,6 +119,9 @@ export default {
visible() {
return this.meta ? this.meta.code !== 'KC_NO' : false;
},
displayLanguage() {
return this.meta.language_prefix
},
tenstad marked this conversation as resolved.
Show resolved Hide resolved
displayName() {
switch (this.legends) {
case 'size': {
Expand Down
10 changes: 9 additions & 1 deletion src/components/ContainerKey.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@
@dragenter.prevent="dragenterContents"
@dragleave.prevent="dragleaveContents"
@click.prevent.stop="clickContents"
>{{ contents }}</div></div><div
>{{ contents }}</div>
<span
v-if="visible"
class="language"
>{{ displayLanguage }}</span>
</div><div
v-if="visible"
class="remove"
@click.stop="remove"
Expand All @@ -46,6 +51,9 @@ export default {
}
return '';
},
displayLanguage() {
return this.meta.contents.language_prefix;
},
tenstad marked this conversation as resolved.
Show resolved Hide resolved
contentClasses() {
let classes = [];
if (this.contentsInHover) {
Expand Down
17 changes: 17 additions & 0 deletions src/scss/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,23 @@ button {
cursor: pointer;
}

.key .language {
position: absolute;
right: 0px;
bottom: 0px;
padding-right: 3px;
font-size: 7px;
opacity: 0.7;
}
.key.key-container div .language {
padding-right: 8px;
padding-bottom: 1px;
}
.key.smaller .language {
padding-right: 2px;
padding-bottom: 2px;
}

.key:hover .remove {
width: unset;
height: unset;
Expand Down
24 changes: 19 additions & 5 deletions src/store/keycodes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,17 @@ import steno from './modules/keycodes/steno';
const keycodePickerTabLayout = {
ANSI_ISO: [...ansi, ...iso_jis],
ISO_ANSI: [...iso_jis, ...ansi],
special: [...quantum, ...settings, ...media]
special: [...quantum, ...settings, ...media],
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 •
edited
Loading

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.

name,
language_prefix: prefix,
title: code
}))
)
.flat()
};

/**
Expand Down Expand Up @@ -77,9 +87,12 @@ function generateKeycodes(osKeyboardLayout, isSteno = false) {
...(isSteno ? steno : [])
];
const { keycodeLUT } = keymapExtras[getOSKeyboardLayout()];
return keycodes.map((keycodeObject) =>
toLocaleKeycode(keycodeLUT, keycodeObject)
);
return [
...keycodes.map((keycodeObject) =>
toLocaleKeycode(keycodeLUT, keycodeObject)
),
...keycodePickerTabLayout.extra
];
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

}

/**
Expand Down Expand Up @@ -118,7 +131,8 @@ export const useKeycodesStore = defineStore('keycodes', {
state: () => ({
keycodes: [
...keycodePickerTabLayout.ANSI_ISO,
...keycodePickerTabLayout.special
...keycodePickerTabLayout.special,
...keycodePickerTabLayout.extra
],
searchFilter: '',
searchCounters: {
Expand Down
7 changes: 4 additions & 3 deletions src/store/modules/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ function newKey(metadata, keycode, obj) {
var key = {
name: metadata.name,
code: keycode,
type: metadata.type
type: metadata.type,
language_prefix: metadata.language_prefix
};

if (obj !== undefined) {
Expand Down Expand Up @@ -181,6 +182,7 @@ function parseKeycode(keycodesStore, keycode, stats) {
}
let internal = splitcode[1];
internal = internal.split(')')[0];
metadata = keycodesStore.lookupKeycode(internal);

// check for an OSM keycode
if (maincode === 'OSM') {
Expand All @@ -189,13 +191,12 @@ function parseKeycode(keycodesStore, keycode, stats) {
}

//Check whether it is a layer switching code, mod-tap, or combo keycode
if (internal.includes('KC')) {
if (internal.includes('KC') || metadata?.language_prefix !== undefined) {
// Layer Tap keycode
if (maincode === 'LT') {
return newLayerContainerKey(keycodesStore, maincode, internal);
}
internal = longFormKeycodes[internal] || internal;
metadata = keycodesStore.lookupKeycode(internal);
if (metadata === undefined) {
stats.any += 1;
return newAnyKey(keycodesStore, keycode);
Expand Down