tap-dance: Restructure code and document in more detail

[joukewitteveen]

Description

These changes were developed when studying the tap dance code.

Crucially, let code and documentation reflect the following.
on_each_tap_fn: Called on each key-down of a dance.
on_dance_finished_fn: Called on timeout and interrupt, unless suppressed.
on_dance_reset_fn: Called on (or after) final key-up.

The structure packing on the action struct and per-action state struct is most likely a pointless optimization, but I don't think it hurts readability too much.

There was one bug in reset_tap_dance, where it would be a no-op in case a key was still pressed. This was not what appeared to be assumed by external usage. I decided to put all the responsibility in the process_tap_dance_action_… functions and admit that reset_tap_dance was not only used internally (i.e. I moved it around in process_tap_dance.h and documented it.

I think the added example shows a lot of tap-dance internals at work that were not (or less clearly) shown by other examples.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

It isn't clear to me what constitutes a 'Core PR'. This PR targets develop just to be sure. I expect this PR may feel a bit all over the place. If so, let me know how I can restructure it to make reviewing it easier.

One thing I left out in this PR is changing tap_dance_task to use &tap_dance_actions[last_td]. As far as I can tell, there is only ever one tap dance 'active', so there is no reason to iterate through all of them.

I was hesitant to touch other peoples keymaps, but thought the change to using user_data in keyboards/thevankeyboards/minivan/keymaps/belak/keymap.c and the replacements in keyboards/jones/v03*/keymaps/default_*/keymap.c were concise enough.

[joukewitteveen]

One thing I left out in this PR is changing tap_dance_task to use &tap_dance_actions[last_td]. As far as I can tell, there is only ever one tap dance 'active', so there is no reason to iterate through all of them.

I added this as well. This would be the most functional change in this PR, so I gave it a separate commit to ease reviewing.

[joukewitteveen]

@algernon, @jackhumbert, what do you think? The code was in pretty good shape already, but I could shave some bytes and cycles without sacrificing readability (I think). I also isolated the action responsibilities more thoroughly, pulling them out of the process_… and …_task functions.

The added documentation is something I would have liked to read myself.

[drashna]

This could probably be nuked to, in favor of the get_tapping_term support.

Suggested change

uint16_t custom_tapping_term;

[joukewitteveen]

Yeah, that would clean up the tap-dance implementation nicely. I added a commit where I do so. I looked into usage of ACTION_TAP_DANCE_FN_ADVANCED_TIME and provided an alternative for all instances. It looks like some could just as well set TAPPING_TERM to the desired value, but I didn't want to be too invasive.

While this achieves a unified way of setting per-key tapping terms, it is somewhat more verbose for the keymaps.

If we want this, I assume we need a changelog entry, which I did not write.

[sigprof]

ACTION_TAP_DANCE_FN_ADVANCED_TIME(), which uses that field, was deprecated back in #6259 (merged on 25 Jul 2020) in favor of the generic custom tapping term support; that's probably enough time to get it removed completely.

[joukewitteveen]

I'm a bit unsure what to do about the TAPPING_TERM_PER_KEY macro. It doesn't seem to be orthogonal to the other config settings such as DYNAMIC_TAPPING_TERM and NO_ACTION_TAPPING. Especially the latter I don't fully understand the intent of. If tap-dance is supposed to work even with NO_ACTION_TAPPING, we might want to have #define get_tapping_term(keycode, event) (TAPPING_TERM) somewhere on a NO_ACTION_TAPPING code path (this does mean that all the get_tapping_term definitions need to be guarded). Part of my confusion is that I don't really see how the setting of TAPPING_TERM_PER_KEY would have a big impact on the firmware size/speed.

[joukewitteveen]

To keep this MR a little more focused, I restored the old TAPPING_TERM_PER_KEY behavior and spun out my thoughts on it into #16472. That should make this MR a bit easier to process.

edit: #16472 turned into a merge request: #16681. If that is merged, I'll rebase and adjust the current MR.

[joukewitteveen]

I rebased this MR on top of #16681. The churn is high, but that is mostly due to the last commit that drops ACTION_TAP_DANCE_FN_ADVANCED_TIME (as requested in this thread) and updates all the places in the repo where it was used.

As far as I can tell, this is ready to go in.

[joukewitteveen]

This MR has been in development for 3 months now and the churn is only getting higher. @drashna, can you confirm we are still progressing toward an eventual merge? We now have a restructuring of the code, including more efficient action/state structs and simplification of the overall implementation, fixes to corner cases and initial test coverage, and deprecation of ACTION_TAP_DANCE_FN_ADVANCED_TIME.

[KarlK90]

To accommodate the changes it would be very good to have unit-test which at least cover the examples which are in the documentation. So we can be sure that any future refactoring of QMKs core implementations won't break the tap dance implementation.

[joukewitteveen]

To accommodate the changes it would be very good to have unit-test which at least cover the examples which are in the documentation. So we can be sure that any future refactoring of QMKs core implementations won't break the tap dance implementation.

I agree that would be nice, but since this PR doesn't really change the existing interfaces, maybe the addition of tests can be left to a follow-up PR in order to not pile any more on this one? How extensive do we want the tests to be? I guess at least the presence of the 'public' interface:

struct qk_tap_dance_state_t;
qk_tap_dance_user_fn_t;
struct qk_tap_dance_action_t;

ACTION_TAP_DANCE_DOUBLE(kc1, kc2)
ACTION_TAP_DANCE_DUAL_ROLE(kc, layer)
ACTION_TAP_DANCE_LAYER_TOGGLE(kc, layer)
ACTION_TAP_DANCE_LAYER_MOVE(kc, layer)
ACTION_TAP_DANCE_FN(user_fn)
ACTION_TAP_DANCE_FN_ADVANCED(user_fn_on_each_tap, user_fn_on_dance_finished, user_fn_on_dance_reset)

TD(n)
TD_INDEX(code)
TAP_DANCE_KEYCODE(state)

void reset_tap_dance(qk_tap_dance_state_t *state);

but maybe also some state correctness including mod handling?

[KarlK90]

I agree that would be nice, but since this PR doesn't really change the existing interfaces, maybe the addition of tests can be left to a follow-up PR in order to not pile any more on this one?

Cool! I don't mind if the test get added in a different PR if they add considerable amounts of code.

How extensive do we want the tests to be?

There is no established testing rule for QMK e.g. necessary branch or line coverage. So I would go with your recommendation of the public interface. From a brief look over the examples in the documentation it seemed that these would be a good baseline (hence the suggestion). Tricky edge case interactions which were problematic before should be covered as well. As I'm not that familiar with the code or feature (yet) I'll rely on your judgement on which parts to cover.

Thanks for improving QMK.

[joukewitteveen]

I added the simple tap dance example and the first four complex ones as test cases. I did not include tests for all the possible interrupts and not even for all elements of the public interface, but to be honest I currently don't feel like writing even more test code. There is already twice as many lines for testing than for implementing tap dance.

While writing tests, I found some erroneous corner cases in the tracking of the last tap dance, as anticipated by @sigprof. These are now fixed.

[KarlK90]

Sorry for leaving this a bit in a limbo. I'm busy with the rp2040 support right now, but I intend to push your PR for the Q3 cycle.

[joukewitteveen]

I rebased on top of #17284 and updated the tests to use the new macros. Note that the tests need to be changed again once #17028 goes in. If the current PR goes in first, #17028 can quite easily be updated with the necessary changes to the tapdance tests.

[KarlK90]

Thank you for all your effort that went into this refactor and also sticking with this PR though the churn was high. I read your changes and also tested all the examples from the test harness on a keyboard and everything works as expected. (I cheated on the egg dance and made it trigger at 5 repeats though.) There are some minor nits but you have my approval.

[joukewitteveen]

Given that this MR is already taking its sweet time, I added one more commit to it, where I remove the ACTION_TAP_DANCE_DUAL_ROLE macro which was made redundant 3 years ago. This further simplifies tap dance code.

@drashna @sigprof If this extra commit is controversial in any way, I'll remove it from this MR.

[KarlK90]

...and it is in! 🎉