[Bug] Combos not working with shift and Achoridion

[Sigvah]

Describe the bug

Whenever I try to use combos on the opposite hand while holding shift it produces the keys instead of the combo. For example shift + U + L --> becomes UL instead of (Æ shifted æ). Might be because my combo term is 35?

Information

Do the keys involved use any of the following features?

• [x] Achordion (from this repo)
• [ ] Auto Shift
• [ ] Combos
• [ ] Key Overrides
• [ ] Mod-tap or Layer-tap keys
• [ ] Other custom userspace code:

[SavageMessiah]

I also ran into something similar enough to this to think it's the same issue. It only broke going one way ie I could shift a combo on the left (master) side using the shift on the other side. If I tried the other way I would get the key the shift was on and the unshifted version of the combo. For example I have shift on t on the left hand and the combo is on the right, producing ';'. Pressing shift and the combo gets me 't;' when I would expect it to produce ':'.

[getreuer]

@Sigvah, @SavageMessiah are either of you using the ACHORDION_STREAK option (added Nov 24)? This bug report comes a few days after that was added, though perhaps that's just coincidence. I'm thinking the streak implementation needs tuning to handle combo events.

[SavageMessiah]

I changed my layout to not need shift+combos anymore but I don't think I was using that option.

[Sigvah]

I think it's just a coincidence. After some more testing I can confirm that the issue is as the other guy described.

[SavageMessiah]

I went back in my git history and confirmed that I did not have streak turned on.

[SavageMessiah]

I did a bit of debugging on this issue and have narrowed down the source of the problem and implemented a workaround. The problem seems to be that the key records emitted by the combo have a keypos with a row and col of 0. This means that on_left_hand always returns true. This explains why it works when the mod is on the right side. Since I'm not particularly worried about accidental activations of mods on the same hand as a combo, I simply skipped the opposite hand check if the record is not a key event. Here's a minimal example:

bool achordion_chord(uint16_t tap_hold_keycode,
                     keyrecord_t* tap_hold_record,
                     uint16_t other_keycode,
                     keyrecord_t* other_record) {

    if (!IS_KEYEVENT(other_record->event)) {
        return true;
    }

    return achordion_opposite_hands(tap_hold_record, other_record);
}

I've only used this for a few minutes at this point and I don't know the internals of QMK so this might have unintended side effects that I haven't come across yet. Still, I hope this helps.

[getreuer]

That's helpful, @SavageMessiah! Thanks to your comment, I realize now what has happened.

The intention is that Achordion bypasses combos and other non-key events. This is being checked as

// Check that this is a normal key event, don't act on combos.
#ifdef IS_KEYEVENT
const bool is_key_event = IS_KEYEVENT(record->event);
#else
const bool is_key_event =
    (record->event.key.row < 254 && record->event.key.col < 254);
#endif

where the #ifdef condition was added in ~2022 for backwards compatibility, when IS_KEYEVENT was introduced in QMK.

The problem with this is that despite the all-caps name, IS_KEYEVENT is actually a function, not a macro. Consequently, the preprocessor considers IS_KEYEVENT undefined and unintentionally takes the #else branch. Further, newer QMK no longer uses matrix position (255, 255) to indicate non-key events, so that condition no longer works.

I'll push a commit to remove this busted preprocessing logic and simply use is_key_event = IS_KEYEVENT(record->event).

[SavageMessiah]

Great! Thanks for the fix and the explanation (and achordion and layer lock and....). I saw that part of the code but didn't know nearly enough about QMK or how you're manipulating it to have any idea what might be going on.