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

feat extra MEH and HYPER combined modifier and copilot key support #2341

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nicenemo
Copy link
Contributor

@nicenemo nicenemo commented Jun 18, 2024

Add left and right combinations of modifiers such as HYPER and MEH.
Adds the copilot key.
Closes #2339

Tested on my Cornish-Zen with https://github.com/nicenemo/zmk-config/tree/test_custom_modifier_patch_for_zmk_config

@nicenemo nicenemo requested a review from a team as a code owner June 18, 2024 17:41
app/include/dt-bindings/zmk/extra_modifiers.h Outdated Show resolved Hide resolved
docs/docs/codes/extra_modifiers.mdx Outdated Show resolved Hide resolved
docs/docs/codes/extra_modifiers.mdx Outdated Show resolved Hide resolved
docs/docs/codes/extra_modifiers.mdx Outdated Show resolved Hide resolved
docs/docs/codes/extra_modifiers.mdx Outdated Show resolved Hide resolved
docs/docs/codes/extra_modifiers.mdx Outdated Show resolved Hide resolved
@caksoylar
Copy link
Contributor

Note that I haven't given this as detailed a look as lesshonor yet, but I have some high level thoughts:

  • I don't believe that having all combinations pre-defined is particularly useful. e.g. LS(LA(key)) is not less unwieldy than remembering LSAK(key), and the former has the advantage of implying/teaching to the user that the functions are easily combined in an intuitive way
    • I can sort of see HYPER and MEH as special cases since they are used in the wider keyboard community and it isn't immediately obvious what modifiers they are composed of
  • I strongly agree with @lesshonor on the point of namings breaking existing conventions w.r.t. *K pattern, as well as the modifier side being on the end rather than the beginning
  • I also agree about the comment on us not needing an extra docs page for this

docs/docs/codes/extra_modifiers.mdx Outdated Show resolved Hide resolved
@nicenemo nicenemo changed the title feat extra combined modifiers: Include left and Right HYPER, MEH and … feat extra MEH and HYPER combined modifier support Jul 12, 2024
@nicenemo nicenemo changed the title feat extra MEH and HYPER combined modifier support feat extra MEH and HYPER combined modifier and copilot key support Jul 12, 2024
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.

You don't need to close this PR and open a new one to squash commits—rebasing and force pushing is fine. Adhering to conventional commit guidelines is encouraged. (Don't worry about preserving any of the co-author stuff; I don't care.)

ZMK is not an ultra-fast moving project; there are maybe three people with merge permissions (this does not include me). Your patience with discussions and reviews is appreciated. Taking the extra time to carefully audit your proposed changes for any typos and inconsistencies would be productive.

docs/docs/codes/modifiers.mdx Outdated Show resolved Hide resolved
docs/docs/codes/modifiers.mdx Outdated Show resolved Hide resolved
docs/docs/codes/modifiers.mdx Outdated Show resolved Hide resolved
app/include/dt-bindings/zmk/modifiers.h Outdated Show resolved Hide resolved
docs/docs/codes/modifiers.mdx Outdated Show resolved Hide resolved
docs/src/data/hid.js Outdated Show resolved Hide resolved
docs/src/data/groups.js Outdated Show resolved Hide resolved
docs/src/data/hid.js Outdated Show resolved Hide resolved
docs/src/data/hid.js Outdated Show resolved Hide resolved
docs/docs/codes/modifiers.mdx Outdated Show resolved Hide resolved
@nicenemo
Copy link
Contributor Author

nicenemo commented Jul 13, 2024

You don't need to close this PR and open a new one to squash commits—rebasing and force pushing is fine. Adhering to conventional commit guidelines is encouraged. (Don't worry about preserving any of the co-author stuff; I don't care.)

ZMK is not an ultra-fast moving project; there are maybe three people with merge permissions (this does not include me). Your patience with discussions and reviews is appreciated. Taking the extra time to carefully audit your proposed changes for any typos and inconsistencies would be productive.

I just want it to be valuable to many people. The best possible naming is important here. Changing it later is painful. I like your naming solutions for the functions.
I will accept your suggestions.

I do understand and respect that some do not see the value of defines for 2 key modifiers.

Regarding the concrete example, I do agree too. I initially did not to keep it similar with the rest.

Regarding the missing back tick, locally I did not get an error.

I did not add the copilot key, initially as I consider it a possible temporary thing. I kept that in my keyboard configuration.

What is the best way to squash the commits on the same branch for this case? Not being git fluent. I looked up several options.

I converted this pull request to a draft for now.
I will clear up the linting and do the rebase later today.
I have to do other things now.

@nicenemo nicenemo marked this pull request as draft July 13, 2024 07:31
@nicenemo nicenemo closed this Jul 13, 2024
@nicenemo nicenemo reopened this Jul 13, 2024
@nicenemo
Copy link
Contributor Author

nicenemo commented Jul 14, 2024

I tested on Windows, Linux(KDE) and i tried iOS. I do not have a Mac so I cannot test that.
The iOS tests are inconclusive because of some iOS device issues and I am less familiar with iOS.
Hyper and MEH work fine on Windows and Linux. The Copilot key starts a search on some configurations of Windows 11 and will probably start copilot if configured. On Windows 10 it will select the first icon on the Desktop. On Linux higher function key support varies sice. It depends on your Desktop environment configuration, X, Wayland or something more low level. Therefore I did not set that to true or false.
Given the previous discussions, and changes I made based on the input , I think this pull request is ready to be reviewed.

Updated my test branch for zmk-config for a corn-ish zen again: https://github.com/nicenemo/zmk-config/tree/test_custom_modifier_patch_for_zmk_config

@nicenemo nicenemo marked this pull request as ready for review July 14, 2024 11:29
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.

Feature request: Hyper, MEH and Copilot
3 participants