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

refactor: Improve keymap upgrader #2126

Merged
merged 7 commits into from
Jan 26, 2024

Conversation

joelspadin
Copy link
Collaborator

@joelspadin joelspadin commented Jan 22, 2024

Moved the keymap upgrader to a top-level page like the power profiler to make it more discoverable. It upgrades more things than key codes now, so putting it in the codes category doesn't make much sense.

Updated the page with syntax highlighting, highlighting of changed lines, and a more visible warning that it cannot handle codes in some locations such as inside ZMK_MACRO().

Converted the upgrader code to TypeScript and split it up into smaller files to make it easier to add new upgrade functions.

Added upgrade functions to remove/replace "label" properties, rename matrix-transform.h to matrix_transform.h, and fix renamed behavior nodes in the unlikely event that someone was changing behavior settings using those instead of with node references.

Fixed key code replacement to only apply inside bindings properties, since identifiers such as MOD_LCTL should be upgraded when used as key codes, but they are still used in mods properties.

Copy link
Contributor

@caksoylar caksoylar left a 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 very helpful, and the new highlighting of changed lines is great, thanks! I noted a few questions, some for my learning.

Also the highlighting if one line is deleted doesn't seem quite right, here is a reproducer:

        gresc: grave_escape {
            label = "xx";
            bindings = <&kp ESC>, <&kp GRAVE>;
            mods = <(MOD_LGUI|MOD_LSFT|MOD_RGUI|MOD_RSFT)>;
        };

Lines both 3 and 4 end up getting highlighted after label = "xx"; line is deleted. There isn't a great way to show deleted lines by just highlighting lines in the output, but maybe highlighting the line after it is good enough. But it is pretty prone to edge cases, so if it is easier to not highlight anything then that might be fine too.

Edit: I missed that the upgrader replaces mod flags like MOD_LGUI with LGUI etc., hence line 4 also being highlighted. That seems like it would be good to fix, I guess we should remove them from the CODES map?

@joelspadin
Copy link
Collaborator Author

Lines both 3 and 4 end up getting highlighted after label = "xx"; line is deleted. There isn't a great way to show deleted lines by just highlighting lines in the output, but maybe highlighting the line after it is good enough. But it is pretty prone to edge cases, so if it is easier to not highlight anything then that might be fine too.

Yeah. Ideally, we'd be able to put some sort of highlight in between the lines, but I couldn't find a way to do that. Stripping removed lines from the highlighting was harder than just having it highlight the following line.

Edit: I missed that the upgrader replaces mod flags like MOD_LGUI with LGUI etc., hence line 4 also being highlighted. That seems like it would be good to fix, I guess we should remove them from the CODES map?

Will fix. I think it would be better to just restrict the replacements to within a bindings array, since MOD_RSFT is still valid inside mods = <MOD_RSFT> but no longer in bindings = <&kp MOD_RSFT>

Moved the keymap upgrader to a top-level page like the power profiler
to make it more discoverable. It upgrades more things than key codes
now, so putting it in the codes category doesn't make much sense.

Converted the upgrader code to TypeScript and split it up into smaller
files to make it easier to add new upgrade functions.

Added upgrade functions to remove/replace "label" properties and rename
matrix-transform.h to matrix_transform.h.
Updated the keymap upgrader to highlight which lines it changed as well
as indicate when nothing needed to be upgraded.

Also adjusted the line highlight colors to be more readable in both
light and dark color schemes.
@joelspadin joelspadin force-pushed the keymap-upgrader-remove-labels branch from e0692ed to ae2be65 Compare January 22, 2024 18:45
Changed the key code upgrader to only replace codes that appear in
"bindings" properties. Modifier flags such as MOD_LCTL are no longer
valid as key codes, but they are still used in "mods" properties and
should not be replaced there.
@joelspadin joelspadin force-pushed the keymap-upgrader-remove-labels branch from ae2be65 to b84ba20 Compare January 22, 2024 21:12
@joelspadin
Copy link
Collaborator Author

I added another upgrade function to handle renamed nodes. I don't expect many people were changing behavior settings this way, but in case they were, this should fix it for them.

Added an upgrade function to fix renamed behavior nodes in the unlikely
event that someone was changing behavior settings this way instead of
using references.
@joelspadin joelspadin force-pushed the keymap-upgrader-remove-labels branch from b84ba20 to 0c55c67 Compare January 23, 2024 02:19
Copy link
Contributor

@caksoylar caksoylar 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 to me, thanks for implementing the enhancements.

Given the utility functions you added, at some point in the future it might be good to pull out the DT-related helpers to a separate package, what you think? (Perhaps it can be utilized by e.g. ZMK Tools as well.)

@joelspadin
Copy link
Collaborator Author

Given the utility functions you added, at some point in the future it might be good to pull out the DT-related helpers to a separate package, what you think? (Perhaps it can be utilized by e.g. ZMK Tools as well.)

Might be useful at some point. Right now I'm not doing much devicetree inspection or editing in ZMK Tools.

@joelspadin
Copy link
Collaborator Author

An upgrade function for resolution -> steps + triggers-per-rotation would be useful, but is much more complex, so I'll try that in a separate PR.

@joelspadin joelspadin merged commit 1dbd691 into zmkfirmware:main Jan 26, 2024
7 checks passed
@joelspadin joelspadin deleted the keymap-upgrader-remove-labels branch January 26, 2024 00:03
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