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 <algernon@keyboard.io>

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.

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

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

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

@@ -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().`"