From 8a332e6f0105d2db9239e3c3f997bae754522804 Mon Sep 17 00:00:00 2001 From: Pete Sevander Date: Wed, 10 May 2023 18:59:52 +0300 Subject: [PATCH] Fix Mod-Tap combo regression (#20669) * Add keyevent for combo keyrecord * Fix formatting * Update quantum/process_keycode/process_combo.c Co-authored-by: Sergey Vlasov * Add combo unit-tests and hot-fix process_record_tap_hint ...as this function tries to lookup the combo keys passed in. This will be refactored in a later pr. --------- Co-authored-by: Sergey Vlasov Co-authored-by: Stefan Kerkmann --- quantum/action.c | 8 ++- quantum/action_tapping.c | 10 ++-- quantum/process_keycode/process_combo.c | 1 + tests/combo/config.h | 8 +++ tests/combo/test.mk | 4 ++ tests/combo/test_combo.cpp | 72 +++++++++++++++++++++++++ 6 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 tests/combo/config.h create mode 100644 tests/combo/test.mk create mode 100644 tests/combo/test_combo.cpp diff --git a/quantum/action.c b/quantum/action.c index 9a6bbcca11..41c204f689 100644 --- a/quantum/action.c +++ b/quantum/action.c @@ -187,7 +187,7 @@ bool is_swap_hands_on(void) { */ void process_hand_swap(keyevent_t *event) { keypos_t pos = event->key; - if (pos.row < MATRIX_ROWS && pos.col < MATRIX_COLS) { + if (IS_KEYEVENT(*event) && pos.row < MATRIX_ROWS && pos.col < MATRIX_COLS) { static uint8_t matrix_swap_state[((MATRIX_ROWS * MATRIX_COLS) + (CHAR_BIT)-1) / (CHAR_BIT)]; size_t index = (size_t)(pos.row * MATRIX_COLS) + pos.col; bool do_swap = should_swap_hands(index, matrix_swap_state, event->pressed); @@ -200,7 +200,7 @@ void process_hand_swap(keyevent_t *event) { } } # ifdef ENCODER_MAP_ENABLE - else if (pos.row == KEYLOC_ENCODER_CW || pos.row == KEYLOC_ENCODER_CCW) { + else if (IS_ENCODEREVENT(*event) && pos.row == KEYLOC_ENCODER_CW || pos.row == KEYLOC_ENCODER_CCW) { static uint8_t encoder_swap_state[((NUM_ENCODERS) + (CHAR_BIT)-1) / (CHAR_BIT)]; size_t index = pos.col; bool do_swap = should_swap_hands(index, encoder_swap_state, event->pressed); @@ -242,6 +242,10 @@ __attribute__((weak)) void post_process_record_quantum(keyrecord_t *record) {} * FIXME: Needs documentation. */ void process_record_tap_hint(keyrecord_t *record) { + if (!IS_KEYEVENT(record->event)) { + return; + } + action_t action = layer_switch_get_action(record->event.key); switch (action.kind.id) { diff --git a/quantum/action_tapping.c b/quantum/action_tapping.c index 362b15105c..f94e5e6f69 100644 --- a/quantum/action_tapping.c +++ b/quantum/action_tapping.c @@ -167,8 +167,10 @@ bool process_tapping(keyrecord_t *keyp) { // state machine is in the "reset" state, no tapping key is to be // processed - if (IS_NOEVENT(tapping_key.event) && IS_EVENT(event)) { - if (event.pressed && is_tap_record(keyp)) { + if (IS_NOEVENT(tapping_key.event)) { + if (!IS_EVENT(event)) { + // early return for tick events + } else if (event.pressed && is_tap_record(keyp)) { // the currently pressed key is a tapping key, therefore transition // into the "pressed" tapping key state ac_dprintf("Tapping: Start(Press tap key).\n"); @@ -176,13 +178,13 @@ bool process_tapping(keyrecord_t *keyp) { process_record_tap_hint(&tapping_key); waiting_buffer_scan_tap(); debug_tapping_key(); - return true; } else { // the current key is just a regular key, pass it on for regular // processing process_record(keyp); - return true; } + + return true; } TAP_DEFINE_KEYCODE; diff --git a/quantum/process_keycode/process_combo.c b/quantum/process_keycode/process_combo.c index b1b49d3019..2670ccabed 100644 --- a/quantum/process_keycode/process_combo.c +++ b/quantum/process_keycode/process_combo.c @@ -335,6 +335,7 @@ void apply_combo(uint16_t combo_index, combo_t *combo) { // this in the end executes the combo when the key_buffer is dumped. record->keycode = combo->keycode; record->event.type = COMBO_EVENT; + record->event.key = MAKE_KEYPOS(0, 0); qrecord->combo_index = combo_index; ACTIVATE_COMBO(combo); diff --git a/tests/combo/config.h b/tests/combo/config.h new file mode 100644 index 0000000000..8052932634 --- /dev/null +++ b/tests/combo/config.h @@ -0,0 +1,8 @@ +// Copyright 2023 Stefan Kerkmann (@KarlK90) +// SPDX-License-Identifier: GPL-2.0-or-later + +#pragma once + +#include "test_common.h" + +#define TAPPING_TERM 200 diff --git a/tests/combo/test.mk b/tests/combo/test.mk new file mode 100644 index 0000000000..ce6f9fc2b0 --- /dev/null +++ b/tests/combo/test.mk @@ -0,0 +1,4 @@ +# Copyright 2023 Stefan Kerkmann (@KarlK90) +# SPDX-License-Identifier: GPL-2.0-or-later + +COMBO_ENABLE = yes diff --git a/tests/combo/test_combo.cpp b/tests/combo/test_combo.cpp new file mode 100644 index 0000000000..b7aea27f4c --- /dev/null +++ b/tests/combo/test_combo.cpp @@ -0,0 +1,72 @@ +// Copyright 2023 Stefan Kerkmann (@KarlK90) +// Copyright 2023 @filterpaper +// SPDX-License-Identifier: GPL-2.0-or-later + +#include "keyboard_report_util.hpp" +#include "quantum.h" +#include "keycode.h" +#include "test_common.h" +#include "test_driver.hpp" +#include "test_fixture.hpp" +#include "test_keymap_key.hpp" + +extern "C" { +enum combos { modtest, osmshift, COMBO_LENGTH }; +uint16_t COMBO_LEN = COMBO_LENGTH; + +uint16_t const modtest_combo[] = {KC_Y, KC_U, COMBO_END}; +uint16_t const osmshift_combo[] = {KC_Z, KC_X, COMBO_END}; + +// clang-format off +combo_t key_combos[] = { + [modtest] = COMBO(modtest_combo, RSFT_T(KC_SPACE)), + [osmshift] = COMBO(osmshift_combo, OSM(MOD_LSFT)) +}; +// clang-format on +} + +using testing::_; +using testing::InSequence; + +class Combo : public TestFixture {}; + +TEST_F(Combo, combo_modtest_tapped) { + TestDriver driver; + KeymapKey key_y(0, 0, 1, KC_Y); + KeymapKey key_u(0, 0, 2, KC_U); + set_keymap({key_y, key_u}); + + EXPECT_REPORT(driver, (KC_SPACE)); + EXPECT_EMPTY_REPORT(driver); + tap_combo({key_y, key_u}); + VERIFY_AND_CLEAR(driver); +} + +TEST_F(Combo, combo_modtest_held_longer_than_tapping_term) { + TestDriver driver; + KeymapKey key_y(0, 0, 1, KC_Y); + KeymapKey key_u(0, 0, 2, KC_U); + set_keymap({key_y, key_u}); + + EXPECT_REPORT(driver, (KC_RIGHT_SHIFT)); + EXPECT_EMPTY_REPORT(driver); + tap_combo({key_y, key_u}, TAPPING_TERM + 1); + VERIFY_AND_CLEAR(driver); +} + +TEST_F(Combo, combo_osmshift_tapped) { + TestDriver driver; + KeymapKey key_z(0, 0, 1, KC_Z); + KeymapKey key_x(0, 0, 2, KC_X); + KeymapKey key_i(0, 0, 3, KC_I); + set_keymap({key_z, key_x, key_i}); + + EXPECT_NO_REPORT(driver); + tap_combo({key_z, key_x}); + VERIFY_AND_CLEAR(driver); + + EXPECT_REPORT(driver, (KC_I, KC_LEFT_SHIFT)); + EXPECT_EMPTY_REPORT(driver); + tap_key(key_i); + VERIFY_AND_CLEAR(driver); +}