fix(diagnostic): allow virtual_{lines,text} cursor exclusivity #33517
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
I think a simpler way to implement this would be to check for This will need a docs update too to indicate that
Or as @justinmk suggested, using a string enum instead of a nullable boolean (if we do that we'd also want to change the option for |
This concise framing makes this PR much easier to understand. So I'm in favor of the |
runtime/lua/vim/diagnostic.lua
Outdated
| api.nvim_buf_clear_namespace(bufnr, namespace, 0, -1) | ||
|
|
||
| for line, line_diagnostics in pairs(diagnostics) do | ||
| local virt_texts = M._get_virt_text_chunks(line_diagnostics, opts) | ||
| local should_render_current_line = (opts.current_line == true and line == lnum) |
There was a problem hiding this comment.
If opts.current_line is nil this should also be true.
There was a problem hiding this comment.
That case is handled by the last part of the condition: or (opts == true or type(opts) == 'table')
There was a problem hiding this comment.
opts will always be a table here, so opts == true will always be false and type(opts) == 'table' will always be true.
I think the logic would be simplified here by testing whether or not the line should be skipped:
local skip = (opts.current_line == true and line ~= lnum) or (opts.current_line == false and line == lnum)
if virt_texts and not skip thenThere was a problem hiding this comment.
Ah, thanks, that helps a lot. I saw examples int he docs of passing in virtual_text = true which seemed to be passed directly into render_virtual_text, so I thought true was a possible value.
vim.diagnostic.config({ virtual_text = true })render_virtual_text(
ns.user_data.virt_text_ns,
bufnr,
line_diagnostics,
opts.virtual_text -- <---
)
Thanks, that does sound a lot simpler. I reimplemented it there and it's a lot cleaner. If you're happy with the code, I'll update the docs. Do you think this needs tests? |
could be a 2-line change in an existing test. docs neovim/runtime/lua/vim/diagnostic.lua Lines 193 to 195 in 99e754a
|
4ee9b23 to
733b625
Compare
Problem:
virtual_text diagnostics are great when skimming a file, and
virtual_lines are great when "zooming in" on a particular problem.
Having both enabled results in duplicate diagnostics on-screen.
Solution:
This PR expands the behavior of `current_line` for virtual_text and
virtual_lines by making `virtual_text.current_line = false` distinct
from `nil`. If you set:
vim.diagnostic.config({
virtual_text = { current_line = false },
virtual_lines = { current_line = true },
})
With this configuration, virtual_text will be used to display
diagnostics until the cursor reaches the same line, at which point they
will be hidden and virtual_lines will take its place.
733b625 to
a12bc5d
Compare
|
I updated docs, added a test, applied @gpanders's logic simplification, and squashed up all the commits. Just two final things. First, I've been using the settings this PR enables ( Second, if this is merged, |
Yeah, in favor of that. Meanwhile, the code simplification makes this PR worthwhile in any case. Thank you! |
Problem:
virtual_text diagnostics are great when skimming a file, and virtual_lines are great when "zooming in" on a particular problem. Having both enabled results in duplicate diagnostics on-screen.
First reported in #33092, with this image as an example:
Solution:
This PR expands the behavior of
current_linefor virtual_text and virtual_lines by makingvirtual_text.current_line = falsedistinct fromnil. If you set:With this configuration, virtual_text will be used to display diagnostics until the cursor reaches the same line, at which point they will be hidden and virtual_lines will take its place.
The situation in the image above now behaves like this:
Note to maintainers: this is my first PR to neovim and I haven't looked into writing tests for this yet but I will look into it if desired.