Issue #9, Implement Keyboard mgmt engine

[gurobokum]

Pull Request Description

Implementation of #9 ticket

Important Notes

• Closures uses forget() and aren't bounded to instance lifetime because of the strange issue with browser (browser events aren't captured if forget isn't used, but works in test environment). Hence removing event listeners isn't happen on KeyboardEngine drop
• Tests aren't developed correctly cause requires understanding Futures in wasm, will do in next PR

Checklist

Please include the following checklist in your PR:

• The documentation has been updated if necessary.
• All code conforms to the Rust, Scala, Java or Haskell style guides as appropriate.
• All code has been tested where possible.

[wdanilo]

Using combination as String is not the best idea here as well. We need some custom parser, which will always be slow and error-prone. Why not keyboard_engine.capture([key::Control, key::A], ...) instead? :)

It could be easily changed to bitfield mask inside of the function!

[gurobokum]

I've reworked the section, but would like to note that keyCode is deprecated
KeyboardEventInit section

In practice, keyCode and charCode are inconsistent across platforms and even the same implementation on different operating systems or using different localizations. This specification does not define values for either keyCode or charCode, or behavior for charCode. In conforming UI Events implementations, content authors can instead use key and code.

but key and code are strings

[gurobokum]

Hello @wdanilo , thanks for such detailed review!
During the current week I will provide the changes

[wdanilo]

This logic is a responsibility of some kind of callback manager, not capturing function!

[gurobokum]

Why not? I'm capturing here the key press, and callback management is a part of implementation

[wdanilo]

isn't it leaking memory here?

[gurobokum]

It's, the main idea, that if I would like to avoid to use .forget() I must to make my KeyboardEngine instance static for having ownership for closures. I did few attempts to implement it, but all of them were looked ugly, so I stayed with forget()

[wdanilo]

Here is a longer comment about why I think its sub-optimal and some ideas to improve it, but before we make it better, let's have a chat if these ideas are correct :)

So, in the future we want to display in GUI something like you know from other editors - a table telling - if you press a + b you get action foo, and if you press g then you get action bar and action baz. You know, something like:

Binding	Action
cmd + backspace	editor:clear_line
cmd + arrow left	editor:jump_to_line_start

So we will keep somewhere a bidirectional map of String <-> combination (string is the action).

This mechanism is very gui-specific, sometimes you want to handle it in different way, so I'm not sure we would like to keep such bidirectional map as part of this PR / core design. So here are 2 ways - either we keep it as the core design or not. Let me first focus on the later case:

1. In such a case we can create another library around this library which defines bidirectional map keeping String <-> CallbackID information (whatever CallbackID is).
2. The client always needs to keep track of all callbacks that were registered (by keeping their IDs).
3. There should be an easy way to clean up the callback. I'm not sold to the idea to manually calling uncapture, as if you forgot to do it, you lose the information about the id and you have no way to get it easily back - you are leaking memory and the best way to revert from this stage is to reset all callbacks, which is kind of ugly.
4. So in such a case we could redefine the function to fn captur(combination: ..., callback: F) -> CallbackHandle. The CallbackHandle would be a special guard which when dropped (when it goes out of scope) will update the callback map. In such scenario you owuld never want to use uncapture manually, cause you just need to drop CallbackHandle and thats it. Then in the high-level API we can just keep bimap of String <-> CallbackHandle and if we want to uncapture something we can just thorw the CallbackHandle from the map and we are done - always safe and always without leaking memory.

---

The second approach would be to just embed the String <-> Actionmap here, so you coulduncapture` callbacks by their names. However, this limits the use cases of this library only to things which have names. In our GUI its always true, but designing libraries in less generic way without really benefits of it is not always the best method, so I feel the former approach is better.

What do u think about it ? :)

[gurobokum]

Hm, what do you think of such design

1. bindings uses

{
   U256: [ ActionTrait ]
}

2. So we need to implement ActionTrait for any type, for example for Callback and Action
3. For examle it could look like

{
    U256: [ Callback1, Callback2, ActionOpenEditor, ActionCloseEditor]
}

4. Action type contains its ID
5. GUI setup will look as 2 actions:
   1. Create Action

    let action = Action::new(open::editor)
   

   2. Add Action callbacks

    keyboard_engine.capture(combination, action)
   

6. We can create global singleton ActionEngine with predefined actions and implement getting action by its String name / ID

     action_engine.bind(ID, combination)
   

   That does

     let action = getFromActionRegistry(ID);
     if !action {
       action = createDumpActionAndAddToRegistry(ID)
     }
     keyboard_engine.capture(combination, action)
   

   and provide

      action.get()
      action.list()
   

[wdanilo]

If I understand correctly, your solution assumes the existence of some kind of a global counter, right? So every time you create an action, you get a new id? If so, I don't like it because of the following reasons:

• Global counters are always bad. It's hard to use them in a multithreaded / multitenancy environment.
• The IDs are used only to differentiate actions in the action map right? If so, they are not part of the action per se. They are used only by their management layer, so logically they belong there.

Does it make sense? If so, I think I'd still go with the guard design here. :)

[gurobokum]

I think you understood me not 100% clear
From your first post I like more the first approach - create another library around this library which defines bidirectional map

the existence of some kind of a global counter, right

Some instance, is it global or not depends on the usage and implementation. The instance contains all registered actions and their ids. In fact we need to register actions somewhere

So every time you create an action, you get a new id?

Why not? In this case ID can be the string as you mentioned in String <-> CallbackID

The IDs are used only to differentiate actions in the action map right?

Yes, I mean that case

They are used only by their management layer, so logically they belong there

Agreed. I'm just wondering about your described feature. IMHO it would be better not to merge Actions with KeyboardEngine

What do you think?

[wdanilo]

@JIoJIaJIu Oh damn, I haven't seen your reply, sorry for that. I was sure you will ping me on Discord when you progress with this issue and I just realized you replied here :(

So, I like the approach to create another library around this library which defines bidirectional map as well. Let's stay with it.

Why not? In this case ID can be the string as you mentioned ...
Sure! I was rather asking if I understand that correctly. There is just one problem with ID: you need to unregister the callbacks manually. If you drop the ID you have a problem.

Guro, take a look at a callback engine we just included in our system. It uses CallbackHandle as a result type and unregisters callbacks lazily (when firing the event). A very similar version could be created to unregister callbacks immediately when handle gets dropped. I'd love to know your thoughts about it vs your proposition above: https://github.com/luna/basegl/blob/wip/wd/buffer-management/lib/core/src/control/callback.rs

[wdanilo]

@JIoJIaJIu we used a lot of ideas from your implementation in the frp engine we have developed. It has now keyboard bindings which base on the same assumptions that you have developed. Thank you for this contribution. I will close this PR as it will not be merged in this form, but I wanted to let you know that actually it was very helpful for us. To learn more, take a look here:
https://github.com/luna/ide/tree/master/lib/frp

[gurobokum]

@wdanilo sure, thank you
make me know if you need any other contributions from my side, I wasn't able to contact you in the discord