Support for lewis6991/gitsigns.nvim broken

[richardstrnad]

Hi,

Thanks for the amazing color theme! :)
Unfortunately the PR #294 (or commit a4bf0a6) broke support for the gitsigns plugin as the required styles are no longer loaded.

Would you accept an PR for this?

Cheers

[spywhere]

I can confirm the breaking on the vim-signify plugin as well.

Here's a reproducible configurations for neovim

plugin_home = vim.fn.stdpath('cache') .. '/nord.tmp'
vim_plug_url = 'https://raw.githubusercontent.com/junegunn/vim-plug/master/plug.vim'
vim_plug = plugin_home .. '/plug.vim'

if vim.fn.filereadable(vim_plug) == 0 then
  vim.cmd(
  'silent !curl -fLo ' .. vim_plug .. ' --create-dirs ' .. vim_plug_url
  )
end
vim.opt.runtimepath:append(plugin_home)

vim.fn['plug#begin'](plugin_home)
vim.fn['plug#']('mhinz/vim-signify')
vim.fn['plug#']('arcticicestudio/nord-vim')
vim.fn['plug#end']()
if vim.fn.isdirectory(plugin_home .. '/nord-vim') == 0 then
  vim.cmd('PlugInstall --sync | q')
end

print('before loaded_signify: ', vim.g.loaded_signify)
vim.cmd('colorscheme nord')
print('after loaded_signify: ', vim.g.loaded_signify)

Save it as some-name.lua and run it via nvim -u some-name.lua

So this block of code is not loaded when setting the color scheme

https://github.com/arcticicestudio/nord-vim/blob/291e05d96f7b938ab975e19205a42f6b41a35900/colors/nord.vim#L596-L601

[zxlvera]

I'm not sure should I start a new issue, it is an aesthetic issue but in regards to Gitsigns as well. The highlights for nord is thick compare to other themes. Where can we configure this?

Gruvbox-material:

Catppuccin:

[richardstrnad]

That's exactly the issue I have!
Temp fix

cd ~/.config/nvim/plugged/nord-vim
git checkout a8256787edbd4569a7f92e4e163308ab8256a6e5

[svengreb]

Hi @richardstrnad 👋, thanks for your contribution 👍

Basically, the PR can't have broken the support because there has never been explicit support for the lewis6991/gitsigns.nvim plugin.
I was wondering why it might break anyway and found out that it is the way how the plugin uses highlighting groups, or better said which ones: it defines it own ones, which is the best way, but they also fall back to groups of other plugins like SignifySign* that is defined by mhinz/vim-signify.
Before PR #294 the groups for the mhinz/vim-signify plugin where loaded unconditionally so lewis6991/gitsigns.nvim picked them up since Nord does not define any GitSigns* group, but now the groups are only loaded when the mhinz/vim-signify plugin has been loaded too. Therefore the style with Nord “break“ when only the lewis6991/gitsigns.nvim plugin is loaded.

I'd say this is not a problem of the PR but only just a side effect. We could add dedicated support for the lewis6991/gitsigns.nvim plugin, but I would like to avoid adding more Neovim specific plugins/features as a dedicated Nord port for Neovim is in the pipeline.

@spywhere thanks for the reproduction snippet 👍🏼
This definitely looks more like a problem which could be related to the way how “vanilla“ Vim(8) and Neovim loading code, especially when using third-party plugin managers which might behave differently as well.

After playing around with multiple plugins that are explicitly supported by Nord, loaded with different Vim(8) and Neovim plugin managers, I noticed various problems with the way how plugins and loaded, especially their order and when the code is applied to global namespaces to be available to other scripts. I've often had reports regarding this issue and up to today this is still the most annoying reason why users having problems when it comes to loading Nord plugin options.

Anyway, I decided to revert the changes from PR #294 because I haven't thought about the fact that the plugin loading order could be random and the Nord plugin might be loaded before the ones that are supported, but the highlighting groups will be missing because of this. The advantage of having faster loading times which are only in the nanosecond range is way to

The advantage that it is only minimally faster (we're talking about nanoseconds here) is too small compared to the fact that the actual functionally of the plugin could no longer work properly.

[spywhere]

Instead of simply either conditionally or un-conditionally loaded plugin related highlights. I think we could introduce a new option to let the user choose if they want to change how it behaves.

In general, more features always good to have but more importantly is how to get those features roll out. And one easy way to make it happy for all is to opt-in into a new feature through configuration options.

Regardless, I think you would know best if it would be benefit or not. So revert the change is fine to me in this case.

[svengreb]

Feature flags are often a good way, but it also comes with the cost of cluttering code with more and more conditions, especially when you have to maintain multiple deployment paths and some features are mutual.
However, in this case a feature flag would also not because the problem that plugins might be loaded in random order still exists. It also depends whether the user calls the function to load the Nord plugin before or after other plugins that it supports, so e.g. when the line to load the mhinz/vim-signify plugin comes after the one of Nord in the users configuration the current condition will fail because the global loaded_signify variable has not been declared and set.

Reverting the change it currently the only reliable way to ensure that it works for all users regardless of their own configurations or plugin managers behavior.