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

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

Closed
luketpeterson opened this issue Aug 29, 2023 · 11 comments
Labels

Comments

@luketpeterson
Copy link

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
Copy link
Author

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 gwenn added the bug label Aug 29, 2023
@gwenn
Copy link
Collaborator

gwenn commented Aug 29, 2023

We already refresh the line here:

rustyline/src/command.rs

Lines 28 to 32 in 1c4091a

if s.has_hint() || !s.is_default_prompt() {
// Force a refresh without hints to leave the previous
// line as the user typed it after a newline.
s.refresh_line_with_msg(None)?;
}

But indeed currently, we cannot check / know that the Highlighter needs a final refresh.

@luketpeterson
Copy link
Author

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
Copy link
Collaborator

gwenn commented Aug 29, 2023

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

luketpeterson added a commit to luketpeterson/rustyline that referenced this issue Aug 29, 2023
…atchingBracketHighlighter to re-render line without highlighting. Issue kkawakam#726
@luketpeterson
Copy link
Author

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.

  • The lowest risk change is to make the internals of MatchingBracketHighlighter be tri-state. So final_highlight would set a "final" bool variable in the Cell, and that flag tells highlight_char() not to make any updates. (or tells highlight to render without any cursor-based info). This is unlikely to affect the behavior of any other clients, but it feels a little ugly to me.

  • The other solutions are to somehow signal to highlight_char() or highlight() that a final render is expected. For example, calling those methods with a line that includes an EOF at the end would accomplish this. Unfortunately it looks like plumbing the information that a given refresh is "final" through the Refresher interface looks like it might cause some serious code disruption.

I raised a PR (#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
Copy link
Collaborator

gwenn commented Aug 29, 2023

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

  • a new flag in State that indicates that we are in the "final" phase.
  • Use State.highlight_char to know if we need a final refresh.
  • don't call Highlighter::highlight_char in "final" phase (set State.highlight_char to false).

See draft #729

@luketpeterson
Copy link
Author

luketpeterson commented Aug 30, 2023

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
Copy link
Collaborator

gwenn commented Aug 30, 2023

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
Copy link
Author

luketpeterson commented Aug 30, 2023

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 gwenn closed this as completed Sep 2, 2023
@gwenn
Copy link
Collaborator

gwenn commented Dec 5, 2023

Version 13.0.0 released.

@luketpeterson
Copy link
Author

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants