Vi mode indicators #369

elshize · 2020-04-19T19:38:26Z

My first take on #208

I personally agree with @AdminXVII that it would be nice to have a line cursor, maybe behind an option if there are support concerns. But I wasn't sure how to do that, so I started off with this.

I realize it's not finished yet (for one, I haven't included any unit test that work with the indicators) but I figured it would be nice to get some feedback to see if I'm going the right direction.

gwenn · 2020-04-20T18:05:33Z

I am not sure it is feasible but I would prefer a less intrusive change:

add a dedicated placeholder in your default (not highlighted) prompt. (You need this because the prompt size must not change after highlighting)
refactor highlight_prompt such as you have access to current editing mode. If possible, try to introduce an indirection (like PromptState) because recently a similar change was introduced (see https://github.com/kkawakam/rustyline/pull/338/files#diff-3f1033c6837e548c84459c1469e59a4bR38):

pub struct PromptState {
     pub default: bool,
     pub mode: InputMode,
 }

pub trait Highlighter {
...
    fn highlight_prompt<'b, 's: 'b, 'p: 'b>(
        &'s self,
        prompt: &'p str,
        state: PromptState,
    ) -> Cow<'b, str> {
...

reuse/keep the changes you already made to refresh the line when the mode changes.
implement a ViModeHighlighter either in a dedicated example or as a reusable component like MatchingBracketHighlighter by replacing the placeholder with indicator matching the current editing mode.

I may investigate further if I have time.
Thanks.

elshize · 2020-04-20T18:39:12Z

I see. I think this is quite doable. My only concern would be that it essentially would mean that all your indicators must be the same width. This could be a bit limiting in general (I see no reason why someone shouldn't be able to use - INSERT - and - COMMAND - if they want to). The solution could be (a) we don't care, that's all we allow, or (b) we don't care right now, but will reconsider later.

Regarding (b), is there a plan to allow for highlighting to have a different width or is it here to stay?

elshize · 2020-04-20T19:49:08Z

Is this one more like what you were thinking? (Probably want a separate example for Vi mode, but wanted to illustrate quickly the approach.)

gwenn · 2020-04-21T17:02:55Z

The limitation width(raw prompt) = width(highlighted prompt) is because the repaint/refresh phase/code is not clever nor fast.

gwenn · 2020-04-21T17:14:20Z

src/tty/mod.rs

@@ -39,6 +40,7 @@ pub trait Renderer {
        old_layout: &Layout,
        new_layout: &Layout,
        highlighter: Option<&dyn Highlighter>,
+        vi_mode: Option<InputMode>,


You can factorize code by passing a PromptState instead of Option<InputMode>.

gwenn

If I ignore dead code and unrelated changes, it looks good to me.
Maybe we should have a PrompState trait instead of a struct ?

elshize · 2020-04-21T19:48:49Z

About the trait, are you thinking of having different implementations for vi/emacs?

elshize · 2020-04-21T19:58:38Z

Also, I wasn't sure what your preference on example is, whether extract the common code or maybe use a more minimal example for Vi mode? Or maybe parameterize the default example with input mode?

gwenn · 2020-04-22T17:14:25Z

I would prefer you keep the default example untouched.

gwenn · 2020-04-22T17:14:43Z

Oops!

gwenn · 2020-04-22T17:20:44Z

About the trait, are you thinking of having different implementations for vi/emacs?

I am not sure but it may be easier to extend the trait in the future and we already have State as implementation.

gwenn · 2020-04-22T17:53:00Z

examples/vi.rs

@@ -0,0 +1,132 @@
+use std::borrow::Cow::{self, Borrowed, Owned};


You should have keep your example minimalist (like https://github.com/kkawakam/rustyline/blob/master/examples/read_password.rs)

gwenn · 2020-04-22T17:53:52Z

src/config.rs

@@ -2,7 +2,7 @@
 use std::default::Default;

 /// User preferences
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq)]


To be reverted.

👍 missed that.

gwenn · 2020-04-22T17:57:59Z

src/edit.rs

@@ -33,6 +34,7 @@ pub struct State<'out, 'prompt, H: Helper> {
    pub ctx: Context<'out>,   // Give access to history for `hinter`
    pub hint: Option<String>, // last hint displayed
    highlight_char: bool,     // `true` if a char has been highlighted
+    input_mode: Option<InputMode>,


We may avoid to duplicate input_mode if we move InputState in State ?
I will give a try and do it in a dedicated PR.

gwenn · 2020-04-22T18:00:49Z

src/history.rs

@@ -319,7 +319,7 @@ mod tests {
    #[test]
    fn add() {
        let config = Config::builder().history_ignore_space(true).build();
-        let mut history = History::with_config(config);
+        let mut history = History::with_config(config.clone());


To be reverted.

gwenn · 2020-04-22T18:04:39Z

src/lib.rs

@@ -784,7 +784,7 @@ impl<H: Helper> Editor<H> {
        );
        Self {
            term,
-            history: History::with_config(config),
+            history: History::with_config(config.clone()),


To be reverted.

gwenn

Thanks for your patience.

elshize · 2020-04-22T18:29:51Z

Thanks for the feedback, I'll try to push some changes soon.

elshize · 2020-04-23T00:42:08Z

The problem with using State as implementation of PromptState is that we'd have to pass it to refresh by reference while the writer that is part of the state is passed by a mutable reference at the same time. So I'm not sure if it's worth it at this point. I could extract default_prompt from layout and put into an object together with input_mode inside a PromptState implementation but I don't think it's worth the hassle right now.

What do you think?

gwenn · 2020-04-23T17:28:54Z

We should make PromptState compatible with #372.

elshize · 2020-04-23T17:50:04Z

Ok, so extracting Prompt helps with the issue I raised (Prompt could implement PromptState trait and be passed) but that commit also introduces PromptInfo, which has eve more data in it. So if you want it to be compatible, I think the easiest way would be to add Option<InputMode> to PromptInfo.

Either way, do you want me to wait for that other PR to be merged and work on top of that? It might be easier for everyone.

gwenn · 2020-04-26T06:59:03Z

I am afraid there will be no indicator when colors are disabled (ColorMode::Disabled) or not supported (Windows < 10).

gwenn · 2020-05-10T08:33:40Z

There is a draft here to support dynamic prompt even if there is no highlighting.
First, I will try to make it build/work with current master branch: only static prompt.
Then I will try to integrate your changes.

elshize · 2020-05-18T14:47:44Z

Thanks for the update, let me know if I can help with anything.

rustyline doesn't currently support changing the prompt while in the core readline loop. There are a number of open PRs and issues for this functionality, but all of them appear to be stalled for more than a year. Looking at kkawakam#696 and 4ec26e8, the traditional appoach to this is to provide a reference to a trait object (`Prompt` or `ToString`), but with that appoach there's no way to cause the prompt to be redrawn for a change without user input. This means for these appoaches the prompt could change without being displayed to the user. There's an existing mechanism to allow another async task/thread to push input into the core readline loop, the `ExternalPrinter`. In this commit, I expand `ExternalPrinter` to add `set_prompt()`. With various plumbing, this function results in `wait_for_input` to return `Cmd::SetPrompt(String)`. One of key change here is `State.prompt` changes from `&str` to `String`. There is a performance hit here from the copy, but rustyline would need to prompt and receive input hundreds of times per second for the copy to have a noticable performance inpact. Added examples/dynamic_prompt.rs to demonstrate the functionality. Closes kkawakam#417 Related kkawakam#208, kkawakam#372, kkawakam#369, kkawakam#417, kkawakam#598, kkawakam#696

Vi mode indicators

5b369f3

elshize added 2 commits April 20, 2020 14:42

Merge branch 'master' into vi-indicators

9c0ebb7

Use highlighter to print indicators

2234f78

Fix windows build

783d1c9

gwenn reviewed Apr 21, 2020

View reviewed changes

elshize added 2 commits April 21, 2020 15:50

Use PromptState in refresh signature

a73f517

Separate Vi example from Emacs example

5950b7f

gwenn closed this Apr 22, 2020

gwenn reopened this Apr 22, 2020

gwenn reviewed Apr 22, 2020

View reviewed changes

elshize added 3 commits April 22, 2020 20:34

Make Config Copy again

c0b8c7a

Simplify Vi example

23e05df

Merge remote-tracking branch 'upstream/master' into vi-indicators

3beb69a

gwenn mentioned this pull request Jul 18, 2020

Changing the prompt string dynamically #417

Open

gwenn mentioned this pull request Apr 11, 2023

Generic Prompt Type #696

Open

dmlary mentioned this pull request Apr 6, 2024

Add dynamic prompt support via ExternalPrinter #772

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vi mode indicators #369

Vi mode indicators #369

elshize commented Apr 19, 2020

gwenn commented Apr 20, 2020 •

edited

Loading

elshize commented Apr 20, 2020

elshize commented Apr 20, 2020

gwenn commented Apr 21, 2020

gwenn Apr 21, 2020

gwenn left a comment •

edited

Loading

elshize commented Apr 21, 2020

elshize commented Apr 21, 2020

gwenn commented Apr 22, 2020

gwenn commented Apr 22, 2020

gwenn commented Apr 22, 2020 •

edited

Loading

gwenn Apr 22, 2020

gwenn Apr 22, 2020

elshize Apr 22, 2020

gwenn Apr 22, 2020 •

edited

Loading

gwenn Apr 22, 2020

gwenn Apr 22, 2020

gwenn left a comment

elshize commented Apr 22, 2020

elshize commented Apr 23, 2020

gwenn commented Apr 23, 2020

elshize commented Apr 23, 2020

gwenn commented Apr 26, 2020

gwenn commented May 10, 2020

elshize commented May 18, 2020

		@@ -0,0 +1,132 @@
		use std::borrow::Cow::{self, Borrowed, Owned};

Vi mode indicators #369

Are you sure you want to change the base?

Vi mode indicators #369

Conversation

elshize commented Apr 19, 2020

gwenn commented Apr 20, 2020 • edited Loading

elshize commented Apr 20, 2020

elshize commented Apr 20, 2020

gwenn commented Apr 21, 2020

gwenn Apr 21, 2020

Choose a reason for hiding this comment

gwenn left a comment • edited Loading

Choose a reason for hiding this comment

elshize commented Apr 21, 2020

elshize commented Apr 21, 2020

gwenn commented Apr 22, 2020

gwenn commented Apr 22, 2020

gwenn commented Apr 22, 2020 • edited Loading

gwenn Apr 22, 2020

Choose a reason for hiding this comment

gwenn Apr 22, 2020

Choose a reason for hiding this comment

elshize Apr 22, 2020

Choose a reason for hiding this comment

gwenn Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

gwenn Apr 22, 2020

Choose a reason for hiding this comment

gwenn Apr 22, 2020

Choose a reason for hiding this comment

gwenn left a comment

Choose a reason for hiding this comment

elshize commented Apr 22, 2020

elshize commented Apr 23, 2020

gwenn commented Apr 23, 2020

elshize commented Apr 23, 2020

gwenn commented Apr 26, 2020

gwenn commented May 10, 2020

elshize commented May 18, 2020

gwenn commented Apr 20, 2020 •

edited

Loading

gwenn left a comment •

edited

Loading

gwenn commented Apr 22, 2020 •

edited

Loading

gwenn Apr 22, 2020 •

edited

Loading