From 55646ceeb77dbdb455c282875dd80934e89e67fe Mon Sep 17 00:00:00 2001 From: Sergey Vlasov Date: Sat, 6 Aug 2022 23:18:13 +0300 Subject: [PATCH 1/2] Add tap dance tests for layer switching dances The set of existing tap dance tests did not cover predefined actions for layer switching (`ACTION_TAP_DANCE_LAYER_MOVE(kc, layer)`, `ACTION_TAP_DANCE_LAYER_TOGGLE(kc, layer)` - these actions perform layer switching on double tap), and also did not cover the case when a tap dance is used to implement an extended version of the `LT()` keycode (performing layer switching on hold). Add some tests for these kinds of tap dances, which actually uncover a bug in the tap dance implementation (if the tap dance finishes due to being interrupted by pressing some other key before the tapping term had expired, the layer switch may not affect that other key properly). Tests which are known to fail due to the mentioned bug: Layers/TapDanceLayers.SingleHoldFastLayer/CustomLTCustomFast Layers/TapDanceLayers.SingleHoldFastLayer/CustomLTCustomSlow Layers/TapDanceLayers.DoubleTapFastLayer/LayerToggleCustomFast Layers/TapDanceLayers.DoubleTapFastLayer/LayerToggleCustomSlow Layers/TapDanceLayers.DoubleHoldFastLayer/LayerToggleCustomFast Layers/TapDanceLayers.DoubleHoldFastLayer/LayerToggleCustomSlow All those failures manifest only when using custom keycodes, but only because there is another bug (the source layers cache gets updated inside `process_record_handler()`, overwriting its previous state) which covers up the main bug for builtin keycodes. --- tests/tap_dance/tap_dance_layers/config.h | 6 + .../tap_dance_layers/tap_dance_defs.c | 97 +++ .../tap_dance_layers/tap_dance_defs.h | 29 + tests/tap_dance/tap_dance_layers/test.mk | 10 + .../test_tap_dance_layers.cpp | 717 ++++++++++++++++++ 5 files changed, 859 insertions(+) create mode 100644 tests/tap_dance/tap_dance_layers/config.h create mode 100644 tests/tap_dance/tap_dance_layers/tap_dance_defs.c create mode 100644 tests/tap_dance/tap_dance_layers/tap_dance_defs.h create mode 100644 tests/tap_dance/tap_dance_layers/test.mk create mode 100644 tests/tap_dance/tap_dance_layers/test_tap_dance_layers.cpp diff --git a/tests/tap_dance/tap_dance_layers/config.h b/tests/tap_dance/tap_dance_layers/config.h new file mode 100644 index 000000000000..32a19a8c685c --- /dev/null +++ b/tests/tap_dance/tap_dance_layers/config.h @@ -0,0 +1,6 @@ +// Copyright 2022 Sergey Vlasov (@sigprof) +// SPDX-License-Identifier: GPL-2.0-or-later + +#pragma once + +#include "test_common.h" diff --git a/tests/tap_dance/tap_dance_layers/tap_dance_defs.c b/tests/tap_dance/tap_dance_layers/tap_dance_defs.c new file mode 100644 index 000000000000..5ec900c0412d --- /dev/null +++ b/tests/tap_dance/tap_dance_layers/tap_dance_defs.c @@ -0,0 +1,97 @@ +// Copyright 2022 Sergey Vlasov (@sigprof) +// SPDX-License-Identifier: GPL-2.0-or-later + +#include "quantum.h" +#include "tap_dance_defs.h" + +// Implement custom keycodes which are used to check that the layer switching +// behaves properly. +bool process_record_user(uint16_t keycode, keyrecord_t *record) { + switch (keycode) { + case FAST_AB: + case SLOW_AB: + if (record->event.pressed) { + tap_code(KC_A); + } else { + tap_code(KC_B); + } + return keycode == SLOW_AB; + case FAST_CD: + case SLOW_CD: + if (record->event.pressed) { + tap_code(KC_C); + } else { + tap_code(KC_D); + } + return keycode == SLOW_CD; + } + return true; +} + +// Implement a custom tap dance with the following behavior: +// - single tap: KC_APP +// - single hold: MO(1) +// - double tap/hold: KC_RCTL +// (The single tap and hold actions are mostly equivalent to LT(1, KC_APP).) + +enum lt_app_state { + LTA_NONE, + LTA_SINGLE_TAP, + LTA_SINGLE_HOLD, + LTA_DOUBLE_HOLD, +}; + +static enum lt_app_state saved_lt_app_state; + +static enum lt_app_state get_lt_app_state(qk_tap_dance_state_t *state) { + if (state->count == 1) { + if (!state->pressed) { + return LTA_SINGLE_TAP; + } else { + return LTA_SINGLE_HOLD; + } + } else if (state->count == 2) { + return LTA_DOUBLE_HOLD; + } else { + return LTA_NONE; + } +} + +static void lt_app_finished(qk_tap_dance_state_t *state, void *user_data) { + saved_lt_app_state = get_lt_app_state(state); + switch (saved_lt_app_state) { + case LTA_NONE: + break; + case LTA_SINGLE_TAP: + register_code(KC_APP); + break; + case LTA_SINGLE_HOLD: + layer_on(1); + break; + case LTA_DOUBLE_HOLD: + register_code(KC_RCTL); + break; + } +} + +static void lt_app_reset(qk_tap_dance_state_t *state, void *user_data) { + switch (saved_lt_app_state) { + case LTA_NONE: + break; + case LTA_SINGLE_TAP: + unregister_code(KC_APP); + break; + case LTA_SINGLE_HOLD: + layer_off(1); + break; + case LTA_DOUBLE_HOLD: + unregister_code(KC_RCTL); + break; + } +} + +qk_tap_dance_action_t tap_dance_actions[] = { + [TD_L_MOVE] = ACTION_TAP_DANCE_LAYER_MOVE(KC_APP, 1), + [TD_L_TOGG] = ACTION_TAP_DANCE_LAYER_TOGGLE(KC_APP, 1), + [TD_LT_APP] = ACTION_TAP_DANCE_FN_ADVANCED(NULL, lt_app_finished, lt_app_reset), +}; diff --git a/tests/tap_dance/tap_dance_layers/tap_dance_defs.h b/tests/tap_dance/tap_dance_layers/tap_dance_defs.h new file mode 100644 index 000000000000..37cab0c2cbf2 --- /dev/null +++ b/tests/tap_dance/tap_dance_layers/tap_dance_defs.h @@ -0,0 +1,29 @@ +// Copyright 2022 Sergey Vlasov (@sigprof) +// SPDX-License-Identifier: GPL-2.0-or-later + +#pragma once + +#ifdef __cplusplus +extern "C" { +#endif + +enum custom_keycodes { + // (FAST|SLOW)_xy = tap KC_x on press, tap KC_y on release. For FAST_xy + // process_record_user() returns false to stop processing early; for + // SLOW_xy process_record_user() returns true, therefore all other key + // handlers are invoked. + FAST_AB = SAFE_RANGE, + FAST_CD, + SLOW_AB, + SLOW_CD, +}; + +enum tap_dance_ids { + TD_L_MOVE, // ACTION_TAP_DANCE_LAYER_MOVE(KC_APP, 1) + TD_L_TOGG, // ACTION_TAP_DANCE_LAYER_TOGGLE(KC_APP, 1) + TD_LT_APP, // similar to LT(1, KC_APP) with KC_RCTL on tap+hold or double tap +}; + +#ifdef __cplusplus +} +#endif diff --git a/tests/tap_dance/tap_dance_layers/test.mk b/tests/tap_dance/tap_dance_layers/test.mk new file mode 100644 index 000000000000..b4cdc9b0880e --- /dev/null +++ b/tests/tap_dance/tap_dance_layers/test.mk @@ -0,0 +1,10 @@ +# Copyright 2022 Sergey Vlasov (@sigprof) +# SPDX-License-Identifier: GPL-2.0-or-later + +# -------------------------------------------------------------------------------- +# Keep this file, even if it is empty, as a marker that this folder contains tests +# -------------------------------------------------------------------------------- + +TAP_DANCE_ENABLE = yes + +SRC += tap_dance_defs.c diff --git a/tests/tap_dance/tap_dance_layers/test_tap_dance_layers.cpp b/tests/tap_dance/tap_dance_layers/test_tap_dance_layers.cpp new file mode 100644 index 000000000000..8b736b19c63a --- /dev/null +++ b/tests/tap_dance/tap_dance_layers/test_tap_dance_layers.cpp @@ -0,0 +1,717 @@ +// Copyright 2022 Sergey Vlasov (@sigprof) +// SPDX-License-Identifier: GPL-2.0-or-later + +#include "keyboard_report_util.hpp" +#include "keycode.h" +#include "test_common.hpp" +#include "action_tapping.h" +#include "test_keymap_key.hpp" +#include "tap_dance_defs.h" + +using testing::_; +using testing::InSequence; + +struct TapDanceKeyParams { + std::string name; // Tap dance name (part of test name) + uint16_t keycode; // Tap dance keycode (TD(n)) + uint16_t expect_on_tap; // Keycode for single tap + uint16_t expect_on_hold; // Keycode for single hold (may be MO(1)) + uint16_t expect_on_double_tap; // Keycode for double tap (may be MO(1)) + uint16_t expect_on_double_hold; // Keycode for double hold (may be MO(1)) +}; + +struct OtherKeyLayerParams { + uint16_t keycode; // Keycode in the keymap + uint16_t expect_on_press; // Keycode to expect on press + uint16_t expect_on_release; // Keycode to expect on release (may be KC_NO if none) +}; + +struct OtherKeyParams { + std::string name; // Other key name (part of test name) + OtherKeyLayerParams l0; // Keycodes for layer 0 + OtherKeyLayerParams l1; // Keycodes for layer 1 +}; + +typedef std::tuple TapDanceLayersParams; + +class TapDanceLayers : public ::testing::WithParamInterface, public TestFixture { + protected: + TapDanceKeyParams tap_dance; + OtherKeyParams other_key; + + std::unique_ptr key_td, key_td_l1, key_other, key_other_l1; + + void SetUp() override { + std::tie(tap_dance, other_key) = GetParam(); + + key_td = std::make_unique(0, 1, 0, tap_dance.keycode); + key_td_l1 = std::make_unique(1, 1, 0, KC_TRNS); + key_other = std::make_unique(0, 2, 0, other_key.l0.keycode); + key_other_l1 = std::make_unique(1, 2, 0, other_key.l1.keycode); + + set_keymap({*key_td, *key_td_l1, *key_other, *key_other_l1}); + } +}; + +static const TapDanceKeyParams tap_dance_keys[] = { + TapDanceKeyParams{ + "LayerMove", + TD(TD_L_MOVE), + KC_APP, + KC_APP, + MO(1), + MO(1), + }, + TapDanceKeyParams{ + "LayerToggle", + TD(TD_L_TOGG), + KC_APP, + KC_APP, + MO(1), + MO(1), + }, + TapDanceKeyParams{ + "CustomLT", + TD(TD_LT_APP), + KC_APP, + MO(1), + KC_RCTL, + KC_RCTL, + }, +}; + +static const OtherKeyParams other_keys[] = { + OtherKeyParams{ + "Builtin", + OtherKeyLayerParams{KC_A, KC_A, KC_NO}, + OtherKeyLayerParams{KC_B, KC_B, KC_NO}, + }, + OtherKeyParams{ + "CustomFast", + OtherKeyLayerParams{FAST_AB, KC_A, KC_B}, + OtherKeyLayerParams{FAST_CD, KC_C, KC_D}, + }, + OtherKeyParams{ + "CustomSlow", + OtherKeyLayerParams{SLOW_AB, KC_A, KC_B}, + OtherKeyLayerParams{SLOW_CD, KC_C, KC_D}, + }, +}; + +// clang-format off +INSTANTIATE_TEST_CASE_P( + Layers, + TapDanceLayers, + ::testing::Combine( + ::testing::ValuesIn(tap_dance_keys), + ::testing::ValuesIn(other_keys) + ), + [](const ::testing::TestParamInfo& info) { + return std::get<0>(info.param).name + std::get<1>(info.param).name; + } +); +// clang-format on + +// Test single tap of the tap dance key with tapping term delay after the tap. +TEST_P(TapDanceLayers, SingleTap) { + TestDriver driver; + InSequence s; + + // The tap of the tap dance key does not result in sending a report + // immediately. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + + // After the tapping term expires, a tap event for the single tap keycode + // is generated. + idle_for(TAPPING_TERM - 1); + EXPECT_REPORT(driver, (tap_dance.expect_on_tap)); + EXPECT_EMPTY_REPORT(driver); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 0 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the other key produces the reports for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test single tap of the tap dance key without a delay between the tap dance +// key and the other key. +TEST_P(TapDanceLayers, SingleTapFast) { + TestDriver driver; + InSequence s; + + // The tap of the tap dance key does not result in sending a report + // immediately. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + + // A quick press of the other key causes the tap event for the tap dance to + // be sent before the press event for the other key, and the layer 0 + // mapping is used for the other key. + EXPECT_REPORT(driver, (tap_dance.expect_on_tap)); + EXPECT_EMPTY_REPORT(driver); + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the other key produces the reports for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test single hold of the tap dance key with tapping term delay after the hold +// (test variant for tap dances which switch the layer on hold). +TEST_P(TapDanceLayers, SingleHoldLayer) { + if (tap_dance.expect_on_hold != MO(1)) { + // Do nothing - the SingleHoldKeycode test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the hold of the tap dance key. + EXPECT_NO_REPORT(driver); + key_td->press(); + run_one_scan_loop(); + + // After the tapping term expires, the tap dance finishes and switches the + // layer, but does not send a report. + EXPECT_NO_REPORT(driver); + idle_for(TAPPING_TERM); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 1 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l1.expect_on_press)); + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key does not produce a report. + EXPECT_NO_REPORT(driver); + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 1 mapping of + // that key. + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l1.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test single hold of the tap dance key with tapping term delay after the hold +// (test variant for tap dances which send a keycode on single hold). +TEST_P(TapDanceLayers, SingleHoldKeycode) { + if (tap_dance.expect_on_hold == MO(1)) { + // Do nothing - the SingleHoldLayer test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the hold of the tap dance key. + EXPECT_NO_REPORT(driver); + key_td->press(); + run_one_scan_loop(); + + // After the tapping term expires, the tap dance sends the report with the + // hold keycode. + EXPECT_NO_REPORT(driver); + idle_for(TAPPING_TERM); + EXPECT_REPORT(driver, (tap_dance.expect_on_hold)); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 0 mapping of + // that key. + EXPECT_REPORT(driver, (tap_dance.expect_on_hold, other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (tap_dance.expect_on_hold)); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key sends the release report for the + // corresponding hold keycode. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } else { + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + } + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the reports for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test single hold of the tap dance key without tapping term delay after the +// hold (test variant for tap dances which switch the layer on hold). +TEST_P(TapDanceLayers, SingleHoldFastLayer) { + if (tap_dance.expect_on_hold != MO(1)) { + // Do nothing - the SingleHoldFastKeycode test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the hold of the tap dance key. + EXPECT_NO_REPORT(driver); + key_td->press(); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 1 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l1.expect_on_press)); + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key does not produce a report. + EXPECT_NO_REPORT(driver); + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the reports for the layer 1 mapping of + // that key. + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l1.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test single hold of the tap dance key without tapping term delay after the hold +// (test variant for tap dances which send a keycode on single hold). +TEST_P(TapDanceLayers, SingleHoldFastKeycode) { + if (tap_dance.expect_on_hold == MO(1)) { + // Do nothing - the SingleHoldFastLayer test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the hold of the tap dance key. + EXPECT_NO_REPORT(driver); + key_td->press(); + run_one_scan_loop(); + + // Pressing the other key produces first the report for the tap dance hold + // keycode, and then the reports for the layer 0 mapping of the other key. + EXPECT_REPORT(driver, (tap_dance.expect_on_hold)); + EXPECT_REPORT(driver, (tap_dance.expect_on_hold, other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (tap_dance.expect_on_hold)); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key sends a release report for the corresponding + // hold keycode. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } else { + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + } + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double tap of the tap dance key with tapping term delay after the hold +// (test variant for tap dances which switch the layer on double tap). +TEST_P(TapDanceLayers, DoubleTapLayer) { + if (tap_dance.expect_on_double_tap != MO(1)) { + // Do nothing - the DoubleTapKeycode test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double tap of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + tap_key(*key_td); + + // After the tapping term this tap dance does not send a report too. + EXPECT_NO_REPORT(driver); + idle_for(TAPPING_TERM); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 1 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l1.expect_on_press)); + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 1 mapping of + // that key. + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l1.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double tap of the tap dance key with tapping term delay after the hold +// (test variant for tap dances which send a keycode on double tap). +TEST_P(TapDanceLayers, DoubleTapKeycode) { + if (tap_dance.expect_on_double_tap == MO(1)) { + // Do nothing - the DoubleTapLayer test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double tap of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + tap_key(*key_td); + + // After the tapping term this tap dance sends the double tap keycode. + idle_for(TAPPING_TERM - 1); + EXPECT_REPORT(driver, (tap_dance.expect_on_double_tap)); + EXPECT_EMPTY_REPORT(driver); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 0 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double tap of the tap dance key without tapping term delay after the +// hold (test variant for tap dances which switch the layer on double tap). +TEST_P(TapDanceLayers, DoubleTapFastLayer) { + if (tap_dance.expect_on_double_tap != MO(1)) { + // Do nothing - the DoubleTapFastKeycode test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double tap of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + tap_key(*key_td); + + // Pressing the other key produces the reports for the layer 1 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l1.expect_on_press)); + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 1 mapping of + // that key. + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l1.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double tap of the tap dance key without tapping term delay after the +// hold (test variant for tap dances which send a keycode on double tap). +TEST_P(TapDanceLayers, DoubleTapFastKeycode) { + if (tap_dance.expect_on_double_tap == MO(1)) { + // Do nothing - the DoubleTapFastLayer test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double tap of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + tap_key(*key_td); + + // Pressing the other key produces first the report for the tap dance + // double tap keycode, and then the reports for the layer 0 mapping of the + // other key. + EXPECT_REPORT(driver, (tap_dance.expect_on_double_tap)); + EXPECT_EMPTY_REPORT(driver); + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double hold of the tap dance key with tapping term delay after the hold +// (test variant for tap dances which switch the layer on double hold). +TEST_P(TapDanceLayers, DoubleHoldLayer) { + if (tap_dance.expect_on_double_hold != MO(1)) { + // Do nothing - the DoubleHoldKeycode test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double hold of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + key_td->press(); + run_one_scan_loop(); + + // After the tapping term expires, the tap dance finishes and switches the + // layer, but does not send a report. + EXPECT_NO_REPORT(driver); + idle_for(TAPPING_TERM); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 1 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l1.expect_on_press)); + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key does not produce a report. + EXPECT_NO_REPORT(driver); + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 1 mapping of + // that key. + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l1.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double hold of the tap dance key with tapping term delay after the hold +// (test variant for tap dances which send a keycode on double hold). +TEST_P(TapDanceLayers, DoubleHoldKeycode) { + if (tap_dance.expect_on_double_hold == MO(1)) { + // Do nothing - the DoubleHoldLayer test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double hold of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + key_td->press(); + run_one_scan_loop(); + + // After the tapping term expires, the tap dance sends the report with the + // double hold keycode. + EXPECT_NO_REPORT(driver); + idle_for(TAPPING_TERM); + EXPECT_REPORT(driver, (tap_dance.expect_on_double_hold)); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 0 mapping of + // that key. + EXPECT_REPORT(driver, (tap_dance.expect_on_double_hold, other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (tap_dance.expect_on_double_hold)); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key sends the release report for the + // corresponding double hold keycode. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } else { + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + } + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the reports for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double hold of the tap dance key without tapping term delay after the +// hold (test variant for tap dances which switch the layer on double hold). +TEST_P(TapDanceLayers, DoubleHoldFastLayer) { + if (tap_dance.expect_on_double_hold != MO(1)) { + // Do nothing - the DoubleHoldFastKeycode test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double hold of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + key_td->press(); + run_one_scan_loop(); + + // Pressing the other key produces the reports for the layer 1 mapping of + // that key. + EXPECT_REPORT(driver, (other_key.l1.expect_on_press)); + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key does not produce a report. + EXPECT_NO_REPORT(driver); + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the reports for the layer 1 mapping of + // that key. + if (other_key.l1.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l1.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} + +// Test double hold of the tap dance key without tapping term delay after the hold +// (test variant for tap dances which send a keycode on double hold). +TEST_P(TapDanceLayers, DoubleHoldFastKeycode) { + if (tap_dance.expect_on_double_hold == MO(1)) { + // Do nothing - the DoubleHoldFastLayer test would run instead. + return; + } + + TestDriver driver; + InSequence s; + + // No report gets sent immediately after the double hold of the tap dance + // key. + EXPECT_NO_REPORT(driver); + tap_key(*key_td); + key_td->press(); + run_one_scan_loop(); + + // Pressing the other key produces first the report for the tap dance + // double hold keycode, and then the reports for the layer 0 mapping of the + // other key. + EXPECT_REPORT(driver, (tap_dance.expect_on_double_hold)); + EXPECT_REPORT(driver, (tap_dance.expect_on_double_hold, other_key.l0.expect_on_press)); + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (tap_dance.expect_on_double_hold)); + } + key_other->press(); + run_one_scan_loop(); + + // Releasing the tap dance key sends a release report for the corresponding + // double hold keycode. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_EMPTY_REPORT(driver); + } else { + EXPECT_REPORT(driver, (other_key.l0.expect_on_press)); + } + key_td->release(); + run_one_scan_loop(); + + // Releasing the other key produces the report for the layer 0 mapping of + // that key. + if (other_key.l0.expect_on_release != KC_NO) { + EXPECT_REPORT(driver, (other_key.l0.expect_on_release)); + } + EXPECT_EMPTY_REPORT(driver); + key_other->release(); + run_one_scan_loop(); +} From ca66d8b599d8aca39d019061297f4eb7eb9713a6 Mon Sep 17 00:00:00 2001 From: Sergey Vlasov Date: Sat, 6 Aug 2022 23:41:23 +0300 Subject: [PATCH 2/2] Repeat keymap lookup after finishing a tap dance Pressing a different key after starting a tap dance sequence interrupts the tap dance and invokes the corresponding `on_dance_finished()` function; that function may perform various operations, including changing the layer state. However, at this point in time the keymap lookup in `process_record_quantum()` had already been performed, therefore the layer state changes done by the tap dance function did not actually affect the key pressed immediately after the tap dance. Although in some cases it was possible to work around this issue by using the `on_each_tap()` function of the tap dance, this workaround is cumbersome and in some cases even impossible to implement (e.g., if you want to activate different layers on tap and hold). Moreover, because of another bug in `process_record_handler()` layer switching from tap dances actually seemed to work properly, but only for keycodes handled inside `process_action()` (the lookup inside `process_record_handler()` was overwriting the source layer cache data that was populated by the lookup in `process_record_quantum()` - this actually results in a mismatch of keycodes for press and release events in some cases). Simply fixing the erroneous lookup in `process_record_handler()` would break layer switching from tap dances for existing users who relied on that buggy behavior, therefore the proper fix for layer switching from tap dances needs to be added first. Looks like the simplest way to fix the problem is to add a `bool` return value to `preprocess_tap_dance()` and then repeat the keymap lookup in `process_record_quantum()` if `preprocess_tap_dance()` indicates that some tap dance has been finished. At this point in the code some functions may had used the keycode that ended up incorrect (including `preprocess_tap_dance()` itself, where that keycode may end up in `state->interrupting_keycode` for the current tap dance), but in some cases this does not really matter (e.g., `preprocess_secure()` does not use the keycode at all), and in other cases this may be fixed by reordering the handlers (e.g., `update_wpm()` may be moved after `preprocess_tap_dance()`). --- quantum/process_keycode/process_tap_dance.c | 12 +++++++++--- quantum/process_keycode/process_tap_dance.h | 2 +- quantum/quantum.c | 6 +++++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/quantum/process_keycode/process_tap_dance.c b/quantum/process_keycode/process_tap_dance.c index 3270a1b00086..6e8e596673b4 100644 --- a/quantum/process_keycode/process_tap_dance.c +++ b/quantum/process_keycode/process_tap_dance.c @@ -115,12 +115,12 @@ static inline void process_tap_dance_action_on_dance_finished(qk_tap_dance_actio } } -void preprocess_tap_dance(uint16_t keycode, keyrecord_t *record) { +bool preprocess_tap_dance(uint16_t keycode, keyrecord_t *record) { qk_tap_dance_action_t *action; - if (!record->event.pressed) return; + if (!record->event.pressed) return false; - if (!active_td || keycode == active_td) return; + if (!active_td || keycode == active_td) return false; action = &tap_dance_actions[TD_INDEX(active_td)]; action->state.interrupted = true; @@ -130,6 +130,12 @@ void preprocess_tap_dance(uint16_t keycode, keyrecord_t *record) { // Tap dance actions can leave some weak mods active (e.g., if the tap dance is mapped to a keycode with // modifiers), but these weak mods should not affect the keypress which interrupted the tap dance. clear_weak_mods(); + + // Signal that a tap dance has been finished due to being interrupted, + // therefore the keymap lookup for the currently processed event needs to + // be repeated with the current layer state that might have been updated by + // the finished tap dance. + return true; } bool process_tap_dance(uint16_t keycode, keyrecord_t *record) { diff --git a/quantum/process_keycode/process_tap_dance.h b/quantum/process_keycode/process_tap_dance.h index d97900d96b3c..d6d6c136dc86 100644 --- a/quantum/process_keycode/process_tap_dance.h +++ b/quantum/process_keycode/process_tap_dance.h @@ -81,7 +81,7 @@ void reset_tap_dance(qk_tap_dance_state_t *state); /* To be used internally */ -void preprocess_tap_dance(uint16_t keycode, keyrecord_t *record); +bool preprocess_tap_dance(uint16_t keycode, keyrecord_t *record); bool process_tap_dance(uint16_t keycode, keyrecord_t *record); void tap_dance_task(void); diff --git a/quantum/quantum.c b/quantum/quantum.c index 9a0016b15060..be8fb8a94bc4 100644 --- a/quantum/quantum.c +++ b/quantum/quantum.c @@ -251,7 +251,11 @@ bool process_record_quantum(keyrecord_t *record) { #endif #ifdef TAP_DANCE_ENABLE - preprocess_tap_dance(keycode, record); + if (preprocess_tap_dance(keycode, record)) { + // The tap dance might have updated the layer state, therefore the + // result of the keycode lookup might change. + keycode = get_record_keycode(record, true); + } #endif if (!(