Fix pressing two keys with the same keycode but different modifiers (#2710)

* Fix extra keyboard report during test_fixture teardown

* Add tests for pressing two keys with only different modifers

* Fix #1708

When two keys that use the same keycode, but different modifiers were
pressed at the same time, the second keypress wasn't registered. This is
fixed by forcing a key release when we detect a new press for the same
keycode.

* Fix the NKRO version of is_key_pressed

* Fix uninitalized loop variable

Co-authored-by: Jack Humbert <jack.humb@gmail.com>

2 files changed, 118 insertions, 2 deletions

diff --git a/tests/basic/keymap.c b/tests/basic/keymap.c
index 02b3cc961a..2b5747abb7 100644
--- a/tests/basic/keymap.c
+++ b/tests/basic/keymap.c
@@ -28,7 +28,7 @@ const uint16_t PROGMEM
                 {
                     // 0    1      2      3        4        5        6       7            8      9
                     {KC_A, KC_B, KC_NO, KC_LSFT, KC_RSFT, KC_LCTL, COMBO1, SFT_T(KC_P), M(0), KC_NO},
-                    {KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
+                    {KC_EQL, KC_PLUS, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
                     {KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
                     {KC_C, KC_D, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
                 },
diff --git a/tests/basic/test_keypress.cpp b/tests/basic/test_keypress.cpp
index e5247911cb..ecd6e98e44 100644
--- a/tests/basic/test_keypress.cpp
+++ b/tests/basic/test_keypress.cpp
@@ -17,6 +17,7 @@
 #include "test_common.hpp"
 
 using testing::_;
+using testing::InSequence;
 using testing::Return;
 
 class KeyPress : public TestFixture {};
@@ -121,4 +122,119 @@ TEST_F(KeyPress, RightShiftLeftControlAndCharWithTheSameKey) {
     EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_RSFT, KC_RCTRL)));
     EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
     keyboard_task();
-}
\ No newline at end of file
+}
+
+TEST_F(KeyPress, PressPlusEqualReleaseBeforePress) {
+    TestDriver driver;
+    InSequence s;
+
+    press_key(1, 1);  // KC_PLUS
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(1, 1);  // KC_PLUS
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    press_key(0, 1);  // KC_EQL
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(0, 1);  // KC_EQL
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+}
+
+TEST_F(KeyPress, PressPlusEqualDontReleaseBeforePress) {
+    TestDriver driver;
+    InSequence s;
+
+    press_key(1, 1);  // KC_PLUS
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    press_key(0, 1);  // KC_EQL
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(1, 1);  // KC_PLS
+    // BUG: Should really still return KC_EQL, but this is fine too
+    // It's also called twice for some reason
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(2);
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(0, 1);  // KC_EQL
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+}
+
+TEST_F(KeyPress, PressEqualPlusReleaseBeforePress) {
+    TestDriver driver;
+    InSequence s;
+
+    press_key(0, 1);  // KC_EQL
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(0, 1);  // KQ_EQL
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    press_key(1, 1);  // KC_PLUS
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(1, 1);  // KC_PLUS
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+}
+
+TEST_F(KeyPress, PressEqualPlusDontReleaseBeforePress) {
+    TestDriver driver;
+    InSequence s;
+
+    press_key(0, 1);  // KC_EQL
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    press_key(1, 1);  // KC_PLUS
+    // BUG: The sequence is a bit strange, but it works, the end result is that
+    // KC_PLUS is sent
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(0, 1);  // KC_EQL
+    // I guess it's fine to still report shift here
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+
+    release_key(1, 1);  // KC_PLUS
+    // This report is not needed
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
+    run_one_scan_loop();
+    testing::Mock::VerifyAndClearExpectations(&driver);
+}