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

Permissive Hold with Custom Tap Dance Functions[Feature Request] #9158

Closed nathanvercaemert closed 4 years ago

nathanvercaemert commented 4 years ago

Feature Request Type

Description

This might be a question of how to use the firmware, as opposed to changing the firmware, but here it goes: I have used some of the documentation to write custom tap dance functions that treat non-basic keycodes (TO layer switches) with modifiers like MT keys. In other words, I needed to write some custom functions to have keys that applied modifiers to some TO layer switches like dual function keys. My functions work, but I would like to see the permissive hold functionality for the "custom" MT keys.

Detailed explanation: the specific use case that caused me to notice the issue was control-clicking. I use mouse keys, and my control modifier on my mouse layer happens to be one of these "custom" MT keys with a layer switch. Currently I have to hold the "custom" MT down for the TAPPING_TERM and then click the mouse click key to get a control-click. If it were a regular MT and I enabled permissive hold, I could (all within the TAPPING_TERM) simply: press the "custom" MT, press and release the mouse click, and release the "custom" MT.

Here is the code for the custom functions that is relevant:

// determine the tapdance state to return
int cur_dance (qk_tap_dance_state_t *state) {
  if (state->count == 1) {
    if (state->interrupted || !state->pressed) { return SINGLE_TAP; }
    else { return SINGLE_HOLD; }
  }
  else { return 2; } // any number higher than the maximum state value you return above
}

and:

void ctrlto12_finished (qk_tap_dance_state_t *state, void *user_data) {
  td_state = cur_dance(state);
  switch (td_state) {
    case SINGLE_TAP:
      layer_on(12);
      break;
    case SINGLE_HOLD:
      register_mods(MOD_BIT(KC_LCTRL));
      break;
  }
}

Now here's the twist: I can partly achieve the functionality that I want by removing

state->interrupted ||

from the if statement in the first function. When I do this, I can get the quick control-click. Unfortunately, it does not mirror the permissive hold functionality exactly. If I "interrupt" the tap dance with another key (for example, the next key I intended to click after a layer switch tap,) the "custom" MT still outputs a modifier. In this case: I depress the custom key, depress the next key, release the custom key, and the release the next key. If it functioned like "permissively-held" keys, this would simply send the two tap keys (a layer switch and the key on the switched layer) instead of modifying the key on the un-switched layer.

I am mostly looking for tips regarding how to approach this, even if it's just the files in which the functionality takes place, as I am hoping to be able to contribute this feature on my own. I am a student with some knowledge of C, and this is something I would like to work on. I just need a little help being pointed in the right direction. I'm working through process_tap_dance.c, but it's slow going!

Sorry if my wording is confusing. Putting these issues into words can be difficult sometimes!

nathanvercaemert commented 4 years ago

Closed and reopened accidentally.

james-mchugh commented 4 years ago

I am interested in similar functionality as well. The closest I have gotten so far is this:

int current_dance(qk_tap_dance_state_t *state) {                                                                                                                                                                                                                                          
    int current_state = 0;                                                                                                                                                                                                                                                                
    bool is_new_dance = QK_TAP_DANCE <= state->interrupting_keycode && state->interrupting_keycode <= QK_TAP_DANCE_MAX;                                                                                                                                                                   
    if (state->count == 1) {                                                                                                                                                                                                                                                              
        if (state->pressed && !is_new_dance) {                                                                                                                                                                                                                                            
            current_state = SINGLE_HOLD;                                                                                                                                             
        } else {                                                                                                                                                                                                                                                                          
            current_state = SINGLE_TAP;                                                                                                                                                                                                                                                   
        }                               

I handle the SINGLE_HOLD first because if a new key is pressed during the tapping term, it will be considered interrupted, which we do not want. I also check if the interrupting key is a tap dance key. This is to compensate for how tap dances are handled when a new one starts while another one is still in action. Typically, if a new tap dance key is hit while another one is in progress, it will first the old one off and then start the new one. That causes the above function to get in a weird state if we do not check whether the new key is a tap dance key. The main downside of the above approach is that you are still subject to waiting the tapping term if you want to hold a tap dance key and then hit another one. I hope to eventually find a way around this as well, but the above approach seems to be working for other keys so far. If you see a flaw in my approach, please let me know.

sigprof commented 4 years ago

If I "interrupt" the tap dance with another key (for example, the next key I intended to click after a layer switch tap,) the "custom" MT still outputs a modifier. In this case: I depress the custom key, depress the next key, release the custom key, and the release the next key.

So you apparently need exactly the PERMISSIVE_HOLD mode of tap/hold detection: if the next key is pressed and released while the custom key is still pressed even within the tapping term, you want to choose the hold action for your custom key (output a modifier), but in case the next key is pressed and not released before the custom key is released, you want to choose the tap action (layer switch).

Unfortunately, I think that achieving this behavior is not possible using the tap dance code, or even using a completely custom keycode handled in process_record_user() without heavy hacks. The problem is that you apparently need to go backwards in time: the appropriate action for the second keypress can be determined only after one of the following happens:

The code which handles LT and MT keys can perform these “go backwards in time” actions, because it does its work by placing key events in a queue until the tap/hold decision can be finished, and passing those queued events to process_record() only after the appropriate action for the dual role key has been determined. It also does this queuing at the level where key events are still represented by the row/col location in the matrix (not yet mapped to keycodes), so these events can be interpreted according to the layer state changes which depend on the tap/hold decision.

In theory you could try to perform the same kind of queuing in your process_record_user(), but this function may be invoked too late (process_record_quantum() does some thing before invoking process_record_kb()), therefore some events might not be queued properly.

Alternatively, you can try to hijack the builtin tap/hold detection code for your own use. Choose a layer number which is not actually used in your keymap (let's call it _DUAL_ROLE_FAKE_LAYER), then define your custom dual role keycode:

#define U_C_12 LT(_DUAL_ROLE_FAKE_LAYER, 0)

Then configure PERMISSIVE_HOLD for that keycode (beware that at the moment the PERMISSIVE_HOLD_PER_KEY feature does not behave in the expected way: #8999), and handle that keycode in your process_record_user:

    case U_C_12:
        if (record->event.pressed) {
            if (record->tap.count > 0 {
                layer_on(12);
            } else {
                register_mods(MOD_BIT(KC_LCTRL));
            }
        } else {
            if (record->tap.count > 0 {
                // do nothing for the layer switch on tap
            } else {
                unregister_mods(MOD_BIT(KC_LCTRL));
            }
        }
        return false; // important: do not perform the standard LT action

Note that the builtin tap/hold detector does not support multi-tap actions in the manner the tap dance code does — process_record_user() would still be invoked for every key press and release, and there is no way to determine when the tapping term expires (and therefore the number of taps really became final).

This method of hijacking the LT keycodes also looks like a hack, but I don't know any other way to reuse the builtin tap/hold detector for custom functions (there was a way to do it usingfn_actions and action_function(), but this is deprecated code which was inherited from TMK and is already on the way out).

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs. For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

stale[bot] commented 4 years ago

This issue has been automatically closed because it has not had activity in the last 30 days. If this issue is still valid, re-open the issue and let us know.