Merge pull request #260 from keyboardio/f/layer/activate

Layer: Rename a few functions, to use more understandable names

UPGRADING.md

@@ -16,6 +16,8 @@ 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)
+    - [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)
@@ -434,6 +436,22 @@ 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**.
+
+### 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.

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,11 +27,10 @@
 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];
-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) {
@@ -41,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:
@@ -69,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);
   }
 }
 
@@ -102,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, DefaultLayer, 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 > DefaultLayer) {
-        if (Layer.isOn(layer)) {
+      while (layer > 0) {
+        if (Layer.isActive(layer)) {
           Key mappedKey = (*getKey)(layer, row, col);
 
           if (mappedKey != Key_Transparent) {
-            activeLayers[row][col] = layer;
+            active_layers_[row][col] = layer;
             break;
           }
         }
@@ -127,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 > DefaultLayer; i--) {
-    if (bitRead(LayerState, i)) {
-      highestLayer = i;
+  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:
-  highestLayer = DefaultLayer;
+  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();
 
@@ -173,49 +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);
-}
-
-void Layer_::defaultLayer(uint8_t layer) {
-  move(layer);
-  DefaultLayer = layer;
-}
-
-uint8_t Layer_::defaultLayer(void) {
-  return DefaultLayer;
-}
-
-uint32_t Layer_::getLayerState(void) {
-  return LayerState;
+void Layer_::deactivateTop(void) {
+  deactivate(top_active_layer_);
 }
 
 }

src/kaleidoscope/layers.h

@@ -67,31 +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 void next(void);
-  static void previous(void);
-
-  static boolean isOn(uint8_t 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);
-  static uint8_t defaultLayer(void);
+  static void defaultLayer(uint8_t layer) DEPRECATED(LAYER_DEFAULT) {}
+  static uint8_t defaultLayer(void) DEPRECATED(LAYER_DEFAULT) {
+    return 0;
+  }
 
-  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);
 
@@ -103,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);
 };
 }
 

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);

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)  {

src/kaleidoscope_internal/deprecations.h

@@ -26,3 +26,23 @@
   "------------------------------------------------------------------------\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."
+
+#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().`"