From 8d16a13131496c5d99c8ebb4b6d37276e5cf1f28 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 8 Dec 2018 10:06:22 +0100 Subject: [PATCH 1/2] Deprecate Layer.defaultLayer() In the original `KeyboardioFirmware`, this was used to store the default keymap index in EEPROM. When we removed the EEPROM storage, it was meant to be used by sketches to set a default layer (the minimum layer one can switch to), which was not nearly as useful. Even worse, the behaviour was complicated to reason about, and it wasn't used anyway. For these reasons, it is now deprecated, and will eventually be removed. Signed-off-by: Gergely Nagy --- UPGRADING.md | 7 +++++++ src/kaleidoscope/layers.cpp | 18 ++++-------------- src/kaleidoscope/layers.h | 8 +++++--- src/kaleidoscope_internal/deprecations.h | 5 +++++ 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 8169e739..36b99fb8 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -16,6 +16,7 @@ If any of this does not make sense to you, or you have trouble updating your .in - [MagicCombo](#magiccombo) - [TypingBreaks](#typingbreaks) + [Deprecated APIs and their replacements](#deprecated-apis-and-their-replacements) + - [Removal of Layer.defaultLayer](#removal-of-layerdefaultlayer) - [Finer OneShot stickability control](#finer-oneshot-stickability-control) - [Source code and namespace rearrangement](#source-code-and-namespace-rearrangement) * [Removed APIs](#removed-apis) @@ -434,6 +435,12 @@ Storing the settable settings in EEPROM makes it depend on `Kaleidoscope-EEPROM- ## Deprecated APIs and their replacements +### Removal of Layer.defaultLayer + +The `Layer.defaultLayer()` method has been deprecated, because it wasn't widely used, nor tested well, and needlessly complicated the layering logic. If one wants to set a default layer, which the keyboard switches to when booting up, `EEPROMSettings.default_layer()` may be of use. + +`Layer.defaultLayer` is slated for removal by **2019-02-14**. + ### Finer OneShot stickability control The [OneShot plugin](doc/plugin/OneShot.md) has much improved stickability control. Instead of only being able to control if one-shot layers should be stickable too, or disabling the sticky feature in general, it is now possible to control stickiness on a per-key basis with the new `OneShot.enableStickability()` and `OneShot.disableStickablity()` methods. diff --git a/src/kaleidoscope/layers.cpp b/src/kaleidoscope/layers.cpp index 2c6fd0b5..05914567 100644 --- a/src/kaleidoscope/layers.cpp +++ b/src/kaleidoscope/layers.cpp @@ -27,7 +27,6 @@ uint8_t layer_count __attribute__((weak)) = MAX_LAYERS; namespace kaleidoscope { -uint8_t Layer_::DefaultLayer; uint32_t Layer_::LayerState; uint8_t Layer_::highestLayer; Key Layer_::liveCompositeKeymap[ROWS][COLS]; @@ -107,12 +106,12 @@ void Layer_::updateLiveCompositeKeymap(byte row, byte col) { } void Layer_::updateActiveLayers(void) { - memset(activeLayers, DefaultLayer, ROWS * COLS); + memset(activeLayers, 0, ROWS * COLS); for (byte row = 0; row < ROWS; row++) { for (byte col = 0; col < COLS; col++) { int8_t layer = highestLayer; - while (layer > DefaultLayer) { + while (layer > 0) { if (Layer.isOn(layer)) { Key mappedKey = (*getKey)(layer, row, col); @@ -130,7 +129,7 @@ void Layer_::updateActiveLayers(void) { void Layer_::updateHighestLayer(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 > DefaultLayer; i--) { + for (byte i = (layer_count - 1); i > 0; i--) { if (bitRead(LayerState, i)) { highestLayer = i; return; @@ -138,7 +137,7 @@ void Layer_::updateHighestLayer(void) { } // It's not possible to turn off the default layer (see // updateActiveLayers()), so if no other layers are active: - highestLayer = DefaultLayer; + highestLayer = 0; } void Layer_::move(uint8_t layer) { @@ -205,15 +204,6 @@ void Layer_::previous(void) { off(highestLayer); } -void Layer_::defaultLayer(uint8_t layer) { - move(layer); - DefaultLayer = layer; -} - -uint8_t Layer_::defaultLayer(void) { - return DefaultLayer; -} - uint32_t Layer_::getLayerState(void) { return LayerState; } diff --git a/src/kaleidoscope/layers.h b/src/kaleidoscope/layers.h index db3c29d5..7d21546b 100644 --- a/src/kaleidoscope/layers.h +++ b/src/kaleidoscope/layers.h @@ -83,14 +83,16 @@ class Layer_ { static uint8_t top(void) { return highestLayer; } + + static void defaultLayer(uint8_t layer) DEPRECATED(LAYER_DEFAULT) {} + static uint8_t defaultLayer(void) DEPRECATED(LAYER_DEFAULT) { + return 0; + } static void next(void); static void previous(void); static boolean isOn(uint8_t layer); - static void defaultLayer(uint8_t layer); - static uint8_t defaultLayer(void); - static uint32_t getLayerState(void); static Key eventHandler(Key mappedKey, byte row, byte col, uint8_t keyState); diff --git a/src/kaleidoscope_internal/deprecations.h b/src/kaleidoscope_internal/deprecations.h index 8e53baae..018eabe7 100644 --- a/src/kaleidoscope_internal/deprecations.h +++ b/src/kaleidoscope_internal/deprecations.h @@ -26,3 +26,8 @@ "------------------------------------------------------------------------\n" \ /* Messages */ +#define _DEPRECATED_MESSAGE_LAYER_DEFAULT \ + "Layer.defaultLayer() is deprecated, and a no-op.\n" \ + "\n" \ + "If you want to set the default layer for the keyboard, consider using\n" \ + "EEPROMSettings.default_layer() instead." From dbaba6c1ef068aa34a0a7970494594b0a80ae9bd Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 8 Dec 2018 10:13:39 +0100 Subject: [PATCH 2/2] Layer: Rename a few methods, for better clarity In general, we want method names to be verbs, because they convey actions. The `Layer.on()`, `Layer.off()`, etc family of functions weren't like that. It wasn't immediately obvious that they turn layers on and off. Furthermore, `Layer.next()` and `Layer.previous()` were even more confusing, because one could easily think they implied moving to the selected layers, while in practice, they just activated them. All of these have new names, that better describe what they do. The old names are still present, but emit a deprecation warning. Signed-off-by: Gergely Nagy --- UPGRADING.md | 11 ++ src/kaleidoscope/layers.cpp | 102 +++++++++--------- src/kaleidoscope/layers.h | 51 +++++---- .../plugin/LED-ActiveModColor.cpp | 2 +- src/kaleidoscope/plugin/NumPad.cpp | 2 +- src/kaleidoscope_internal/deprecations.h | 15 +++ 6 files changed, 110 insertions(+), 73 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 36b99fb8..f0b5da73 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -17,6 +17,7 @@ If any of this does not make sense to you, or you have trouble updating your .in - [TypingBreaks](#typingbreaks) + [Deprecated APIs and their replacements](#deprecated-apis-and-their-replacements) - [Removal of Layer.defaultLayer](#removal-of-layerdefaultlayer) + - [More clarity in Layer method names](#more-clarity-in-layer-method-names) - [Finer OneShot stickability control](#finer-oneshot-stickability-control) - [Source code and namespace rearrangement](#source-code-and-namespace-rearrangement) * [Removed APIs](#removed-apis) @@ -441,6 +442,16 @@ The `Layer.defaultLayer()` method has been deprecated, because it wasn't widely `Layer.defaultLayer` is slated for removal by **2019-02-14**. +### More clarity in Layer method names + +A number of methods on the `Layer` object have been renamed, to make their intent clearer: + +- `Layer.on()` and `Layer.off()` became `Layer.activate()` and `Layer.decativate()`, repsectively. +- `Layer.next()` and `Layer.previous()` became `Layer.activateNext()` and `Layer.deactivateTop()`. +- `Layer.isOn` became `Layer.isActive()`. + +The goal was to have a method name that is a verb, because these are actions we do. The old names are still present, but emit a deprecation warning, and will be removed by **2019-02-14**. + ### Finer OneShot stickability control The [OneShot plugin](doc/plugin/OneShot.md) has much improved stickability control. Instead of only being able to control if one-shot layers should be stickable too, or disabling the sticky feature in general, it is now possible to control stickiness on a per-key basis with the new `OneShot.enableStickability()` and `OneShot.disableStickablity()` methods. diff --git a/src/kaleidoscope/layers.cpp b/src/kaleidoscope/layers.cpp index 05914567..e581703e 100644 --- a/src/kaleidoscope/layers.cpp +++ b/src/kaleidoscope/layers.cpp @@ -16,7 +16,7 @@ #include "kaleidoscope/Kaleidoscope.h" -// The maximum number of layers allowed. `LayerState`, which stores +// The maximum number of layers allowed. `layer_state_`, which stores // the on/off status of the layers in a bitfield has only 32 bits, and // that should be enough for almost any layout. #define MAX_LAYERS sizeof(uint32_t) * 8; @@ -27,10 +27,10 @@ uint8_t layer_count __attribute__((weak)) = MAX_LAYERS; namespace kaleidoscope { -uint32_t Layer_::LayerState; -uint8_t Layer_::highestLayer; -Key Layer_::liveCompositeKeymap[ROWS][COLS]; -uint8_t Layer_::activeLayers[ROWS][COLS]; +uint32_t Layer_::layer_state_; +uint8_t Layer_::top_active_layer_; +Key Layer_::live_composite_keymap_[ROWS][COLS]; +uint8_t Layer_::active_layers_[ROWS][COLS]; Key(*Layer_::getKey)(uint8_t layer, byte row, byte col) = Layer.getKeyFromPROGMEM; void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { @@ -40,16 +40,16 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { switch (target) { case KEYMAP_NEXT: if (keyToggledOn(keyState)) - next(); + activateNext(); else if (keyToggledOff(keyState)) - previous(); + deactivateTop(); break; case KEYMAP_PREVIOUS: if (keyToggledOn(keyState)) - previous(); + deactivateTop(); else if (keyToggledOff(keyState)) - next(); + activateNext(); break; default: @@ -68,19 +68,19 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) { * layer will toggle back on in the same cycle. */ if (keyIsPressed(keyState)) { - if (!Layer.isOn(target)) - on(target); + if (!Layer.isActive(target)) + activate(target); } else if (keyToggledOff(keyState)) { - off(target); + deactivate(target); } break; } } else if (keyToggledOn(keyState)) { // switch keymap and stay there - if (Layer.isOn(keymapEntry.keyCode) && keymapEntry.keyCode) - off(keymapEntry.keyCode); + if (Layer.isActive(keymapEntry.keyCode) && keymapEntry.keyCode) + deactivate(keymapEntry.keyCode); else - on(keymapEntry.keyCode); + activate(keymapEntry.keyCode); } } @@ -101,22 +101,22 @@ Key Layer_::getKeyFromPROGMEM(uint8_t layer, byte row, byte col) { } void Layer_::updateLiveCompositeKeymap(byte row, byte col) { - int8_t layer = activeLayers[row][col]; - liveCompositeKeymap[row][col] = (*getKey)(layer, row, col); + int8_t layer = active_layers_[row][col]; + live_composite_keymap_[row][col] = (*getKey)(layer, row, col); } void Layer_::updateActiveLayers(void) { - memset(activeLayers, 0, ROWS * COLS); + memset(active_layers_, 0, ROWS * COLS); for (byte row = 0; row < ROWS; row++) { for (byte col = 0; col < COLS; col++) { - int8_t layer = highestLayer; + int8_t layer = top_active_layer_; while (layer > 0) { - if (Layer.isOn(layer)) { + if (Layer.isActive(layer)) { Key mappedKey = (*getKey)(layer, row, col); if (mappedKey != Key_Transparent) { - activeLayers[row][col] = layer; + active_layers_[row][col] = layer; break; } } @@ -126,45 +126,45 @@ void Layer_::updateActiveLayers(void) { } } -void Layer_::updateHighestLayer(void) { +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(LayerState, i)) { - highestLayer = 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: - highestLayer = 0; + top_active_layer_ = 0; } void Layer_::move(uint8_t layer) { - LayerState = 0; - on(layer); + layer_state_ = 0; + activate(layer); } // Activate a given layer -void Layer_::on(uint8_t layer) { +void Layer_::activate(uint8_t layer) { // If we're trying to turn on a layer that doesn't exist, abort (but // if the keymap wasn't defined using the KEYMAPS() macro, proceed anyway if (layer >= layer_count) return; // If the target layer was already on, return - if (isOn(layer)) + if (isActive(layer)) return; - // Otherwise, turn on its bit in LayerState - bitSet(LayerState, 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 highestLayer - if (layer > highestLayer) - updateHighestLayer(); + // update top_active_layer_ + if (layer > top_active_layer_) + updateTopActiveLayer(); - // Update the keymap cache (but not liveCompositeKeymap; that gets + // Update the keymap cache (but not live_composite_keymap_; that gets // updated separately, when keys toggle on or off. See layers.h) updateActiveLayers(); @@ -172,40 +172,36 @@ void Layer_::on(uint8_t layer) { } // Deactivate a given layer -void Layer_::off(uint8_t layer) { +void Layer_::deactivate(uint8_t layer) { // If the target layer was already off, return - if (!bitRead(LayerState, layer)) + if (!bitRead(layer_state_, layer)) return; - // Turn off its bit in LayerState - bitClear(LayerState, layer); + // Turn off its bit in layer_state_ + bitClear(layer_state_, layer); // If the target layer was the previous highest active layer, - // update highestLayer - if (layer == highestLayer) - updateHighestLayer(); + // update top_active_layer_ + if (layer == top_active_layer_) + updateTopActiveLayer(); - // Update the keymap cache (but not liveCompositeKeymap; that gets + // Update the keymap cache (but not live_composite_keymap_; that gets // updated separately, when keys toggle on or off. See layers.h) updateActiveLayers(); kaleidoscope::Hooks::onLayerChange(); } -boolean Layer_::isOn(uint8_t layer) { - return bitRead(LayerState, layer); +boolean Layer_::isActive(uint8_t layer) { + return bitRead(layer_state_, layer); } -void Layer_::next(void) { - on(highestLayer + 1); +void Layer_::activateNext(void) { + activate(top_active_layer_ + 1); } -void Layer_::previous(void) { - off(highestLayer); -} - -uint32_t Layer_::getLayerState(void) { - return LayerState; +void Layer_::deactivateTop(void) { + deactivate(top_active_layer_); } } diff --git a/src/kaleidoscope/layers.h b/src/kaleidoscope/layers.h index 7d21546b..c81575f7 100644 --- a/src/kaleidoscope/layers.h +++ b/src/kaleidoscope/layers.h @@ -67,33 +67,50 @@ class Layer_ { * `lookupOnActiveLayer`. */ static Key lookup(byte row, byte col) { - return liveCompositeKeymap[row][col]; + return live_composite_keymap_[row][col]; } static Key lookupOnActiveLayer(byte row, byte col) { - uint8_t layer = activeLayers[row][col]; + uint8_t layer = active_layers_[row][col]; return (*getKey)(layer, row, col); } static uint8_t lookupActiveLayer(byte row, byte col) { - return activeLayers[row][col]; + return active_layers_[row][col]; + } + + static void activate(uint8_t layer); + static void deactivate(uint8_t layer); + static void on(uint8_t layer) DEPRECATED(LAYER_ON) { + activate(layer); + } + static void off(uint8_t layer) DEPRECATED(LAYER_OFF) { + deactivate(layer); + } + static void activateNext(); + static void deactivateTop(); + static void next(void) DEPRECATED(LAYER_NEXT) { + activateNext(); + } + static void previous(void) DEPRECATED(LAYER_PREVIOUS) { + deactivateTop(); } - static void on(uint8_t layer); - static void off(uint8_t layer); static void move(uint8_t layer); static uint8_t top(void) { - return highestLayer; + return top_active_layer_; } + static boolean isActive(uint8_t layer); + static boolean isOn(uint8_t layer) DEPRECATED(LAYER_ISON) { + return isActive(layer); + }; static void defaultLayer(uint8_t layer) DEPRECATED(LAYER_DEFAULT) {} static uint8_t defaultLayer(void) DEPRECATED(LAYER_DEFAULT) { return 0; } - static void next(void); - static void previous(void); - - static boolean isOn(uint8_t layer); - static uint32_t getLayerState(void); + static uint32_t getLayerState(void) { + return layer_state_; + } static Key eventHandler(Key mappedKey, byte row, byte col, uint8_t keyState); @@ -105,15 +122,13 @@ class Layer_ { static void updateActiveLayers(void); private: - static void updateHighestLayer(void); - - static uint8_t DefaultLayer; - static uint32_t LayerState; - static uint8_t highestLayer; - static Key liveCompositeKeymap[ROWS][COLS]; - static uint8_t activeLayers[ROWS][COLS]; + static uint32_t layer_state_; + static uint8_t top_active_layer_; + static Key live_composite_keymap_[ROWS][COLS]; + static uint8_t active_layers_[ROWS][COLS]; static void handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState); + static void updateTopActiveLayer(void); }; } diff --git a/src/kaleidoscope/plugin/LED-ActiveModColor.cpp b/src/kaleidoscope/plugin/LED-ActiveModColor.cpp index e0dfb324..55906de4 100644 --- a/src/kaleidoscope/plugin/LED-ActiveModColor.cpp +++ b/src/kaleidoscope/plugin/LED-ActiveModColor.cpp @@ -78,7 +78,7 @@ EventHandlerResult ActiveModColorEffect::beforeReportingState() { if (layer >= LAYER_SHIFT_OFFSET) layer -= LAYER_SHIFT_OFFSET; - if (Layer.isOn(layer)) + if (Layer.isActive(layer)) ::LEDControl.setCrgbAt(r, c, highlight_color); else ::LEDControl.refreshAt(r, c); diff --git a/src/kaleidoscope/plugin/NumPad.cpp b/src/kaleidoscope/plugin/NumPad.cpp index 6b49ae05..64257af7 100644 --- a/src/kaleidoscope/plugin/NumPad.cpp +++ b/src/kaleidoscope/plugin/NumPad.cpp @@ -86,7 +86,7 @@ void NumPad::setKeyboardLEDColors(void) { } EventHandlerResult NumPad::afterEachCycle() { - if (!Layer.isOn(numPadLayer)) { + if (!Layer.isActive(numPadLayer)) { cleanupNumlockState(); } else { if (numlockUnsynced) { diff --git a/src/kaleidoscope_internal/deprecations.h b/src/kaleidoscope_internal/deprecations.h index 018eabe7..b54a9243 100644 --- a/src/kaleidoscope_internal/deprecations.h +++ b/src/kaleidoscope_internal/deprecations.h @@ -31,3 +31,18 @@ "\n" \ "If you want to set the default layer for the keyboard, consider using\n" \ "EEPROMSettings.default_layer() instead." + +#define _DEPRECATED_MESSAGE_LAYER_ON \ + "Please use `Layer.activate()` instead of `Layer.on()`." + +#define _DEPRECATED_MESSAGE_LAYER_OFF \ + "Please use `Layer.deactivate()` instead of `Layer.off()`." + +#define _DEPRECATED_MESSAGE_LAYER_NEXT \ + "Please use `Layer.activateNext()` instead of `Layer.next()`." + +#define _DEPRECATED_MESSAGE_LAYER_PREVIOUS \ + "Please use `Layer.deactivateTop()` instead of `Layer.previous()`." + +#define _DEPRECATED_MESSAGE_LAYER_ISON \ + "Please use `Layer.isActive()` instead of `Layer.isOn().`"