-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Syntax-highlight regex prompts #7738
Conversation
We can use tree-sitter-regex highlighting in prompts for entering regexes, like `search` or `global_search`. The `highlighted_code_block` function from the markdown component makes this a very small change. This could be improved in the future by leaving the parsed syntax tree on the prompt, allowing incremental updates. Prompt lines are usually so short though and tree-sitter-regex is rather small and uncomplicated, so that improvement probably wouldn't make a big difference.
@@ -50,4 +50,3 @@ | |||
]) | |||
|
|||
(class_character) @constant.character | |||
(pattern_character) @string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the (ERROR)
class so that invalid regexes are highlighted? That's something that has been a TODO in the prompt for a while, the idea was to highlight the whole line as red but I guess we can actually do better and use (ERROR)
nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This TODO:
helix/helix-term/src/ui/mod.rs
Line 147 in fb103ee
// TODO: mark command line as error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the error highlights don't quite match the rust regex syntax. Unclosed brackets don't seem to be highlighted for example. That kind of makes sense since the regex grammar is not just specific to the rust regex engine so it might be more permissive (since usually rejecting invalid syntax is not a goal for a syntax highlighters).
So I think it would still make sense to implement #3050 (comment) as a separate PR in the future (although the error highlighting is nice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could overlay some error highlights for regex syntax errors according to rust's regex without too much extra work. We could check the syntax with the regex-syntax
dep and merge the location span's range in the additional_highlight_spans
:
helix/helix-term/src/ui/markdown.rs
Lines 29 to 35 in 5a52897
pub fn highlighted_code_block<'a>( | |
text: String, | |
language: &str, | |
theme: Option<&Theme>, | |
config_loader: Arc<syntax::Loader>, | |
additional_highlight_spans: Option<Vec<(usize, std::ops::Range<usize>)>>, | |
) -> Text<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a really neat idea 👍 Although you need to be sure to convert byte/utf8 indeces to char/utf32 indices
fb103ee
to
c23edfb
Compare
Since regex is almost always injected into other languages, `pattern_character`s will inherit the highlight for the structure that injects them (for example `/foo/` in JavaScript or `~r/foo/` in Elixir). This removes the string highlight when used in the prompt. We also add `ERROR` node highlighting so that errors in regex syntax appear in the prompt. This resolves a TODO in the `regex_prompt` function about highlighting errors in the regex.
Pascal and I discussed this and we think it's generally better to take a 'RopeSlice' rather than a '&Rope'. The code block rendering function in the markdown component module is a good example for how this can be useful: we can remove an allocation of a rope and instead directly turn a '&str' into a 'RopeSlice' which is very cheap. A change to prefer 'RopeSlice' to '&Rope' whenever the rope isn't modified would be nice, but it would be a very large diff (around 500+ 500-). Starting off with just the syntax functions seems like a nice middle-ground, and we can remove a Rope allocation because of it. Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>
This removes a handful of allocations for functions calling into the function, which is nice because the prompt may call this function on every keypress.
c23edfb
to
ef47075
Compare
We can re-use the code-block highlighting function from the markdown component to highlight regex prompts with tree-sitter-regex. To make this change a little cheaper, we switch some functions from taking
&Rope
s toRopeSlice
s (see 50557c6's message).