Replace `handleKeymapKeyswitchEvent()` with `handleLayerKeyEvent()`

The new version of the layer change `Key` handler is more consistent with the
other `KeyEvent` handling functions, and properly checks for a second layer
shift key being held when releasing the first one.

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

src/kaleidoscope/Runtime.cpp

@@ -177,7 +177,7 @@ Runtime_::handleKeyEvent(KeyEvent event) {
 
   // If it's a built-in Layer key, we handle it here, and skip sending report(s)
   if (event.key.isLayerKey()) {
-    Layer.handleKeymapKeyswitchEvent(event.key, event.state);
+    Layer.handleLayerKeyEvent(event);
     //return;
   }
 

src/kaleidoscope/layers.cpp

@@ -18,6 +18,8 @@
 #include "kaleidoscope/hooks.h"
 #include "kaleidoscope/layers.h"
 #include "kaleidoscope/keyswitch_state.h"
+#include "kaleidoscope/KeyEvent.h"
+#include "kaleidoscope/LiveKeys.h"
 
 // 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
@@ -54,30 +56,36 @@ void Layer_::setup() {
   Layer.updateActiveLayers();
 }
 
-void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) {
-  if (keymapEntry.getKeyCode() >= LAYER_MOVE_OFFSET) {
+void Layer_::handleLayerKeyEvent(const KeyEvent &event) {
+  // The caller is responsible for checking that this is a Layer `Key`, so we
+  // skip checking for it here.
+  uint8_t key_code = event.key.getKeyCode();
+  uint8_t target_layer;
+
+  if (key_code >= LAYER_MOVE_OFFSET) {
     // MoveToLayer()
-    if (keyToggledOn(keyState)) {
-      move(keymapEntry.getKeyCode() - LAYER_MOVE_OFFSET);
+    if (keyToggledOn(event.state)) {
+      target_layer = key_code - LAYER_MOVE_OFFSET;
+      move(target_layer);
     }
-  } else if (keymapEntry.getKeyCode() >= LAYER_SHIFT_OFFSET) {
-    // layer shift keys
-    uint8_t target = keymapEntry.getKeyCode() - LAYER_SHIFT_OFFSET;
+  } else if (key_code >= LAYER_SHIFT_OFFSET) {
+    // layer shift keys (two types)
+    target_layer = key_code - LAYER_SHIFT_OFFSET;
 
-    switch (target) {
+    switch (target_layer) {
     case KEYMAP_NEXT:
       // Key_KeymapNext_Momentary
-      if (keyToggledOn(keyState))
+      if (keyToggledOn(event.state))
         activateNext();
-      else if (keyToggledOff(keyState))
+      else
         deactivateMostRecent();
       break;
 
     case KEYMAP_PREVIOUS:
       // Key_KeymapPrevious_Momentary
-      if (keyToggledOn(keyState))
+      if (keyToggledOn(event.state))
         deactivateMostRecent();
-      else if (keyToggledOff(keyState))
+      else
         activateNext();
       break;
 
@@ -97,30 +105,48 @@ void Layer_::handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState) {
        * that does turn the layer off, but with the other still being held, the
        * layer will toggle back on in the same cycle.
        */
-      if (keyIsPressed(keyState)) {
-        if (!Layer.isActive(target))
-          activate(target);
-      } else if (keyToggledOff(keyState)) {
-        deactivate(target);
+      if (keyToggledOn(event.state)) {
+        // Re-think this: maybe we want to bring an already-active layer to the
+        // 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
+        // it now.
+        deactivate(target_layer);
       }
       break;
     }
-  } else if (keyToggledOn(keyState)) {
+  } else if (keyToggledOn(event.state)) {
     // LockLayer()/UnlockLayer()
-    uint8_t target = keymapEntry.getKeyCode();
+    target_layer = key_code;
     // switch keymap and stay there
-    if (Layer.isActive(target))
-      deactivate(keymapEntry.getKeyCode());
+    if (isActive(target_layer))
+      deactivate(target_layer);
     else
-      activate(keymapEntry.getKeyCode());
+      activate(target_layer);
   }
 }
 
+#ifndef NDEPRECATED
+void Layer_::handleKeymapKeyswitchEvent(Key key, uint8_t key_state) {
+  if (key.getFlags() == (SYNTHETIC | SWITCH_TO_KEYMAP))
+    handleLayerKeyEvent(KeyEvent(KeyAddr::none(), key_state, key));
+}
+
 Key Layer_::eventHandler(Key mappedKey, KeyAddr key_addr, uint8_t keyState) {
   if (mappedKey.getFlags() == (SYNTHETIC | SWITCH_TO_KEYMAP))
-    handleKeymapKeyswitchEvent(mappedKey, keyState);
+    handleLayerKeyEvent(KeyEvent(key_addr, keyState, mappedKey));
   return mappedKey;
 }
+#endif
 
 Key Layer_::getKeyFromPROGMEM(uint8_t layer, KeyAddr key_addr) {
   return keyFromKeymap(layer, key_addr);

src/kaleidoscope/layers.h

@@ -19,6 +19,7 @@
 #include <Arduino.h>
 #include "kaleidoscope/key_defs.h"
 #include "kaleidoscope/keymaps.h"
+#include "kaleidoscope/KeyEvent.h"
 #include "kaleidoscope/device/device.h"
 #include "kaleidoscope_internal/device.h"
 #include "kaleidoscope_internal/sketch_exploration/sketch_exploration.h"
@@ -109,10 +110,15 @@ class Layer_ {
   }
   static boolean isActive(uint8_t layer);
 
+  static void handleLayerKeyEvent(const KeyEvent &event);
+
+#ifndef NDEPRECATED
+  DEPRECATED(LAYER_HANDLE_KEYMAP_KEYSWITCH_EVENT)
   static void handleKeymapKeyswitchEvent(Key keymapEntry, uint8_t keyState);
 
   DEPRECATED(LAYER_EVENTHANDLER)
   static Key eventHandler(Key mappedKey, KeyAddr key_addr, uint8_t keyState);
+#endif
 
   typedef Key(*GetKeyFunction)(uint8_t layer, KeyAddr key_addr);
   static GetKeyFunction getKey;

src/kaleidoscope_internal/deprecations.h

@@ -39,6 +39,10 @@
   "`Layer.eventHandler()` is deprecated.\n"                  __NL__ \
   "Please use `Layer.handleKeymapKeyswitchEvent()` instead."
 
+#define _DEPRECATED_MESSAGE_LAYER_HANDLE_KEYMAP_KEYSWITCH_EVENT  __NL__ \
+  "`Layer.handleKeymapKeyswitchEvent()` is deprecated.\n"        __NL__ \
+  "Please use `Layer.handleLayerKeyEvent()` instead."
+
 #define _DEPRECATED_MESSAGE_LAYER_LOOKUP                                      __NL__ \
   "`Layer.lookup(key_addr)` is deprecated.\n"                                 __NL__ \
   "Please use `Runtime.lookupKey(key_addr)` instead. Alternatively, if you\n" __NL__ \