Separate tracking of layer shifts & locks

This (hopefully) makes layer changes more intuitive for users, and more
straightforward to explain and reason about.  I believe this is a natural
extension of the activation-order layer stack, and away from the fixed-order
stack of old.

Specifically, it changes things so that layer lock keys will deactivate the
target layer if it is at the top of the active layer stack, but otherwise it
will bring the target layer to the top of the stack, regardless of whether or
not it was previously active.  This ought to provide less surprising behaviour
in the case where a layer was active, but hidden (possibly for long enough that
the user can't recall its status).  Pressing the layer lock key is now
guaranteed to produce a user-visible change (unless the target layer is 100%
transparent).

Layer shift behaviour is also changed, but not quite in the same way.  A layer
shift key will now activate the target layer, as before, but pressing a second
layer shift key for the same target layer won't bring the target layer to the
top of the stack.  The idea is that pressing a second identical layer shift
should be interpreted as a continuation of the already-active shift, probably so
that the user can change hands.  A user could still forget about a shifted
layer, but it would have to be a sticky OneShot layer shift, so I think this is
reasonable.

The major change is that locked and shifted layers are now tracked separately on
the layer stack (using an offset), which means that locking a layer, then
pressing and releasing a layer shift key for that layer will not result in the
layer being unlocked.

Closes #1009

Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>

src/kaleidoscope/layers.cpp

@@ -30,9 +30,6 @@
 #include "kaleidoscope/layers.h"           // for Layer_, Layer, Layer_::GetKeyFunction, Layer_:...
 #include "kaleidoscope_internal/device.h"  // for device
 
-// The maximum number of layers allowed.
-#define MAX_LAYERS 32;
-
 // The following definitions of layer_count and keymaps_linear
 // are used if the user does not define a keymap within the sketch
 // by means of the KEYMAP macro.
@@ -48,7 +45,7 @@ __attribute__((weak)) extern constexpr Key keymaps_linear[][kaleidoscope_interna
 
 namespace kaleidoscope {
 uint8_t Layer_::active_layer_count_ = 1;
-int8_t Layer_::active_layers_[31];
+int8_t Layer_::active_layers_[MAX_ACTIVE_LAYERS];
 
 uint8_t Layer_::active_layer_keymap_[kaleidoscope_internal::device.numKeys()];
 Layer_::GetKeyFunction Layer_::getKey = &Layer_::getKeyFromPROGMEM;
@@ -75,53 +72,39 @@ void Layer_::handleLayerKeyEvent(const KeyEvent &event) {
     target_layer = key_code - LAYER_SHIFT_OFFSET;
 
     switch (target_layer) {
-    case KEYMAP_NEXT:
-      // Key_KeymapNext_Momentary
-      if (keyToggledOn(event.state))
-        activateNext();
-      else
-        deactivateMostRecent();
-      break;
 
+    case KEYMAP_NEXT:
     case KEYMAP_PREVIOUS:
+      if (keyToggledOn(event.state)) {
+        uint8_t top_layer = active_layers_[active_layer_count_ - 1];
+        if (target_layer == KEYMAP_NEXT) {
+          // Key_KeymapNext_Momentary
+          target_layer = top_layer + 1;
+        } else {
           // Key_KeymapPrevious_Momentary
-      if (keyToggledOn(event.state))
+          target_layer = top_layer - 1;
-        deactivateMostRecent();
+        }
-      else
+        if (target_layer >= layer_count) {
-        activateNext();
+          live_keys.mask(event.addr);
+          return;
+        }
+        uint8_t target_layer_shifted = target_layer + LAYER_SHIFT_OFFSET;
+        activate(target_layer_shifted);
+        // We can't just change `event.key` here because `live_keys[]` has
+        // already been updated by the time `handleLayerKeyEvent()` gets called.
+        live_keys[event.addr] = ShiftToLayer(target_layer);
+      }
       break;
 
     default:
       // ShiftToLayer()
-      /* The default case is when we are switching to a layer by its number, and
+      target_layer += LAYER_SHIFT_OFFSET;
-       * is a bit more complicated than switching there when the key toggles on,
+
-       * and away when it toggles off.
-       *
-       * We want to handle the case where we have more than one momentary layer
-       * key on our keymap that point to the same target layer, and we hold
-       * both, and release one. In this case, the layer should remain active,
-       * because the second momentary key is still held.
-       *
-       * To do this, we turn the layer back on if the switcher key is still
-       * held, not only when it toggles on. So when one of them is released,
-       * that does turn the layer off, but with the other still being held, the
-       * layer will toggle back on in the same cycle.
-       */
       if (keyToggledOn(event.state)) {
-        // Re-think this: maybe we want to bring an already-active layer to the
+        if (stackPosition(target_layer) < 0) {
-        // top when a layer shift key is pressed.
-        if (!isActive(target_layer))
           activate(target_layer);
-      } else {
-        // If there's another layer shift key keeping the target layer active,
-        // we need to abort before deactivating it.
-        for (Key key : live_keys.all()) {
-          if (key == event.key) {
-            return;
-          }
         }
-        // No other layer shift key for the target layer is pressed; deactivate
+      } else {
-        // it now.
         deactivate(target_layer);
       }
       break;
@@ -129,12 +112,13 @@ void Layer_::handleLayerKeyEvent(const KeyEvent &event) {
   } else if (keyToggledOn(event.state)) {
     // LockLayer()/UnlockLayer()
     target_layer = key_code;
-    // switch keymap and stay there
+
-    if (isActive(target_layer))
+    if (stackPosition(target_layer) == active_layer_count_ - 1) {
       deactivate(target_layer);
-    else
+    } else {
       activate(target_layer);
     }
+  }
 }
 
 Key Layer_::getKeyFromPROGMEM(uint8_t layer, KeyAddr key_addr) {
@@ -150,9 +134,9 @@ void Layer_::updateActiveLayers(void) {
   // of the top active layer that has a non-transparent entry for that address.
   for (auto key_addr : KeyAddr::all()) {
     for (uint8_t i = active_layer_count_; i > 0; --i) {
-      uint8_t layer = active_layers_[i - 1];
+      uint8_t layer = unshifted(active_layers_[i - 1]);
-      Key key       = (*getKey)(layer, key_addr);
 
+      Key key = (*getKey)(layer, key_addr);
       if (key != Key_Transparent) {
         active_layer_keymap_[key_addr.toInt()] = layer;
         break;
@@ -170,9 +154,9 @@ 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
   // layer change.
-  if (layer >= layer_count) {
+  if (layer >= layer_count)
-    layer = 0;
+    return;
-  }
+
   active_layer_count_ = 1;
   active_layers_[0]   = layer;
 
@@ -185,13 +169,22 @@ void Layer_::move(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)
+  uint8_t layer_unshifted = unshifted(layer);
-    return;
 
-  // If the target layer was already on, return
+  if (layer_unshifted >= layer_count)
-  if (isActive(layer))
     return;
 
+  int8_t old_pos = stackPosition(layer);
+  if (old_pos >= 0) {
+    remove(old_pos);
+  }
+
+  // Guarantee that we don't overflow by removing layers from the bottom if
+  // we're about to exceed the size of the active layers array.
+  while (active_layer_count_ >= MAX_ACTIVE_LAYERS) {
+    remove(0);
+  }
+
   // Otherwise, push it onto the active layer stack
   active_layers_[active_layer_count_++] = layer;
 
@@ -202,10 +195,17 @@ void Layer_::activate(uint8_t layer) {
   kaleidoscope::Hooks::onLayerChange();
 }
 
+void Layer_::remove(uint8_t i) {
+  memmove(&active_layers_[i], &active_layers_[i + 1], active_layer_count_ - (i + 1));
+  --active_layer_count_;
+}
+
 // Deactivate a given layer
 void Layer_::deactivate(uint8_t layer) {
+  int8_t current_pos = stackPosition(layer);
+
   // If the target layer was already off, return
-  if (!isActive(layer))
+  if (current_pos < 0)
     return;
 
   // If the sole active layer is being deactivated, turn on the base layer and
@@ -216,44 +216,50 @@ void Layer_::deactivate(uint8_t layer) {
   }
 
   // Remove the target layer from the active layer stack, and shift any layers
-  // above it down to fill in the gap
+  // above it down to fill in the gap.
-  for (uint8_t i = 0; i < active_layer_count_; ++i) {
+  remove(current_pos);
-    if (active_layers_[i] == layer) {
-      memmove(&active_layers_[i], &active_layers_[i + 1], active_layer_count_ - (i + 1));
-      --active_layer_count_;
-      break;
-    }
-  }
 
-  // Update the keymap cache (but not live_composite_keymap_; that gets
+  // Update the keymap cache.
-  // updated separately, when keys toggle on or off. See layers.h)
   updateActiveLayers();
 
   kaleidoscope::Hooks::onLayerChange();
 }
 
-bool Layer_::isActive(uint8_t layer) {
+boolean Layer_::isActive(uint8_t layer) {
+  return ((stackPosition(layer) >= 0) ||
+          (stackPosition(layer + LAYER_SHIFT_OFFSET) >= 0));
+}
+
+int8_t Layer_::stackPosition(uint8_t layer) {
   for (int8_t i = 0; i < active_layer_count_; ++i) {
     if (active_layers_[i] == layer)
-      return true;
+      return i;
   }
-  return false;
+  return -1;
 }
 
-void Layer_::activateNext(void) {
+void Layer_::activateNext() {
   activate(active_layers_[active_layer_count_ - 1] + 1);
 }
 
-void Layer_::deactivateMostRecent(void) {
+void Layer_::deactivateMostRecent() {
-  deactivate(active_layers_[active_layer_count_ - 1]);
+  uint8_t layer = unshifted(active_layers_[active_layer_count_ - 1]);
+  deactivate(layer);
 }
 
 void Layer_::forEachActiveLayer(forEachHandler h) {
   for (uint8_t i = 0; i < active_layer_count_; i++) {
-    (*h)(i, active_layers_[i]);
+    uint8_t layer = unshifted(active_layers_[i]);
+    (*h)(i, layer);
   }
 }
 
+uint8_t Layer_::unshifted(uint8_t layer) {
+  if (layer >= LAYER_SHIFT_OFFSET)
+    layer -= LAYER_SHIFT_OFFSET;
+  return layer;
+}
+
 }  // namespace kaleidoscope
 
 kaleidoscope::Layer_ Layer;

src/kaleidoscope/layers.h

@@ -30,6 +30,9 @@
 #include "kaleidoscope_internal/sketch_exploration/sketch_exploration.h"  // for _INIT_SKETCH_EX...
 
 // clang-format off
+#ifndef MAX_ACTIVE_LAYERS
+#define MAX_ACTIVE_LAYERS 16
+#endif
 
 #define START_KEYMAPS                                                   __NL__ \
    constexpr Key keymaps_linear[][kaleidoscope_internal::device.matrix_rows * kaleidoscope_internal::device.matrix_columns] PROGMEM = {
@@ -120,8 +123,12 @@ class Layer_ {
 
  private:
   static uint8_t active_layer_count_;
-  static int8_t active_layers_[31];
+  static int8_t active_layers_[MAX_ACTIVE_LAYERS];
   static uint8_t active_layer_keymap_[kaleidoscope_internal::device.numKeys()];
+
+  static int8_t stackPosition(uint8_t layer);
+  static void remove(uint8_t stack_index);
+  static uint8_t unshifted(uint8_t layer);
 };
 }  // namespace kaleidoscope