-
-
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
Inline Diagnostics #6417
Inline Diagnostics #6417
Conversation
(one test is failing for a type of virtual text that is currently not used yet. I forgot to clean that up, will look into that tomorrow, this shouldn't affect usage) |
I noticed a bug (or strange behavior) trying this when combining wrapping with splits: on insert, the cursor moves away from the place where the actual insertion takes place. See an example here: https://asciinema.org/a/DlnOVhiM8g2kXVtGBjwVFk8lo (Maybe it is also a problem in master, mark as off-topic if that's the case). |
thanks for testing! This is a new regression with this PR and doesn't occur on master. |
After merged this PR, LSP Type Hints #5934 have no effect. |
4de41cc
to
95f96ef
Compare
Thank you for your feedback @yuyidegit] both bugs have been fixed (the test failures too). This PR now depends on #6440 which fixes a bunch of bugs relevant to this PR. I will create another PR for the more complex virtual text API improvements once that one is merged |
95f96ef
to
708cfaf
Compare
c0eb133
to
c0f5627
Compare
found a pretty nasty bug to reproduce:
I can reproduce this same panic for any inline diagnostic, seems like an edge case when the line that an error comes from does not fit on the screen. So the diagnostic would be the first line. But the first line rendered in this case would be line 27 which is technically row 2 so it has a |
22f82b2
to
60f4dc7
Compare
The line annotation as implemented in helix-editor#5420 had two shortcomings: * It required the height of virtual text lines to be known ahead time * It checked for line anchors at every grapheme The first problem made the API impractical to use in practice because almost all virtual text needs to be softwrapped. For example inline diagnostics should be softwrapped to avoid cutting off the diagnostic message (as no scrolling is possible). While more complex virtual text like side by side diffs must dynamically calculate the number of empty lines two align two documents (which requires taking account both softwrap and virtual text). To address this, the API has been refactored to use a trait. The second issue caused some performance overhead and unnecessarily complicated the `DocumentFormatter`. It was addressed by only calling the trait mentioned above at line breaks (instead of always). This allows offers additional flexibility to annotations as it offers the flexibility to align lines (needed for side by side diffs).
This commit brings the text decoration API inline with the LineAnnotation API (so they are consistent) resulting in a single streamlined API instead of multiple ADHOK callbacks.
5cf283e
to
386fa37
Compare
That was on 418ac23 which was the latest build at that point. It is just slightly difficult to reproduce, as I said, because you need to have a very specific editor width and diagnostic for it to occur, for example rust-analyzer may change its output between versions. I can still reproduce on commit 386fa37 obviously as nothing important was changed And a terminal width Here is a recording of it https://asciinema.org/a/cd8tFNbUwqCmcay91vXH1pKXB Also multiple lines are still concatenated without a space for an eol diagnostic. Well now that this was merged can just file a bug report. |
I can reproduce @kanashimia's issue, and I'm assuming it's caused by the line break in the 4th syntax error. There is a mismatch between the actual height of the diagnostics and the number of "virtual lines" inserted. |
@pascalkuthe It’s merged – yay! 🎉 Thank you so much for your 1yr+ of work on this. 💕 |
Thanks for this long awaited feature <3. I have a stupid question: I 've installed version from master |
You need to enable it in the config, for example: [editor.inline-diagnostics]
cursor-line = "hint"
other-lines = "error" |
Thank you @mxmerz! It helped! Thanks again to the contributors of this great feature. |
tfw the feature you've been waiting a year for misses the release train by 1 day Anyway, congratulations @pascalkuthe on landing this! 🎉 |
@pascalkuthe thank you for this PR. Now I don't need to move my head to the upper right corner 😃 🎉 |
just tested it, it's awesome!!! |
Can confirm, this is lovely (after you get used to it 😉). Thank you for getting this over the line, all! |
Closes #3272
Closes #3078
closes #1462
Closes #1953
Supersedes #6059
This PR implements inline diagnostics just like #6059. However, it also contains a streamlined virtual text API and significantly improves on the implementation in #6059.
The various improvements and bugfixes made to the virtual text API are currently part of this PR until it's been tested more. I will spin those out into a separate PR later. These should address the various bugs and panics discovered by poeple in #6059. Only the last commit contains an implementation of inline diagnostics.
Due to the changes to the virtual text API I had to essentially rebuild #6059 as most changes in that PR became unnecessary by the improvements in the virtual text API.
I also rebuild the ASCII rendering to support soft wrapping. Diagnostics which exceed the width of the screen are now soft wrapped automatically. Additionally, there is a minimum width setting for diagnostics. Any diagnostics so far to the right that there is not this minimum width available are displayed using a backwards arching line. See screenshot below:
Additionally, this PR implements a severity-based filter for diagnostics. This allows users to choose one of the following:
"warning"
["warning", "errror"]
Using an empty list disables inline diagnosticsThe diagnostics to display can be separately configured for the line that currently contains the cursor. By disabling diagnostics for all other lines users can therefore only display diagnostics on the current line too. By default, only warnings/errors are displayed, on the cursor line all diagnostics are displayed. I am open to changing the default here, but this did feel quite nice to use in my testing.
Additionally, some details of the soft wrapping can be configured.
Currently, a draft as this needs more documentation, a separate PR for most of the virtual text changes, and more testing. That said it should work quite well already, and I invite people to try this out.