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: default layer setter #2222

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

Conversation

elpekenin
Copy link
Contributor

I plan to do a PR to persistently change the default layer, for multi-layout uses (eg win/mac or qwerty/dvorak)

However, while activating the layer or to'ing into it is similar, it wouldn't mimic the behavior of default layer. Thus, the follow-up PR needs an API of this kind.

Notes:

  • New event for default layer changes?
  • I dont quite like the function's name and log texts, ideas are welcome.

@elpekenin elpekenin requested a review from a team as a code owner March 23, 2024 11:29
Copy link
Contributor

@huber-th huber-th left a comment

Choose a reason for hiding this comment

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

Being able to set the default layer on startup would be wonderful for multi layout configurations.

app/src/keymap.c Outdated Show resolved Hide resolved
app/src/keymap.c Outdated Show resolved Hide resolved
@huber-th
Copy link
Contributor

huber-th commented Mar 23, 2024

If I may add an idea to be considered in your follow up PR. Might be worth to consider being able to trigger automatic layout changes on BLE profile change.

Not sure how this would be best made configurable (maybe via KConfig?, Could ZMK remember default layer per profile?), but it would be convenient if someone with Windows and macOS system for example, could configure a default layer for each BLE profile and one for USB to automatically trigger a default layer change depending on which profile is activated.

@huber-th
Copy link
Contributor

I acknowledge the potential advantage of the planned behavior for users with dual systems, but after thinking about it a bit more I'm wondering whether this proposed change should be directly integrated into the feature's implementation itself rather than as its separate PR.

@caksoylar caksoylar added core Core functionality/behavior of ZMK behaviors labels Mar 25, 2024
@elpekenin
Copy link
Contributor Author

elpekenin commented Mar 25, 2024

I dont mind either way, the idea of making a small PR was to start rolling it and see if it had missed a small edge case or whatever.

Also making two different PRs with the respective self-contained logic seemed better review-wise.

I already have the code (or at least, its first revision) written & slightly tested it (with to instead of setting default layer at ZMK level). So, if collabs think it is better to keep everything together just let me know and i will push it on this branch too

@huber-th
Copy link
Contributor

huber-th commented Apr 4, 2024

I dont mind either way, the idea of making a small PR was to start rolling it and see if it had missed a small edge case or whatever.

Also making two different PRs with the respective self-contained logic seemed better review-wise.

Sorry, I have been quite busy lately. Personally, I would recommend adding everything related to this feature in one PR so when it's merged it adds the full feature.

I tested your code from your personal repo and I would recommend to incorporate it into this PR for full review/testing of the complete feature.

@elpekenin
Copy link
Contributor Author

Aight, will do.

Again, my main idea was go test if default layer setting really worked well before relying on it for the behavior, for easier debug when (if) edge cases poped up.

Will add it to make the process faster and keep it together. Glad to hear the current implementation (layer toggle instead of default layer, which is worse) is working fine for you

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.

One major concern regarding the usage of the endpoints API. Thanks for the PR!

app/src/behaviors/behavior_default_layer.c Outdated Show resolved Hide resolved
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.

Two other docs pieces need updating in addition to below comments:

app/src/behaviors/behavior_default_layer.c Outdated Show resolved Hide resolved
docs/docs/behaviors/layers.md Outdated Show resolved Hide resolved
@Nick-Munnich
Copy link
Contributor

#1500 seems like it could be relevant here and hasn't been mentioned yet, just dropping a link.

@elpekenin elpekenin requested a review from a team as a code owner August 19, 2024 13:28
@TrangPham
Copy link

@petejohanson I'm interested in having this feature get merged. Would you have time in the near future to re-review this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviors core Core functionality/behavior of ZMK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants