apply https://github.com/qmk/qmk_firmware/pull/2710.diff

callum-oakley · Oct 5, 2018 · b344f99 · b344f99
1 parent fa24ad0
commit b344f99
Show file tree

Hide file tree

Showing 6 changed files with 163 additions and 11 deletions.
diff --git a/tests/basic/keymap.c b/tests/basic/keymap.c
@@ -23,24 +23,24 @@
 
 const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {
     [0] = {
-        // 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_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},
+        // 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_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},
     },
 };
 
 const macro_t *action_get_macro(keyrecord_t *record, uint8_t id, uint8_t opt) {
     if (record->event.pressed) {
         switch(id) {
         case 0:
-            return MACRO(D(LSFT), T(H), U(LSFT), T(E), T(L), T(L), T(O), T(SPACE), W(100), 
+            return MACRO(D(LSFT), T(H), U(LSFT), T(E), T(L), T(L), T(O), T(SPACE), W(100),
             D(LSFT), T(W), U(LSFT), I(10), T(O), T(R), T(L), T(D), D(LSFT), T(1), U(LSFT), END);
         }
     }
     return MACRO_NONE;
 };
 
 void action_function(keyrecord_t *record, uint8_t id, uint8_t opt) {
-}
+}
diff --git a/tests/basic/test_keypress.cpp b/tests/basic/test_keypress.cpp
@@ -18,6 +18,7 @@
 
 using testing::_;
 using testing::Return;
+using testing::InSequence;
 
 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();
-}
+}
+
+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);
+}
diff --git a/tests/test_common/test_fixture.cpp b/tests/test_common/test_fixture.cpp
@@ -34,14 +34,15 @@ TestFixture::TestFixture() {
 
 TestFixture::~TestFixture() {
     TestDriver driver;
-    layer_clear();
-    clear_all_keys();
     // Run for a while to make sure all keys are completely released
     EXPECT_CALL(driver, send_keyboard_mock(_)).Times(AnyNumber());
+    layer_clear();
+    clear_all_keys();
     idle_for(TAPPING_TERM + 10);
     testing::Mock::VerifyAndClearExpectations(&driver);
     // Verify that the matrix really is cleared
-    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(Between(0, 1));
+    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0);
+    idle_for(TAPPING_TERM + 10);
 }
 
 void TestFixture::run_one_scan_loop() {

diff --git a/tmk_core/common/action.c b/tmk_core/common/action.c
@@ -759,6 +759,13 @@ void register_code(uint8_t code)
 */
 #endif
         {
+            // Force a new key press if the key is already pressed
+            // without this, keys with the same keycode, but different
+            // modifiers will be reported incorrectly, see issue #1708
+            if (is_key_pressed(keyboard_report, code)) {
+              del_key(code);
+              send_keyboard_report();
+            }
             add_key(code);
             send_keyboard_report();
         }

diff --git a/tmk_core/common/report.c b/tmk_core/common/report.c
@@ -62,6 +62,32 @@ uint8_t get_first_key(report_keyboard_t* keyboard_report)
 #endif
 }
 
+/** \brief Checks if a key is pressed in the report
+ *
+ * Returns true if the keyboard_report reports that the key is pressed, otherwise false
+ * Note: The function doesn't support modifers currently, and it returns false for KC_NO
+ */
+bool is_key_pressed(report_keyboard_t* keyboard_report, uint8_t key) {
+  if (key == KC_NO) {
+    return false;
+  }
+#ifdef NKRO_ENABLE
+  if (keyboard_protocol && keymap_config.nkro) {
+    if ((key>>3) < KEYBOARD_REPORT_BITS) {
+        return keyboard_report->nkro.bits[key>>3] & 1<<(key&7);
+    } else {
+      return false;
+    }
+  }
+#endif
+  for (int i=0; i < KEYBOARD_REPORT_KEYS; i++) {
+      if (keyboard_report->keys[i] == key) {
+          return true;
+      }
+  }
+  return false;
+}
+
 /** \brief add key byte
  *
  * FIXME: Needs doc

diff --git a/tmk_core/common/report.h b/tmk_core/common/report.h
@@ -19,6 +19,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #define REPORT_H
 
 #include <stdint.h>
+#include <stdbool.h>
 #include "keycode.h"
 
 
@@ -179,6 +180,7 @@ typedef struct {
 
 uint8_t has_anykey(report_keyboard_t* keyboard_report);
 uint8_t get_first_key(report_keyboard_t* keyboard_report);
+bool is_key_pressed(report_keyboard_t* keyboard_report, uint8_t key);
 
 void add_key_byte(report_keyboard_t* keyboard_report, uint8_t code);
 void del_key_byte(report_keyboard_t* keyboard_report, uint8_t code);