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(keymap): Replace some keypad keycodes #2103

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

TheTuxis
Copy link
Contributor

@TheTuxis TheTuxis commented Jan 9, 2024

This change fixes the key mapping for the coner keyboard in the raise_layer.

The previous configuration works only for keyboards with an English layout, with this change it works in all languages.

This change fix keymapping in MacOS and Linux
Copy link
Contributor

@lesshonor lesshonor left a comment

Choose a reason for hiding this comment

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

The previous configuration works only for keyboards with an English layout, with this change it works in all languages.

This is definitely incorrect...but using a shifted keycode instead of a keypad keycode is at least more consistent with all the other punctuation on this layer, so it gets my vote.

@TheTuxis
Copy link
Contributor Author

TheTuxis commented Jan 9, 2024

Thank @lesshonor, I tried the original configuration in Spanish and the problem was that it strictly uses the *, however with the current configuration it considers shift + 8.

@caksoylar
Copy link
Contributor

How about fixing this in other default keymaps as well? I see the same issue for:

  • microdox
  • splitkb_aurora_corne
  • lily58
  • sofle
  • iris
  • corneish_zen

@TheTuxis
Copy link
Contributor Author

TheTuxis commented Jan 9, 2024

How about fixing this in other default keymaps as well? I see the same issue for:

  • microdox
  • splitkb_aurora_corne
  • lily58
  • sofle
  • iris
  • corneish_zen

Do you want me to include it in the same PR?

@caksoylar
Copy link
Contributor

I think that would be appropriate, if you update the PR name accordingly.

@TheTuxis TheTuxis changed the title Change KP_MULTIPLY to ASTRK in corne.keymap Change KP_MULTIPLY to ASTRK in corne.keymap, microdox,keymap, and splitkb_aurora_corne.keymap Jan 9, 2024
@TheTuxis TheTuxis changed the title Change KP_MULTIPLY to ASTRK in corne.keymap, microdox,keymap, and splitkb_aurora_corne.keymap Change KP_MULTIPLY to ASTRK in corne.keymap, microdox,keymap, iris.keymap and splitkb_aurora_corne.keymap Jan 9, 2024
@TheTuxis
Copy link
Contributor Author

TheTuxis commented Jan 9, 2024

I think that would be appropriate, if you update the PR name accordingly.

I already changed them in corne, microdox, iris and splitkb_aurora_corne.

In lily58 and sofle it would not correspond because the character that is represented is strictly the *, not shift + 8.

Note: I didn't find the corneish_zen keymap, will you use any of the others?

@caksoylar
Copy link
Contributor

caksoylar commented Jan 10, 2024

Re: lily58 and sofle, I mean e.g. this KP_MULTIPLY and KP_PLUS which don't seem to belong there, since they aren't near the other keypad keycodes. You can see they are fixed in the splitkb variants already.

Here is the corneish_zen one: https://github.com/zmkfirmware/zmk/blob/main/app/boards/arm/corneish_zen/corneish_zen.keymap#L61C80-L61C92

For the PR title, something like "fix(keymap): Replace some keypad keycodes" might be generic and informative enough to cover these changes.

@TheTuxis TheTuxis changed the title Change KP_MULTIPLY to ASTRK in corne.keymap, microdox,keymap, iris.keymap and splitkb_aurora_corne.keymap fix(keymap): Replace some keypad keycodes Jan 11, 2024
@TheTuxis
Copy link
Contributor Author

All complete suggestions

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.

LGTM, thanks.

@TheTuxis
Copy link
Contributor Author

@caksoylar @lesshonor
I don't have permission to merge the pr, do I have to do something else?

@caksoylar
Copy link
Contributor

No, one of the core contributors will need to have a look and merge.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks!

@petejohanson petejohanson merged commit 8929355 into zmkfirmware:main Mar 18, 2024
27 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.

4 participants