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.

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 curled and dotted underline for diagnostics #7

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

epbuennig
Copy link
Contributor

I'm wondering if support for underline styles (helix#3015, superseded by helix#4064) is planned. This would bring the themes in this repo on par with the default themes in terms of diagnostic support.

There are currently 4 underline styles:

  • curl
  • dashed
  • dotted (docs mention dot but that is not correct on master)
  • double_underline

These should ideally feel familiar, so the color should match the gutter icon's color, the style should match what other popular editors do.

Other Editors

Diagnostic differences:

helix/lsp intelij-based vscode
error error error
warning warning warning
info hint info
hint suggestion info (different styling)

IntelliJ-based IDEs

severity underline type color styling
error undercurled vibrant red -
warning undercurled soft red -
info underdotted vibrant cyan short span, nothing we have control over
hint undercurled soft yellow -

Visual Studio Code

severity underline type color styling
error undercurled vibrant red -
warning undercurled vibrant yellow -
info underdashed vibrant cyan short span, nothing we have control over
hint underdashed soft yellow -

Most themes for these editors don't interfere with the default styling of the diagnostics. The changes are simple

+ "diagnostic.hint" = { underline = { color = <hint>, style = "dotted" } }
+ "diagnostic.info" = { underline = { color = <info>, style = "dotted" } }
+ "diagnostic.warning" = { underline = { color = <warning>, style = "curl" } }
+ "diagnostic.error" = { underline = { color = <error>, style = "curl" } }

As well as removing the deprecated option for the fallback style and changing it to curl (same as the default theme):

- "diagnostic" = { modifiers = ["underlined"] }
+ "diagnostic" = { underline = { style = "line" } }

I've tried this with gruvbox_original_dark_hard only, but I figure this Is simple enough that it should work for all of them.
It's up to you if prefer curl for all of them or dashed over dotted, I thought the distinction from color only is a bit hard to see, and the dashed underline looked kind of weird to me.

@CptPotato
Copy link
Owner

Thanks for the heads up, I'm definitely in favor of this.

I'll have to check which versions I prefer, but I think info/hint as dotted and warn/error as curled sounds good.

The terminal I'm using (wezterm) doesn't seem to support these underline styles, so it's difficult for me to test this. Would you mind providing a screenshot?

@epbuennig
Copy link
Contributor Author

Using gruvbox original dark hard in alacritty:
image

Using gruvbox original dark hard in kitty:
image

The same with dashed as reference:
image
image

@epbuennig
Copy link
Contributor Author

epbuennig commented Dec 7, 2022

I see this on wezterm (20221119-145034-49b9839f), are you up to date?
image

@CptPotato
Copy link
Owner

Looks great! Interestingly, I'm using the same wezterm version but the styled underlines don't show up for me:

image

I'll have to check why that happens. Regardless, the changes and visuals look good to me, so unless there's other things you want to address I'd go ahead and merge this.

@epbuennig
Copy link
Contributor Author

Perhaps you're still on helix 22.08, I'm running master because the arch package is always a bit late. So while it is released, it may not be available on your system either.

@epbuennig
Copy link
Contributor Author

If you're ok with dotted over dashed then it's good to merge.

@CptPotato CptPotato merged commit 0ebf77d into CptPotato:main Dec 7, 2022
@CptPotato
Copy link
Owner

Thanks!

@epbuennig epbuennig deleted the diagnostic-underlines branch December 8, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants