Cannot use Function/Layer change keys in Combinations

[paradox460]

tl;dr: Add something like U"X" + U"FUN1" to your keymaps, and compiling fails with error: 'KEY_FUN1' undeclared here, but works just fine for U"FUN1" (without any combination)

---

Backstory

I use a mac, and this means that function keys have many different functions, far beyond their normal F1-12. The way you swap between these is using a Fn key, on the apple keyboards, which is hardware level, just like Kll function layers. Using a piece of software like Karabiner, one can rebind another key that does nothing on OS X (in my case, the App key) to serve as a function key.

The goal

To be able to press a single key, moving my ergodox to Layer 1 and send the "App" keycode to the OS. In effect, I'd be able to press a key, and then press one of my "function" keys, and the OS just sees it as Fn + F1 which it understands as F1.

The problem

I added U"HOME" : U"APP" + U"FUN1"; to my KLL and attempt a compilation.

This results in

error: 'KEY_FUN1' undeclared here (not in a function)

This error is suspicious because elsewhere in the file, I have U"END" : U"FUN1"; and it compiles just fine

[haata]

Did you add the lcdFuncMap to that particular layer? U"FUN1" is a symbolic name.

Just like here -> https://github.com/kiibohd/controller/blob/master/Keyboards/ergodox.bash#L26

This is the file I'm using. https://github.com/kiibohd/kll/blob/master/layouts/lcdFuncMap.kll

[paradox460]

Yes I did. I have lcdFuncMap on all my layers.

FUN1 works when its the only binding on a key, but it fails when used in a combination

[ckaros]

I noticed this as well. I was able to get around the problem by calling the functions explicitly in the key map kll as such.

U"HOME" : U"APP" + layerShift( 1 );

and if you have an infinity ergodox, don't forget to add the lcd update function like this:

U"HOME" : U"APP" + layerShift( 1 )+ LCDLayerDisplay();

[patrickdepinguin]

I confirm both the problem as the workaround suggested by @ckaros.

I investigated this problem in the code. Following patch solves the compilation error, but does not yet render a functional key: the real key assignment is lost in the process and only the layer shift remains (see below).

commit af95af66508a53e49b5695f199916f0c6094bc66
Author: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>
Date:   Fri Mar 10 15:41:33 2017 +0100

    lookupUSBCodes: support FunctionX,LockX,LatchX in result combination

    A KLL file entry like the following:
        U"LALT" : U"LGUI" + U"Function3"
    currently fails with the error:
        error: 'KEY_FUN3' undeclared here (not in a function)

    After analysis it turns out that this is because when stdFuncMap.kll is
    read and the Function3 symbol resolved, no scancodes are found using this symbol, and KEY_FUN3 remains as-is.
    This, in turn, is caused by the search logic only considering single result
    entries.

    This patch extends the search logic to consider this extra case. However, it
    is unclear (to me) whether:
    a. the original condition is still relevant
    b. if there are other cases to consider rather than the hardcoded [0][0]

    Partially fixes issue kiibohd/controller#137 .

    ---

    Remark: the usage of anonymous tuples/lists for e.g. usbCode made this
    analysis very complex for the untrained. Introduction of a separate class to
    represent a usbCode and other entities could help here. To simplify the
    writing of such classes, the 'attr.s' module can help:
    https://attrs.readthedocs.io

diff --git a/kll_lib/containers.py b/kll_lib/containers.py
index 9b0d79e..1d6bc1a 100644
--- a/kll_lib/containers.py
+++ b/kll_lib/containers.py
@@ -221,7 +221,11 @@ class Macros:

        # Scan current layer for USB Codes
        for macro in self.macros[ self.layer ].keys():
-           if usbCode in self.macros[ self.layer ][ macro ]:
+           # example usbCode: ((('usbKeyOut', ('KEY_FUN3',)),),)
+           # example search list entries:
+           #   [((('usbKeyOut', ('KEY_TAB',)),),)]
+           #   [((('usbKeyOut', ('KEY_LGUI',)), ('usbKeyOut', ('KEY_FUN3',))),)]
+           if usbCode in self.macros[ self.layer ][ macro ] or usbCode[0][0] in self.macros[ self.layer ][ macro ][0][0]:
                scanCodeList.append( macro )

        if len(scanCodeList) == 0:

With this patch, the file generated_keymap.h is created, but is not correct for the special key. In particular, the difference between bad and good is:

-Guide_RM( 3 ) = { 1, 7, 3, 0, 0 };
+Guide_RM( 3 ) = { 2, 16, KEY_LGUI, 7, 3, 0, 0 };

In my case, I'm mapping LGUI + layershift(3). As you can see, the LGUI part is not present if you use U"Function3" rather than 'layerShift(3)'.

The reason is in kll/kll_lib/containers.py , Macros.replaceScanCode(): when reading the stdFuncMap.kll file which maps 'FunctionX' to 'layerShift(X)', the replaceScanCode will be called with the scancode entry that contains FunctionX and replaces it completely with 'layerShift(X)'. If the scancode entry contains not only KEY_FUNX but also other items, as in this issue, then the other items are lost.

self.macros[ self.layer ][ scanCode ] = [ result ]

However, solving this is not completely trivial. Basically, I think the replaceScanCode() method should receive the original trigger for the result, so that it can modify only that one. In particular, when reading stdFuncMap.kll, the original trigger for result 'layerShift(3)' is 'KEY_FUN3' (nested in some tuples) (== U"Function3"). Currently the information 'KEY_FUN3' is not present in the cache. Then when replaceScanCode is called to make the transformation 'KEY_FUN3' -> 'layerShift(3)' on entry:

[((('usbKeyOut', ('KEY_LGUI',)), ('usbKeyOut', ('KEY_FUN3',))),)]

it should only replace the second part and keep LGUI intact. To achieve that, this extra information needs to be stored in the assignmentCache as well.

But these changes have repercussions in several places, and I'm not familiar enough with the code to make good judgements here. Moreover, as also indicated in the commit msg of the patch above, the 'list of tuple of tuple of tuples' constructs are very hard to manage and understand IMHO. And the fact that the entries above are tuples means we cannot easily replace an item because we need to iterate over them first, which will require transformation to list first. My feeling is that this should be doable in a different way, making more use of classes for entities like usbCode, scanCode, trigger, result, ...

Feedback very welcome.