Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: new editor.enable-diagnostics option #12203

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

feat: new editor.enable-diagnostics option #12203

wants to merge 10 commits into from
+25 −14

Conversation

NikitaRevenco
Copy link
Contributor

@NikitaRevenco NikitaRevenco commented Dec 6, 2024 •
edited
Loading

Sometimes diagnostics can get in the way, and we'd like to temporarily hide them

this PR adds a new editor.enable-diagnostics option. It will hide obtrusive diagnostics, such as inline diagnostics or the diagnostics which are overlaid on top of text. In the editor, it can be toggled with :toggle enable-diagnostics. In the config file, it can be set as follows:

[editor]
# defaults to "true"
enable-diagnostics = false

It won't hide gutter symbols indicating that there are diagnostics on lines, as those aren't obtrusive and it is useful to be aware that there are diagnostics on certain lines. If we want to inspect what those diagnostics are, we can just run :toggle enable-diagnostics again to see them.

Closes #12154

@darshanCommits
Copy link

darshanCommits commented Dec 8, 2024 •
edited
Loading

I think it makes more sense for it to be a config option.

This syntax is inconsistent with the rest of toggle-able options. As they all use the unified "toggle-option "

A enable diagnostics in editor and enable in inline diagnostics would be more consistent instead of a new typabale command.

The inline-diagnostics.enable can just set cursor-line and other-line to disable.

@NikitaRevenco
Copy link
Contributor Author

I think it makes more sense for it to be a config option.

This syntax is inconsistent with the rest of toggle-able options. As they all use the unified "toggle-option "

A enable diagnostics in editor and enable in inline diagnostics would be more consistent instead of a new typabale command.

The inline-diagnostics.enable can just set cursor-line and other-line to disable.

I'm happy to move this functionality to a separate config option, e.g editor.diagnostics which can be either true or false. And then you can use e.g. :toggle diagnostics. I think this makes sense.

I feel like having two toggle options for both types of the diagnostics would be overwhelming. What if you enable 2 options, are both of the diagnostic types shown at once? What if in the future there's a 3rd way added to display diagnostics? And this requires the user to think about which type of diagnostic they currently have enable and disable that one, rather than having a global option to toggle the diagnostics.

I think the current implementation for diagnostics makes intuitive sense. You either enable inline diagnostics, or overlay but not both (as far as I'm aware). And then having a global option to either enable or disable diagnostics makes sense

that's just my thoughts, what do you think about it?

@darshanCommits
Copy link

What if in the future there's a 3rd way added to display diagnostics

good point, theres also work going on to make it config streamlined and that will make it redundant.

editor.enable_diagnostics sounds good.

@NikitaRevenco
Copy link
Contributor Author

What if in the future there's a 3rd way added to display diagnostics

good point, theres also work going on to make it config streamlined and that will make it redundant.

editor.enable_diagnostics sounds good.

Ok, I added the option

darshanCommits
darshanCommits suggested changes Dec 9, 2024
View reviewed changes
helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved
book/src/editor.md Outdated Show resolved Hide resolved
@NikitaRevenco NikitaRevenco changed the title feat: new :toggle-diagnostics command feat: new editor.enable-diagnostics option Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darshanCommits darshanCommits darshanCommits approved these changes

@TornaxO7 TornaxO7 TornaxO7 approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add toggle to completely disable diagnostic rendering
3 participants
@NikitaRevenco @darshanCommits @TornaxO7