Key Chords - #1606
Conversation
Allows effect to last until the last key Lacks support for macro checks for key release Also removed an unused parameter
firngrod
left a comment
firngrod
left a comment
There was a problem hiding this comment.
A couple of comments to start off the discussion.
| bool ErrorBefore = Macros_ParserError; | ||
| Macros_ParserError = false; | ||
| uint8_t num = Macros_ConsumeInt(ctx); | ||
| if (Macros_ParserError) { | ||
| bool ErrorAfter = Macros_ParserError; | ||
| Macros_ParserError |= ErrorBefore; | ||
| if (ErrorAfter) { |
There was a problem hiding this comment.
Pretty sure I did this because I was getting misleading error messages.
| //partentMacroSlot == 255 means no parent | ||
| uint8_t startMacro( | ||
| uint8_t index, | ||
| key_state_t *keyState, | ||
| uint16_t argumentOffset, | ||
| uint8_t keyActivationId, | ||
| uint8_t parentMacroSlot, | ||
| bool runFirstAction, | ||
| const char *inlineText, | ||
| const macro_state_t *parentMacroState | ||
| ); |
There was a problem hiding this comment.
The same refactoring from #1507 because I need more initial state carry-over.
| S->ms.currentMacroStartTime = CurrentPostponedTime; | ||
| if (keyState != NULL) { | ||
| S->ms.isDoubletap = KeyHistory_WasLastDoubletap(); | ||
| S->ms.chordActivationId = KeyHistory_GetChordActivationIdOfLastAction(); // returns invalid id if it was not a chord. |
There was a problem hiding this comment.
I'm not a big fan of pulling the fact of chord activation out of the ether like this, transferring it indirectly through history. The same thing is done for doubletap though.
Avoiding it would probably require the previously discussed key event type in usb_report_updater.c.
| } | ||
|
|
||
| if (!Chords_TryAddChord(layerId, keys, keyCount, &chordAction)) { | ||
| Macros_ReportErrorPos(ctx, "Could not add chords, no more free chord slots."); |
There was a problem hiding this comment.
Presumptions of error cause from a bool. So far it's the only way it can fail, but maybe I should just do enum error codes to be proper?
There was a problem hiding this comment.
Either return bool and an additional &failReason parameter; or an enum as you suggest (0=OK, others are error codes).
Couldn't it also fail if the same chord (same keys) has already been registered?
| .Macros_OneShotTimeout = 500, | ||
| .AutoShiftDelay = 0, | ||
| .ChordingDelay = 0, | ||
| .Chords_Timeout = 75, |
There was a problem hiding this comment.
I use 30ms, @mhantsch mentioned using 100ms. Put it somewhere in the middle.
There was a problem hiding this comment.
Sounds reasonable as a default. Is it configurable somewhere?
| { | ||
| for (int i = 0; i < count; i++) { | ||
| consumeOneKeypress(suppress); | ||
| consumeOneKeypress(); |
There was a problem hiding this comment.
It was not even in the function definition anymore.
| } | ||
| } | ||
|
|
||
| static bool mouseButtonsChanged(void) { |
There was a problem hiding this comment.
Warnings getting treated as errors. As far as I can tell, these value actually were unused.
| if ( IS_SECONDARY_ROLE_LAYER_SWITCHER(secondaryRole) ) { | ||
| // If the cached action is the current base role, then hold, otherwise keymap was changed. In that case do nothing just | ||
| // as a well behaved hold action should. | ||
| if(KeyState_Active(keyState) && action->type == actionBase->type && action->keystroke.secondaryRole == actionBase->keystroke.secondaryRole) { |
There was a problem hiding this comment.
The uncached action was only used for this purpose: To disable layer hold for secondary keys on keymap switch if the key was not mapped to the same layer on the new map. I cannot see any other reasonable case for this code. There is the unreasonable case of remapping the key from a macro while it's held.
I decided to remove that to get rid of passing the base action here because the base action has very little relation to the cached action when a chord is pressed. Also, pure layer holds, (not secondary keys) are not subject to the same disabling on map change.
| } else { | ||
| KEY_TIMING(KeyTiming_RecordKeystroke(keyState, active, Timer_GetCurrentTime(), Timer_GetCurrentTime())); | ||
| keyState->current = active; | ||
| EventVector_Set(EventVector_NativeActionsPostponing); |
There was a problem hiding this comment.
As discussed, keys can now only get activated one per cycle. Any more and the rest are postponed to future cycles.
| ChordApplicationType_LeadingKey, | ||
| ChordApplicationType_AllKeys, |
There was a problem hiding this comment.
Technically, the leading keys version has no benefit that I can see over the other except for being cheaper to perform. Is that worth the complexity of the choice? Maybe there should only be the all keys version?
There was a problem hiding this comment.
On the other hand, the macro support for release detection for all-keys lifetime was pretty complex, basically the entirety of this commit, probably a fifth of the entire PR, so maybe that should be the one to go. Trouble is that I can actually conceive of real-world usecases for it, especially on the 60 where thumb keys are less abundant than on the 80. With chords, the mod button can be overloaded to be various layers depending on which key(s) it was chorded with on press, and the other key can be released while modkey is held to maintain the layer. Not really useful for me with my "No mapped combination keys" philosophy, but probably for someone else.
2 participants
Added key chords as a macro accessible feature.
I have left a self-review with some additional context which may not be readily available from the code, along with some questions about implementation strategy preferences.
There are also a lot of openers for discussions in the code comments, so don't skip those, by the way.