kkawakam / rustyline

Readline Implementation in Rust
https://crates.io/crates/rustyline/
MIT License
1.52k stars 175 forks source link

Is it possible for a Highlighter to re-render a line to undo cursor-based highlights? #726

Closed luketpeterson closed 12 months ago

luketpeterson commented 1 year ago

Consider the HistoryHinter example here: https://github.com/kkawakam/rustyline/blob/master/examples/example.rs which uses the MatchingBracketHighlighter.

If you type ( x ), it will bold-blue the first '(', as it should, when the closing ')' is typed. But if you then hit Enter, the '(' remains bold-blued on the now-complete line.

I have tried to fix this with my own implementation of a Highligher object, but neither the highlight_char nor the highlight Highlighter methods are called again on that original line. (They are called on a new line with a len=0, pos=0)

Is there a way (perhaps with the Validator?) to get the editor to re-render the line one last time? I see people may not want this as the default behavior if their tty speed is limited, but is there a way to make it happen?

Thank you.

luketpeterson commented 1 year ago

I was able to get the behavior I want by adding an additional "needs_final_refresh" method into Highlighter, that could cause this code path to be taken:

https://github.com/kkawakam/rustyline/blob/1c4091a0b20d68dda50c902abb723a6a74739ab9/src/edit.rs#L237C1-L238C1

Is there a way to do this in a more appropriate way? Or what design would you like to see in a PR?

Thanks again.

gwenn commented 12 months ago

We already refresh the line here: https://github.com/kkawakam/rustyline/blob/1c4091a0b20d68dda50c902abb723a6a74739ab9/src/command.rs#L28-L32 But indeed currently, we cannot check / know that the Highlighter needs a final refresh.

luketpeterson commented 12 months ago

If adding a method to the Highlighter trait is distasteful, perhaps highlight_char could be called on a line that has the final EOL character at the end?

gwenn commented 12 months ago

No worry, we can add a new method to Highlighter trait with a default implementation.

luketpeterson commented 12 months ago

I tried to tie up this logic nicely in a PR, and I added the below method to the Highlighter trait:

fn final_highlight(&self, line: &str) -> bool

But I realized there is a complication.

The internal-mutability pattern (ie Cell) used in the MatchingBracketHighlighter means that the highlight_char() method actually has the side-effect of updating the highlighter state.

I followed this pattern and used the final_highlight() method to similarly clear the cell. But then highlight_char() is called from refresh_line_with_msg() again, resetting the cell to the original value.

There are several solutions but I only see one that doesn't involve changing the client-facing API or contract in some way.

I raised a PR (https://github.com/kkawakam/rustyline/pull/727) with the "least risky" change, but it feel like a little hackish so I understand if you have a better idea for a fix.

Thank you.

gwenn commented 12 months ago

Thanks for your investigation. I am not sure but I guess that we need:

See draft #729

luketpeterson commented 12 months ago

See draft #729

Yeah. I think this is a better solution. I just didn't feel personally comfortable enough changing the behavior of the editor state and trait method definitions, but you have more knowledge and I like your solution better.

My one nit is that I was thinking of it as "final" instead of "forced" (or is_final since final is a reserved keyword). ie. with that bool set to false, highlight_char decides whether highlighter needs to run with a given cursor position. With it set to true, highlight_char decides if it needs to run to redraw the line without any cursor influence. But of course the choice of names is ultimately up to you, and I am happy either way.

gwenn commented 12 months ago

In fact, this is not necessarily the final refresh because depending on Validator (for example if the statement is incomplete) a continuation line will be inserted...

luketpeterson commented 12 months ago

In fact, this is not necessarily the final refresh because depending on Validator (for example if the statement is incomplete) a continuation line will be inserted...

That is a good point. really it's a matter of the cursor position being non-existent after the line has been accepted. So you could make the pos argument into an Option<>, but if you want to keep the "force" naming for a bool it will also be fine.

Thank you again.

gwenn commented 8 months ago

Version 13.0.0 released.

luketpeterson commented 8 months ago

Thank you very much!