Conditional Bind Sequences #293
Conversation
Make possible to return different command depending on input state.
| /// Takes the currently edited `line` with the cursor `pos`ition and | ||
| /// returns the command to be performed or `None` to perform the default | ||
| /// one. | ||
| pub action: fn(line: &str, pos: usize, ctx: &ActionContext) -> Option<Cmd>, |
There was a problem hiding this comment.
I feel like the line and pos should be part of the context.
An fn is pretty restrictive because it doesn't allow adding any of your own context. Having something like
Arc<Box<dyn Fn(&ActionContext) -> Option<Cmd>>>
would be much more flexible.
Also I think it would be better to not make this public to allow non-breaking changes in the future.
There was a problem hiding this comment.
Most of the time, you just need to check line and/or pos. So if you can access them directly, it seems better/easier.
And could you provide a case where an action is not stateless ?
There was a problem hiding this comment.
For example I've been playing around with having a key bindings to insert bits of text with skim. The bits of text can be dependent on the rest of the program.
There was a problem hiding this comment.
We have just merged Validation where anyone would use line for sure, but it only has ValidationContext in the signature. Well, the reason for validator is probably because line is mutable there. But I think having similar signature is nice anyway.
| } | ||
| } | ||
|
|
||
| pub struct ActionContext<'r> { |
There was a problem hiding this comment.
You've made the struct public but it's isn't exposed anywhere so no documentation gets generated for it.
| @@ -53,7 +53,9 @@ use crate::edit::State; | |||
| use crate::highlight::Highlighter; | |||
| use crate::hint::Hinter; | |||
| use crate::history::{Direction, History}; | |||
| pub use crate::keymap::{Anchor, At, CharSearch, Cmd, Movement, RepeatCount, Word}; | |||
| pub use crate::keymap::{ | |||
| Action, Anchor, At, CharSearch, Cmd, InputMode, Movement, RepeatCount, Word, | |||
There was a problem hiding this comment.
| Action, Anchor, At, CharSearch, Cmd, InputMode, Movement, RepeatCount, Word, | |
| Action, ActionContext, Anchor, At, CharSearch, Cmd, InputMode, Movement, RepeatCount, Word, |
| /// Custom dynamic action | ||
| #[derive(Clone)] | ||
| pub struct Action { | ||
| pub name: String, |
There was a problem hiding this comment.
Can you say what this name is used for? It's not clear to me.
There was a problem hiding this comment.
It is used only for tracing / debugging.
|
@cchalmers We still need to support composite command. For example, clear whole line and interrupt. And last, we need to fix/move this |
|
@cchalmers I mean that if your custom action replaces some text, we cannot just do a pattern matching anymore (Cmd::Custom doesn't match Cmd::Replace)... |
|
|
||
| if let Some(cmd) = self.custom_binding(wrt, key) { | ||
| return Ok(if cmd.is_repeatable() { | ||
| cmd.redo(Some(n), wrt) |
There was a problem hiding this comment.
This replaces number returned from the custom action. I.e. I can't return Move(BackwardChar(10).
There was a problem hiding this comment.
Right. This should not be done for Action, only for simple bindings.
| /// Takes the currently edited `line` with the cursor `pos`ition and | ||
| /// returns the command to be performed or `None` to perform the default | ||
| /// one. | ||
| pub action: fn(line: &str, pos: usize, ctx: &ActionContext) -> Option<Cmd>, |
There was a problem hiding this comment.
I don't understand why Action::action contains a function. Rather than Action being a trait.
There was a problem hiding this comment.
Mainly because Action is Sized.
There was a problem hiding this comment.
Box<dyn Action> is also sized. And I don't think it can be a performance issue here.
Most commands are factored out to the function, except: 1. Commands that need extra input 2. Suspend, which needs access to the terminal This should be enough to implement `ContextValidator::invoke`. Also this commit brings `CompleteHint`, `SelfInsert`, `Insert` commands to the `match` over `Cmd` instead of keeping them as separate `if` statements as latter doesn't seem to change their behavior. This is needed to implement kkawakam#453, and kkawakam#293 (although, latter might need input-accepting commands too or another approach).
Most commands are factored out to the function, except: 1. Commands that need extra input 2. Suspend, which needs access to the terminal This should be enough to implement `ContextValidator::invoke`. Also this commit brings `CompleteHint`, `SelfInsert`, `Insert` commands to the `match` over `Cmd` instead of keeping them as separate `if` statements as latter doesn't seem to change their behavior. This is needed to implement kkawakam#453, and kkawakam#293 (although, latter might need input-accepting commands too or another approach).
Make possible to return different command depending on input state.
Try to fix #269.
No breaking change but a
Cmdenum withCustom(Action)entry which can return aCmdlooks bad...