-
-
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
Add support for intl key legends based on the host keyboard layout #1161
Conversation
I'm still digesting the PR, so I'm not ignoring you I promise. I think it looks pretty good, I just want to think about it. I'm not thrilled to add python to the repo as it's even less portable than node in some respects. |
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.
Likewise, I'm not ignoring you or this PR either; unfortunately you've caught me just a few days ahead of being out of town for six days. I probably won't be able to properly review this until next week.
This may be related to how the key name is being updated. The UI won't render if the reference doesn't change, depending on how it's rendering. I haven't looked at this code in a long time, It might be as simple as re-assigning the object to a new one and copying all the fields. The styling code should react to length changes, or it could just be a bug. |
I can translate the script into node.js if needs be. It is mostly text and hashmap manipulation so it is not hard to port. |
7b3f406
to
bbd6dc9
Compare
If we ignore the deprecated keymap_nordic.h and the steno/plover header files that are out-of-scope, the converter script generates (syntaxically) correct output for every† keymap_*.h header in qmk_firmware/quantum/keymap_extras! †: Assuming that one merges the PRs I opened to fix the bugs in keymap_bepo.h (qmk/qmk_firmware#17999) and keymap_lithuanian_qwerty.h (qmk/qmk_firmware#18000) |
Thanks for doing this. I am traveling at the moment, but I will take a look this weekend. I need to test it myself. |
7f97d24
to
fa18fe3
Compare
While working on localising My suggestion: diff --git a/src/store/modules/keycodes/quantum.js b/src/store/modules/keycodes/quantum.js
index 734e9a1..90c5921 100644
--- a/src/store/modules/keycodes/quantum.js
+++ b/src/store/modules/keycodes/quantum.js
@@ -392,9 +392,9 @@ export default [
{ label: 'Special action keys', width: 'label' },
{
- name: 'Esc / ~',
+ name: '` / ~\nEsc',
code: 'QK_GESC',
- title: 'Esc normally, but ~ when Shift or GUI is pressed'
+ title: 'Esc normally, but ` when GUI is active or ~ when Shift is active'
},
{
name: 'LS / (',
Displaying the legend on 2 lines would also be more coherent with the rest of the shift pairs. If you think about it, the shift pair of See commit 5b36bf9 |
fa18fe3
to
eed17d7
Compare
This is the main commit of PR qmk#1161.
e3b5406
to
c8a9da5
Compare
This is the main commit of PR qmk#1161.
c8a9da5
to
15bd89c
Compare
I have no opinion on this, as it doesn't work on macOS at all due to a different binding policy. @qmk/collaborators may have other opinions. |
bdd4a65
to
7a22895
Compare
I just realised that since I mostly rely on the old pre-existing ISO toggle mechanism to update the key legends text, the test page (config.qmk.fm/#/test) doesn't dynamically update the key legends when switching OS layouts in the menu (since the old ISO toggle has no effect on that page). You need to switch and reload the page for the new OS keyboard layout to apply. I have a feeling that fixing the first known bug will also patch this. However, I'm in the middle of a move right now and I will not have Wi-Fi in the next few days so I don't know when I'll be able to look into the issue. |
This never needed to work iirc as you could
As there are explicit ISO and ANSI buttons it was never really a priority to make this toggle either. It wouldn't be too hard to fix this though. |
super busy at work. I'll try and make time for this on thursday |
Thank you for all the hardwork. I will be reviewing this properly today, evening PST. |
As I've guessed, the two known bugs share a common root cause. The root cause is that the values of My solution, as presented in commits b84a675 and 1bad93e, is to use |
There's an issue for the very first time it loads, the localstorage stuff is out of sync until the first reload. |
when switching to russian the ISO big enter doesn't kick in. Is that the expected behavior? |
Yes, the Russian keymap is set to be ANSI because the OS layout produces the same keysym for /*
* ┌───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───────┐
* │ Ё │ 1 │ 2 │ 3 │ 4 │ 5 │ 6 │ 7 │ 8 │ 9 │ 0 │ - │ = │ │
* ├───┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─────┤
* │ │ Й │ Ц │ У │ К │ Е │ Н │ Г │ Ш │ Щ │ З │ Х │ Ъ │ \ │
* ├─────┴┬──┴┬──┴┬──┴┬──┴┬──┴┬──┴┬──┴┬──┴┬──┴┬──┴┬──┴┬──┴─────┤
* │ │ Ф │ Ы │ В │ А │ П │ Р │ О │ Л │ Д │ Ж │ Э │ │
* ├──────┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴─┬─┴────────┤
* │ │ Я │ Ч │ С │ М │ И │ Т │ Ь │ Б │ Ю │ . │ │
* ├────┬───┴┬──┴─┬─┴───┴───┴───┴───┴───┴──┬┴───┼───┴┬────┬────┤
* │ │ │ │ │ │ │ │ │
* └────┴────┴────┴────────────────────────┴────┴────┴────┴────┘
*/ |
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 is looking really good. I'll try and re-test it this weekend and I think we can probably get it merged then. Thank you for your hard work and patience.
Issue is fixed. ✔️ |
// so it is important to merge them with the default configurator settings to ensure that all | ||
// settings are set. | ||
let conf = { | ||
...getDefaultConfiguratorSettings(), |
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.
nice.
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.
A few minor changes need to manipulating the configurator saved state, otherwise LGTM.
@precondition I think we're almost ready to merge this. Thanks again for this. Very cool feature. Just a couple of very small clean ups and we're good to go. |
This is done so that the Shift+GESC and GUI+GESC keysyms/keycode names remain on the same line because the `breakLines` functions of PrintKey.vue and TesterKey.vue replace spaces ' ' with newlines '\n'.
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.
Lgtm. @noroadsleft can you give it one more test. Otherwise I think it's ready to merge.
@noroadsleft bump |
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.
Thanks for this; been wanting this feature for a long time.
Should the linked issues be closed now? |
Description
demo-intl-configurator.webm
(I capped the framerate at 20fps to reduce the file size, that's why it looks choppy)
I replaced the ISO toggle switch in the settings by a more general dropdown that lets the users pick the correct logical/functional keyboard layout of their OS. The only effect of this setting is to change how each keycode is displayed; nothing about the constructed
keymap.json
is altered.To get started, I added the following host keyboard layouts:
The lookup tables used for those host keyboard layouts can be found in
src/i18n/keymap_extras
and were generated with the help of thesrc/i18n/keymap_extras/convert_keymap_extras_header.js
script. The script would allow me to easily add support for every keymap_extra input language that qmk_firmware supports, but for the sake of keeping this PR more concise, I limited myself to four OS keyboard layouts.The bulk of the relatively impressive amount of added lines in this PR comes from those generated lookup tables, and to a lesser extend from the optional converter script used to generate them. The actual core changes to qmk_configurator are small.
Future Work
Can be addressed in follow-up PRs
"host_language"
in thekeymap.json
convert_keymap_extras.py
convert_keymap_extras_header.js
scriptKnown Bugs
KC_SCLN
would have been ":\n;". If the user switches to keymap_german, the locale keysym ofKC_SCLN
("ö") will be shown in small text despite being a single character.(Solved in b84a675)Indirectly solved by Explore how to support internationalization of the keymap #1166(Solved in 1bad93e)Indirectly solved by Explore how to support internationalization of the keymap #1166osKeyboardLayout
property is in localstorage. (Solved in 82cb033)Motivation
Linked Issues
Feature Request: Add European layouts to the onscreen keyboard #866
[Feature Request] Support for other software keyboard layouts #846
Unable to add ISO-specific keys in configurator #768
Feature: Non--US Keyboard support #229