[Bug] get_permissive_hold() logic is reversed

[sigprof]

In my testing keymap I added this to config.h:

#define TAPPING_TERM 499
#define PERMISSIVE_HOLD_PER_KEY
#define IGNORE_MOD_TAP_INTERRUPT // needed to make MT() behave more like LT()

Then I added a very simple implementation of get_permissive_hold() to keymap.c:

bool get_permissive_hold(uint16_t keycode, keyrecord_t *record) { return true; }

The above should be equivalent to having #define PERMISSIVE_HOLD; however, testing with both MT(MOD_LCTL, KC_CAPS) and LT(1, KC_APP) shows that they behave as if PERMISSIVE_HOLD was not enabled.

However, if I make get_permissive_hold() always return false:

bool get_permissive_hold(uint16_t keycode, keyrecord_t *record) { return false; }

then the behavior of MT(MOD_LCTL, KC_CAPS) and LT(1, KC_APP) changes to the behavior which is documented for PERMISSIVE_HOLD.

So it seems that the logic to handle get_permissive_hold() is reversed from what it should be.

Looking at the code in question:

#    if defined(TAPPING_TERM_PER_KEY) || (TAPPING_TERM >= 500) || defined(PERMISSIVE_HOLD) || defined(PERMISSIVE_HOLD_PER_KEY)
                else if (
#        ifdef TAPPING_TERM_PER_KEY
                    (get_tapping_term(get_event_keycode(tapping_key.event, false)) >= 500) &&
#        endif
#        ifdef PERMISSIVE_HOLD_PER_KEY
                    !get_permissive_hold(get_event_keycode(tapping_key.event, false), keyp) &&
#        endif
                    IS_RELEASED(event) && waiting_buffer_typed(event)) {
                    debug("Tapping: End. No tap. Interfered by typing key\n");
                    process_record(&tapping_key);
                    tapping_key = (keyrecord_t){};
                    debug_tapping_key();
                    // enqueue
                    return false;
                }
#    endif

it seems that because the whole block is enabled by defining PERMISSIVE_HOLD, it should also be enabled when get_permissive_hold() returns true, and not false as it is coded currently.

System Information

• Keyboard: id80
• Operating system: Void Linux
• AVR GCC version: 8.4.0
• ARM GCC version: 9.3.0
• QMK Firmware version: 0.8.141

[sigprof]

Another potential problem with that block of code is that if TAPPING_TERM_PER_KEY is enabled, there is no way to get the PERMISSIVE_HOLD behavior for a key for which get_tapping_term() returns a value which is less than 500. I think that enabling PERMISSIVE_HOLD_PER_KEY should just override all those tapping term checks, e.g., like this:

#    if defined(TAPPING_TERM_PER_KEY) || (TAPPING_TERM >= 500) || defined(PERMISSIVE_HOLD) || defined(PERMISSIVE_HOLD_PER_KEY)
                else if (
#        ifdef PERMISSIVE_HOLD_PER_KEY
                    get_permissive_hold(get_event_keycode(tapping_key.event, false), keyp) &&
#	 else
#            ifdef TAPPING_TERM_PER_KEY
                    (get_tapping_term(get_event_keycode(tapping_key.event, false)) >= 500) &&
#            endif
#        endif
                    IS_RELEASED(event) && waiting_buffer_typed(event)) {
                    debug("Tapping: End. No tap. Interfered by typing key\n");
                    process_record(&tapping_key);
                    tapping_key = (keyrecord_t){};
                    debug_tapping_key();
                    // enqueue
                    return false;
                }
#    endif

(I see that PR #8348 touches the same code, so I should probably base my PR on it.)

[caksoylar]

Is this closed by #12125?

[blueyed]

Is this closed by #12125?

Yes, this appears to be addressed by it - please close it if you can.