-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add virtual text support #417
Conversation
I think for an initial implementation, just end-of-line annotations are fine. As I mentioned in the linked issue, any rendering work you do here is likely to be redundant with planned future work. So keeping it to the simple case probably makes the most sense. I'm also a little concerned that doing inline rendering right now will be a bit delicate, as the current rendering architecture isn't really set up to support it, nor is the cursor navigation system, etc. Inline annotations will need to touch a lot of systems to work properly. |
In that case I'll drop the inline rendering plan and keep it as is. Cursor and selection rendering for inline annotations will definitely look hacky with the current architecture. |
4fe1e11
to
a091e11
Compare
Yeah, kak-lsp did the same and it is quite broken in kakoune too. |
a091e11
to
89385bf
Compare
As discussed on Matrix, this PR has been revived (yay !), and I have rebased the original changes on top of current master. |
Missing update in https://github.com/helix-editor/helix/blob/master/helix-core/src/position.rs I think, the |
89385bf
to
ae50561
Compare
26b211b
to
2abb57f
Compare
This is awesome, thanks for adding support! |
This PR as of now doesn't implement inlayed virtual text, i.e. virtual text in between normal text, which is what we would need to properly add support for rust-analyzer's type hints. Also the type hint API isn't part of the LSP spec yet, so we would have to probably wait on that too. |
Ah, I see - that's unfortunate. The type hint API seems to work in vim and vs-code, what is it that they do differently? |
They usually have dedicated plugins that do the setup for you, for example I use https://github.com/nvim-lua/lsp_extensions.nvim for Neovim which sets up inlay hints for rust. VSCode's rust extension also hooks into the decoration API provided by VSC and renders them. |
The new spec seems final, rust-analyzer recently switched over: https://rust-analyzer.github.io/thisweek/2022/03/14/changelog-120.html#new-features |
This seems to now be part of the LSP spec: microsoft/language-server-protocol#956 |
@sudormrfbin I quite like the inlay hints from https://github.com/simrat39/rust-tools.nvim#inlay-hints |
Is it possible to make the hints show up below/above the line? If the hint is long it gets cut off when it is displayed beside the line. |
+1 |
Does this branch have working type hints? |
Currently supports putting annotations at EOL and overlaying.
2abb57f
to
4d8be0a
Compare
LSP 3.17 has support for inlay hints https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_inlayHint lsp-type@0.93(0.93.1) has support language support:
|
That should be done in another PR I think |
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 don't have a very solid understanding of the rendering code, but this looks reasonable from what I can tell.
// we still want to render an empty cell with the style | ||
surface.set_string( | ||
viewport.x + visual_x - offset.col as u16, | ||
viewport.y + line, | ||
&newline, | ||
style.patch(whitespace_style), | ||
); | ||
visual_x += 1; |
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.
Why was this not necessary before, but is now?
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 believe because it is due to the addition of virtual text.
But this += 1
does not seemed to support double width characters like CJK, otherwise I think it shouldn't always += 1
and sometimes it should += 2
.
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 this works as intended in all cases.
visual_x
point is placed after the last character (always incremented by the correct grapheme
width in the else clause). The visual_x += 1
only moves the virtual text to the right by one cell so there is space between the annotation and the content of the line. It might be nice to configure the amount of space in the future but I think that can be done in a followup PR.
This was not required before this PR because visual_x
is reset back to 0
right after this condition (as a new line starts) on master
. However this PR still requires the visual_x
(plus the space) to render the EOL annotations.
} | ||
} | ||
|
||
// pub fn slice<R>(&self, range: R) -> RopeSlice where R: RangeBounds { |
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.
Still needed?
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.
Apart from two minor nits this looks good to me
// It's slightly more efficient to produce a full RopeSlice from the Rope, then slice that a bunch | ||
// of times than it is to always call Rope::slice/get_slice (it will internally always hit RSEnum::Light). | ||
let text = text.slice(..); |
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.
// It's slightly more efficient to produce a full RopeSlice from the Rope, then slice that a bunch | |
// of times than it is to always call Rope::slice/get_slice (it will internally always hit RSEnum::Light). | |
let text = text.slice(..); |
this is a noop (text is already a RopeSlice
@@ -119,6 +121,7 @@ pub struct Document { | |||
pub(crate) modified_since_accessed: bool, | |||
|
|||
diagnostics: Vec<Diagnostic>, | |||
text_annotations: HashMap<TextAnnotationGroup, Vec<TextAnnotation>>, |
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.
Might be more performant to implement this as a BTreeMap
in tne future with the lines as keys.
This would make iterating a small range of lines (during rendering) more efficient when there are a large amount of annotations.
Can probably be done in a followup PR tough
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.
Just as a helix user, I'd really love to see this PR merged! Thanks for your time on this. I had time today to check out your branch and POC for LSP diagnostic virtual text, and it works as expected on my end.
The POC was a little outdated, but here's an updated one you can try (after merging the PR branch to master)
diff --git a/helix-term/src/application.rs b/helix-
term/src/application.rs
index c7d98fce..4f24b4b9 100644
--- a/helix-term/src/application.rs
+++ b/helix-term/src/application.rs
@@ -633,7 +633,36 @@ pub async fn handle_language_server_message(
source: diagnostic.source.clone()
})
})
- .collect();
+ .collect::<Vec<_>>();
+
+ doc.clear_text_annotations("diagnostics");
+
+ use helix_core::diagnostic::Severity;
+ use helix_view::decorations::{TextAnnotation, TextAnnotationKind};
+ use helix_view::graphics::{Color, Style};
+ let style = Style::default().bg(Color::Gray);
+ doc.push_text_annotations(
+ "diagnostics",
+ diagnostics
+ .iter()
+ .map(|d| TextAnnotation {
+ text: d.message.clone().into(),
+ style: d
+ .severity
+ .as_ref()
+ .map(|s| match s {
+ Severity::Error => style.clone().fg(Color::Red),
+ Severity::Warning => {
+ style.clone().fg(Color::Yellow)
+ }
+ Severity::Info => style.clone().fg(Color::Blue),
+ Severity::Hint => style.clone().fg(Color::Green),
+ })
+ .unwrap_or(style.clone().fg(Color::Yellow)),
+ line: d.line,
+ kind: TextAnnotationKind::Eol,
+ })
+ );
doc.set_diagnostics(diagnostics);
}
} | ||
} | ||
|
||
/// Namespaces and identifes similar annotations |
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.
/// Namespaces and identifes similar annotations | |
/// Namespaces to identify similar annotations |
} | ||
|
||
/// Namespaces and identifes similar annotations | ||
pub type TextAnnotationGroup = &'static str; |
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 this could be an enum instead of a string, since it will be known at compile time anyway.
pub enum TextAnnotationGroup {
Diagnostic,
JumpMode,
}
Then hashing or b-tree look up & filtering will be fast, or possibly even faster would be to store annotations directly into a named field:
struct DocumentAnnotations {
diagnostics: Vec<TextAnnotation>,
jump_mode: Vec<TextAnnotation>,
}
pub struct Document {
// -----8<-----
text_annotations: DocumentAnnotations,
}
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, another review note: Document
implements Debug
manually, so consider adding the text_annotations
field there as well
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 making a TextAnnotationGroup
enum would make it hard to use annotations from a plugin, once a plugin system is implemented.
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.
that's a fair point! it's certainly not a blocking issue.
to reap benefits (which may be slight?) of the enum approach and allow plugins to add annotations, something like this would work:
/// first-party annotation groups (only used to differentiate access to the Vecs below)
pub enum TextAnnotationGroup {
Diagnostic,
JumpMode,
}
/// plugin annotation groups
/// (might have to reserve "diagnostic" and "jump_mode" strs?
pub type TextAnnotationPluginGroup = &'static str;
struct DocumentAnnotations {
diagnostics: Vec<TextAnnotation>,
jump_mode: Vec<TextAnnotation>,
from_plugins: BTreeMap<&'static str, TextAnnotation>,
}
pub struct Document {
// -----8<-----
text_annotations: DocumentAnnotations,
}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This could also be extended to debugging, so it displays the values of variable and objects, which would be of tremendous help. |
Update on this? Code folding and inlays are quite useful features currently blocked on this PR. |
An update for everyone interested in this: After discussion in the matrix chat, I will be taking over work on virtual text. |
Closes #411, called text annotations in code.
TODO
Future Works
POC using diagnostics (click to expand patch):