nickcoutsos / keymap-editor

A web based graphical editor of ZMK keymaps.
http://nickcoutsos.github.io/keymap-editor
MIT License
1.27k stars 321 forks source link

Using key_repeat in hold-tap causes UI crash #81

Closed z5huang closed 1 year ago

z5huang commented 1 year ago

I have the following custom behavior added to my .keymap :

        lr: layer_rep {
            compatible = "zmk,behavior-hold-tap";
            label = "LAYER_REPEAT";
            #binding-cells = <2>;
            bindings =
                <&mo>,
                <&key_repeat>;

            tapping-term-ms = <200>;
            quick-tap-ms = <140>;
            global-quick-tap;
            flavor = "tap-preferred";
        };

This defines a hold-tap behavior where hold triggers a layer and tap triggers &key_repeat. I then bind it to a key by &lr 4 0. The 0 is here to conform to how hold-tap should be invoked (must have two parameters?). The firmware compiles successfully and is behaving as intended. In the keymap editor UI, it would show up as missing the second parameter, and when I click the second parameter, the UI crashes with the following error:

TypeError

n is undefined

  ValuePicker/SearchFilter.js:18:21
  ../node_modules/react-dom/cjs/react-dom.production.min.js:166:136
  ../node_modules/react-dom/cjs/react-dom.production.min.js:289:385
  ../node_modules/react-dom/cjs/react-dom.production.min.js:279:391
  ../node_modules/react-dom/cjs/react-dom.production.min.js:279:322
  ../node_modules/react-dom/cjs/react-dom.production.min.js:279:179
  ../node_modules/react-dom/cjs/react-dom.production.min.js:270:87
  ../node_modules/react-dom/cjs/react-dom.production.min.js:272:299
  ../node_modules/react-dom/cjs/react-dom.production.min.js:127:104
  ../node_modules/react-dom/cjs/react-dom.production.min.js:266:268
nickcoutsos commented 1 year ago

Hmm, yeah, that would be a shortcoming in how I'm resolving the parameters for custom behaviors. The parser is doing this as generically as possible but hold-tap is a weird one since it requires 2 parameters no matter what, so I'll need to recognize that and treat the leftover parameters as "placeholders" to not trip up the UI.

In the meantime, does the keymap save correctly if you don't attempt to change that second value? Showing as missing isn't ideal, especially because it isn't missing, but generally when the editor doesn't know how to handle a value it should ignore it. As long as you don't try to alter that value it should persist when saving other changes.

I've started a discussion to share some high-level notes about the ongoing behavior editing work: https://github.com/nickcoutsos/keymap-editor/discussions/78

z5huang commented 1 year ago

Yes it saves the keymap correctly (I made a trivial change in another key and then saved). Thanks for sharing your thoughts (and thanks for the great work in the first place!)

nickcoutsos commented 1 year ago

Thanks for your support and kind words! I'll try and have a look at the param resolution stuff soon, there might be an easier short term fix so that it at least displays the 0 as a "raw" value.

nickcoutsos commented 1 year ago

Just pushed a change so that placeholders are treated like raw values. It's not a proper fix by any means, but this should avoid crashing. When the behavior stuff is off my plate I can update this to automatically insert the placeholder and not try to render it in the UI.

Have a look when you have a moment and see if it works alright.

z5huang commented 1 year ago

Thanks for the quick fix, can confirm it no longer crashes. I also tested changing the dummy 0 to 1 using the editor, and it saved the change as expected. I think this is robust enough for now. Thanks again!

I closed the issue if you don't mind