Initial implementation of frontend-configurable command runner #663
Conversation
|
Related to #584 |
There was a problem hiding this comment.
Thank you very much for this. It looks like this is going in the right direction! Before I take a more detailed look, I have two high-level comments. In general, the more code we can move from numbat-cli towards numbat, the better. It doesn't look like we made a lot of progress on this front, given that main.rs is longer than it was before.
It might make sense to also work on the WASM frontend, to see how it all plays out.
The `CommandRunner` is now the entry point to commands (no need for eg `SourcelessCommandParser`) `SessionHistory` is now owned by `CommandRunner`
Moved most logic into the CommandRunner itself As @sharkdp suggested, this helped to remove some duplicated checks of enabled-ness
Now it's perfectly testable and isn't a massive pile of spaghetti
|
Making a note here to not forget to also merge the |
numbat/src/command.rs
Outdated
| pub fn enable_print_markup(mut self, action: fn(&Markup)) -> Self { | ||
| self.print_markup = Some(action); | ||
| self | ||
| } |
There was a problem hiding this comment.
So this basically enables list/info/help and some other things. Maybe we could call this something like
| pub fn enable_print_markup(mut self, action: fn(&Markup)) -> Self { | |
| self.print_markup = Some(action); | |
| self | |
| } | |
| pub fn print_markup_using(mut self, action: fn(&Markup)) -> Self { | |
| self.print_markup = Some(action); | |
| self | |
| } |
or just
| pub fn enable_print_markup(mut self, action: fn(&Markup)) -> Self { | |
| self.print_markup = Some(action); | |
| self | |
| } | |
| pub fn print_using(mut self, action: fn(&Markup)) -> Self { | |
| self.print_markup = Some(action); | |
| self | |
| } |
There was a problem hiding this comment.
How about print_with?
There was a problem hiding this comment.
This is fantastic, thank you very much!
I have added a few minor review comments, and I have one remaining question:
The web frontend has a reset command:
numbat/numbat-wasm/www/index.js
Lines 44 to 49 in 77decee
Do you think we can implement this using the current architecture?
We basically need mutable access to the Context to reset it to some (frontend-configurable) default state. And we currently also clear the screen from the reset command. I guess that could be done frontend-independently. If reset is exectuted, we could check if clear_fn is set, and just run it in addition.
Making a note here to not forget to also merge the
copy-to-clipboardbranch into this one, as this currently does not include that command.
Can we maybe do this in a second step after this has been merged?
…o command-runner
Also added `Default` implementation for `CommandRunner`
|
Pushed an initial implementation of |
|
Thank you for the updates! |
No description provided.