qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.24k stars 39.3k forks source link

[Bug] Caps Word doesn't work with Tap Dance keys #19574

Open dropsonic opened 1 year ago

dropsonic commented 1 year ago

Describe the Bug

Problem definition

When the Caps Word feature is enabled, pressing any Tap Dance keys:

  1. Turns Caps Word off;
  2. Does not capitalize the letter on the key.

Steps to reproduce

  1. Using the documentation for Caps Word, enable the Caps Word feature. It doesn't matter how you turn this mode on when typing.
  2. Add a tap dance key for any Latin letter that types this letter on tap and sends a modifier on hold. For example, add a tap dance to W; W on tap, and Ctrl+W on hold.
  3. Enable Caps Word and type any word that contains the letter from step 2 (e.g. , owl). When pressing W, Caps Word would be disabled, and Owl would be typed instead of OWL.

I used the default Moonlander firmware with Caps Word mode enabled and Ctrl+... on hold to reproduce the issue.

Why this might happen

Tap dance keys are defined as TD(DANCE_n).

TD(n) is a macro: #define TD(n) (QK_TAP_DANCE | ((n)&0xFF))

process_caps_word (caps_word_press_user, in particular) checks only basic keycodes like KC_A ... KC_Z and KC_MINS:

__attribute__((weak)) bool caps_word_press_user(uint16_t keycode) {
    switch (keycode) {
        // Keycodes that continue Caps Word, with shift applied.
        case KC_A ... KC_Z:
        case KC_MINS:
            add_weak_mods(MOD_BIT(KC_LSFT)); // Apply shift to next key.
            send_keyboard_report();
            return true;

        // Keycodes that continue Caps Word, without shifting.
        case KC_1 ... KC_0:
        case KC_BSPC:
        case KC_DEL:
        case KC_UNDS:
            return true;

        default:
            return false; // Deactivate Caps Word.
    }
}

So when keycode is QC_TAP_DANCE ... QC_TAP_DANCE_MAX, nothing happens to the typed letter.

Also, it seems that QC_TAP_DANCE ... QC_TAP_DANCE_MAX are not ignored in process_caps_word: link to the code

Tap dance code from the Moonlander firmware that I used:

void on_dance_5(qk_tap_dance_state_t *state, void *user_data) {
    if(state->count == 3) {
        tap_code16(KC_W);
        tap_code16(KC_W);
        tap_code16(KC_W);
    }
    if(state->count > 3) {
        tap_code16(KC_W);
    }
}

void dance_5_finished(qk_tap_dance_state_t *state, void *user_data) {
    dance_state[5].step = dance_step(state);
    switch (dance_state[5].step) {
        case SINGLE_TAP: register_code16(KC_W); break;
        case SINGLE_HOLD: register_code16(LCTL(KC_W));
            break;
        case DOUBLE_TAP: register_code16(KC_W); register_code16(KC_W); break;
        case DOUBLE_SINGLE_TAP: tap_code16(KC_W); register_code16(KC_W);
    }
}

void dance_5_reset(qk_tap_dance_state_t *state, void *user_data) {
    wait_ms(10);
    switch (dance_state[5].step) {
        case SINGLE_TAP: unregister_code16(KC_W); break;
        case SINGLE_HOLD: unregister_code16(LCTL(KC_W)); break;
        case DOUBLE_TAP: unregister_code16(KC_W); break;
        case DOUBLE_SINGLE_TAP: unregister_code16(KC_W); break;
    }
    dance_state[5].step = 0;
}

qk_tap_dance_action_t tap_dance_actions[] = {
    [DANCE_5] = ACTION_TAP_DANCE_FN_ADVANCED(on_dance_5, dance_5_finished, dance_5_reset),
};

Keyboard Used

Moonlander

Link to product page (if applicable)

No response

Operating System

Windows 10

qmk doctor Output

No response

Is AutoHotKey / Karabiner installed

Other keyboard-related software installed

No response

Additional Context

No response

sigprof commented 1 year ago

Supporting Tap Dance in Caps Word may be really complicated:

Probably the only way to implement the Tap Dance support would be:

  1. Implement caps_word_press_user() which returns true for the relevant TD(n) keycodes (if some tap dances definitely can't be used in the middle of a word, those keycodes can be left out, so the Caps Word mode would be turned off immediately when such tap dance sequence is started)
  2. Implement the custom finished handler for the tap dance so that it checks the Caps Word status and performs some actions accordingly. This might require effectively duplicating some Caps Word code, but there is no way around that, because the keycode handlers are not invoked for any register_code16()/tap_code16() calls that might be inside the tap dance implementation. Maybe it could be possible to refactor the Caps Word code to provide some reusable helpers for this use case.
wuwendell commented 1 year ago

Wanted to +1 this issue, discovered why my caps word wasn't working due to this. Is there a fork with this in the works? I think I'll just turn off tap dance in the meantime for my alphanumeric keys, I wasn't using them often anyways.

LauraLangdon commented 1 year ago

Same, I've spent several hours trying to debug this problem this week, and finally found this issue.

ImKifu commented 1 year ago

Oh well I'm not alone here haha, spend hours trying to debug this problem this weekend hahaha. Hope someone can fix this soon <3 <3

thubble commented 10 months ago

Based on the suggestions in https://github.com/qmk/qmk_firmware/issues/19574#issuecomment-1381065727 I managed to get my layout to work the way I want, by returning true from caps_word_press_user() and manually handling the keycodes in the tapdance callbacks. The original code was generated by ZSA Oryx for a Voyager keyboard: https://github.com/thubble/qmk_firmware/commit/69e6e13d3bc2420bc7aaf2e892653927acca9e2f

It needed more manual modifications than I'd like, and it will need to be manually changed if I make changes to the layout, but it does seem to work properly so feel free to take a look if you need ideas on how to get around this issue.

rlaybourn commented 4 months ago

Just stumbled on this issue as I use home row mods . Seems this will be an issue for anyone wanting to use home row mods. A fix to this would be very beneficial.

marcelruland commented 1 month ago

Fwiw on my moonlander (on macOS, have yet to check linux) it works fine for keys which only have a tap and a hold action. Adding tap-hold is where it breaks down.

Nebucatnetzer commented 1 month ago

On Linux it works as well if it is only a simple hold action. It stops working as soon as the hold action does more then one thing (e.g. ctrl+v, ctrl+c) as mentioned in the issue.

getreuer commented 3 weeks ago

Yes to all of sigprof's comment on why Caps Word <> Tap Dance interoperation is complicated.

I suggest that where it's desired that Caps Word capitalize a Tap Dance key, that this is done through defining caps_word_press_user() to

  1. continue Caps Word when the Tap Dance key is pressed (if desired)
  2. setting weak shift mod add_weak_mods(MOD_BIT(KC_LSFT)) to capitalize (if desired)

as documented here. Then, if needed, the Tap Dance handler can conditionally act on the weak shift to know when to capitalize:

void my_dance_finished(tap_dance_state_t* state, void* user_data) {
  if ((get_weak_mods() & MOD_MASK_SHIFT) != 0) {
    clear_weak_mods();  // If necessary, clear the shift first.
    // Produce capitalized output...
  } else {
    // Produce lower case output...
  }
}

Or alternatively instead of weak shift mod, use the is_caps_word_on() function within the Tap Dance handler to know when Caps Word is active:

void my_dance_finished(tap_dance_state_t* state, void* user_data) {
  if (is_caps_word_on()) {
    // Produce capitalized output...
  } else {
    // Produce lower case output...
  }
}