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

fix(klc): Correct VK in the KLC #104

Merged
merged 7 commits into from
Feb 16, 2024
Merged

Conversation

Geobert
Copy link
Collaborator

@Geobert Geobert commented Feb 10, 2024

I’ve refactored the key_codes.yaml as VK are not bound to SC.

There’s also a change to make the double 1dk works again, but it breaks test_intl_deadkeys but I think the test needs to be updated.

test_intl_keymap is also broken, but this time, I’m unsure of the correct path to fix it.
I produce:

56	OEM_5	0	005c	007c	-1	-1	// \ |

when the test is expecting:

56	OEM_102	0	005c	007c	-1	-1	// \\ |

Which means my asumption that a char and a VK have a link is also wrong :-/

Need your insight on this :)

EDIT: I think the intl.toml is wrong. We have \ | two times. From what I’ve seen on pictures, they often change one of them to something else (<> or # or "`")

@fabi1cazenave
Copy link
Collaborator

EDIT: I think the intl.toml is wrong. We have \ | two times. From what I’ve seen on pictures, they often change one of them to something else (<> or # or "`")

I agree we could totally change this intl.toml file, but the \| is very common on that physical key. It’s even called NUBS for “non-US BackSlash” in many APIs and apps (e.g. kanata).

@Geobert
Copy link
Collaborator Author

Geobert commented Feb 10, 2024

on that physical key.

Which one? The one left of [Z] or the one on the right of the homerow?

@fabi1cazenave
Copy link
Collaborator

The ISO key has many names — IntlBackslash, Non-US Backslash, LSGT, 102d… but it’s always the same, i.e. the one between Shift and [Z].

I add special handling for the ISO key assuming its scancode is 56.

I’ve updated the `test_intl_deadkeys` test as it was missing a line due to my fix of the 1dk case.
Copy link
Collaborator

@fabi1cazenave fabi1cazenave left a comment

Choose a reason for hiding this comment

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

This is a valuable move forward and it does solve the issue we’re facing with Ergo‑L at the moment, but I think we should aim for a more “single-responsibility” approach while we’re at it. Namely :

  • the klc data in key_codes.yaml should be just a dictionary associating XKB identifiers to scan codes (numbers in hex format would be nice here), like for the other dicts in that data file ;
  • the VK thing should probably be implemented as a dedicated KLC function in layout.py, to make it easier to read and debug.

kalamine/template.py Outdated Show resolved Hide resolved
kalamine/template.py Outdated Show resolved Hide resolved
kalamine/data/key_codes.yaml Outdated Show resolved Hide resolved
kalamine/data/key_codes.yaml Outdated Show resolved Hide resolved
kalamine/template.py Outdated Show resolved Hide resolved
kalamine/template.py Outdated Show resolved Hide resolved
kalamine/data/key_codes.yaml Outdated Show resolved Hide resolved
kalamine/data/key_codes.yaml Outdated Show resolved Hide resolved
kalamine/data/key_codes.yaml Outdated Show resolved Hide resolved
kalamine/data/key_codes.yaml Outdated Show resolved Hide resolved
@Geobert
Copy link
Collaborator Author

Geobert commented Feb 11, 2024

  • the VK thing should probably be implemented as a dedicated KLC function in layout.py, to make it easier to read and debug.

Did you meant "dedicated KLC function in template.py" (not layout.py)

@fabi1cazenave
Copy link
Collaborator

I totally meant that, sorry for the confusion.

Copy link
Collaborator

@fabi1cazenave fabi1cazenave left a comment

Choose a reason for hiding this comment

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

Looks good ! I’ve left a couple comments but nothing worth blocking the PR.

kalamine/template.py Show resolved Hide resolved
kalamine/template.py Show resolved Hide resolved
kalamine/template.py Show resolved Hide resolved
kalamine/data/scan_codes.yaml Show resolved Hide resolved
kalamine/template.py Outdated Show resolved Hide resolved
@fabi1cazenave fabi1cazenave merged commit 697f804 into OneDeadKey:main Feb 16, 2024
15 checks passed
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.

2 participants