-
Notifications
You must be signed in to change notification settings - Fork 35
Keycodes refactor #171
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
base: main
Are you sure you want to change the base?
Keycodes refactor #171
Conversation
cf73dfa to
f418454
Compare
- Moved all the info about scan codes in the YAML file - Added a dedicated class `Key` that gather all the scan codes - Simplify the code
Using ASCII art to declare the layout is limited to the keys declared in the geometries. Added an extra section `mapping` to the TOML layout configuration file, that enable mappings key explicitly with a dictionary.
f418454 to
d8be6a9
Compare
|
@fabi1cazenave I left as draft as it:
|
Use full scan codes, in preparation for all key remapping.
ac701dc to
c866103
Compare
|
|
Added test for Windows and improve VK handling, especially to avoid same VK with multiple mappings. |
fabi1cazenave
left a comment
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.
You probably didn’t want any review at this stage but I had to give a look. :-) Impressive work !
I’m not sure the hand name is very self-explanatory. And I’d like to see a TOML file with your custom mapping proposal. ^_^
kalamine/data/scan_codes.yaml
Outdated
| web: "Digit3" | ||
| windows: "T04" | ||
| macos: "20" | ||
| hand: null |
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.
Nitpick, maybe the following could be easier to read :
digits:
{ web: Digit1, xkb: ae01, windows: T02, macos: "18", hand: null }
{ web: Digit3, xkb: ae02, windows: T03, macos: "19", hand: null }
{ web: Digit3, xkb: ae03, windows: T04, macos: "20", hand: null }FTR & TBH I disliked this change… until I saw how it improves the code readability.
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.
Good idea. But wouldn’t a CSV format be even more adapted then?
kalamine/key.py
Outdated
| return bool(self.category & KeyCategory.AlphaNum) | ||
|
|
||
|
|
||
| KEYS = Key.load_data(load_data("scan_codes")) |
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.
This line triggers my PTSD. :-)
I like data classes but I’m not a fan of creating void objects first, then fill them with data. This looks like an anti-pattern to me.
Couldn’t we use the constructor instead for that , and drop all parse / load_data methods in these data classes ? Otherwise, factory functions parsing data and returning such objects would make sense.
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.
As discussed in the Discord server, we are not creating void objects. It is merely putting the parsing functions under a proper namespace.
I think you mean the initializer __init__, not the constructor __new__.
It is not a good idea to modify the initializer nor the default constructor, because they are created automatically with @dataclass.
But in fact Key.parse is just another constructor, as you cannot overload functions properly in Python.
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.
But the Key.load_data method has a poor name. Fixed.
|
|
||
| @classmethod | ||
| def parse(cls, raw: str) -> Optional["Layer"]: | ||
| rawʹ = raw.casefold() |
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.
this is probably a dumb question but is this trailing single quote sign a special Python stuff ?
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.
That is not Python specific: it a prime, and I find it very practical for closely related variables. Common practice from Haskell I guess.
| "esc": {"base": "\x1b", "shift": "(ae11)"}, | ||
| # NOTE: clone key | ||
| "henk": "(lsgt)", | ||
| } |
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 assume this is a work in progress. Otherwise the comments could be more explicit, I’m not following you here. 🙇
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.
Updated. Is it clearer?
| ALTGR_SHIFT = 5 | ||
|
|
||
| @classmethod | ||
| def parse(cls, raw: str) -> Optional["Layer"]: |
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.
As mentioned in another comment, I’m not a fan of having a parse method to be called after the object has been instantiated. To me it should either be a constructor, or we should have a factory that returns such an object.
wismill
left a comment
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.
Updated a few things
kalamine/key.py
Outdated
| return bool(self.category & KeyCategory.AlphaNum) | ||
|
|
||
|
|
||
| KEYS = Key.load_data(load_data("scan_codes")) |
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.
As discussed in the Discord server, we are not creating void objects. It is merely putting the parsing functions under a proper namespace.
I think you mean the initializer __init__, not the constructor __new__.
It is not a good idea to modify the initializer nor the default constructor, because they are created automatically with @dataclass.
But in fact Key.parse is just another constructor, as you cannot overload functions properly in Python.
kalamine/key.py
Outdated
| return bool(self.category & KeyCategory.AlphaNum) | ||
|
|
||
|
|
||
| KEYS = Key.load_data(load_data("scan_codes")) |
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.
But the Key.load_data method has a poor name. Fixed.
kalamine/data/scan_codes.yaml
Outdated
| web: "Digit3" | ||
| windows: "T04" | ||
| macos: "20" | ||
| hand: null |
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.
Good idea. But wouldn’t a CSV format be even more adapted then?
|
Not going to work on kalamine anymore. |
Simplify code and allow direct mapping of keys.
The new keys are only supported by XKB atm.