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

Themes are not applied correctly on entering Neovim in v.2.0. #173

Closed
LeonHeidelbach opened this issue Mar 20, 2023 · 64 comments
Closed

Themes are not applied correctly on entering Neovim in v.2.0. #173

LeonHeidelbach opened this issue Mar 20, 2023 · 64 comments

Comments

@LeonHeidelbach
Copy link

LeonHeidelbach commented Mar 20, 2023

Describe the bug
It seems like themes are not applied properly on entering Neovim. This happens when using a custom theme or one of the default themes. Apparently this only concerns specific highlight groups that are not specifically needed in Lua projects so it is only very apparent in C, Rust, Java etc. projects.

To Reproduce
Steps to reproduce the behavior:
2. Open any file containing code in one of the above mentioned languages
3. Colors are not matching the theme
4. Open the theme switcher <leader>th
5. Press Enter to re-select your default theme
6. Colors are applied correctly.

Expected behavior
The entire theme should be applied correctly on entering neovim.

Screenshots

color_scheme_does_not_load_properly.mp4

Desktop (please complete the following information):

  • Arch Linux
  • Alacritty
  • nvim 0.8.3
@ghost
Copy link

ghost commented Mar 20, 2023

I agree with this. For example, Nvchad Catppuccin has different set of colors compared to the Catppuccin theme. Seem like they are too bright. I have compared LunarVim Catppuccin against the Nvchad Catppuccin and they are not even the same. The background color is different too.

@siduck
Copy link
Member

siduck commented Mar 21, 2023

nvchad is using old version of catppuccin ( when it didnt had variants )

@siduck siduck transferred this issue from NvChad/NvChad Mar 21, 2023
@LeonHeidelbach
Copy link
Author

@hackedWifi What you are pointing out has nothing to do with what this issue is about. The issue here is that the color scheme is not applied correctly on "VimEnter" i.e. the colors from the theme file are not properly applied no matter if they are acutally the correct colors of a theme. You have to manually reload the theme by either opening the theme switcher or calling this function manually to get the correct colors specified in the theme file:

M.load_all_highlights = function()

@siduck This could maybe be related to the load order? The strange thing is though that most colors are applied correctly. Only some are left out as you can see in my video.

@siduck
Copy link
Member

siduck commented Mar 21, 2023

hi @LeonHeidelbach im unable to reproduce the issue

@siduck
Copy link
Member

siduck commented Mar 21, 2023

@LeonHeidelbach
Copy link
Author

@siduck Strange... I will investigate this further. The thing is, I have literally just made minimal adjustments to my previous setup to work with lazy and everything was fine before. I'll let you know when I find the issue...

@siduck
Copy link
Member

siduck commented Mar 21, 2023

Yea this is the first time someone has faced this issue, also FYI our highlights don't load at vimenter etc.

Can you delete old nvim dirs ( backup custom dir ) and reinstall nvchad v2.0? Also select the starter template during installation, it'll get you an example custom config.

@lucario387
Copy link

In my case, I have a lot of hlgroups that should be defined in hl_override but mistaken defined in hl_add, leading to overriden themes.
That may be one of the issues
Which is also why I genned out this file
for all the hl groups that should be inside hl_override

@LeonHeidelbach
Copy link
Author

LeonHeidelbach commented Mar 21, 2023

@siduck Of course I did all of the necessary setup stuff... everything works.

I have narrowed the issue down and I am pretty sure that this is a load order issue with Lazy. In my setup two plugins that have absolutely nothing to do with each other cause this to happen. Since Lazy basically hijacks the standard require function to load plugins on demand, I think that both of these plugins somehow depend on/call some function that resets the highlighting for specific TreeSitter nodes during Lazys setup after base46 has already applied the theme. If I disable those plugins the theme is applied correctly right out of the box.

This means that the theme should probably be applied after Lazy's setup is complete to fix this.

@LeonHeidelbach
Copy link
Author

LeonHeidelbach commented Mar 21, 2023

@siduck you can try to reproduce this issue (at least for C projects) by adding this plugin:

  { "andymass/vim-matchup",   lazy = false },

Since the lazy load order is not fixed but depends on the plugins that are loaded, which varies depending on the type of project, the issue persists in projects that use other languages.

@LeonHeidelbach
Copy link
Author

LeonHeidelbach commented Mar 21, 2023

@siduck I found the problem! It is this line:

https://github.com/NvChad/NvChad/blob/edb80f4e0ca4b05ae1053d9e5d35e0f5c168d5a4/init.lua#L22

dofile(vim.g.base46_cache .. "defaults")

This changes the highlights to some wrong default colors after the plugins have loaded.

What fixes the problem is simply applying all highlights using the load_all_highlights function instead. This points to some bug in the way colors are cached with plugins being able to change those values during initialization or simply wrong values being saved to the cache file.

-- dofile(vim.g.base46_cache .. "defaults") -- delete this line
require("base46").load_all_highlights()

@siduck
Copy link
Member

siduck commented Mar 21, 2023

@LeonHeidelbach on phone rn do can't debug, BTW the defaults highlight only contains the hl groups that aren't required by any plug-ins etc, those are just default lazy highlights.

If you load all highlights then it'd defeat the purpose of lazyloading hl groups, I'll investigate through this when I'm on my pc

@LeonHeidelbach
Copy link
Author

@siduck Sure, have a look at it as soon as you can. Whatever highlight groups are stored in the "default" cache file have incorrect values that mess up themes. Depending on when/how you save the cache file, those runtime color values (if you are taking runtime values) might have been altered by some plugins and do no longer reflect the initial colorscheme values. This causes incorrect colors to be restored on entering neovim after loading the data from your cache file. The cache file should only store fixed values from the loaded theme files that cannot be altered.

If you load all highlights then it'd defeat the purpose of lazyloading hl groups, I'll investigate through this when I'm on my pc

Yes, I know. This is just a quick fix and will defeat the purpose of lazy loading the base46 plugin. Fixing the caching problem will be the better solution.

@siduck
Copy link
Member

siduck commented Mar 23, 2023

hi @LeonHeidelbach , does this work?

 { "andymass/vim-matchup", event = "VeryLazy" }

@lucario387
Copy link

lucario387 commented Mar 23, 2023

This doesn't seem solve the underlying problem tho :bruh:
It seems to be with plugins defining hlgroups inside plugin, which lazy sourced first, and making it no longer overridable, or so it seems to me
Example https://github.com/andymass/vim-matchup/blob/master/plugin/matchup.vim`
Quick way out of this is probably to set a colors/nvchad.lua file, with the file being a carbon copy of load_all_highlights function

@siduck
Copy link
Member

siduck commented Mar 23, 2023

This doesn't seem solve the underlying problem tho :bruh: It seems to be with plugins defining hlgroups inside plugin, which lazy sourced first, and making it no longer overridable, or so it seems to me Example [https://github.com/andymass/vim-matchup/blob/master/plugin/matchup.vim](https://github.com/andymass/vim-matchup/blob/master/plugin/matchup.vim%60) Quick way out of this is probably to set a colors/nvchad.luafile, with the file being a carbon copy ofload_all_highlights` function

this is the first time we're facing this issue and its just with vim-matchup. We've had very weird issues with that plugin in the past and eventually it got removed from default plugins

@siduck
Copy link
Member

siduck commented Mar 23, 2023

dofile(vim.g.base46_cache .. "syntax") this works too

@siduck
Copy link
Member

siduck commented Mar 23, 2023

no idea how vim-matchup overrides some treesitter highlight groups

@LeonHeidelbach
Copy link
Author

This doesn't seem solve the underlying problem tho :bruh: It seems to be with plugins defining hlgroups inside plugin, which lazy sourced first, and making it no longer overridable, or so it seems to me Example [https://github.com/andymass/vim-matchup/blob/master/plugin/matchup.vim](https://github.com/andymass/vim-matchup/blob/master/plugin/matchup.vim%60) Quick way out of this is probably to set a colors/nvchad.luafile, with the file being a carbon copy ofload_all_highlights` function

this is the first time we're facing this issue and its just with vim-matchup. We've had very weird issues with that plugin in the past and eventually it got removed from default plugins

@siduck It’s not just vim-matchup though. That plugin is just one example. Apparently many of my plugins do the same thing. For C projects it was enough to remove vim-matchup and project.nvim for the highlighting to work properly again. However, I still had the same issues in Java, Lua and Rust projects. This is a more deeply nested issue and not just a one-off with a weird plugin and it is caused by incorrect cache values.

This doesn't seem solve the underlying problem tho :bruh:
It seems to be with plugins defining hlgroups inside plugin, which lazy sourced first, and making it no longer overridable, or so it seems to me
Example [https://github.com/andymass/vim-matchup/blob/master/plugin/matchup.vim](https://github.com/andymass/vim-matchup/blob/master/plugin/matchup.vim%60) Quick way out of this is probably to set a colors/nvchad.luafile, with the file being a carbon copy ofload_all_highlights` function

@lucario387 Those highlight groups can be overridden, otherwise you would not be able to restore the correct colors when changing the theme as shown in my demo video. The correct values just don’t get saved to the cache file for some reason.

@siduck Can you direct me to the code that creates and saves those cache files? I’ll have a look and see how this could be improved. I’m sure I am not the only one that experiences this issue but maybe others have just not noticed or just embraced the incorrect colors.

@siduck
Copy link
Member

siduck commented Mar 23, 2023

@LeonHeidelbach check the compile function in the main init.lua file of base46

@LeonHeidelbach
Copy link
Author

@siduck Thanks, I’ll check it out as soon as I have some time and let you know if I found a way to fix this.

@siduck
Copy link
Member

siduck commented Mar 23, 2023

@LeonHeidelbach Also, I tried project.nvim but I do not face this bug

@siduck
Copy link
Member

siduck commented Mar 23, 2023

So this is the issue only with vim-matchup i think

@LeonHeidelbach
Copy link
Author

LeonHeidelbach commented Mar 23, 2023

dofile(vim.g.base46_cache .. "syntax") this works too

@siduck I had a quick look and apparently just reapplying the syntax cache also does the trick. From what I saw the "default" cache includes all highlight groups from all non-integration plugins (correct me if I'm wrong). Apparently some plugins register some highlight groups that are relevant to the syntax highlighting, thus overriding all theme highlighting when applying the default cache. So simply applying the syntax cache last fixes the entire problem:

https://github.com/NvChad/NvChad/blob/effec96b538319c6091194f59407d6d6b265dda0/init.lua#L22

dofile(vim.g.base46_cache .. "defaults")
dofile(vim.g.base46_cache .. "syntax")

I have a custom config with project.nvim that enables some non-default stuff that might cause the same thing with this plugin.

@siduck
Copy link
Member

siduck commented Mar 23, 2023

@LeonHeidelbach i believe loading such plugins at VimEnter also prevents the issue, the syntax highlight groups are loaded only when you open a file

@LeonHeidelbach
Copy link
Author

@siduck Also I saw that you are using the deprecated function loadlines here:

file:write(loadstring(lines)())

This has been replaced by load. Do you want me to push the change or do you want to do it yourself?

@LeonHeidelbach
Copy link
Author

@LeonHeidelbach i believe loading such plugins at VimEnter also prevents the issue, the syntax highlight groups are loaded only when you open a file

Simply applying the syntax highlights last, to me seems like the more user friendly solution, given that the "VimEnter" solution even works for all plugins.

@siduck
Copy link
Member

siduck commented Mar 23, 2023

@siduck Also I saw that you are using the deprecated function loadlines here:

file:write(loadstring(lines)())

This has been replaced by load. Do you want me to push the change or do you want to do it yourself?

doesnt neovim use 5.1 lua? I've seen catppuccin and nightfox theme ( who do the same compile stuff ) use that function @nullchilly , wyt

@siduck
Copy link
Member

siduck commented Mar 23, 2023

@LeonHeidelbach i believe loading such plugins at VimEnter also prevents the issue, the syntax highlight groups are loaded only when you open a file

Simply applying the syntax highlights last, to me seems like the more user friendly solution, given that the "VimEnter" solution even works for all plugins.

hmm i think you can put dofile(syntax) in your custom/init.lua as well.

@siduck
Copy link
Member

siduck commented Mar 24, 2023

Everything is up to date. I have tried with the docker image and it works as you show, but for some reason for me it doesn't work.

very weird :/ , make another issue on this repo and also make a video in which you delete old nvim dirs and clean install nvchad & press y to the chadrc template during installation

@LeonHeidelbach
Copy link
Author

LeonHeidelbach commented Mar 24, 2023

@dgmora Do you have any other plugins installed than the default ones on the version on which it doesn't work? One of them might register a highlight group that cannot be changed later on. For me it was the same thing, just opening the theme switcher did the trick as well.

You can try this and see if it works on restarting neovim:

https://github.com/NvChad/NvChad/blob/effec96b538319c6091194f59407d6d6b265dda0/init.lua#L22

dofile(vim.g.base46_cache .. "defaults")
dofile(vim.g.base46_cache .. "syntax") -- just add this line

@siduck siduck reopened this Mar 24, 2023
@dgmora
Copy link

dgmora commented Mar 24, 2023

I'll try to make a minimal configuration to run with docker over the weekend where it can be easily reproduced

@alankopetman
Copy link

If it helps, I also found that JoosepAlviste/nvim-ts-context-commentstring causes the issue too.

@alankopetman
Copy link

hi @LeonHeidelbach , does this work?

 { "andymass/vim-matchup", event = "VeryLazy" }

This worked for me for both vim-matchup and nvim-ts-context-commentstring. However, I'm just reporting. My knowledge in this area is very limited, so I can't say whether this is a good long-term solution or not.

@LeonHeidelbach
Copy link
Author

LeonHeidelbach commented Mar 24, 2023

@alankopetman Thanks for reporting. Seeing that @siduck does not take my word for it and needs an exhaustive list of plugins that break NvChad's lazy highlight loading implementation the second users try to add custom plugins to their setup, this is quite helpful 😄. Having to add a specific event to all plugins that cause this behavior is not something that I would consider a feasable long-term solution, especially since all highlight groups that belong to a theme can and should in my opinion be set during NvChads setup and not only once a buffer is entered. This is by the way also how Folke handles theming in his example configuration for Lazy.nvim in which you can see that he specifically disables lazy loading for his theme and even annotates this decision with comments on why this is important. He also assigns a high priority to his theme plugin in order to make sure that it is also loaded before any other non-lazy loaded plugin, which people might also want to do when using NvChad. This is all done to prevent exactly the problems described in this issue. Here is the config snippet from Lazy.nvim's example config:

  {
    "folke/tokyonight.nvim",
    lazy = false, -- make sure we load this during startup if it is your main colorscheme
    priority = 1000, -- make sure to load this before all the other start plugins
    config = function()
      -- load the colorscheme here
      vim.cmd([[colorscheme tokyonight]])
    end,
  },

But seeing @siduck's reply to @lucario387's proposal for exactly that:

Counter-counter-point: Maybe it's time to just load all highlightgroups on startup? thinking

oops no :(

I don't think he is going to be willing to go that route 😄. The only solution for NvChad users now is to either make the proposed changes from this issue locally on their own or "VeryLazy"-load all of their plugins that cause this behavior.

@siduck
Copy link
Member

siduck commented Mar 24, 2023

I didn't take your word before cuz I faced this issue with vimmatchup only, tried with project.nvim but that had nothing to do with types syntax hl groups. @LeonHeidelbach but now I take your word :)

I will wait and investigate further if we could get this bug fixed without doing any workarounds / hacks.

Ok so let's start with the loading sequence in the main init.lua

require "plugins" -- loads all lazy.nvim stuff and startup plug-ins

dofile(defaults)

Now if we put vim match-up as startup plug-in and then it loads & changes some highlights groups?

Then default highlights get loaded

Then treesitter related & syntax highlight groups get loaded.

shouldn't syntax cache loading after match-up fix the issue? Like run it manually in cmd mode and it fixes it which is weird because syntax cache was already loaded.

@LeonHeidelbach
Copy link
Author

LeonHeidelbach commented Mar 24, 2023

I didn't take your word before cuz I faced this issue with vimmatchup only, tried with project.nvim but that had nothing to do with types syntax hl groups. @LeonHeidelbach but now I take your word :)

@siduck That's nice I guess, even though you should by now know that I don't just make random claims about stuff being broken if it's not or because I just don't know how things work in Lua land :D. So if I do say "take my word for it it is not just one random plugin that causes this issue" then that usually actually is the case... 🤣 People just didn't notice right away or thought it was intended to look like this. Also maybe there just are not that many people using v2.0 as of now.

I will wait and investigate further if we could get this bug fixed without doing any workarounds / hacks.

Ok so let's start with the loading sequence in the main init.lua

require "plugins" -- loads all lazy.nvim stuff and startup plug-ins

dofile(defaults)

Now if we put vim match-up as startup plug-in and then it loads & changes some highlights groups?

Yes, apparently vim-matchup alongside many other plugins register some highlight groups that interfere with highlight groups that are set by base46. Thus we should, just as Folke did in his example config and as @lucario387 proposed, load important theme highlight groups first (i.e. before any other plugin can mess with them).

Then default highlights get loaded

Then treesitter related & syntax highlight groups get loaded.

shouldn't syntax cache loading after match-up fix the issue? Like run it manually in cmd mode and it fixes it which is weird because syntax cache was already loaded.

Well apparently plugins cannot change highlight groups that were set by other plugins afterwards so the first plugin to set any particular highlight is in charge of it. If it would work like you described we would not have this issue as syntax highlights should be properly applied as soon as a buffer is entered and dofile(syntax) is called (i.e. overriding previous vim-matchup highlight groups for example).

@siduck
Copy link
Member

siduck commented Mar 24, 2023

@LeonHeidelbach ok so vim-matchup loads on start , then sets some highlight groups, then if I our syntax cache gets loaded , why dont those highlight groups set by matchup change?

But those change when u manually do lua dofile(vim.g.base46_cache .. "syntax") in command mode which is weird

@LeonHeidelbach
Copy link
Author

@siduck That does fix the syntax issue 👍 NvChad/NvChad@0678b32. @dgmora Does this also fix your issue?

@siduck
Copy link
Member

siduck commented Mar 24, 2023

@siduck That does fix the syntax issue +1 NvChad/NvChad@0678b32. @dgmora Does this also fix your issue?

very weird indeed. Defaults hl groups has no syntax related groups at all lol

@lucario387
Copy link

lucario387 commented Mar 24, 2023

Eh I don't see the reason why loading highlights needs to have priority like how folke is doing. Makes me feel like the loading order of lazy is different from neovim's default, which is weird. Just because it took over plugin loading doesn't mean it should be doing in a different order from the default.
I'm using Packer with base46 v2.0 and have not been running into this issue (yes the mystical forgotten dev branch of NvChad)

@lucario387
Copy link

Actually, @siduck, what if you load the colorgroup before loading lazy?

Like

dofile(... "defaults")
require("plugins")

@siduck
Copy link
Member

siduck commented Mar 25, 2023

@lucario387 that works too

@siduck
Copy link
Member

siduck commented Mar 25, 2023

see @LeonHeidelbach this issue wasnt due to our incorrect highlights cache but a loading order issue probably cuz of lazy.nvim 🤔

Im glad we didnt had to load all highlight groups to fix this bug xD

@LeonHeidelbach
Copy link
Author

@siduck both of these commits NvChad/NvChad@0678b32 and NvChad/NvChad@432b2c1 essentially do the same thing. The first solution just tells Lazy.nvim to apply the default highlights when loading the ui plugin, which is loaded before any other plugin due to its high priority value and non-lazyness. In the second one you simply apply all highlights manually before lazy even starts loading any other plugin which is arguably the better solution 👍.

see @LeonHeidelbach this issue wasnt due to our incorrect highlights cache but a loading order issue probably cuz of lazy.nvim thinking

The default highlight cache still contains highlight groups that it shouldn't, but now due to the new load order the respective highlight groups get properly overridden by all the other theme highlights 😉. The main issue here was that before, the default cache was applied at the end, when syntax highlights etc. had already been applied, thus overriding some groups with its incorrect values. Anyways, I'm glad this is fixed now 😄.

@lucario387
Copy link

lucario387 commented Mar 25, 2023

Tbh, Im still surprised? that Lazy loads all plugins/ and stuffs before default config. Packer doesn't modify the default loading order of vim/neovim, so it was easier to control there.

@LeonHeidelbach
Copy link
Author

@lucario387 You are absolutely right. This behavior actually messes with some plugins. I already had this problem come up with Dashboard.nvim and Folke even explained why he does it like that nvimdev/dashboard-nvim#222 (comment). In any case, I would also prefer if he would load the config first. This messes with some functionality in my own plugin TrailBlazer.nvim when auto-saving sessions. Users should be able to enable this feature through their config, but as the config is loaded after requiring all plugin/ files the default value is always false.

@lucario387
Copy link

lucario387 commented Mar 25, 2023

It's just that
instead of user explicitly saying packadd
now it's user explicitly having to find ways to not have packadd to be triggered

@LeonHeidelbach
Copy link
Author

Yeah it would've been nice to have Lazy behave like Packer in this regard. It would enable a more seemless transition between both package managers but Folke seems pretty set on his opinion that plugin files should be sourced first.

@lucario387
Copy link

I mean, from what it looks like, Packer v2.0 also seems to load plugins the way Lazy does :)
So maybe it's for the better

Vealcoo pushed a commit to Vealcoo/NvChad that referenced this issue Apr 21, 2023
fixes NvChad/base46#173, not adding this will let plugins like vim-matchup messup our syntax highlight groups
camfrey pushed a commit to camfrey/NvFrey that referenced this issue Jun 23, 2023
fixes NvChad/base46#173, not adding this will let plugins like vim-matchup messup our syntax highlight groups
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

No branches or pull requests

6 participants