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

Extend Qukeys to support adding Qukeys in the keymap #1408

Closed
wants to merge 13 commits into from

Conversation

EvyBongers
Copy link
Contributor

No description provided.

Copy link
Contributor Author

@EvyBongers EvyBongers left a comment

Choose a reason for hiding this comment

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

Just a first draft of what I think might be a good way to approach a new Qukeys api. Please let me know about anything that should change before I continue with this.

Comment on lines 248 to 253
/// This defines a `Key` object that will be handled by the Qukey plugin.
/// The argument `n` is the index number of the `Qukey` in the array (starting
/// at zero).
constexpr kaleidoscope::Key QK(uint8_t n) {
return kaleidoscope::Key(kaleidoscope::ranges::QK_FIRST + n);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly copy/paste from CharShift. I noticed that LayerTapKey and ModTapKey are defined within kaleidoscope::plugin above and that macros are used to expose those in a user friendly way. Any preference on either of those?

@gedankenexperimenter
Copy link
Collaborator

I don't feel like I have time to help much (I haven't even looked at Kaleidoscope code in what seems a very long time), but I can recall that the idea was to have it work in much the same way a CharShift, with essentially the same (or equivalent) KeyPair structure in an array stored in PROGMEM or optionally in EEPROM (yeah, okay, preferably in EEPROM so it can be configured with Chrysalis, but it would be best if that wasn't required…).

It seems like you've got the same idea I had, so 👍.

As for how to do deprecation of the old stuff…I'm sure I could remind myself how to go about it at some point. When I did that in the past, I tried to use preprocessor directives to isolate the code that would later be removed, both to make it easier to do the removal correctly, and to make it possibly to compile a smaller binary with the deprecated code stripped out. I'm sure there are a few old PRs that demonstrate how it was done then.

I'm sure I can manage to find time to do an actual code review at an appropriate time, if that would be helpful.

@@ -55,6 +57,12 @@ constexpr Key LayerTapKey(uint8_t layer, Key tap_key) {
(layer << 8) + tap_key.getKeyCode());
}

constexpr Key Qkey(Key primary_key, Key secondary_key) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gonna need to find a way to fix the name collision with constexpr Qukey(int8_t layer, KeyAddr k, Key alternate_key) below, unless you're okay with keeping it like this.

Signed-off-by: Evy Bongers <[email protected]>
Signed-off-by: Evy Bongers <[email protected]>
Copy link
Contributor Author

@EvyBongers EvyBongers 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 the idea I have for making this work is about finished now. There's two TODOs left for figuring out how to keep track of the alternate values and one error to fix.

I would appreciate suggestions on how to best approach this.

@@ -158,6 +167,10 @@ class Qukeys : public kaleidoscope::Plugin {
EventHandlerResult afterEachCycle();

private:
// An array of Qkeys in PROGMEM.
Key const *qkeys_ PROGMEM;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this needs to be initialized somehow.

Comment on lines 250 to 254
constexpr Key Qkey(Key tap_key, Key qkey) {
uint8_t qkey_index = Qukeys.storeQkey(qkey);
// TODO(EvyBongers): store the qkey index somewhere, somehow
return Key(kaleidoscope::ranges::QK_FIRST + tap_key.getRaw());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first thought was to use the same approach as with the ModTap and LayerTap keys, but if I grokked the logic of storing key code and flags in the Key class correctly, that would not be possible without functionality. Now I just need to find some way to store the reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, simply using the sum of these values seems bound to go wrong. I might have to store both key values in PROGMEM (or EEPROM) and return the sum of QK_FIRST and the returned index.

@EvyBongers
Copy link
Contributor Author

I fail to come up with any way to make this work. I would love to see the full functionality of Qukeys available through Chrysalis, but I'm out of ideas on how to work toward that goal.

@EvyBongers EvyBongers closed this May 10, 2024
@EvyBongers EvyBongers deleted the feat/inKeymapQukeys branch May 10, 2024 04:45
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