From ab3b661cd57c7c906f68dceb4d5beffba7bac5dc Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Mon, 29 Jun 2020 12:21:08 +0200 Subject: [PATCH 1/5] Switch to activation-order for Layers Previously, we used index-ordering for layers, meaning, we looked keys up based on the index of active layers. This turned out to be confusing, and in many cases, limiting, since we couldn't easily shift to a lower layer from a higher one. As such, index-ordering required careful planning of one's layers, and a deeper understanding of the system. This patch switches us to activation-ordering: the layer subsystem now keeps track of the order in which layers are activated, and uses that order to look keys up, instead of the index of layers. This makes it easier to understand how the system works, and allows us to shift to lower layers too. It does require a bit more resources, since we can't just store a bitmap of active layers, but need 32 bytes to store the order. We still keep the bitmap, to make `Layer.isActive()` fast: looking up a bit in the bitmap is more efficient than walking the active layer array, and this function is often used in cases where speed matters. As a side effect of the switch, a number of methods were deprecated, and similar ones with more appropriate names were introduced. See the updated `UPGRADING.md` document for more details. Plugins that used the deprecated methods were updated to use the new ones. Fixes #857. Signed-off-by: Gergely Nagy --- docs/UPGRADING.md | 21 ++++ docs/layers.md | 54 +++++---- examples/Features/Layers/Layers.ino | 104 ++++++++++++++++++ src/kaleidoscope/layers.cpp | 73 ++++++------ src/kaleidoscope/layers.h | 31 ++++-- src/kaleidoscope/plugin/Colormap.cpp | 2 +- .../plugin/EEPROM-Keymap-Programmer.cpp | 8 +- .../plugin/LED-ActiveLayerColor.cpp | 2 +- src/kaleidoscope_internal/deprecations.h | 13 +++ 9 files changed, 226 insertions(+), 82 deletions(-) create mode 100644 examples/Features/Layers/Layers.ino diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 70dce9cb..16d83adc 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -12,6 +12,7 @@ If any of this does not make sense to you, or you have trouble updating your .in - [Bidirectional communication for plugins](#bidirectional-communication-for-plugins) - [Consistent timing](#consistent-timing) + [Breaking changes](#breaking-changes) + - [Layer system switched to activation-order](#layer-system-switched-to-activation-order) - [Deprecation of the HID facade](#deprecation-of-the-hid-facade) - [Implementation of type Key internally changed from C++ union to class](#implementation-of-type-key-internally-changed-from-union-to-class) - [The `RxCy` macros and peeking into the keyswitch state](#the-rxcy-macros-and-peeking-into-the-keyswitch-state) @@ -314,6 +315,26 @@ As a developer, one can continue using `millis()`, but migrating to `Kaleidoscop ## Breaking changes +### Layer system switched to activation order + +The layer system used to be index-ordered, meaning that we'd look keys up on +layers based on the _index_ of active layers. Kaleidoscope now uses activation +order, which looks up keys based on the order of layer activation. + +This means that the following functions are deprecated, and will be removed by **2020-09-16**: + +- `Layer.top()`, which used to return the topmost layer index. Use + `Layer.mostRecent()` instead, which returns the most recently activated layer. + Until removed, the old function will return the most recent layer. +- `Layer.deactivateTop()`, which used to return the topmost layer index. Use + `Layer.deactivateMostRecent()` instead. The old function will deactivate the + most recent layer. +- `Layer.getLayerState()`, which used to return a bitmap of the active layers. + With activation-order, a simple bitmap is not enough. For now, we still return + the bitmap, but without the ordering, it is almost useless. Use + `Layer.forEachActiveLayer()` to walk the active layers in order (from least + recent to most). + ### Deprecation of the HID facade With the new Device APIs it became possible to replace the HID facade (the `kaleidoscope::hid` family of functions) with a driver. As such, the old APIs are deprecated, and will be removed by **2020-09-16**. Please use `Kaleidoscope.hid()` instead. diff --git a/docs/layers.md b/docs/layers.md index e724275f..3e1c4d9e 100644 --- a/docs/layers.md +++ b/docs/layers.md @@ -21,6 +21,10 @@ will be active, without any stacking. If you want the layer switch to be active only while the key is held, like in the case of modifiers, the `ShiftToLayer(n)` method does just that. +While switching layers this way is similar to how modifiers work, there are +subtle differences. For a longer explanation, see +[later](#layers-transparency-and-how-lookup-works). + ## Layer theory First of all, the most important thing to remember is that layers are like a @@ -111,36 +115,28 @@ They're like overrides. Any layer you place on top of the existing stack, will override keys in the layers below. When you have multiple layers active, to figure out what a key does, the -firmware will first look at the key position on the topmost active layer, and -see if there's a non-transparent key there. If there is, it will use that. If -there isn't, it will start walking backwards on the stack of _active_ layers to -find the highest one with a non-transparent key. The first one it finds is whose -key it will use. If it finds none, then a transparent key will act like a blank -one, and do nothing. +firmware will first look at the key position on the most recently activated +layer, and see if there's a non-transparent key there. If there is, it will use +that. If there isn't, it will start walking backwards on the stack of _active_ +layers to find the highest one with a non-transparent key. The first one it +finds is whose key it will use. If it finds none, then a transparent key will +act like a blank one, and do nothing. It is important to note that transparent keys are looked up from active layers -only, from highest to lowest. Lets consider that we have three layers, 0, 1, -and 2. On a given position, we have a non-transparent key on layers 0 and 1, but -the same position is transparent on layer 2. If we have layer 0 and 2 active, -the key will be looked up from layer 0, because layer 2 is transparent. If we -activate layer 1 too, it will be looked up from there, since layer 1 is higher -in the stack than layer 0. +only, from most recently activated to least. Lets consider that we have three +layers, 0, 1, and 2. On a given position, we have a non-transparent key on +layers 0 and 1, but the same position is transparent on layer 2. If we have +layer 0 and 2 active, the key will be looked up from layer 0, because layer 2 is +transparent. If we activate layer 1 too, it will be looked up from there, since +layer 1 is higher in the stack than layer 0. In this case, since we activated +layer 1 most recently, layer 2 wouldn't even be looked at. As we just saw, another important factor is that layers are ordered by their -index, not by the order of activation. Whether you activate layer 1 or 2 first -doesn't matter. What matters is their index. In other words, when you have layer -0 and 2 active, then activate layer 1, that is like placing the layer 1 foil -between 0 and 2. - -A side-effect of this is that while you can activate a lower-indexed layer from -a higher index, the higher indexed one will still override the lower-indexed -one. As such, as a general rule of thumb, you want to order your layers in such -a way that this doesn't become a problem. - -This can be a little confusing at first, but it is easy to get used to with some -practice. Once mastered, layers are an incredibly powerful feature. - -Nevertheless, we have the `MoveToLayer(n)` method, where only one layer is ever -active. This way, we do not need to care about transparency, and we can freely -move from a higher layer to a lower one, because the higher one will get -disabled, and will not be able to shadow the lower-indexed one. +order of activation. Whether you activate layer 1 or 2 first, matters. Lets look +at another example: we have three layers, 0, 1, and 2. On a given position, we +have a non-transparent key on every layer. If we have just layer 0 active, it +will be looked up from there. If we activate layer 2, then the firmware will +look there first. If we activate layer 1 as well, then - since now layer 1 is +the most recently activated layer - the firmware will look the code up from +layer 1, without looking at layer 2. It would only look at layer 2 if the key +was transparent on layer 1. diff --git a/examples/Features/Layers/Layers.ino b/examples/Features/Layers/Layers.ino new file mode 100644 index 00000000..a1f749c4 --- /dev/null +++ b/examples/Features/Layers/Layers.ino @@ -0,0 +1,104 @@ +/* -*- mode: c++ -*- + * Kaleidoscope - Firmware for computer input devices + * Copyright (C) 2020 Keyboard.io, Inc. + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +#include +#include +#include + +enum { PRIMARY, NUMPAD, FUNCTION }; // layers + +// *INDENT-OFF* +KEYMAPS( + [PRIMARY] = KEYMAP_STACKED + (___, Key_1, Key_2, Key_3, Key_4, Key_5, XXX, + Key_Backtick, Key_Q, Key_W, Key_E, Key_R, Key_T, Key_Tab, + Key_PageUp, Key_A, Key_S, Key_D, Key_F, Key_G, + Key_PageDown, Key_Z, Key_X, Key_C, Key_V, Key_B, Key_Escape, + Key_LeftControl, Key_Backspace, Key_LeftGui, Key_LeftShift, + ShiftToLayer(FUNCTION), + + XXX, Key_6, Key_7, Key_8, Key_9, Key_0, LockLayer(NUMPAD), + Key_Enter, Key_Y, Key_U, Key_I, Key_O, Key_P, Key_Equals, + Key_H, Key_J, Key_K, Key_L, Key_Semicolon, Key_Quote, + Key_RightAlt, Key_N, Key_M, Key_Comma, Key_Period, Key_Slash, Key_Minus, + Key_RightShift, Key_LeftAlt, Key_Spacebar, Key_RightControl, + ShiftToLayer(FUNCTION)), + + [NUMPAD] = KEYMAP_STACKED + (___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, ___, ___, ___, + ___, ___, ___, ___, + ___, + + XXX, ___, Key_7, Key_8, Key_9, Key_KeypadSubtract, ___, + ___, ___, Key_4, Key_5, Key_6, Key_KeypadAdd, ___, + ___, Key_1, Key_2, Key_3, Key_Equals, ___, + ___, ___, Key_0, Key_Period, Key_KeypadMultiply, Key_KeypadDivide, Key_Enter, + ___, ___, ___, ___, + ___), + + [FUNCTION] = KEYMAP_STACKED + (ShiftToLayer(NUMPAD), Key_F1, Key_F2, Key_F3, Key_F4, Key_F5, Key_CapsLock, + Key_Tab, ___, Key_mouseUp, ___, Key_mouseBtnR, Key_mouseWarpEnd, Key_mouseWarpNE, + Key_Home, Key_mouseL, Key_mouseDn, Key_mouseR, Key_mouseBtnL, Key_mouseWarpNW, + Key_End, Key_PrintScreen, Key_Insert, ___, Key_mouseBtnM, Key_mouseWarpSW, Key_mouseWarpSE, + ___, Key_Delete, ___, ___, + ___, + + Consumer_ScanPreviousTrack, Key_F6, Key_F7, Key_F8, Key_F9, Key_F10, Key_F11, + Consumer_PlaySlashPause, Consumer_ScanNextTrack, Key_LeftCurlyBracket, Key_RightCurlyBracket, Key_LeftBracket, Key_RightBracket, Key_F12, + Key_LeftArrow, Key_DownArrow, Key_UpArrow, Key_RightArrow, ___, ___, + Key_PcApplication, Consumer_Mute, Consumer_VolumeDecrement, Consumer_VolumeIncrement, ___, Key_Backslash, Key_Pipe, + ___, ___, Key_Enter, ___, + ___) +) +// *INDENT-OFF* + +namespace kaleidoscope { +class LayerDumper: public Plugin { + public: + LayerDumper() {} + + static void dumpLayerState(uint8_t index, uint8_t layer) { + Serial.print(index); + Serial.print(" -> "); + Serial.println(layer); + } + + EventHandlerResult onLayerChange() { + Serial.println("Active Layers:"); + Layer.forEachActiveLayer(&dumpLayerState); + Serial.println(); + return EventHandlerResult::OK; + } +}; + +} + +kaleidoscope::LayerDumper LayerDumper; + +KALEIDOSCOPE_INIT_PLUGINS(Focus, LayerDumper, MouseKeys); + +void setup() { + Kaleidoscope.setup(); +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/src/kaleidoscope/layers.cpp b/src/kaleidoscope/layers.cpp index a3442b03..af283773 100644 --- a/src/kaleidoscope/layers.cpp +++ b/src/kaleidoscope/layers.cpp @@ -39,9 +39,11 @@ extern constexpr Key keymaps_linear[][kaleidoscope_internal::device.matrix_rows namespace kaleidoscope { uint32_t Layer_::layer_state_; -uint8_t Layer_::top_active_layer_; +uint8_t Layer_::active_layer_count_ = 1; +int8_t Layer_::active_layers_[31]; + Key Layer_::live_composite_keymap_[Runtime.device().numKeys()]; -uint8_t Layer_::active_layers_[Runtime.device().numKeys()]; +uint8_t Layer_::active_layer_keymap_[Runtime.device().numKeys()]; Layer_::GetKeyFunction Layer_::getKey = &Layer_::getKeyFromPROGMEM; void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { @@ -57,12 +59,12 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { if (keyToggledOn(keyState)) activateNext(); else if (keyToggledOff(keyState)) - deactivateTop(); + deactivateMostRecent(); break; case KEYMAP_PREVIOUS: if (keyToggledOn(keyState)) - deactivateTop(); + deactivateMostRecent(); else if (keyToggledOff(keyState)) activateNext(); break; @@ -112,43 +114,29 @@ Key Layer_::getKeyFromPROGMEM(uint8_t layer, KeyAddr key_addr) { } void Layer_::updateLiveCompositeKeymap(KeyAddr key_addr) { - int8_t layer = active_layers_[key_addr.toInt()]; + int8_t layer = active_layer_keymap_[key_addr.toInt()]; live_composite_keymap_[key_addr.toInt()] = (*getKey)(layer, key_addr); } void Layer_::updateActiveLayers(void) { - memset(active_layers_, 0, Runtime.device().numKeys()); + memset(active_layer_keymap_, 0, Runtime.device().numKeys()); for (auto key_addr : KeyAddr::all()) { - int8_t layer = top_active_layer_; - - while (layer > 0) { + int8_t layer_index = active_layer_count_; + while (layer_index > 0) { + uint8_t layer = active_layers_[layer_index - 1]; if (Layer.isActive(layer)) { Key mappedKey = (*getKey)(layer, key_addr); if (mappedKey != Key_Transparent) { - active_layers_[key_addr.toInt()] = layer; + active_layer_keymap_[key_addr.toInt()] = layer; break; } } - layer--; + layer_index--; } } } -void Layer_::updateTopActiveLayer(void) { - // If layer_count is set, start there, otherwise search from the - // highest possible layer (MAX_LAYERS) for the top active layer - for (byte i = (layer_count - 1); i > 0; i--) { - if (bitRead(layer_state_, i)) { - top_active_layer_ = i; - return; - } - } - // It's not possible to turn off the default layer (see - // updateActiveLayers()), so if no other layers are active: - top_active_layer_ = 0; -} - void Layer_::move(uint8_t layer) { // We do pretty much what activate() does, except we do everything // unconditionally, to make sure all parts of the firmware are aware of the @@ -159,8 +147,9 @@ void Layer_::move(uint8_t layer) { layer = 0; } bitSet(layer_state_, layer); + active_layer_count_ = 1; + active_layers_[0] = layer; - updateTopActiveLayer(); updateActiveLayers(); kaleidoscope::Hooks::onLayerChange(); @@ -179,11 +168,7 @@ void Layer_::activate(uint8_t layer) { // Otherwise, turn on its bit in layer_state_ bitSet(layer_state_, layer); - - // If the target layer is above the previous highest active layer, - // update top_active_layer_ - if (layer > top_active_layer_) - updateTopActiveLayer(); + active_layers_[active_layer_count_++] = layer; // Update the keymap cache (but not live_composite_keymap_; that gets // updated separately, when keys toggle on or off. See layers.h) @@ -201,10 +186,16 @@ void Layer_::deactivate(uint8_t layer) { // Turn off its bit in layer_state_ bitClear(layer_state_, layer); - // If the target layer was the previous highest active layer, - // update top_active_layer_ - if (layer == top_active_layer_) - updateTopActiveLayer(); + // Rearrange the activation order array... + uint8_t idx = 0; + for (uint8_t i = active_layer_count_; i > 0; i--) { + if (active_layers_[i] == layer) { + idx = i; + break; + } + } + memmove(&active_layers_[idx], &active_layers_[idx + 1], active_layer_count_ - idx); + active_layer_count_--; // Update the keymap cache (but not live_composite_keymap_; that gets // updated separately, when keys toggle on or off. See layers.h) @@ -218,11 +209,17 @@ boolean Layer_::isActive(uint8_t layer) { } void Layer_::activateNext(void) { - activate(top_active_layer_ + 1); + activate(active_layers_[active_layer_count_ - 1] + 1); } -void Layer_::deactivateTop(void) { - deactivate(top_active_layer_); +void Layer_::deactivateMostRecent(void) { + deactivate(active_layers_[active_layer_count_ - 1]); +} + +void Layer_::forEachActiveLayer(forEachHandler h) { + for (uint8_t i = 0; i < active_layer_count_; i++) { + (*h)(i, active_layers_[i]); + } } } diff --git a/src/kaleidoscope/layers.h b/src/kaleidoscope/layers.h index 61b08ea6..22ceb9cb 100644 --- a/src/kaleidoscope/layers.h +++ b/src/kaleidoscope/layers.h @@ -23,6 +23,7 @@ #include "kaleidoscope_internal/device.h" #include "kaleidoscope_internal/sketch_exploration/sketch_exploration.h" #include "kaleidoscope_internal/shortname.h" +#include "kaleidoscope_internal/deprecations.h" #define START_KEYMAPS __NL__ \ constexpr Key keymaps_linear[][kaleidoscope_internal::device.matrix_rows * kaleidoscope_internal::device.matrix_columns] PROGMEM = { @@ -83,25 +84,31 @@ class Layer_ { return live_composite_keymap_[key_addr.toInt()]; } static Key lookupOnActiveLayer(KeyAddr key_addr) { - uint8_t layer = active_layers_[key_addr.toInt()]; + uint8_t layer = active_layer_keymap_[key_addr.toInt()]; return (*getKey)(layer, key_addr); } static uint8_t lookupActiveLayer(KeyAddr key_addr) { - return active_layers_[key_addr.toInt()]; + return active_layer_keymap_[key_addr.toInt()]; } static void activate(uint8_t layer); static void deactivate(uint8_t layer); static void activateNext(); - static void deactivateTop(); + static void deactivateTop() DEPRECATED(LAYER_DEACTIVATETOP) { + deactivateMostRecent(); + } + static void deactivateMostRecent(); static void move(uint8_t layer); - static uint8_t top(void) { - return top_active_layer_; + static uint8_t top(void) DEPRECATED(LAYER_TOP) { + return mostRecent(); + } + static uint8_t mostRecent() { + return active_layers_[active_layer_count_ - 1]; } static boolean isActive(uint8_t layer); - static uint32_t getLayerState(void) { + static uint32_t getLayerState(void) DEPRECATED(LAYER_GETLAYERSTATE) { return layer_state_; } @@ -118,14 +125,20 @@ class Layer_ { static void updateLiveCompositeKeymap(KeyAddr key_addr); static void updateActiveLayers(void); + private: + using forEachHandler = void(*)(uint8_t index, uint8_t layer); + + public: + static void forEachActiveLayer(forEachHandler h); + private: static uint32_t layer_state_; - static uint8_t top_active_layer_; + static uint8_t active_layer_count_; + static int8_t active_layers_[31]; static Key live_composite_keymap_[kaleidoscope_internal::device.numKeys()]; - static uint8_t active_layers_[kaleidoscope_internal::device.numKeys()]; + static uint8_t active_layer_keymap_[kaleidoscope_internal::device.numKeys()]; static void handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState); - static void updateTopActiveLayer(void); }; } diff --git a/src/kaleidoscope/plugin/Colormap.cpp b/src/kaleidoscope/plugin/Colormap.cpp index b3e87ab3..157a0d02 100644 --- a/src/kaleidoscope/plugin/Colormap.cpp +++ b/src/kaleidoscope/plugin/Colormap.cpp @@ -40,7 +40,7 @@ void ColormapEffect::TransientLEDMode::onActivate(void) { if (!Runtime.has_leds) return; - parent_->top_layer_ = Layer.top(); + parent_->top_layer_ = Layer.mostRecent(); if (parent_->top_layer_ <= parent_->max_layers_) ::LEDPaletteTheme.updateHandler(parent_->map_base_, parent_->top_layer_); } diff --git a/src/kaleidoscope/plugin/EEPROM-Keymap-Programmer.cpp b/src/kaleidoscope/plugin/EEPROM-Keymap-Programmer.cpp index 3749a239..07fa56e6 100644 --- a/src/kaleidoscope/plugin/EEPROM-Keymap-Programmer.cpp +++ b/src/kaleidoscope/plugin/EEPROM-Keymap-Programmer.cpp @@ -59,10 +59,10 @@ EventHandlerResult EEPROMKeymapProgrammer::onKeyswitchEvent(Key &mapped_key, Key if (state_ == WAIT_FOR_KEY) { if (keyToggledOn(key_state)) { - update_position_ = Layer.top() * Runtime.device().numKeys() + key_addr.toInt(); + update_position_ = Layer.mostRecent() * Runtime.device().numKeys() + key_addr.toInt(); } if (keyToggledOff(key_state)) { - if ((uint16_t)(Layer.top() * Runtime.device().numKeys() + key_addr.toInt()) == update_position_) + if ((uint16_t)(Layer.mostRecent() * Runtime.device().numKeys() + key_addr.toInt()) == update_position_) nextState(); } return EventHandlerResult::EVENT_CONSUMED; @@ -70,10 +70,10 @@ EventHandlerResult EEPROMKeymapProgrammer::onKeyswitchEvent(Key &mapped_key, Key if (state_ == WAIT_FOR_SOURCE_KEY) { if (keyToggledOn(key_state)) { - new_key_ = Layer.getKeyFromPROGMEM(Layer.top(), key_addr); + new_key_ = Layer.getKeyFromPROGMEM(Layer.mostRecent(), key_addr); } if (keyToggledOff(key_state)) { - if (new_key_ == Layer.getKeyFromPROGMEM(Layer.top(), key_addr)) + if (new_key_ == Layer.getKeyFromPROGMEM(Layer.mostRecent(), key_addr)) nextState(); } return EventHandlerResult::EVENT_CONSUMED; diff --git a/src/kaleidoscope/plugin/LED-ActiveLayerColor.cpp b/src/kaleidoscope/plugin/LED-ActiveLayerColor.cpp index 0cd7c081..b63cd0d6 100644 --- a/src/kaleidoscope/plugin/LED-ActiveLayerColor.cpp +++ b/src/kaleidoscope/plugin/LED-ActiveLayerColor.cpp @@ -36,7 +36,7 @@ void LEDActiveLayerColorEffect::setColormap(const cRGB colormap[]) { cRGB LEDActiveLayerColorEffect::TransientLEDMode::getActiveColor() { cRGB color; - uint8_t top_layer = ::Layer.top(); + uint8_t top_layer = ::Layer.mostRecent(); color.r = pgm_read_byte(&(parent_->colormap_[top_layer].r)); color.g = pgm_read_byte(&(parent_->colormap_[top_layer].g)); diff --git a/src/kaleidoscope_internal/deprecations.h b/src/kaleidoscope_internal/deprecations.h index 9279c892..fdf87b95 100644 --- a/src/kaleidoscope_internal/deprecations.h +++ b/src/kaleidoscope_internal/deprecations.h @@ -33,6 +33,19 @@ "For further information and examples on how to do that, \n" __NL__ \ "please see UPGRADING.md" +#define _DEPRECATED_MESSAGE_LAYER_DEACTIVATETOP __NL__ \ + "`Layer.deactivateTop()` is deprecated.\n" __NL__ \ + "Please use `Layer.deactivateMostRecent()` instead." + +#define _DEPRECATED_MESSAGE_LAYER_TOP __NL__ \ + "`Layer.top()` is deprecated.\n" __NL__ \ + "Please use `Layer.mostRecent()` instead." + +#define _DEPRECATED_MESSAGE_LAYER_GETLAYERSTATE __NL__ \ + "`Layer.getLayerState()` is deprecated.\n" __NL__ \ + "Layers are now in activation-order, please use" __NL__ \ + "`Layer.forEachActiveLayer()` instead." + #define _DEPRECATED_MESSAGE_HID_FACADE __NL__ \ "The HID facade in the `kaleidoscope::hid` namespace is deprecated.\n" __NL__ \ "Please use `Kaleidoscope.hid()` instead." From d90820afc7fec7d50ec65578d0070b003ff3ccc1 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 27 Sep 2020 09:52:00 +0200 Subject: [PATCH 2/5] tests: Add a test case for activation-ordered layer switching The new test case exercises a few corners of the system: we test that without any layer changes, we start out on the right one. We also test that shifting to another layer preserves the current caching behaviour. We test that the layers are indeed looped through in activation order, rather than index order. And finally, we test that explicitly turning off the single active layer will have us fall back to layer 0. We only test layer shifts, not locks and moves, because we only changed the ordering, not how those behave otherwise. Signed-off-by: Gergely Nagy --- .../layers/activation-order/sketch.ino | 98 ++++++++++ .../layers/activation-order/test/testcase.cpp | 177 ++++++++++++++++++ 2 files changed, 275 insertions(+) create mode 100644 tests/features/layers/activation-order/sketch.ino create mode 100644 tests/features/layers/activation-order/test/testcase.cpp diff --git a/tests/features/layers/activation-order/sketch.ino b/tests/features/layers/activation-order/sketch.ino new file mode 100644 index 00000000..cf592980 --- /dev/null +++ b/tests/features/layers/activation-order/sketch.ino @@ -0,0 +1,98 @@ +/* -*- mode: c++ -*- + * Copyright (C) 2020 Keyboard.io, Inc. + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +#include "Kaleidoscope.h" + +#include "Kaleidoscope-Macros.h" + +// *INDENT-OFF* + +KEYMAPS( + [0] = KEYMAP_STACKED + ( + Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,M(0) + ,ShiftToLayer(1) + + ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A + ,Key_A ,Key_A ,Key_A ,Key_A + ,ShiftToLayer(2) + ), + + [1] = KEYMAP_STACKED + ( + Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B + ,___ + + ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B + ,Key_B ,Key_B ,Key_B ,Key_B + ,___ + ), + + [2] = KEYMAP_STACKED + ( + Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C + ,___ + + ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C + ,Key_C ,Key_C ,Key_C ,Key_C + ,___ + ) +) // KEYMAPS( + +// *INDENT-ON* + +KALEIDOSCOPE_INIT_PLUGINS(Macros); + +const macro_t *macroAction(uint8_t macroIndex, uint8_t keyState) { + switch (macroIndex) { + case 0: + if (keyToggledOn(keyState)) + Layer.deactivate(0); + else + Layer.activate(0); + break; + } + return MACRO_NONE; +} + +void setup() { + Kaleidoscope.setup(); +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/tests/features/layers/activation-order/test/testcase.cpp b/tests/features/layers/activation-order/test/testcase.cpp new file mode 100644 index 00000000..31ab26c1 --- /dev/null +++ b/tests/features/layers/activation-order/test/testcase.cpp @@ -0,0 +1,177 @@ +/* -*- mode: c++ -*- + * Copyright (C) 2020 Keyboard.io, Inc. + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +#include "testing/setup-googletest.h" + +SETUP_GOOGLETEST(); + +namespace kaleidoscope { +namespace testing { +namespace { + +using ::testing::IsEmpty; + +class LayerActivationOrder : public VirtualDeviceTest { + public: + void SingleKeyTest(Key k) { + sim_.Press(0, 0); // k + auto state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(k)); + + sim_.Release(0, 0); // k + state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + IsEmpty()); + + state = RunCycle(); + + // 2 cycles after releasing k + EXPECT_EQ(state->HIDReports()->Keyboard().size(), 0); + } +}; + +TEST_F(LayerActivationOrder, BaseLayerHasNotRegressed) { + SingleKeyTest(Key_A); +} + +TEST_F(LayerActivationOrder, ShifToLayerOne) { + // Pressing (3,6) shifts to Layer 1, and we stay there until release. + + sim_.Press(3, 6); // ShiftToLayer(1) + auto state = RunCycle(); + + SingleKeyTest(Key_B); + + // Releasing (3,6) gets us back to the base layer + sim_.Release(3, 6); // ShiftToLayer(1) + state = RunCycle(); + + SingleKeyTest(Key_A); +} + +TEST_F(LayerActivationOrder, ShiftingWithCaching) { + // Pressing (0, 0) will activate A + sim_.Press(0, 0); + auto state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_A)); + + // Pressing (3, 6) will switch to Layer 1 + sim_.Press(3, 6); + state = RunCycle(); + + // ...since we're still pressing (0, 0), and there was no change in the HID + // states, we shouldn't emit a report. + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 0); + + // Pressing (0, 1), the report shall contain 'A' _and_ 'B'. + sim_.Press(0, 1); + state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_A)); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_B)); + + // Releasing (0, 0), the report should now contain B only + sim_.Release(0, 0); + state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_B)); + + // Release (0, 1) + sim_.Release(0, 1); + state = RunCycle(); + + // Test B in isolation again + SingleKeyTest(Key_B); + + // Release the layer key as well. + sim_.Release(3, 6); + state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 0); +} + +TEST_F(LayerActivationOrder, Ordering) { + // Pressing (3, 9) will switch to Layer 2 + sim_.Press(3, 9); + auto state = RunCycle(); + + // Pressing (0, 0) will activate 'C' + sim_.Press(0, 0); + state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_C)); + + // Pressing (3, 6) will activate Layer 1 + sim_.Press(3, 6); + state = RunCycle(); + + // Pressing (0, 1) will activate 'B' now, due to activation ordering. + sim_.Press(0, 1); + state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_C)); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_B)); + + sim_.Release(0, 0); + sim_.Release(0, 1); + sim_.Release(3, 6); + sim_.Release(3, 9); + state = RunCycle(); +} + +TEST_F(LayerActivationOrder, LayerZero) { + sim_.Press(3, 7); // Macro#0: Layer.deactivate(0) + auto state = RunCycle(); + + sim_.Press(0, 0); // A + state = RunCycle(); + + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + Contains(Key_A)); +} + +} // namespace +} // namespace testing +} // namespace kaleidoscope From ffcd17ba6e18f01c4647849a5a7c6f341bf9049f Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 10 Oct 2020 22:53:22 +0200 Subject: [PATCH 3/5] tests: Greatly simplify and clean up the layer activation-order test The test has been through a major refactor, lifting out common parts, improving comments, and naming. Signed-off-by: Gergely Nagy --- .../layers/activation-order/sketch.ino | 60 ++--- .../layers/activation-order/test/testcase.cpp | 225 ++++++++++-------- 2 files changed, 157 insertions(+), 128 deletions(-) diff --git a/tests/features/layers/activation-order/sketch.ino b/tests/features/layers/activation-order/sketch.ino index cf592980..60f39a09 100644 --- a/tests/features/layers/activation-order/sketch.ino +++ b/tests/features/layers/activation-order/sketch.ino @@ -23,52 +23,52 @@ KEYMAPS( [0] = KEYMAP_STACKED ( - Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,M(0) + Key_0 ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,M(0) ,ShiftToLayer(1) - ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A ,Key_A - ,Key_A ,Key_A ,Key_A ,Key_A + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,Key_0 + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,ShiftToLayer(2) ), [1] = KEYMAP_STACKED ( - Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B + Key_1 ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,___ - ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B ,Key_B - ,Key_B ,Key_B ,Key_B ,Key_B + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,Key_1 + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,___ ), [2] = KEYMAP_STACKED ( - Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C + Key_2 ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,___ - ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C ,Key_C - ,Key_C ,Key_C ,Key_C ,Key_C + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,Key_2 + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,___ ) ) // KEYMAPS( diff --git a/tests/features/layers/activation-order/test/testcase.cpp b/tests/features/layers/activation-order/test/testcase.cpp index 31ab26c1..320dc3d6 100644 --- a/tests/features/layers/activation-order/test/testcase.cpp +++ b/tests/features/layers/activation-order/test/testcase.cpp @@ -26,150 +26,179 @@ using ::testing::IsEmpty; class LayerActivationOrder : public VirtualDeviceTest { public: - void SingleKeyTest(Key k) { - sim_.Press(0, 0); // k - auto state = RunCycle(); + const KeyAddr KEYSWITCH_TOP_LEFT = KeyAddr{0, 0}; // layer-dependent key + const KeyAddr KEYSWITCH_TOP_RIGHT = KeyAddr{0, 15}; // layer-dependent key + const KeyAddr KEYSWITCH_LEFT_PALM = KeyAddr{3, 6}; // ShiftToLayer(1) + const KeyAddr KEYSWITCH_RIGHT_PALM = KeyAddr{3, 9}; // ShiftToLayer(2) + const KeyAddr KEYSWITCH_LEFT_THUMB_RIGHTMOST = KeyAddr{3, 7}; // L0 deactivate macro + + const Key LAYER0_KEY = Key_0; + const Key LAYER1_KEY = Key_1; + const Key LAYER2_KEY = Key_2; + + void pressKeyswitch(const KeyAddr& addr) { + sim_.Press(addr.row(), addr.col()); + } + + void releaseKeyswitch(const KeyAddr& addr) { + sim_.Release(addr.row(), addr.col()); + } + + auto pressKeyswitchAndCycle(const KeyAddr& addr) { + pressKeyswitch(addr); + return RunCycle(); + } + + auto releaseKeyswitchAndCycle(const KeyAddr& addr) { + releaseKeyswitch(addr); + return RunCycle(); + } + void assertSingleReportThatContains(std::unique_ptr &state, Key k) { ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); EXPECT_THAT( state->HIDReports()->Keyboard(0).ActiveKeycodes(), Contains(k)); + } - sim_.Release(0, 0); // k - state = RunCycle(); + void assertSingleReportThatDoesNotContain(std::unique_ptr &state, Key k) { + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); + EXPECT_THAT( + state->HIDReports()->Keyboard(0).ActiveKeycodes(), + ::testing::Not(Contains(k))); + } + void assertSingleEmptyReport(std::unique_ptr &state) { ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); EXPECT_THAT( state->HIDReports()->Keyboard(0).ActiveKeycodes(), IsEmpty()); + } + + void assertNoReport(std::unique_ptr &state) { + ASSERT_EQ(state->HIDReports()->Keyboard().size(), 0); + } + + void assertNoReportAfterCycle() { + auto state = RunCycle(); + assertNoReport(state); + } + + void TestPressAndRelease(const KeyAddr& addr, Key k) { + auto state = pressKeyswitchAndCycle(addr); + assertSingleReportThatContains(state, k); - state = RunCycle(); + state = releaseKeyswitchAndCycle(addr); + assertSingleEmptyReport(state); - // 2 cycles after releasing k - EXPECT_EQ(state->HIDReports()->Keyboard().size(), 0); + assertNoReportAfterCycle(); } }; TEST_F(LayerActivationOrder, BaseLayerHasNotRegressed) { - SingleKeyTest(Key_A); + TestPressAndRelease(KEYSWITCH_TOP_LEFT, LAYER0_KEY); } TEST_F(LayerActivationOrder, ShifToLayerOne) { - // Pressing (3,6) shifts to Layer 1, and we stay there until release. + // Pressing (KEYSWITCH_LEFT_PALM) shifts to Layer 1, and we stay there until release. + auto state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + TestPressAndRelease(KEYSWITCH_TOP_LEFT, LAYER1_KEY); - sim_.Press(3, 6); // ShiftToLayer(1) - auto state = RunCycle(); - - SingleKeyTest(Key_B); - - // Releasing (3,6) gets us back to the base layer - sim_.Release(3, 6); // ShiftToLayer(1) - state = RunCycle(); - - SingleKeyTest(Key_A); + // Releasing (KEYSWITCH_LEFT_PALM) gets us back to the base layer + state = releaseKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + TestPressAndRelease(KEYSWITCH_TOP_LEFT, LAYER0_KEY); } TEST_F(LayerActivationOrder, ShiftingWithCaching) { - // Pressing (0, 0) will activate A - sim_.Press(0, 0); - auto state = RunCycle(); - - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_A)); + // Pressing (KEYSWITCH_TOP_LEFT) will activate the key on layer 0 + auto state = pressKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); + assertSingleReportThatContains(state, LAYER0_KEY); - // Pressing (3, 6) will switch to Layer 1 - sim_.Press(3, 6); - state = RunCycle(); - - // ...since we're still pressing (0, 0), and there was no change in the HID - // states, we shouldn't emit a report. - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 0); + // Pressing (KEYSWITCH_LEFT_PALM) will switch to Layer 1 + state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); - // Pressing (0, 1), the report shall contain 'A' _and_ 'B'. - sim_.Press(0, 1); - state = RunCycle(); + // ...since we're still pressing (KEYSWITCH_TOP_LEFT), and there was no change + // in the HID states, we shouldn't emit a report. + assertNoReport(state); - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_A)); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_B)); - - // Releasing (0, 0), the report should now contain B only - sim_.Release(0, 0); - state = RunCycle(); + // Pressing (KEYSWITCH_TOP_RIGHT), the report shall contain keys from both + // layer 0 and layer1, because we started holding the layer 0 key prior to + // switching layers, so it's code should remain cached. + state = pressKeyswitchAndCycle(KEYSWITCH_TOP_RIGHT); + assertSingleReportThatContains(state, LAYER0_KEY); + assertSingleReportThatContains(state, LAYER1_KEY); - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_B)); + // Releasing (KEYSWITCH_TOP_LEFT), the report should now contain the key from + // layer1 only, and should not contain the layer0 key anymore. + state = releaseKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); + assertSingleReportThatContains(state, LAYER1_KEY); + assertSingleReportThatDoesNotContain(state, LAYER0_KEY); - // Release (0, 1) - sim_.Release(0, 1); - state = RunCycle(); + // Release (KEYSWITCH_TOP_RIGHT) + state = releaseKeyswitchAndCycle(KEYSWITCH_TOP_RIGHT); - // Test B in isolation again - SingleKeyTest(Key_B); + // Test the layer 1 key in isolation again + TestPressAndRelease(KEYSWITCH_TOP_LEFT, LAYER1_KEY); // Release the layer key as well. - sim_.Release(3, 6); - state = RunCycle(); + state = releaseKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 0); + // Since the layer key release is internal to us, we shouldn't send a report. + assertNoReport(state); } TEST_F(LayerActivationOrder, Ordering) { - // Pressing (3, 9) will switch to Layer 2 - sim_.Press(3, 9); - auto state = RunCycle(); - - // Pressing (0, 0) will activate 'C' - sim_.Press(0, 0); - state = RunCycle(); - - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_C)); - - // Pressing (3, 6) will activate Layer 1 - sim_.Press(3, 6); + // Pressing (KEYSWITCH_RIGHT_PALM) will switch to Layer 2 + auto state = pressKeyswitchAndCycle(KEYSWITCH_RIGHT_PALM); + + // Pressing (KEYSWITCH_TOP_LEFT) will activate a key on layer 2 + state = pressKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); + assertSingleReportThatContains(state, LAYER2_KEY); + + // Pressing (KEYSWITCH_LEFT_PALM) will activate Layer 1 + state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + + // Pressing (KEYSWITCH_TOP_RIGHT) will activate the layer 1 key now, due to + // activation ordering. + state = pressKeyswitchAndCycle(KEYSWITCH_TOP_RIGHT); + + // We should have both the layer 1 and the layer 2 key active, because we're + // holding both. + assertSingleReportThatContains(state, LAYER1_KEY); + assertSingleReportThatContains(state, LAYER2_KEY); + + // Releaseing all held keys, we should get an empty report. + releaseKeyswitch(KEYSWITCH_TOP_LEFT); + releaseKeyswitch(KEYSWITCH_TOP_RIGHT); + releaseKeyswitch(KEYSWITCH_LEFT_PALM); + releaseKeyswitch(KEYSWITCH_RIGHT_PALM); state = RunCycle(); - // Pressing (0, 1) will activate 'B' now, due to activation ordering. - sim_.Press(0, 1); - state = RunCycle(); + assertSingleEmptyReport(state); - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_C)); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_B)); - - sim_.Release(0, 0); - sim_.Release(0, 1); - sim_.Release(3, 6); - sim_.Release(3, 9); + // One more cycle, and we should generate no report at all state = RunCycle(); + assertNoReport(state); } TEST_F(LayerActivationOrder, LayerZero) { - sim_.Press(3, 7); // Macro#0: Layer.deactivate(0) - auto state = RunCycle(); + // Pressing the rightmost of the left thumb keys should deactivate layer 0 + auto state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_THUMB_RIGHTMOST); + + // Pressing KEYSWITCH_TOP_LEFT should fall back to activating the key on layer 0 + state = pressKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); + assertSingleReportThatContains(state, LAYER0_KEY); - sim_.Press(0, 0); // A + // Releasing all keys should generate a single empty report + releaseKeyswitch(KEYSWITCH_TOP_LEFT); + releaseKeyswitch(KEYSWITCH_LEFT_THUMB_RIGHTMOST); state = RunCycle(); - ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); - EXPECT_THAT( - state->HIDReports()->Keyboard(0).ActiveKeycodes(), - Contains(Key_A)); + assertSingleEmptyReport(state); + + // Afterwards, we should generate no more reports. + state = RunCycle(); + assertNoReport(state); } } // namespace From 60c138d756016a4f9b244896e43bd69145bb4b70 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 10 Oct 2020 23:01:41 +0200 Subject: [PATCH 4/5] docs/UPGRADING.md: Adjust the removal date of the deprecated Layer methods Signed-off-by: Gergely Nagy --- docs/UPGRADING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 16d83adc..888d40a3 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -321,7 +321,7 @@ The layer system used to be index-ordered, meaning that we'd look keys up on layers based on the _index_ of active layers. Kaleidoscope now uses activation order, which looks up keys based on the order of layer activation. -This means that the following functions are deprecated, and will be removed by **2020-09-16**: +This means that the following functions are deprecated, and will be removed by **2020-12-31**: - `Layer.top()`, which used to return the topmost layer index. Use `Layer.mostRecent()` instead, which returns the most recently activated layer. From 7d7615ef6f5dedc4b8ea2509f99e6710cd43bd90 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 10 Oct 2020 23:22:30 +0200 Subject: [PATCH 5/5] tests: Update some of the naming in the layer activation order test Thanks @obra for the new names! Signed-off-by: Gergely Nagy --- .../layers/activation-order/test/testcase.cpp | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/tests/features/layers/activation-order/test/testcase.cpp b/tests/features/layers/activation-order/test/testcase.cpp index 320dc3d6..d1e39e25 100644 --- a/tests/features/layers/activation-order/test/testcase.cpp +++ b/tests/features/layers/activation-order/test/testcase.cpp @@ -44,24 +44,24 @@ class LayerActivationOrder : public VirtualDeviceTest { sim_.Release(addr.row(), addr.col()); } - auto pressKeyswitchAndCycle(const KeyAddr& addr) { + auto pressKeyswitchAndRunCycle(const KeyAddr& addr) { pressKeyswitch(addr); return RunCycle(); } - auto releaseKeyswitchAndCycle(const KeyAddr& addr) { + auto releaseKeyswitchAndRunCycle(const KeyAddr& addr) { releaseKeyswitch(addr); return RunCycle(); } - void assertSingleReportThatContains(std::unique_ptr &state, Key k) { + void assertSingleKeyboardReportContaining(std::unique_ptr &state, Key k) { ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); EXPECT_THAT( state->HIDReports()->Keyboard(0).ActiveKeycodes(), Contains(k)); } - void assertSingleReportThatDoesNotContain(std::unique_ptr &state, Key k) { + void assertSingleKeyboardReportNotContaining(std::unique_ptr &state, Key k) { ASSERT_EQ(state->HIDReports()->Keyboard().size(), 1); EXPECT_THAT( state->HIDReports()->Keyboard(0).ActiveKeycodes(), @@ -85,10 +85,10 @@ class LayerActivationOrder : public VirtualDeviceTest { } void TestPressAndRelease(const KeyAddr& addr, Key k) { - auto state = pressKeyswitchAndCycle(addr); - assertSingleReportThatContains(state, k); + auto state = pressKeyswitchAndRunCycle(addr); + assertSingleKeyboardReportContaining(state, k); - state = releaseKeyswitchAndCycle(addr); + state = releaseKeyswitchAndRunCycle(addr); assertSingleEmptyReport(state); assertNoReportAfterCycle(); @@ -101,21 +101,21 @@ TEST_F(LayerActivationOrder, BaseLayerHasNotRegressed) { TEST_F(LayerActivationOrder, ShifToLayerOne) { // Pressing (KEYSWITCH_LEFT_PALM) shifts to Layer 1, and we stay there until release. - auto state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + auto state = pressKeyswitchAndRunCycle(KEYSWITCH_LEFT_PALM); TestPressAndRelease(KEYSWITCH_TOP_LEFT, LAYER1_KEY); // Releasing (KEYSWITCH_LEFT_PALM) gets us back to the base layer - state = releaseKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + state = releaseKeyswitchAndRunCycle(KEYSWITCH_LEFT_PALM); TestPressAndRelease(KEYSWITCH_TOP_LEFT, LAYER0_KEY); } TEST_F(LayerActivationOrder, ShiftingWithCaching) { // Pressing (KEYSWITCH_TOP_LEFT) will activate the key on layer 0 - auto state = pressKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); - assertSingleReportThatContains(state, LAYER0_KEY); + auto state = pressKeyswitchAndRunCycle(KEYSWITCH_TOP_LEFT); + assertSingleKeyboardReportContaining(state, LAYER0_KEY); // Pressing (KEYSWITCH_LEFT_PALM) will switch to Layer 1 - state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + state = pressKeyswitchAndRunCycle(KEYSWITCH_LEFT_PALM); // ...since we're still pressing (KEYSWITCH_TOP_LEFT), and there was no change // in the HID states, we shouldn't emit a report. @@ -124,24 +124,24 @@ TEST_F(LayerActivationOrder, ShiftingWithCaching) { // Pressing (KEYSWITCH_TOP_RIGHT), the report shall contain keys from both // layer 0 and layer1, because we started holding the layer 0 key prior to // switching layers, so it's code should remain cached. - state = pressKeyswitchAndCycle(KEYSWITCH_TOP_RIGHT); - assertSingleReportThatContains(state, LAYER0_KEY); - assertSingleReportThatContains(state, LAYER1_KEY); + state = pressKeyswitchAndRunCycle(KEYSWITCH_TOP_RIGHT); + assertSingleKeyboardReportContaining(state, LAYER0_KEY); + assertSingleKeyboardReportContaining(state, LAYER1_KEY); // Releasing (KEYSWITCH_TOP_LEFT), the report should now contain the key from // layer1 only, and should not contain the layer0 key anymore. - state = releaseKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); - assertSingleReportThatContains(state, LAYER1_KEY); - assertSingleReportThatDoesNotContain(state, LAYER0_KEY); + state = releaseKeyswitchAndRunCycle(KEYSWITCH_TOP_LEFT); + assertSingleKeyboardReportContaining(state, LAYER1_KEY); + assertSingleKeyboardReportNotContaining(state, LAYER0_KEY); // Release (KEYSWITCH_TOP_RIGHT) - state = releaseKeyswitchAndCycle(KEYSWITCH_TOP_RIGHT); + state = releaseKeyswitchAndRunCycle(KEYSWITCH_TOP_RIGHT); // Test the layer 1 key in isolation again TestPressAndRelease(KEYSWITCH_TOP_LEFT, LAYER1_KEY); // Release the layer key as well. - state = releaseKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + state = releaseKeyswitchAndRunCycle(KEYSWITCH_LEFT_PALM); // Since the layer key release is internal to us, we shouldn't send a report. assertNoReport(state); @@ -149,23 +149,23 @@ TEST_F(LayerActivationOrder, ShiftingWithCaching) { TEST_F(LayerActivationOrder, Ordering) { // Pressing (KEYSWITCH_RIGHT_PALM) will switch to Layer 2 - auto state = pressKeyswitchAndCycle(KEYSWITCH_RIGHT_PALM); + auto state = pressKeyswitchAndRunCycle(KEYSWITCH_RIGHT_PALM); // Pressing (KEYSWITCH_TOP_LEFT) will activate a key on layer 2 - state = pressKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); - assertSingleReportThatContains(state, LAYER2_KEY); + state = pressKeyswitchAndRunCycle(KEYSWITCH_TOP_LEFT); + assertSingleKeyboardReportContaining(state, LAYER2_KEY); // Pressing (KEYSWITCH_LEFT_PALM) will activate Layer 1 - state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_PALM); + state = pressKeyswitchAndRunCycle(KEYSWITCH_LEFT_PALM); // Pressing (KEYSWITCH_TOP_RIGHT) will activate the layer 1 key now, due to // activation ordering. - state = pressKeyswitchAndCycle(KEYSWITCH_TOP_RIGHT); + state = pressKeyswitchAndRunCycle(KEYSWITCH_TOP_RIGHT); // We should have both the layer 1 and the layer 2 key active, because we're // holding both. - assertSingleReportThatContains(state, LAYER1_KEY); - assertSingleReportThatContains(state, LAYER2_KEY); + assertSingleKeyboardReportContaining(state, LAYER1_KEY); + assertSingleKeyboardReportContaining(state, LAYER2_KEY); // Releaseing all held keys, we should get an empty report. releaseKeyswitch(KEYSWITCH_TOP_LEFT); @@ -183,11 +183,11 @@ TEST_F(LayerActivationOrder, Ordering) { TEST_F(LayerActivationOrder, LayerZero) { // Pressing the rightmost of the left thumb keys should deactivate layer 0 - auto state = pressKeyswitchAndCycle(KEYSWITCH_LEFT_THUMB_RIGHTMOST); + auto state = pressKeyswitchAndRunCycle(KEYSWITCH_LEFT_THUMB_RIGHTMOST); // Pressing KEYSWITCH_TOP_LEFT should fall back to activating the key on layer 0 - state = pressKeyswitchAndCycle(KEYSWITCH_TOP_LEFT); - assertSingleReportThatContains(state, LAYER0_KEY); + state = pressKeyswitchAndRunCycle(KEYSWITCH_TOP_LEFT); + assertSingleKeyboardReportContaining(state, LAYER0_KEY); // Releasing all keys should generate a single empty report releaseKeyswitch(KEYSWITCH_TOP_LEFT);