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

Improve highlights for Neovim LSP #229

Merged
merged 1 commit into from
May 29, 2021

Conversation

gbrlsnchs
Copy link
Contributor

No description provided.

@clason
Copy link

clason commented Nov 7, 2020

Just a heads-up: these will change again (for the last time?) once neovim/neovim#12655 is merged.

@clason
Copy link

clason commented Nov 13, 2020

And now the PR is merged; the new LSP diagnostic highlight names are listed here: https://github.com/neovim/neovim/blob/35325ddac04b1b59b7982797021cdaabdabc87fb/runtime/doc/lsp.txt#L376

@gbrlsnchs gbrlsnchs marked this pull request as draft November 16, 2020 14:57
@gbrlsnchs gbrlsnchs marked this pull request as ready for review November 16, 2020 15:03
@gbrlsnchs
Copy link
Contributor Author

Done. Seems visually nice to me.

@ojroques
Copy link

ojroques commented Dec 20, 2020

The doc mentions that "Sign, underline and virtual text highlights (by default) are linked to their
corresponding LspDiagnosticsDefault highlight
. Since you're not changing anything for LspDiagnosticsVirtualText* LspDiagnosticsFloating* LspDiagnosticsSign* maybe these lines can be removed?

This is what onedark.vim does (disclaimer: PR was from me).

@gbrlsnchs
Copy link
Contributor Author

Done, @ojroques! Thanks for the tip.

@ojroques
Copy link

ojroques commented Jan 1, 2021

Glad to help. I would have kept these lines though since they differ from the defaults:

call s:hi("LspDiagnosticsUnderlineWarning" , s:nord13_gui, "", s:nord13_term, "", "undercurl", "")
call s:hi("LspDiagnosticsUnderlineError" , s:nord11_gui, "", s:nord11_term, "", "undercurl", "")
call s:hi("LspDiagnosticsUnderlineInformation" , s:nord8_gui, "", s:nord8_term, "", "undercurl", "")
call s:hi("LspDiagnosticsUnderlineHint" , s:nord10_gui, "", s:nord10_term, "", "undercurl", "")

The underline text is quite useful to spot warnings/errors.

After neovim/neovim#12655 has been merged,
highlights have changed a little, so this commit updates the obsolete
highlight groups.
@gbrlsnchs
Copy link
Contributor Author

You're totally right, my bad. Just included those again.

@crispgm
Copy link

crispgm commented Feb 9, 2021

With LspDiagnosticsUnderline* stuffs specified, it would be buggy of showing colors. However, using LspDiagnosticsDefault* looks okay. 👌

@gbrlsnchs
Copy link
Contributor Author

With LspDiagnosticsUnderline* stuffs specified, it would be buggy of showing colors. However, using LspDiagnosticsDefault* looks okay.

How come? I'm using my fork with this PR applied and everything seems fine.

@crispgm
Copy link

crispgm commented Feb 10, 2021

With LspDiagnosticsUnderline* stuffs specified, it would be buggy of showing colors. However, using LspDiagnosticsDefault* looks okay.

How come? I'm using my fork with this PR applied and everything seems fine.

okay, works perfectly on another machine. this is weird.

@gbrlsnchs
Copy link
Contributor Author

With LspDiagnosticsUnderline* stuffs specified, it would be buggy of showing colors. However, using LspDiagnosticsDefault* looks okay.

How come? I'm using my fork with this PR applied and everything seems fine.

okay, works perfectly on another machine. this is weird.

Might be your Neovim version maybe? Anyway, good it's working!

gbrlsnchs added a commit to gbrlsnchs/dotfiles that referenced this pull request Feb 18, 2021
My fork includes my PR (which was opened upstream) to fix LSP-related
highlights.

This shall be reverted once
nordtheme/vim#229 is merged.
@arcticicestudio arcticicestudio added this to the 0.16.0 milestone May 29, 2021
Copy link
Contributor

@arcticicestudio arcticicestudio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gbrlsnchs, @clason, @ojroques and @crispgm 👋, thanks for your contributions 👍
Also thanks for your patience, the large amount of tasks for the Nord project are really time consuming and free time is kind of rare.

Changes looking good to me 👍🏻
The problems regarding the undercurl attribute can be tackled later on in a separate PR by guarding the usage through the nord_underline theme configuration variable. When the terminal is capable to rendering underlines it should also work fine for curly lines.


Closes #248

@arcticicestudio arcticicestudio merged commit a3af928 into nordtheme:develop May 29, 2021
crispgm pushed a commit to crispgm/nord-vim that referenced this pull request Jun 10, 2021
To ensure compatibility with the latest versions of Neovim LSP the
highlighting groups for diagnostics have been adapted to the changes
of neovim/neovim#12655 [1].
See :help lsp-highlight-diagnostics [2] for more details.

Note that LSP will be available as of Neovim 0.5 which is (at the time
of this commit) still in development and only available as nighly build.
Also see great articles from Nord Vim contributors like "Neovim (0.5) Is
Overpowering" [3] for more information about Neovim 0.5 features,
including LSP.

[1]: neovim/neovim#12655
[2]: https://neovim.io/doc/user/lsp.html
[3]: https://crispgm.com/page/neovim-is-overpowering.html

Co-authored-by: Sven Greb <development@svengreb.de>

Closes nordthemeGH-229
Closes nordthemeGH-248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants