-
-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(leaves): Add module for defining user customizable keybindings #22
base: main
Are you sure you want to change the base?
Conversation
Hi @charleslambert, thanks for opening the PR! ✨ Could we refactor this to not use a functor? I'd like the API to be as small/simple as possible. I was thinking maybe we could have a type like module Key_bindings : sig
type 'model t
type 'model binding
= struct
type t = 'model binding list
type 'model binding = key * 'model -> 'model
let on key fn = (key, fn)
let make bs = bs
let update key model t =
match event with
| KeyDown k ->
let fns = find_fns k t in
run_fns fns model
| _ -> model
end
let bindings = Key_bindings.make [
on Key.Enter enter_fullscreen;
on Key.Down move_left
] What do you think? |
I agree removing the functor is probably a good idea. I would still argue to keep the idea of a msg variant rather than directly binding to functions. It makes it easier for a component author to make changes without breaking a consumer of that component. It also gives direct documentation of the possible operations on a component. I will refactor to remove the functor and add a few convenience functions to make the api easier to consume. |
Hey @leostera, I removed the functor and updated the api to hopefully be a little better. What do you think? |
Oh i see what you mean now! Love the update looking super clean 🤩 We also have the ability to send custom events which could work even more uniformly since you'd just have to add: | Event.KeyDown _ as ev -> KeyMap.update m.keys ev and this would be enough for they KeyMap to update itself and send a custom event to your app that you can match in your update loop: | Event.Custom My_event -> (* do thing *) Custom events are just typed process messages so you defined them with i'll have another look in a minute when i get to a computer but overall is looking like we could merge this. thanks again for contributing ✨ |
CursorUp; | ||
] | ||
|
||
let () = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost forgot, all the examples should be working Minttea app to showcase how this component fits into an application.
This also helps you see if the API really figs the way you'd use it in an app.
In this case we can also refactor an existing example that uses several keys (like the views example) so use one or more key maps.
This should also expose the component to the use cases of:
-
how do you deal with multiple key maps in a single app?
-
how do we render key maps for users to see that they are available?
We don't have to have those answers in this PR but they would definitely inform the design you go for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so in this case should I remove the key_map example and refactor say the views example to use the key map feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you :) either making this one a complete tiny app, or refactoring one of the others.
[ Minttea.Event.Up; Minttea.Event.Key "k" ] | ||
CursorUp; | ||
on | ||
~help:{ key = "down"; desc = "↓/j" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about deriving these from the events and using the ~help
for writing more about what the keymap is supposed to do?
For example:
let keymap = Key_map.make [
on Event.[Up; Key "k"] CursorUp ~help:"moves the cursor up";
on Event.[Key "q"] Quit ~help:"quit the app";
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how with this information we can put together a bindings help view:
↑/k: moves the cursor up
q: quit the app
After giving this some more thought I'm not sure what the custom events buy us that the function approach wouldn't. 🤔 It seems to me that not knowing what the model is makes it hard to map from Minttea events to actual behaviors in an app. Maybe I'm missing something here tho! For example, say you're building a vim-style mini editor. You have a rough keymap of With the function keymap I can write this: type model = {
document : string;
cursor : Cursor.t;
mode : [ `insert | `normal ];
keys : Key_map.t;
}
let exit_mode key m =
match (key, m.mode) with
| Event.Escape, `insert -> ({ m with mode = `normal }, Command.Noop)
| _ -> (m, Command.Noop)
let handle_input key m =
match (m.mode, key) with
| `insert, Event.Key char -> ({ m with document = document ^ char }, Command.Noop)
| `normal, Event.Key ("j" | "k" | "l" | "h") -> move_cursor key m
let keymap =
Key_map.
[
on [ Escape ] exit_mode;
on [ Up; Down; Left; Right ] move_cursor;
on [ AnyKey ] handle_input;
]
and then my update function becomes a tidy call to let update ev m =
match ev with
| Event.KeyDown _ -> Key_map.update m.keys ev m
| _ -> (* ... handle other events in my app *) This approach makes one (i think) reasonable assumption that your keymaps will be dependant on the model – it gives you a lot of flexibility and its rather easy to think about: you associate a little update function to a set of keys. When I try to do the same thing with the custom event approach it looked like this: (* same model and other functions from the last example *)
type key_ev = | Exit_mode | Move_cursor | Handle_input
let keymap =
Key_map.
[
on [ Escape ] Exit_mode;
on [ Up; Down; Left; Right ] Move_cursor;
on [ AnyKey ] Handle_input;
]
let update ev m =
match ev with
| Event.KeyDown k ->
(match Key_map.find_match m.keys k with
| Some Exit_mode -> exit_mode k m
| Some Move_cursor -> move_cursor k m
| Some Handle_input -> handle_input k m
| None -> m)
| _ -> (* ... handle other events in my app *) Which seems to be repeating the dispatching code between the custom event ( Maybe I'm missing something, so feel free to let me know if there's something about the custom events approach that I can't see here. Do you see any drawbacks to the function approach? 🤔 |
Hi, just so you know, I am playing around with a toggleable help bar leave, and it requires some representation of key binding, so I am waiting on this to be completed before I submit the leave. |
Hey @leostera, sorry for dropping off the earth. I am getting married in about a month or so and it has eaten most of my time in last couple of months. I think I see the misunderstanding we are hitting here. The way I see it there are two types of keybindings, there are bindings you use in your application code and bindings made by components you use (3rd party). In the case of your application code it totally makes sense to use a function based style. The solution I have been working is more useful for components that don't want to expose their internals to the application layer and don't want to handle key presses directly. For example: Here is a code example of how I thought this would be used module ExampleComponent = struct
type msg = Upcase | Reset
type model = { keymap : msg Key_map.t; value : string }
let update event model =
match event with
| Event.KeyDown (key, _mod) -> (
match Key_map.find_match key model.keymap with
| Some Upcase ->
( { model with value = String.uppercase_ascii model.value },
Command.Noop )
| Some Reset -> ({ model with value = "hello world" }, Command.Noop)
| _ -> (model, Command.Noop))
| _ -> (model, Command.Noop)
let view model = model.value
end
type model = { example_component_model : ExampleComponent.model }
let initial_model =
({
example_component_model =
{
value = "hello world";
keymap =
(let open Key_map in
[
on
~help:{ key = "u"; desc = "upcase" }
[ Minttea.Event.Key "u" ] ExampleComponent.Upcase;
on
~help:{ key = "r"; desc = "reset text" }
[ Minttea.Event.Key "r" ] ExampleComponent.Reset;
]);
};
}
: model)
let init _ = Command.Noop
let update event model =
let ec_model, _ec_cmd =
ExampleComponent.update event model.example_component_model
in
(* Handle some other logic *)
({ example_component_model = ec_model }, Command.Noop)
let view model = ExampleComponent.view model.example_component_model
let app = Minttea.app ~init ~update ~view ()
let () = Minttea.start app ~initial_model Does that make more sense? |
Hi, @charleslambert I think you have a good point: having written a small component myself, I see the value of defining basic actions, but letting the user define the effective key presses. Here is my two cent proposal.
The usage for the user could look something like that: let keymap =
Key_map.
[
on [ Escape ] exit_mode; (* exit_mode is a callback *)
on [ Left; Right ] move_cursor;
on [ Up ] (action My_component.up); (* action is a helper function in Key_map that emit the custom event *)
on [ Down ] (action My_component.down);
on [ AnyKey ] handle_input;
] Compared to @leostera proposition, this one gives a straightforward way to configure component keymaps. |
Hey @Lattay, thank you for the input 😃. The only problem I see with that approach is that we would have to have all the mapped functions be imperative callbacks, as I don't think there would any way to generalize over their combined return types. |
I guess you would have the functions be |
So the current implementation of bindings looks like: type binding = {
keys : Minttea.Event.key list;
help : help option;
disabled : bool;
}
type 'a t = ('a * binding) list We could add a variant type to account for the different types of (actions, functions) type ('model, 'msg) binding_action =
| Fn of ('model -> 'model)
| Action of ('msg -> unit) The problem with this type though is that it can't be generic for multiple msg variants. In other words you could not bind to msgs for more than one component in single keymap. It also requires that Action types be callbacks, which I don't think is ideal. |
This method currently has the downside of not having any way to require that all bindings exist for the given variant type.