See #6417 instead [Feat: CodeLens-style diagnostics]

[poliorcetics]

Replaced by #6417

Closes #3272

[poliorcetics]

This branch should work correctly but I want to refacto and split it into proper commits, I'm just pushing so that other people can try it out

[poliorcetics]

[pascalkuthe]

Thanks for working on this, I am really lookimg forward to.this! Already looks great! I haben't done a full review yet since you aren't done but I have some foundational comments that I think are good to make as early as possible to avoid duplicate work:

The way you compute the exact position where to render a message is not quite correct.

Fundementally due to softwrap and virtual text the on screen position can not ne computed from the text and can change at any time fully async.

Instead the idea is that these decorations are anchored to a char position. When we render we already know the exact onscreen position of each char anyway. The idea is basically that multiline virtual text should only be computed lazily (when visible) in the rendering code. In general the entire idea is that on screen positions are only temporary. Everything that is stored for longer periods is stored as char offsets that get translated to screen coordinates then back. I should write more documentation for that (and maybe even a design doc) but I am sadly busy at the moment.

I actually hadn't thought how to do to handle this specific usecase 100% yet. There is a second API to hook into the rendering: TranslatePosition.These are called when a specific char index is reached with the screen position of that char index. This is a lot closer to what you need. It doesn't quite fit your usecase because you still need to accumelate all positions of the current line and then finally render then (because diagnostics rendering stack right to left not left to right).

The right way to do this is probably to unify these two APIs so you can have a single struct which accumulates positions and then renders them.

For now I think you could just start by using helix_core::visual_offset_from_anchor to move away from doing any rendering in helix-view/document.rs. I will send a PR to streamline the API which you can then base ykurs shouldn't be a large PR and it should be quite easy to replace visual_offset_from_anchor with these positions.

BTW. a small code clarity thing: I would prefer if we didn't keep adding too much code to ui/editor.rs. I purposfukly modularized the text rendering to avoid that.

Since LineDecoration is a trait you can create a neee file ui/inlay diagnostics.rs and create a strucy struct InlayDiagnstics which implement LineAnnotstion and contains any dats you need. This also allows you to split the logic into subfunctions easier.

[poliorcetics]

@pascalkuthe thanks for the tip, I tested with the inlay-hint branch to fix and now it works much better ^^(sill not ready for review, the code is a mess right now, documentation is wrong everywhere too after that fix)

[poliorcetics]

This is ready for review but there are still several problems I know of:

The cursor will go out of view when diagnostics are displayed. I think the LineAnnotations are not taken into account somewhere when computing the view.

In the same vein, I was forced to make commit 4d3ef39 because when the top of the file is modified (for example by rust-analyzer inserting a missing import) while out of the view, it would panic. I think I did something wrong but it's late and that fix make it "work" (i.e. not crash) so that's good enough to ask for help and review.

Finally, when updating text out of view, because of the previous fix I think diagnostics are not moved correctly so we end up with things like the screenshot below (appears after asking rust-analyzer to insert the missing import for use std::arc::Arc):

Again, not a crash so I'll take it for now. I would like to see it fixed though

[pascalkuthe]

This is ready for review but there are still several problems I know of:

The cursor will go out of view when diagnostics are displayed. I think the LineAnnotations are not taken into account somewhere when computing the view.

In the same vein, I was forced to make commit 4d3ef39 because when the top of the file is modified (for example by rust-analyzer inserting a missing import) while out of the view, it would panic. I think I did something wrong but it's late and that fix make it "work" (i.e. not crash) so that's good enough to ask for help and review.

Finally, when updating text out of view, because of the previous fix I think diagnostics are not moved correctly so we end up with things like the screenshot below (appears after asking rust-analyzer to insert the missing import for use std::arc::Arc):

Again, not a crash so I'll take it for now. I would like to see it fixed though

After a quick scan I already have some comments but I am actually working on the improve rendering API I mentioned earlier and those comments are related to do that so I will holdoff a review until I am finished. Hopefully tomorrow if nothing comes up.

I think 4d3ef39 might be a legitimate bug although the cursor going out of view should also not happen. That should ne properly accounted for but I might habe missed something.

Howber the diagnostic mapping should not be affected by any of this so that sounds like a separate bug.

[erasin]

The cursor offset has error at view bottom. When the file content has multiple pages ,cursor will be keep at view bottom when have multiline diagnostics message and use k moving.

Whether to update view.offset.vertical_offset ？

[poliorcetics]

The cursor offset has error at view bottom. When the file content has multiple pages ,cursor will be keep at view bottom when have multiline diagnostics message and use k moving.

Yes it's a known problem, see my previous message, I don't know where it comes from though

[CptPotato]

Two small nits / ideas (might be suited for a follow-up PR):

• Is it possible to color the diagnostics' lines in a neutral color? I think it can look a little odd when different diagnostics types are present:
  

  I had something like this in mind:

  screenshot

  

• What do you think about adding a separate theme key for the code lens background, adding some separation to the code lens diagnostics:

  screenshot

  

[poliorcetics]

@CptPotato did you test on the latest version ? I fixed a bug that highlighted the lines wrong (it used the diagnostic on the same line instead of the target diagnostic), now it works like this:

What do you think about adding a separate theme key for the code lens background, adding some separation to the code lens diagnostics:

Yes, that's a good idea, will do

[CptPotato]

@CptPotato did you test on the latest version ? I fixed a bug that highlighted the lines wrong (it used the diagnostic on the same line instead of the target diagnostic), [...]

Thanks for the hint. This way it gets rid of the visual artifact, too 👍

What do you think about adding a separate theme key for the code lens background, adding some separation to the code lens diagnostics:

Yes, that's a good idea, will do

If you do, I think a theme key like ui.background.code-lens could work well since it should fall back to ui.background if it's not specified.

[poliorcetics]

@CptPotato Added a default style under ui.virtual.diagnostics, it's not necessary to use background: helix will use it if nothing else is set and I can see someone wanting to set ui.virtual to have all virtual text with the same style at once.

@pascalkuthe I discovered a bug where I trigger the assertion in helix-term/src/ui/document.rs, assert_eq!(0, offset.vertical_offset); when using Z mode with inline diagnostics. I'm not sure what's happening here but I suppose the text annotations I create are wrong or something else is mis-handled when computing heights

[SoraTenshi]

Awesome job! i really like the style of the diagnostics!

[poliorcetics]

@SoraTenshi thanks for the find, I had planned to search for it but you made it a 100 times easier 😄 it should be fixed now

There is still the bug with the cursor going out of view and the Z mode panicking when a diagnostic (or more likely, any line annotation) comes into view

[SoraTenshi]

@SoraTenshi thanks for the find, I had planned to search for it but you made it a 100 times easier 😄 it should be fixed now

There is still the bug with the cursor going out of view and the Z mode panicking when a diagnostic (or more likely, any line annotation) comes into view

Have you figured out how to reproduce it?

[erasin]

The length of the error message is out of range in the split window.

[pascalkuthe]

The length of the error message is out of range in the split window.

Yeah it needs to be softwrapped but variable height virtual text isn't currently supported by the virtual text API. I don't have much time at the moment but next week I will post a PR to expand the virtual text API to cover this usecase.

I actually have a similar need for side by side diffing so it's needed anyway. This PR also has a bunch of bugs that are partly caused by the virtual text API being limited (and partly by it not being used correctly). Once I have update the virtual text API I will do an indepth review.

[hnorkowski]

I have merged your PR into the master locally. The feature is awesome! So much easier to read the LSP Hints like this.

Unfortunately it crashes on some files:

thread 'main' panicked at 'index out of bounds: the len is 5332 but the index is 5332', helix-tui/src/buffer.rs:476:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/core/src/panicking.rs:64:14
   2: core::panicking::panic_bounds_check
             at /rustc/2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74/library/core/src/panicking.rs:159:5
   3: helix_tui::buffer::Buffer::set_style
   4: <F as helix_term::ui::document::LineDecoration>::render_background
   5: helix_term::ui::document::render_document
   6: helix_term::ui::editor::EditorView::render_view
   7: <helix_term::ui::editor::EditorView as helix_term::compositor::Component>::render
   8: helix_term::application::Application::render::{{closure}}
   9: hx::main_impl::{{closure}}
  10: tokio::runtime::park::CachedParkThread::block_on
  11: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
  12: tokio::runtime::runtime::Runtime::block_on
  13: hx::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

This happens on this Rust code when the dependencies are not set in the Cargo.toml:

use tower_lsp::jsonrpc::Result;
use tower_lsp::lsp_types::*;
use tower_lsp::{Client, LanguageServer, LspService, Server};

#[derive(Debug)]
struct Backend {
    client: Client,
}

#[tower_lsp::async_trait]
impl LanguageServer for Backend {
    async fn initialize(&self, _: InitializeParams) -> Result<InitializeResult> {
        Ok(InitializeResult::default())
    }

    async fn initialized(&self, _: InitializedParams) {
        self.client
            .log_message(MessageType::INFO, "server initialized!")
            .await;
    }

    async fn shutdown(&self) -> Result<()> {
        Ok(())
    }
}

#[tokio::main]
async fn main() {
    let stdin = tokio::io::stdin();
    let stdout = tokio::io::stdout();

    let (service, socket) = LspService::new(|client| Backend { client });
    Server::new(stdin, stdout, socket).serve(service).await;
}

Another problem is that the view position is not properly adjusted for the size of virtual lines so that the cursor can go out of the view field.

[cidit]

how soon can we expect to see this? looks awesome

[r-odriguez]

Since some stuff weren't mentioned here I think it would be nice to have them mentioned somewhere. Therefore, I'll leave it here just as a note. And thanks for this awesome PR beforehand.

1. why shift the code instead of showing the diagnostics as a contiguous line, like what happens in neovim, and toggling some pop-up with a keybinding (e.g what gl doest in nvim)?
2. I think would be interesting to have an option to toggle it by default. e.g

[editor.lsp]
code-lens.enabled = true # or false

3. having a keybinding to toggle it it's great and have already been mentioned here.
4. this will replace the old style or just add one extra? if so, will it be defined in the config.toml file? e.g

[editor.lsp]
code-lens.style = "default" # or "toggleable" ??

Thanks again for this awesome feature!

[pilucid]

I see a few mentions of verbosity, toggling with a keybinding, etc, so I'll throw another idea in the hat: would it be useful to treat diagnostic severity levels similar to application log levels? For example

[editor.lsp]
code-lens.level = "warn"

Would show warnings and errors inline with the source code as implemented by this PR, but info and hint diagnostics would require triggering a pop-up of some kind.

Thanks for the PR either way! I have been using and enjoying this for a few weeks now.

[pascalkuthe]

Superseded by #6417

That PR contains a bunch of bug fixes and architectural improvements to the virtual text API and a complete reimplementation of this PR on top of the improved API (and to enable soft wrapping). This should fix the artifacts and crashes reported here.

That branch also contains a bunch of additional improvements:

• text is softwrapped so all diagnostics are always readable and never cutoff (although the number of visible diagnostics is limited to 10 by default to avoid filling the entire screen with diagnostics, but that limit can be configured)
• diagnostics can be filtered by their level (warning by default)
• diagnostics on the line that contains the cursor can be filtered by a separate level filter (shows all diagnostics by default)

It would be great if that PR was tested more to shake out any remaining bugs in the virtual text rendering (although I hopefully got most of them). It's still missing some docs but apart from that it should work well.

[nathan-at-least]

I filed #7015 due to a UX problem with 0.6.0 in which brainstormed a different UI. Then I discovered this, which is better than what I imagined while addressing my problem well. Closed my ticket. 👍 and Kudos!