KMKfw / kmk_firmware

Clackety Keyboards Powered by Python
https://kmkfw.zulipchat.com
Other
1.45k stars 485 forks source link

Adding a new key in layers for switching between active layer in a given direction #1043

Closed SergiuToporjinschi closed 2 weeks ago

SergiuToporjinschi commented 2 weeks ago

Hello, this is a new key which add functionality for switching the active layer. I wanted to keep it simple and fast as possible. If you you have any questions or you want me to change something just let me know I can quickly do the changes.

Thank you!

xs5871 commented 2 weeks ago

Can you provide a practical example where this would add any functionality that can not already be achieved with what we have?

SergiuToporjinschi commented 2 weeks ago

I need one layer for each application (there is no key combining thing) just a simple scenario where you have one layer for each app

Apps have different short-cut's but some of them have same key for same functionality for example IntelliJ has "debug resume" functionality on a key, vsCode has a different key for resume. So I want to create a layer for each app and use Transparent key where the keys are the same. I also have an encoder that will change the layer for me.

  1. I don't want to know, on the encoder level, how many active layers are
  2. I want to be able to go back and forward between layers (some times between the first and the second one, some times between the third and the first one, or any other two layers) using TGS is this example (you can use TGS also as key and put transparent for all the other layers and then that key will always go in the specified direction)
keyboard.keymap = [
    [
        KC.N1, KC.N2, KC.N3, KC.N4, KC.N5,
        KC.N6, KC.N7, KC.N8, KC.N9, KC.N0,
    ],
    [
        KC.A, KC.B, KC.C, KC.D, KC.E,
        KC.TRNS, KC.TRNS, KC.TRNS, KC.TRNS, KC.TRNS,
    ],
    [
        KC.Z, KC.X, KC.C, KC.V, KC.B,
        KC.TRNS, KC.TRNS, KC.TRNS, KC.TRNS, KC.TRNS,
    ]
]
keyboard.active_layers = [0, 1, 2]
encoder_handler.map = [
        ((KC.TGS('DOWN'), KC.TGS('UP'), KC.NO),),
        ((KC.TGS('DOWN'), KC.TGS('UP'), KC.NO),),
        ((KC.TGS('DOWN'), KC.TGS('UP'), KC.NO),)
        ]

if __name__ == '__main__':
    keyboard.go()

For this example rotating the encoder the output is :

18807500 kmk.modules.layers: active_layers=[1, 2, 0]
18808734 kmk.modules.layers: active_layers=[2, 0, 1]
18810214 kmk.modules.layers: active_layers=[0, 1, 2]

Using TG(<layer>) the problem is that it deactivates layers so transparent is not working anymore because the layer is missing. I dont understand why_tg_prssedhas a call todeactivate_layer` I tried

keyboard.active_layers = [0, 1, 2]
encoder_handler.map = [
        ((KC.TG(2), KC.TG(1), KC.NO),),
        ((KC.TG(0), KC.TG(2), KC.NO),),
        ((KC.TG(1), KC.TG(0), KC.NO),)
        ]

and I have this output where you can see that it deactivates layers

19035119 kmk.modules.layers: active_layers=[1, 0]
19035560 kmk.modules.layers: active_layers=[2, 1, 0]
19035629 kmk.modules.layers: active_layers=[2, 1]
19037393 kmk.modules.layers: active_layers=[0, 2, 1]
19037579 kmk.modules.layers: active_layers=[0, 2]
19037737 kmk.modules.layers: active_layers=[1, 0, 2]

plus that if is a sliding thing then you don't need to match the correct number of layers as parameter for this the key , is just going up or down on key map array list

If there is a way to achieve this please let me know. The thing is that I'm trying to create an desktop app to change layers based on the current app focus and I need to have all the layers active and not deactivated

xs5871 commented 2 weeks ago

The canonical way to do this would by something like this (untested):

encoder_handler.map = [
    ((KC.TG(2), KC.TG(1), KC.NO),),
    ((KC.TO(0), KC.FD(2), KC.NO),),
    ((KC.FD(1), KC.TO(0), KC.NO),)
]

If you think that defining the three encoder layers is to complicated (or you want to cycle through a million layers instead) you could generate that list programatically during initialisation (code left as excercise to the reader).

In my opinion your solution is a bit too niche to integrate into KMK. We're at a point where we have to be more selective about features, so that single problem solutions don't affect the rest of the user base (RAM and CPU is very precious). In addition, your suggestion doesn't use the activate_layer and deactivate_layer methods which are what people use in derived modules to listen to and act on layer change events (displays, backlighting, etc), so that's a "bug".

If you want to keep using your solution in a personal derived Layer module, I would personally refactor the UP/DOWN fake boolean argument to an arbitrary integer -- 1/-1 in this case -- and use those directly to calculate a generic offset, instead of an if-else. Get rid of the exception, you don't have to verify a parameter that is set once at boot over and over again during runtime (plus the error message is incorrect).

Please leave this PR open for now.

SergiuToporjinschi commented 2 weeks ago

To be honest I did not thought on having this switch between TG and TD on different layer. My scenario is more dynamic, and I cannot generate this sequence, because the layers will be configured by user in a desktop app so to tell them "you know for having a layer sliding you need to set it like this...". In my opinion when an interface that needs some explanation.... is a wrongly implemented, so I have to generate this, by code and if I have to generated then I need to tell the user to let some key slot free just to have this behavior. Is much more easier to just give them a key to use it

Over all is not a problem for me, I already have it on a custom module. I just thought that it might be useful, and I created a pull request. Now, because you said... in deed is creating the key on init so all those are taking memory.... maybe applying the same logic, like in keyboard (by creating on get/set) would take less memory (about 240 for each key, 1680 for all of them)

Knowing what's good for the this is your job :) You know better how most of the people are using it. I think you should add this scenario with TG TD in docs, I've spend some time trying to figure out how to do that.