On-each-release tap dance function

[leep-frog]

Description

The current tap dance logic only provides a hook for every key tap, but not for every key release in the dance. This PR adds another hook function that is called for every key release in the tap dance. It is only enabled when the TAP_DANCE_ON_EACH_RELEASE variable is defined in config.h (which will prevent any effects on current users).

Types of Changes

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

Issues Fixed or Closed by This PR

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).

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[leep-frog]

This CR is still ready for review. What is the best way to get eyes on it?

[drashna]

This is ... messy, at best.

If anything, I'd say that this should just have the on_each_release option, and just have the ifdef in the function (as it is in the c file).

[leep-frog]

Yea, I was thinking of the same approach initially, but I was trying to ensure that this feature didn't affect the memory usage for keyboards that didn't use that functionality, hence why I went with this implementation instead.

But, as you mentioned, the implementation turned out less than ideal; in the newest commit, I removed the macro-conditionals such that the new function parameter is always present. There's no need for the TAP_DANCE_ON_EACH_RELEASE anymore (since the function added in the c file will check for NULL).

Thanks for the initial review and comments, and let me know what you think of the changes.

[leep-frog]

This function checks if on_each_release is null btw (see here)

[leep-frog]

I added this at the end so existing user code that uses positional initialization doesn't break. For example, the following will still compile even though only three of the four arguments are provided:

{ .fn = {user_fn_on_each_tap, user_fn_on_dance_finished, user_fn_on_dance_reset}, ... }

[drashna]

Could you add to the tap dances tests, as well?

They're located in tests/tap_dance.

[leep-frog]

@drashna I just added tests, but the CI build is failing. Seems like an issue with merging from the main branch into the develop branch though. What's the best way to fix that?

[sigprof]

Apparently you merged something wrong into your branch. Undo that merge by resetting to the last good commit:

git reset --hard a607f962086fed74b29e9cd3950fec8765a66b9c

Then try merging with the actual state of the develop branch:

git fetch upstream
git merge upstream/develop

The automatic merge would fail, because there were some other changes to quantum/process_keycode/process_tap_dance.c and quantum/process_keycode/process_tap_dance.h, so you would need to resolve the conflicts manually. Finally, commit and push with --force-with-lease.

[leep-frog]

@sigprof suggestion accepted. Back to you

[leep-frog]

@sigprof @drashna can one of you merge this pull request? I don't have write access and can't merge it. Feel free to squash the commits as well when merging.

[sigprof]

I suspect that the following change would be needed to fix the corner case when the on_each_release callback does state->finished = true; to finish the tap dance early (this kind of usage currently works for on_each_tap and is somewhat documented).

Suggested change

	process_tap_dance_action_on_reset(action);
	process_tap_dance_action_on_reset(action);
	if (active_td == keycode) {
	active_td = 0;
	}

But I don't have time to test this just now (this would probably need another test to make sure that there are no superfluous finished calls after the tap dance timer expires).

And using reset_tap_dance(state); in on_each_release() would work properly anyway, because it resets active_td and clears the whole state (so the duplicate call of the reset callback won't happen); but it's not equivalent to state->finished = true; when used in the on_each_tap callback, so probably both cases should be supported.

[leep-frog]

I just added your change and another test function that verifies the expected behavior. Let me know if there are any other cases you think should be added.

[leep-frog]

@sigprof I made changes for the comment you made earlier. Mind re-reviewing and/or merging?

[leep-frog]

@sigprof @drashna Any chance either of you can merge this?