Skip to content
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

Conditional Bind Sequences #293

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f78700f
Conditional Bind Sequences
gwenn Oct 20, 2019
e9377fe
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Oct 27, 2019
b9e654f
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Jan 1, 2020
1a58df5
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Jan 4, 2020
0d41a9b
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Jan 5, 2020
7f3cbb4
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Jan 5, 2020
bbe6314
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Jan 9, 2020
e880088
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Mar 7, 2020
2a52ede
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Apr 5, 2020
63581d3
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Apr 26, 2020
0053242
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn May 16, 2020
31cf971
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn May 18, 2020
a515d35
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn May 21, 2020
5439a15
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn May 25, 2020
86c2da5
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Dec 6, 2020
45df451
Fix missing doc warnings
gwenn Dec 6, 2020
7e36c69
Merge remote-tracking branch 'upstream/master' into conditional_binding
gwenn Jan 13, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/edit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,14 @@ impl<'out, 'prompt, H: Helper> Refresher for State<'out, 'prompt, H> {
fn has_hint(&self) -> bool {
self.hint.is_some()
}

fn line(&self) -> &str {
self.line.as_str()
}

fn pos(&self) -> usize {
self.line.pos()
}
}

impl<'out, 'prompt, H: Helper> fmt::Debug for State<'out, 'prompt, H> {
Expand Down
133 changes: 98 additions & 35 deletions src/keymap.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Bindings from keys to command for Emacs and Vi modes
use std::collections::HashMap;
use std::fmt;
use std::sync::{Arc, RwLock};

use log::debug;
Expand All @@ -13,6 +14,58 @@ use crate::tty::{RawReader, Term, Terminal};
/// The number of times one command should be repeated.
pub type RepeatCount = usize;

/// Custom dynamic action
#[derive(Clone)]
pub struct Action {
pub name: String,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say what this name is used for? It's not clear to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used only for tracing / debugging.

/// 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>,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why Action::action contains a function. Rather than Action being a trait.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly because Action is Sized.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Box<dyn Action> is also sized. And I don't think it can be a performance issue here.

}

impl std::fmt::Debug for Action {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Action").field("name", &self.name).finish()
}
}

impl PartialEq for Action {
fn eq(&self, other: &Self) -> bool {
self.name == other.name
}
}

pub struct ActionContext<'r> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've made the struct public but it's isn't exposed anywhere so no documentation gets generated for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad.

mode: EditMode,
input_mode: InputMode,
wrt: &'r dyn Refresher,
}

impl<'r> ActionContext<'r> {
fn new(is: &InputState, wrt: &'r dyn Refresher) -> Self {
ActionContext {
mode: is.mode,
input_mode: is.input_mode,
wrt,
}
}

pub fn mode(&self) -> EditMode {
self.mode
}

/// vi only
pub fn input_mode(&self) -> InputMode {
self.input_mode
}

/// Returns `true` if there is a hint displayed.
pub fn has_hint(&self) -> bool {
self.wrt.has_hint()
}
}

/// Commands
// #[non_exhaustive]
#[derive(Debug, Clone, PartialEq)]
Expand Down Expand Up @@ -41,6 +94,8 @@ pub enum Cmd {
EndOfHistory,
/// forward-search-history
ForwardSearchHistory,
/// Custom dynamic action
Custom(Action),
/// history-search-backward
HistorySearchBackward,
/// history-search-forward
Expand Down Expand Up @@ -267,8 +322,8 @@ impl Movement {
}
}

#[derive(PartialEq)]
enum InputMode {
#[derive(Clone, Copy, PartialEq)]
pub enum InputMode {
/// Vi Command/Alternate
Command,
/// Insert/Input mode
Expand Down Expand Up @@ -314,6 +369,10 @@ pub trait Refresher {
fn is_cursor_at_end(&self) -> bool;
/// Returns `true` if there is a hint displayed.
fn has_hint(&self) -> bool;
/// currently edited line
fn line(&self) -> &str;
/// Current cursor position (byte position)
fn pos(&self) -> usize;
}

impl InputState {
Expand Down Expand Up @@ -389,6 +448,21 @@ impl InputState {
}
}

fn custom_binding(&self, wrt: &mut dyn Refresher, key: KeyPress) -> Option<Cmd> {
let bindings = self.custom_bindings.read().unwrap();
let cmd = bindings.get(&key);
if let Some(cmd) = cmd {
debug!(target: "rustyline", "Custom command: {:?}", cmd);
if let Cmd::Custom(action) = cmd {
let ctx = ActionContext::new(self, wrt);
return (action.action)(wrt.line(), wrt.pos(), &ctx);
} else {
return Some(cmd.clone());
}
}
None
}

fn emacs<R: RawReader>(
&mut self,
rdr: &mut R,
Expand All @@ -402,16 +476,13 @@ impl InputState {
key = self.emacs_digit_argument(rdr, wrt, digit)?;
}
let (n, positive) = self.emacs_num_args(); // consume them in all cases
{
let bindings = self.custom_bindings.read().unwrap();
if let Some(cmd) = bindings.get(&key) {
debug!(target: "rustyline", "Custom command: {:?}", cmd);
return Ok(if cmd.is_repeatable() {
cmd.redo(Some(n), wrt)
} else {
cmd.clone()
});
}

if let Some(cmd) = self.custom_binding(wrt, key) {
return Ok(if cmd.is_repeatable() {
cmd.redo(Some(n), wrt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces number returned from the custom action. I.e. I can't return Move(BackwardChar(10).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This should not be done for Action, only for simple bindings.

} else {
cmd
});
}
let cmd = match key {
KeyPress::Char(c) => {
Expand Down Expand Up @@ -547,20 +618,16 @@ impl InputState {
}
let no_num_args = self.num_args == 0;
let n = self.vi_num_args(); // consume them in all cases
{
let bindings = self.custom_bindings.read().unwrap();
if let Some(cmd) = bindings.get(&key) {
debug!(target: "rustyline", "Custom command: {:?}", cmd);
return Ok(if cmd.is_repeatable() {
if no_num_args {
cmd.redo(None, wrt)
} else {
cmd.redo(Some(n), wrt)
}
if let Some(cmd) = self.custom_binding(wrt, key) {
return Ok(if cmd.is_repeatable() {
if no_num_args {
cmd.redo(None, wrt)
} else {
cmd.clone()
});
}
cmd.redo(Some(n), wrt)
}
} else {
cmd
});
}
let cmd = match key {
KeyPress::Char('$') |
Expand Down Expand Up @@ -715,16 +782,12 @@ impl InputState {

fn vi_insert<R: RawReader>(&mut self, rdr: &mut R, wrt: &mut dyn Refresher) -> Result<Cmd> {
let key = rdr.next_key(false)?;
{
let bindings = self.custom_bindings.read().unwrap();
if let Some(cmd) = bindings.get(&key) {
debug!(target: "rustyline", "Custom command: {:?}", cmd);
return Ok(if cmd.is_repeatable() {
cmd.redo(None, wrt)
} else {
cmd.clone()
});
}
if let Some(cmd) = self.custom_binding(wrt, key) {
return Ok(if cmd.is_repeatable() {
cmd.redo(None, wrt)
} else {
cmd
});
}
let cmd = match key {
KeyPress::Char(c) => {
Expand Down
4 changes: 3 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Action, Anchor, At, CharSearch, Cmd, InputMode, Movement, RepeatCount, Word,
Action, ActionContext, Anchor, At, CharSearch, Cmd, InputMode, Movement, RepeatCount, Word,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

};
use crate::keymap::{InputState, Refresher};
pub use crate::keys::KeyPress;
use crate::kill_ring::{KillRing, Mode};
Expand Down