From c797a95fb2377c5c2d068befe3f66cda59d63e56 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 25 Aug 2020 12:20:29 +0200 Subject: [PATCH 1/3] TapDance: Do not mask interrupting keys anymore We introduced the masking to avoid sending extra keys when the mapped key changes prior to release - but since the introduction of the caching mechanism, we no longer need to do this. However, for the caching to work the way we want it to, we need to map the key to `NoKey` once, upon interrupting. Signed-off-by: Gergely Nagy squash! TapDance: Do not mask interrupting keys anymore Signed-off-by: Gergely Nagy --- src/kaleidoscope/plugin/TapDance.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/kaleidoscope/plugin/TapDance.cpp b/src/kaleidoscope/plugin/TapDance.cpp index f844859c..3e066e3a 100644 --- a/src/kaleidoscope/plugin/TapDance.cpp +++ b/src/kaleidoscope/plugin/TapDance.cpp @@ -38,7 +38,6 @@ void TapDance::interrupt(KeyAddr key_addr) { last_tap_dance_key_ = Key_NoKey; - Runtime.device().maskKey(key_addr); Runtime.hid().keyboard().sendReport(); Runtime.hid().keyboard().releaseAllKeys(); @@ -114,13 +113,11 @@ EventHandlerResult TapDance::onKeyswitchEvent(Key &mapped_key, KeyAddr key_addr, if (last_tap_dance_key_ == Key_NoKey) return EventHandlerResult::OK; - if (keyToggledOn(keyState)) + if (keyToggledOn(keyState)) { interrupt(key_addr); - - if (Runtime.device().isKeyMasked(key_addr)) { - Runtime.device().unMaskKey(key_addr); - return EventHandlerResult::EVENT_CONSUMED; + mapped_key = Key_NoKey; } + return EventHandlerResult::OK; } From 9dfe51a2388b7bc19eb08dde9bcbfa7a7d283564 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 25 Aug 2020 12:24:02 +0200 Subject: [PATCH 2/3] Escape-OneShot: Change the key mapping instead of masking We want to remove the use of key masking, so instead of masking the key when escaping a OneShot, map it to `NoKey` instead, and continue doing so until released. Which is effectively what masking did, but localized and simpler. Doing this will make our cache have `NoKey` for the key until release, and we'll avoid sending unintended Escape keycodes, without having to use the global masking functions. Signed-off-by: Gergely Nagy --- src/kaleidoscope/plugin/Escape-OneShot.cpp | 12 +++++++----- src/kaleidoscope/plugin/Escape-OneShot.h | 3 +++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/kaleidoscope/plugin/Escape-OneShot.cpp b/src/kaleidoscope/plugin/Escape-OneShot.cpp index 9e3d58c5..47078c40 100644 --- a/src/kaleidoscope/plugin/Escape-OneShot.cpp +++ b/src/kaleidoscope/plugin/Escape-OneShot.cpp @@ -23,18 +23,20 @@ namespace kaleidoscope { namespace plugin { +bool EscapeOneShot::did_escape_; + EventHandlerResult EscapeOneShot::onKeyswitchEvent(Key &mapped_key, KeyAddr key_addr, uint8_t keyState) { - if (mapped_key != Key_Escape || - (keyState & INJECTED) || - !keyToggledOn(keyState)) + if (mapped_key != Key_Escape || (keyState & INJECTED)) return EventHandlerResult::OK; + if (did_escape_) + mapped_key = Key_NoKey; + did_escape_ = !keyToggledOff(keyState); + if ((!::OneShot.isActive() || ::OneShot.isPressed()) && !::OneShot.isSticky()) { return EventHandlerResult::OK; } - Runtime.device().maskKey(key_addr); - ::OneShot.cancel(true); return EventHandlerResult::EVENT_CONSUMED; } diff --git a/src/kaleidoscope/plugin/Escape-OneShot.h b/src/kaleidoscope/plugin/Escape-OneShot.h index b330cb11..027dabf6 100644 --- a/src/kaleidoscope/plugin/Escape-OneShot.h +++ b/src/kaleidoscope/plugin/Escape-OneShot.h @@ -26,6 +26,9 @@ class EscapeOneShot : public kaleidoscope::Plugin { EscapeOneShot(void) {} EventHandlerResult onKeyswitchEvent(Key &mapped_key, KeyAddr key_addr, uint8_t keyState); + + private: + static bool did_escape_; }; } } From 8d4dfa877b611df5a090f628a0f11ffa5e6ab364 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 25 Aug 2020 13:38:46 +0200 Subject: [PATCH 3/3] Deprecate the key masking API Key masking was a bandaid, and we have better ways to achieve the same thing now. All current users have been switched over to different methods now, so lets deprecate the masking. We only put the `DEPRECATED` label on the `maskKey` method, because the rest are used internally too, and we do not want to emit warnings for those. Fixes #884. Signed-off-by: Gergely Nagy --- docs/NEWS.md | 4 ++++ docs/UPGRADING.md | 9 +++++++++ src/kaleidoscope/device/Base.h | 2 +- src/kaleidoscope_internal/deprecations.h | 6 ++++++ 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/NEWS.md b/docs/NEWS.md index d902f54c..858b02a8 100644 --- a/docs/NEWS.md +++ b/docs/NEWS.md @@ -212,6 +212,10 @@ allowing external tools to aid in migrations. The setting wasn't widely used - if at all -, which is why we chose to repurpose it instead of adding a new field. +### Key masking has been deprecated + +Key masking was a band-aid introduced to avoid accidentally sending unintended keys when key mapping changes between a key being pressed and released. Since the introduction of keymap caching, this is no longer necessary, as long as we can keep the mapping consistent. Users of key masking are encouraged to find ways to use the caching mechanism instead. + ## Bugfixes We fixed way too many issues to list here, so we're going to narrow it down to the most important, most visible ones. diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 7977bfc2..70dce9cb 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -19,6 +19,7 @@ If any of this does not make sense to you, or you have trouble updating your .in - [MagicCombo](#magiccombo) - [TypingBreaks](#typingbreaks) - [Redial](#redial) + - [Key mapping has been deprecated](#key-mapping-has-been-deprecated) + [Deprecated APIs and their replacements](#deprecated-apis-and-their-replacements) - [Source code and namespace rearrangement](#source-code-and-namespace-rearrangement) * [Removed APIs](#removed-apis) @@ -491,6 +492,14 @@ Storing the settable settings in EEPROM makes it depend on `Kaleidoscope-EEPROM- Older versions of the plugin required one to set up `Key_Redial` manually, and let the plugin know about it via `Redial.key`. This is no longer required, as the plugin sets up the redial key itself. As such, `Redial.key` was removed, and `Key_Redial` is defined by the plugin itself. To upgrade, simply remove your definition of `Key_Redial` and the `Redial.key` assignment from your sketch. +### Key masking has been deprecated + +Key masking was a band-aid introduced to avoid accidentally sending unintended keys when key mapping changes between a key being pressed and released. Since the introduction of keymap caching, this is no longer necessary, as long as we can keep the mapping consistent. Users of key masking are encouraged to find ways to use the caching mechanism instead. + +As an example, if you had a key event handler that in some cases masked a key, it should now map it to `Key_NoKey` instead, until released. + +The masking API has been deprecated, and is scheduled to be removed after **2020-11-25**. + ## Deprecated APIs and their replacements ### Source code and namespace rearrangement diff --git a/src/kaleidoscope/device/Base.h b/src/kaleidoscope/device/Base.h index 3d8fe49c..846a026e 100644 --- a/src/kaleidoscope/device/Base.h +++ b/src/kaleidoscope/device/Base.h @@ -269,7 +269,7 @@ class Base { * * @param key_addr is the matrix address of the key. */ - void maskKey(KeyAddr key_addr) { + void maskKey(KeyAddr key_addr) DEPRECATED(KEY_MASKING) { key_scanner_.maskKey(key_addr); } /** diff --git a/src/kaleidoscope_internal/deprecations.h b/src/kaleidoscope_internal/deprecations.h index 7fd85174..9279c892 100644 --- a/src/kaleidoscope_internal/deprecations.h +++ b/src/kaleidoscope_internal/deprecations.h @@ -27,6 +27,12 @@ /* Messages */ +#define _DEPRECATED_MESSAGE_KEY_MASKING __NL__ \ + "Key masking has been deprecated, please map keys to NoKey instead.\n" __NL__ \ + "\n" __NL__ \ + "For further information and examples on how to do that, \n" __NL__ \ + "please see UPGRADING.md" + #define _DEPRECATED_MESSAGE_HID_FACADE __NL__ \ "The HID facade in the `kaleidoscope::hid` namespace is deprecated.\n" __NL__ \ "Please use `Kaleidoscope.hid()` instead."