New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tap dancing repeats key #563

Closed
kthibodeaux opened this Issue Jul 26, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@kthibodeaux

kthibodeaux commented Jul 26, 2016

After rebasing qmk_firmware and compiling my custom keymap on top of it I noticed tap dancing will double press if the combination (and then a different key) is entered too quickly.

For example in vim if I ;;w very fast I should get :w but now I will get :w:

I did a git bisect to find the commit that changed this behavior and found it was 70e4248

My configuration can be found here https://github.com/kthibodeaux/ergodox-layout

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 27, 2016

Contributor

Interesting, that should not have changed the behaviour this way. Looking into it!

Contributor

algernon commented Jul 27, 2016

Interesting, that should not have changed the behaviour this way. Looking into it!

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Jul 27, 2016

Contributor

Managed to reproduce this too. It appears that if you press w while : is on the way up, this happens. It didn't happen before the referenced commit, because we only registered and unregistered on keyup, while we register on keypress now, if possible... mind you, this is with my own keymap, which uses a slightly more complicated :; function... it still should not happen with your variant.

Will be looking into that in a bit.

Contributor

algernon commented Jul 27, 2016

Managed to reproduce this too. It appears that if you press w while : is on the way up, this happens. It didn't happen before the referenced commit, because we only registered and unregistered on keyup, while we register on keypress now, if possible... mind you, this is with my own keymap, which uses a slightly more complicated :; function... it still should not happen with your variant.

Will be looking into that in a bit.

@algernon

This comment has been minimized.

Show comment
Hide comment
@algernon

algernon Aug 11, 2016

Contributor

Just ran into this again when working on my new layout, in a way that's going to be a show stopper, so I'll try to get around and fix this over the weekend, so I can work properly next week.

This time, I'm using a very simple on_finished callback:

void ang_tap_dance_bp_finished (qk_tap_dance_state_t *state, void *user_data) {
  bool left, parens;

  if (state->count > 2) {
    state->count = 0;
    return;
  }

  if (state->keycode == TD(CT_LBP))
    left = true;
  else
    left = false;

  if (state->count == 1)
    parens = false;
  else
    parens = true;

  if (parens) {
    register_code (KC_RSFT);
    if (left) {
      TAP_ONCE(KC_9);
    } else {
      TAP_ONCE(KC_0);
    }
    unregister_code (KC_RSFT);
  } else {
    if (left) {
      TAP_ONCE (KC_LBRC);
    } else {
      TAP_ONCE (KC_RBRC);
    }
  }
}

The idea here is that this types ( or ) on double tap, and [{/]} on single tap. Now, to type {, this has to be shifted. If I tap shift and this key in quick succession, it registers as {[, because the shift's keyup makes it very confused:

  • I press shift (one-shot), and that registers.
  • I press this tap-dance key, and it registers with a count of one.
  • Shift's keyup is registered, interrupting the sequence, so on_finished is called.
  • The tap-dance key's keyup is registered too, and on_finished is called again.

This explains your situation too, and the reason behind this is the same as in #527, and the same solution will fix both.

Apologies for being so slow to fix this, it's been a few busy weeks for me, and this is a tough nut to crack, too.

Contributor

algernon commented Aug 11, 2016

Just ran into this again when working on my new layout, in a way that's going to be a show stopper, so I'll try to get around and fix this over the weekend, so I can work properly next week.

This time, I'm using a very simple on_finished callback:

void ang_tap_dance_bp_finished (qk_tap_dance_state_t *state, void *user_data) {
  bool left, parens;

  if (state->count > 2) {
    state->count = 0;
    return;
  }

  if (state->keycode == TD(CT_LBP))
    left = true;
  else
    left = false;

  if (state->count == 1)
    parens = false;
  else
    parens = true;

  if (parens) {
    register_code (KC_RSFT);
    if (left) {
      TAP_ONCE(KC_9);
    } else {
      TAP_ONCE(KC_0);
    }
    unregister_code (KC_RSFT);
  } else {
    if (left) {
      TAP_ONCE (KC_LBRC);
    } else {
      TAP_ONCE (KC_RBRC);
    }
  }
}

The idea here is that this types ( or ) on double tap, and [{/]} on single tap. Now, to type {, this has to be shifted. If I tap shift and this key in quick succession, it registers as {[, because the shift's keyup makes it very confused:

  • I press shift (one-shot), and that registers.
  • I press this tap-dance key, and it registers with a count of one.
  • Shift's keyup is registered, interrupting the sequence, so on_finished is called.
  • The tap-dance key's keyup is registered too, and on_finished is called again.

This explains your situation too, and the reason behind this is the same as in #527, and the same solution will fix both.

Apologies for being so slow to fix this, it's been a few busy weeks for me, and this is a tough nut to crack, too.

@kthibodeaux

This comment has been minimized.

Show comment
Hide comment
@kthibodeaux

kthibodeaux Aug 11, 2016

Apologies for being so slow to fix this

No worries! I appreciate you bringing the feature in. I haven't used it long but it has become essential for my workflow.

kthibodeaux commented Aug 11, 2016

Apologies for being so slow to fix this

No worries! I appreciate you bringing the feature in. I haven't used it long but it has become essential for my workflow.

algernon added a commit to algernon/qmk_firmware that referenced this issue Aug 17, 2016

tap-dance: Major rework, to make it more reliable
This reworks how the tap-dance feature works: instead of one global
state, we have a state for each tap-dance key, so we can cancel them
when another tap-dance key is in flight. This fixes qmk#527.

Since we have a state for each key, we can avoid situation where a keyup
would mess with our global state. This fixes qmk#563.

And while here, we also make sure to fire events only once, and this
fixes qmk#574.

There is one breaking change, though: tap-dance debugging support was
removed, because dumping the whole state would increase the firmware
size too much. Any keymap that made use of this, will have to be
updated (but there's no such keymap in the repo).

Also, there's a nice trick used in this rework: we need to iterate
through tap_dance_actions in a few places, to check for timeouts, and so
on. For this, we'd need to know the size of the array. We can't discover
that at compile-time, because tap-dance gets compiled separately. We'd
like to avoid having to terminate the list with a sentinel value,
because that would require updates to all keymaps that use the feature.
So, we keep track of the highest tap-dance code seen so far, and iterate
until that index.

Signed-off-by: Gergely Nagy <algernon@madhouse-project.org>

algernon added a commit to algernon/qmk_firmware that referenced this issue Aug 17, 2016

tap-dance: Major rework, to make it more reliable
This reworks how the tap-dance feature works: instead of one global
state, we have a state for each tap-dance key, so we can cancel them
when another tap-dance key is in flight. This fixes qmk#527.

Since we have a state for each key, we can avoid situation where a keyup
would mess with our global state. This fixes qmk#563.

And while here, we also make sure to fire events only once, and this
fixes qmk#574.

There is one breaking change, though: tap-dance debugging support was
removed, because dumping the whole state would increase the firmware
size too much. Any keymap that made use of this, will have to be
updated (but there's no such keymap in the repo).

Also, there's a nice trick used in this rework: we need to iterate
through tap_dance_actions in a few places, to check for timeouts, and so
on. For this, we'd need to know the size of the array. We can't discover
that at compile-time, because tap-dance gets compiled separately. We'd
like to avoid having to terminate the list with a sentinel value,
because that would require updates to all keymaps that use the feature.
So, we keep track of the highest tap-dance code seen so far, and iterate
until that index.

Signed-off-by: Gergely Nagy <algernon@madhouse-project.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment