feline.nvim icon indicating copy to clipboard operation
feline.nvim copied to clipboard

feat: use defined signs for diagnostics provider

Open mintelm opened this issue 2 years ago • 6 comments

Hey, first of all, thanks for creating feline.nvim.

This pull requests tries to get the text field of the defined signs created by the LSP (i.e. DiagnosticSignError, DiagnosticSignWarn, DiagnosticSignInfo, DiagnosticSignHint) instead of using the hardcoded icons. The hardcoded icons are however used as a fallback.

mintelm avatar Nov 16 '22 21:11 mintelm

Not sure if it's a thing with my config or if it's an issue for everyone, but, it seems that if sign_define isn't used, LSP automatically defines 'E' for Error, 'W' for Warning and so on when the sign is needed. If that's the case for everyone, then that means the fallback signs would never be used.

famiu avatar Nov 17 '22 06:11 famiu

This is true. I removed the fallback and also return '' instead of nil if no sign is defined, which should never be the case when using nvim-lspconfig (force-pushed).

However, now that I think about it, there might be some user that uses lsp without nvim-lspconfig and therefore no sign might be defined and the fallback would be used? I do not know how to test this, as I've never tried using lsp without nvim-lspconfig.

mintelm avatar Nov 17 '22 07:11 mintelm

This is true. I removed the fallback and also return '' instead of nil if no sign is defined, which should never be the case when using nvim-lspconfig (force-pushed).

However, now that I think about it, there might be some user that uses lsp without nvim-lspconfig and therefore no sign might be defined and the fallback would be used? I do not know how to test this, as I've never tried using lsp without nvim-lspconfig.

Seems like it's a Neovim core thing and thus is unaffected by whether nvim-lspconfig is used or not

famiu avatar Nov 17 '22 08:11 famiu

This is true. I removed the fallback and also return '' instead of nil if no sign is defined, which should never be the case when using nvim-lspconfig (force-pushed).

However, now that I think about it, there might be some user that uses lsp without nvim-lspconfig and therefore no sign might be defined and the fallback would be used? I do not know how to test this, as I've never tried using lsp without nvim-lspconfig.

Turns out we do need some form of fallback, since the signs are only defined only after diagnostics are shown, they are still undefined until then. Also using E, W, etc. as diagnostics icons would be really ugly and undesirable. Need to find a way around that as well

famiu avatar Nov 17 '22 09:11 famiu

But as long as there are not diagnostics, the icon will never show, therefore it should be acceptable that the icon is undefined until then.

Also using E, W, etc. as diagnostics icons would be ugly indeed. However, Anyone who does not like that sign is supposed to redefine it anyway. I think using the defined sign actually improves uniformity, i.e. the icons in the sign column and statusline are the same.

Which is my usecase -- I want uniform icons across all UI elements. However, this can also be achieved using the icon field of the provider, therefore if you do not like relying on the signs because they are defined as E,W,etc. per default, I do not mind if the PR gets closed. I think it improves overall uniformity which is always good, but that's just my 2 cents.

mintelm avatar Nov 17 '22 12:11 mintelm

But as long as there are not diagnostics, the icon will never show, therefore it should be acceptable that the icon is undefined until then.

That is not true. There is nothing (except an enabled component item which is not mandatory) preventing diagnostics from being shown even if there are none.

famiu avatar Nov 17 '22 13:11 famiu