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

Implement optional rectangular prompt support #372

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tailhook
Copy link
Contributor

This is a reimplementation of #338 which was reverted.

When Highligher::has_continuation_prompt returns true, rustyline
enables a special mode that passes the original prompt for every line to
the highlighter, and highlighter is free to change color or characters
of the prompt as long as it's length is the same (usual contract for the
highlighter)

Note:

  1. Wrapped lines are not prefixed by prompt (this may be fixed in future
    releases either by prepending them, or by collapsing and scrolling
    lines wider than terminal).
  2. Unlike Always use rectangular prompt for multi-line input #338 this PR doesn't change the meaning of cursor and end
    fields of the Layout structure.

@tailhook tailhook force-pushed the prompt4 branch 2 times, most recently from 50f9cc8 to 2ba6f2c Compare April 22, 2020 15:56
@bjorn3
Copy link
Contributor

bjorn3 commented Apr 22, 2020

I think it would be nice to see a screenshot of this.

@tailhook
Copy link
Contributor Author

Force-pushed windows fix

Also, just to make it clear: #350 is fixed in this PR (thanks for the test).

@tailhook
Copy link
Contributor Author

I think it would be nice to see a screenshot of this.

Here is how we use it in edgedb-cli:
rectangle-screenshot

@gwenn
Copy link
Collaborator

gwenn commented Apr 22, 2020

Thanks.
I will try to review your changes this week-end.

@tailhook tailhook force-pushed the prompt4 branch 2 times, most recently from 80fd9a8 to 3925456 Compare April 23, 2020 15:17
@gwenn gwenn mentioned this pull request Apr 23, 2020
@gwenn
Copy link
Collaborator

gwenn commented Apr 26, 2020

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

@tailhook
Copy link
Contributor Author

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

I'm okay with that. Do you think this is a problem?

  • I think color is often disabled when you redirect input from pipe, and disabling as much escape codes as possible in that case is nice.
  • Downgrading features on windows < 10 is fine too, if we already downgrade colors. I think users will miss colors much more than continuation prompt.

@tailhook
Copy link
Contributor Author

tailhook commented May 5, 2020

Just fixed two bugs. One of them is because I've untested on windows previously. Now works fine.

Anything else to update?

@tailhook tailhook mentioned this pull request May 7, 2020
@gwenn
Copy link
Collaborator

gwenn commented May 10, 2020

@tailhook
Copy link
Contributor Author

There is a draft here to support continuation prompt even if there is no highlighting.

Well, that was my original approach. But the problem is that you can't implement Prompt for &str (because latter is already a "fat" pointer). So it looses the simplicity of .readline("abc>") for simpler cases. Since I see continuation prompt as some kind downgradable enhancement of the input, similarly to colors, I am not concerned of the issue on non-colorized terminals. And since continuation prompt in my cases doesn't depend on initial prompt (except width), highlighter is a good place to put such enhancements.

Also, it may be worth mentioning, that zsh shell hints what is unclosed in continuation prompt (saying quote> if single quote ' wasn't closed and dquote> for double-quote ", etc), it depends on the actual input line. So it's a good idea to put that functionality in the Helper where you can reuse or cache pieces of completion/highlighting/hinting code to make that work faster. (Although, this isn't what I use in my use-cases yet).

@gwenn
Copy link
Collaborator

gwenn commented May 12, 2020

Well, that was my original approach. But the problem is that you can't implement Prompt for &str (because latter is already a "fat" pointer). So it looses the simplicity of .readline("abc>")

str implements Prompt and, &str should be fine with Prompt + ?Sized.

@tailhook
Copy link
Contributor Author

Well, that was my original approach. But the problem is that you can't implement Prompt for &str (because latter is already a "fat" pointer). So it looses the simplicity of .readline("abc>")

str implements Prompt and, &str should be fine with Prompt + ?Sized.

As far as I understand you can do readline<T: Prompt + ?Sized>(s: &T) but you can't do readline(s: &dyn Prompt) or Box<dyn Prompt> for that (playground). This means that all functions/structures dealing with prompt must have to be generics. Which is huge inconvenience and large codegen. Not sure if there any hacks to avoid that.

@gwenn
Copy link
Collaborator

gwenn commented May 13, 2020

This means that all functions/structures dealing with prompt must have to be generics. Which is huge inconvenience and large codegen. Not sure if there any hacks to avoid that.

We already need to keep track of 'prompt lifetime. And, at some point, the Prompt is reified into a Cow<'prompt,str>.
But maybe after polishing the draft, I will agree with you...

This is a reimplementation of kkawakam#338 which was reverted.

When `Highligher::has_continuation_prompt` returns true, rustyline
enables a special mode that passes the original prompt for every line to
the highlighter, and highlighter is free to change color or characters
of the prompt as long as it's length is the same (usual contract for the
highlighter)

Note:
1. Wrapped lines are not prefixed by prompt (this may be fixed in future
   releases either by prepending them, or by collapsing and scrolling
   lines wider than terminal).
2. Unlike kkawakam#338 this PR doesn't change the meaning of `cursor` and `end`
   fields of the `Layout` structure.
@naalit
Copy link

naalit commented Feb 1, 2021

Is this still being considered, or is there another way to get a continuation prompt? I'd like to implement a continuation prompt for my language's REPL.

@basile-henry
Copy link
Contributor

I would also be interested in seeing this PR getting merged at some point. It looks like it would work perfectly for my use case. Anything I can do to help? 😄

@@ -232,6 +248,88 @@ fn is_close_bracket(bracket: u8) -> bool {
memchr(bracket, CLOSES).is_some()
}

pub(crate) fn split_highlight<'a>(src: &'a str, offset: usize)
Copy link
Contributor

@thomcc thomcc May 4, 2021

Choose a reason for hiding this comment

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

So it took a while for me to figure out what this function does (src is highlighted, offset is the byte offset in the unhighlighted version of `src), but I think it's pretty bug-prone as-is, and would hit issues in a lot of cases it doesnt need to.

That said, I the problem is that this api makes the problem harder than it needs to be. I think instead of passing in the offset, if you pass in the old unhighlighted line (e.g. the unhighlighted version of src up until the point when it needs to be split), you can identify new stuff since it's whats in src but not its unhighlighted counterpart (and you can then see if it looks like a SGR sequence for the prompt support for the splitting).

This gets you "off the hook" for most of the escape sequences (note that if you use terminfo to source your escapes (as ideally you should), you definitely end up with non \x1b[ stuff. Given the current API, it feels like you'd need to use something like vte to parse it, but... I think that would not even really solve the problem very well.

Obviously, this could fail if the highlight function also changed non-highlighting parts of the text, but that isn't a deal breaker — you still have the line end as sort of a "synchronization point".

I'm going to try to see if I can make it work and paste a gist/patch for it. I think this PR is a lot closer now than the last version, but still hits issues for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, my other PR, improves the function to use offset in the highlighted source instead: https://github.com/kkawakam/rustyline/pull/378/files#diff-9df84bc451a5211110d67dd9b47c627ded197d37c6e04e38b5a8d0fe098257d3R252
(I can backport it here, if this will help).

Comparing with the original is probably sub par because it's harder to optimize. I.e. with current semantics we can use memchr to skip until first \x1b char. While this PR doesn't do that, I should be improved later.

Also we replace characters by highlighter for continuation prompt, so we need to create another mechanism for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, my other PR, improves the function to use offset in the highlighted source instead

Hm, that's not quite what I mean either, I'll get back to you later today on that though, I got sidetracked yesterday.

Comparing with the original is probably sub par because it's harder to optimize

These lines tend to be small enough that the performance here isn't visible, and the slowest thing by far is the IO anyway.

I'm way more concerned about correctness than performance here, mostly because the bugs in this tend to be pretty annoying, both as a user of the library, and as a user of the end application...

Also we replace characters by highlighter for continuation prompt, so we need to create another mechanism for that.

Yeah, that's what I meant when i said: "and you can then see if it looks like a SGR sequence for the prompt support for the splitting". (That said, I also think we shouldn't bother with that in cases where the continuation prompt is either empty or all whitespace, but that's neither here nor there, and is relatively easy to implement no matter what).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another use case for different chars in highlighted and non-highlighted text if someone wants to highlight some whitespace. You can do that with just background color, but it takes too much attention and to hard to remember which colors mean what. I.e. in my vim config have this:

set list listchars=tab:·⁖,trail:¶,nbsp:×

Which means tabs are replaced by ·⁖, trailing characters are replaced by and non-breaking spaces are replaced by ×. This shows characters in more subtle and more recognizable way. So I can see these things in the code, but they are saved as original characters.

While I haven't implemented that in any rustyline use case yet, I can imagine it to be useful. And this has to be done in highlighter but not in original text.

@schungx
Copy link

schungx commented Jan 30, 2022

I would also be interested in seeing this PR getting merged at some point. It looks like it would work perfectly for my use case. Anything I can do to help? 😄

Almost another year and still no continuation prompt... Any idea if this is going to show up soon?

dmlary added a commit to dmlary/rustyline that referenced this pull request Apr 6, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants