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

Core: Allow C and JSON keymaps to be used together #16636

Closed
wants to merge 2 commits into from

Conversation

Erovia
Copy link
Member

@Erovia Erovia commented Mar 13, 2022

Description

Keep your keymaps nice and simple with JSON keymaps, but use the full
power of macros written in C.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Erovia added 2 commits March 13, 2022 17:23
Keep your keymaps nice and simple with JSON keymaps, but use the full
power of macros written in C.
@Erovia Erovia requested a review from a team March 13, 2022 17:59
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

.

Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

Honestly, I would prefer a lighter touch such as what #13197 provided. Be it difficult to implement with the current injection of a generated process_record_user, but could be implemented by a hacky macro to rename any previous declaration.

@@ -108,6 +108,10 @@ __attribute__((weak)) bool process_action_kb(keyrecord_t *record) {
}

__attribute__((weak)) bool process_record_kb(uint16_t keycode, keyrecord_t *record) {
return process_record_json(keycode, record);
Copy link
Member

Choose a reason for hiding this comment

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

Really not a fan of introducing this convention, its already bad enough that keyboards forget to call process_record_user. Now there will be a new convention, and a bunch of "broken" keyboards as they do process_record_kb -> process_record_user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not married to this implementation, but other approaches seemed more "hacky".
Although keyboard's process_record_kb can certainly brake this, keyboards breaking convention might be an other problem we have to tackle in some other way.

I marked this PR to be Draft until we decide how to proceed.

Copy link
Member

Choose a reason for hiding this comment

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

If anything, I would say that it should be moved into to the rest of the process_record_* handling is done, just like a normal feature.

@zvecr zvecr requested a review from a team March 13, 2022 20:26
@Erovia Erovia marked this pull request as draft March 13, 2022 22:03
@zvecr
Copy link
Member

zvecr commented Nov 13, 2023

Closing for now as #19939 was added.

@zvecr zvecr closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants