Skip to content
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.

Sign up for GitHub

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

On-each-release tap dance function #20255

Merged
merged 9 commits into from
Jul 27, 2023
Merged

On-each-release tap dance function #20255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0bf417b
tap dance on_each_release_fn
leep-frog Mar 24, 2023
6378c85
Re-add on_each_tap field
leep-frog Apr 8, 2023
78cb4cd
add function indent to fix linting issue
leep-frog Apr 8, 2023
3ac6107
Switch on-each-release function to simply be a separate macro
leep-frog Jul 7, 2023
dc0da95
Add tap dance AdvancedWithRelease tests
leep-frog Jul 12, 2023
c213430
Update tap dance doc on_each_release_fn description
leep-frog Jul 12, 2023
29b3589
remove extra newline
leep-frog Jul 12, 2023
1313404
Update docs/feature_tap_dance.md
leep-frog Jul 13, 2023
29ac6ae
ensure reset fn is only called once in on_each_release tap dance func…
leep-frog Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/feature_tap_dance.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ After this, you'll want to use the `tap_dance_actions` array to specify what act
* `ACTION_TAP_DANCE_LAYER_TOGGLE(kc, layer)`: Sends the `kc` keycode when tapped once, or toggles the state of `layer`. (this functions like the `TG` layer keycode).
* `ACTION_TAP_DANCE_FN(fn)`: Calls the specified function - defined in the user keymap - with the final tap count of the tap dance action.
* `ACTION_TAP_DANCE_FN_ADVANCED(on_each_tap_fn, on_dance_finished_fn, on_dance_reset_fn)`: Calls the first specified function - defined in the user keymap - on every tap, the second function when the dance action finishes (like the previous option), and the last function when the tap dance action resets.

* `ACTION_TAP_DANCE_FN_ADVANCED_WITH_RELEASE(on_each_tap_fn, on_each_release_fn, on_dance_finished_fn, on_dance_reset_fn)`: This macro is identical to `ACTION_TAP_DANCE_FN_ADVANCED` with the addition of `on_each_release_fn` which is invoked every time the key for the tap dance is released. It is worth noting that `on_each_release_fn` will still be called even when the key is released after the dance finishes (e.g. if the key is released after being pressed and held for longer than the `TAPPING_TERM`).

The first option is enough for a lot of cases, that just want dual roles. For example, `ACTION_TAP_DANCE_DOUBLE(KC_SPC, KC_ENT)` will result in `Space` being sent on single-tap, `Enter` otherwise.

Expand Down
8 changes: 8 additions & 0 deletions quantum/process_keycode/process_tap_dance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ static inline void process_tap_dance_action_on_each_tap(tap_dance_action_t *acti
_process_tap_dance_action_fn(&action->state, action->user_data, action->fn.on_each_tap);
}

static inline void process_tap_dance_action_on_each_release(tap_dance_action_t *action) {
_process_tap_dance_action_fn(&action->state, action->user_data, action->fn.on_each_release);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

static inline void process_tap_dance_action_on_reset(tap_dance_action_t *action) {
_process_tap_dance_action_fn(&action->state, action->user_data, action->fn.on_reset);
del_weak_mods(action->state.weak_mods);
Expand Down Expand Up @@ -158,8 +162,12 @@ bool process_tap_dance(uint16_t keycode, keyrecord_t *record) {
process_tap_dance_action_on_each_tap(action);
active_td = action->state.finished ? 0 : keycode;
} else {
process_tap_dance_action_on_each_release(action);
if (action->state.finished) {
process_tap_dance_action_on_reset(action);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Expand Down
14 changes: 9 additions & 5 deletions quantum/process_keycode/process_tap_dance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ typedef struct {
tap_dance_user_fn_t on_each_tap;
tap_dance_user_fn_t on_dance_finished;
tap_dance_user_fn_t on_reset;
tap_dance_user_fn_t on_each_release;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}, ... }

} fn;
void *user_data;
} tap_dance_action_t;
Expand All @@ -56,19 +57,22 @@ typedef struct {
} tap_dance_dual_role_t;

#define ACTION_TAP_DANCE_DOUBLE(kc1, kc2) \
{ .fn = {tap_dance_pair_on_each_tap, tap_dance_pair_finished, tap_dance_pair_reset}, .user_data = (void *)&((tap_dance_pair_t){kc1, kc2}), }
{ .fn = {tap_dance_pair_on_each_tap, tap_dance_pair_finished, tap_dance_pair_reset, NULL}, .user_data = (void *)&((tap_dance_pair_t){kc1, kc2}), }

#define ACTION_TAP_DANCE_LAYER_MOVE(kc, layer) \
{ .fn = {tap_dance_dual_role_on_each_tap, tap_dance_dual_role_finished, tap_dance_dual_role_reset}, .user_data = (void *)&((tap_dance_dual_role_t){kc, layer, layer_move}), }
{ .fn = {tap_dance_dual_role_on_each_tap, tap_dance_dual_role_finished, tap_dance_dual_role_reset, NULL}, .user_data = (void *)&((tap_dance_dual_role_t){kc, layer, layer_move}), }

#define ACTION_TAP_DANCE_LAYER_TOGGLE(kc, layer) \
{ .fn = {NULL, tap_dance_dual_role_finished, tap_dance_dual_role_reset}, .user_data = (void *)&((tap_dance_dual_role_t){kc, layer, layer_invert}), }
{ .fn = {NULL, tap_dance_dual_role_finished, tap_dance_dual_role_reset, NULL}, .user_data = (void *)&((tap_dance_dual_role_t){kc, layer, layer_invert}), }

#define ACTION_TAP_DANCE_FN(user_fn) \
{ .fn = {NULL, user_fn, NULL}, .user_data = NULL, }
{ .fn = {NULL, user_fn, NULL, NULL}, .user_data = NULL, }

#define ACTION_TAP_DANCE_FN_ADVANCED(user_fn_on_each_tap, user_fn_on_dance_finished, user_fn_on_dance_reset) \
{ .fn = {user_fn_on_each_tap, user_fn_on_dance_finished, user_fn_on_dance_reset}, .user_data = NULL, }
{ .fn = {user_fn_on_each_tap, user_fn_on_dance_finished, user_fn_on_dance_reset, NULL}, .user_data = NULL, }

#define ACTION_TAP_DANCE_FN_ADVANCED_WITH_RELEASE(user_fn_on_each_tap, user_fn_on_each_release, user_fn_on_dance_finished, user_fn_on_dance_reset) \
{ .fn = {user_fn_on_each_tap, user_fn_on_dance_finished, user_fn_on_dance_reset, user_fn_on_each_release}, .user_data = NULL, }

#define TD(n) (QK_TAP_DANCE | TD_INDEX(n))
#define TD_INDEX(code) ((code)&0xFF)
Expand Down
24 changes: 23 additions & 1 deletion tests/tap_dance/examples.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,35 @@ void x_reset(tap_dance_state_t *state, void *user_data) {
xtap_state.state = TD_NONE;
}

static void release_press(tap_dance_state_t *state, void *user_data) {
tap_code16(KC_P);
}

static void release_unpress(tap_dance_state_t *state, void *user_data) {
tap_code16(KC_U);
}

static void release_unpress_mark_finished(tap_dance_state_t *state, void *user_data) {
tap_code16(KC_U);
state->finished = true;
}

static void release_finished(tap_dance_state_t *state, void *user_data) {
tap_code16(KC_F);
}

static void release_reset(tap_dance_state_t *state, void *user_data) {
tap_code16(KC_R);
}

tap_dance_action_t tap_dance_actions[] = {
[TD_ESC_CAPS] = ACTION_TAP_DANCE_DOUBLE(KC_ESC, KC_CAPS),
[CT_EGG] = ACTION_TAP_DANCE_FN(dance_egg),
[CT_FLSH] = ACTION_TAP_DANCE_FN_ADVANCED(dance_flsh_each, dance_flsh_finished, dance_flsh_reset),
[CT_CLN] = ACTION_TAP_DANCE_TAP_HOLD(KC_COLN, KC_SCLN),
[X_CTL] = ACTION_TAP_DANCE_FN_ADVANCED(NULL, x_finished, x_reset)
[X_CTL] = ACTION_TAP_DANCE_FN_ADVANCED(NULL, x_finished, x_reset),
[TD_RELEASE] = ACTION_TAP_DANCE_FN_ADVANCED_WITH_RELEASE(release_press, release_unpress, release_finished, release_reset),
[TD_RELEASE_AND_FINISH] = ACTION_TAP_DANCE_FN_ADVANCED_WITH_RELEASE(release_press, release_unpress_mark_finished, release_finished, release_reset),
};

// clang-format on
2 changes: 2 additions & 0 deletions tests/tap_dance/examples.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ enum {
CT_FLSH,
CT_CLN,
X_CTL,
TD_RELEASE,
TD_RELEASE_AND_FINISH,
};

#ifdef __cplusplus
Expand Down
116 changes: 116 additions & 0 deletions tests/tap_dance/test_examples.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,3 +316,119 @@ TEST_F(TapDance, QuadFunction) {
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();
}

TEST_F(TapDance, DanceFnAdvancedWithRelease) {
TestDriver driver;
InSequence s;
auto key_rls = KeymapKey(0, 1, 0, TD(TD_RELEASE));

set_keymap({key_rls});

/* Single press and unpress */
key_rls.press();
EXPECT_REPORT(driver, (KC_P));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

key_rls.release();
EXPECT_REPORT(driver, (KC_U));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

EXPECT_REPORT(driver, (KC_F));
EXPECT_EMPTY_REPORT(driver);
EXPECT_REPORT(driver, (KC_R));
EXPECT_EMPTY_REPORT(driver);
idle_for(TAPPING_TERM);
run_one_scan_loop();

/* Double press and unpress */
key_rls.press();
EXPECT_REPORT(driver, (KC_P));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

key_rls.release();
EXPECT_REPORT(driver, (KC_U));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

key_rls.press();
EXPECT_REPORT(driver, (KC_P));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

key_rls.release();
EXPECT_REPORT(driver, (KC_U));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

EXPECT_REPORT(driver, (KC_F));
EXPECT_EMPTY_REPORT(driver);
EXPECT_REPORT(driver, (KC_R));
EXPECT_EMPTY_REPORT(driver);
idle_for(TAPPING_TERM);
run_one_scan_loop();

/* Unpress after tapping term has elapsed (key is registered as held) */
key_rls.press();
EXPECT_REPORT(driver, (KC_P));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

EXPECT_REPORT(driver, (KC_F));
EXPECT_EMPTY_REPORT(driver);
idle_for(TAPPING_TERM);
run_one_scan_loop();

key_rls.release();
EXPECT_REPORT(driver, (KC_U));
EXPECT_EMPTY_REPORT(driver);
EXPECT_REPORT(driver, (KC_R));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();
}

TEST_F(TapDance, DanceFnAdvancedWithReleaseAndFinish) {
TestDriver driver;
InSequence s;
auto key_rls = KeymapKey(0, 1, 0, TD(TD_RELEASE_AND_FINISH));

set_keymap({key_rls});

/* Single press and unpress */
key_rls.press();
EXPECT_REPORT(driver, (KC_P));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

key_rls.release();
EXPECT_REPORT(driver, (KC_U));
EXPECT_EMPTY_REPORT(driver);
EXPECT_REPORT(driver, (KC_R));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

// Verify the finished and/or reset functions aren't called
// after the tapping term elapses
idle_for(TAPPING_TERM);
run_one_scan_loop();

/* Unpress after tapping term has elapsed (key is registered as held) */
key_rls.press();
EXPECT_REPORT(driver, (KC_P));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();

EXPECT_REPORT(driver, (KC_F));
EXPECT_EMPTY_REPORT(driver);
idle_for(TAPPING_TERM);
run_one_scan_loop();

key_rls.release();
EXPECT_REPORT(driver, (KC_U));
EXPECT_EMPTY_REPORT(driver);
EXPECT_REPORT(driver, (KC_R));
EXPECT_EMPTY_REPORT(driver);
run_one_scan_loop();
}