Configuration option to set theme to light/dark/auto

[mgax]

Fixes #1912. I've tested on MacOS and it works as intended.

For existing users, nothing changes; the user must explicitly set the --theme command-line option, the NEOVIDE_THEME env var, or the theme option in config.toml g:neovide_theme = 'auto' to enable this new feature.

[mgax]

I've got the tests to pass, the --theme option is now truly optional. The nightly CI jobs are failing but I suspect it may not be caused by the PR itself: https://github.com/mgax/neovide/actions/runs/5441501264.

---

I have a stylistic question: would it be better to rewrite the pattern matching like so? https://gist.github.com/mgax/0506394f61f5d47f856803cc52213a7a I'm guessing it's nice to have fewer calls to set_background, but maybe the convoluted code is not worth it?

[MultisampledNight]

I have a stylistic question: would it be better to rewrite the pattern matching like so? https://gist.github.com/mgax/0506394f61f5d47f856803cc52213a7a I'm guessing it's nice to have fewer calls to set_background, but maybe the convoluted code is not worth it?

The first hunk looks better to me, I think I'd even split up the matching part and actually setting the background by storing the match result first, shadowing theme:

-                    match theme {
-                        Theme::Light => set_background("light"),
-                        Theme::Dark => set_background("dark"),
-                    };
+                    let theme = match theme {
+                        Theme::Light => "light",
+                        Theme::Dark => "dark",
+                    };
+                    set_background(theme);

The second hunk however looks too convoluted to me. I need to backtrack in order to see what braces will actually trigger the background change.

Sidenote: Whatever you end up with, do run rustfmt afterwards, , after blocks are deleted by it.

[MultisampledNight]

Generally I'm not too sure if passing the argument on the command line is really the way to go, since it could also be desired to be configurable with the casual g:neovide_* variable mechanism. But I don't feel strongly about that either way.

[MultisampledNight]

Indeed, the CI failure is not your fault, it also happens locally for me, also on main. No need to do anything about this, #1899 fixes this already.

[mgax]

Generally I'm not too sure if passing the argument on the command line is really the way to go, since it could also be desired to be configurable with the casual g:neovide_* variable mechanism. But I don't feel strongly about that either way.

Hmm, I see what you mean. I wasn't aware of the g:neovide_* variables, but looking at what is already available (fullscreen, transparency, background color, scaling), theme seems to fit right in. I don't mind making the change, in fact it's tempting, but I'd prefer if you, or "the core dev team", make the decision.

The first hunk looks better to me, I think I'd even split up the matching part and actually setting the background by storing the match result first, shadowing theme:

Yep sounds good. I've used background as variable name because it's the name used on the vim side.

---

Thanks for reviewing and for the suggestions! Let me know if I should switch to a g:neovide_theme variable.

[MultisampledNight]

Fwiw, I don't have any kind of permissions or right to go beyond technical reasons when discussing about implementing something one way or the other. Ideally there could also be anyone else in my place reviewing this. It just happens to be me in this moment, a random person not necessarily affiliated with Neovide.

Other than that, yeah, for the sake of consistency and the variables being configurable from Neovim itself for custom mechanisms, the g:neovide_theme solution sounds better to me.

[mgax]

I'm sorry for having pressed you, and thanks for the clarification. I've updated the PR to use a setting.

[MultisampledNight]

I'll take a final look over this in 22 hours or the like, not really in the right state of mind to hit the merge button.

[falcucci]

@mgax really cool feature, I just want to point out that some colorschemes doesnt support light themes or vice-versa. So isnt only about changing the background.

In that case you have more components that could be a mess after updating the background as the Font color, Floaterm and Pmenu.

for example one of the most famous colorscheme dracula hasnt support for light mode.

So the user could choose a different one accordingly to the background.

I think that this should be more customizable, as the same as auto-dark-mode.nvim which works really well but is available only for macos.

but look the hook itself

set_dark_mode = function()
    vim.api.nvim_set_option('background', 'dark')
    vim.cmd('colorscheme gruvbox')
end,
set_light_mode = function()
    vim.api.nvim_set_option('background', 'light')
    vim.cmd('colorscheme gruvbox')
end

I think that something similar should be done here.

[falcucci]

And yet let's keep in mind that neovide is supposed to be fully compatible with neovim, not saying that this feature would bring incompatibility, but it's something to consider.

As a neovim user I would like to have always as possible my neovim configs working out of any neovide feature (themes is one of them).

[falcucci]

to be really concise about it, I think the perfect solution would be a neovim config (lua) or plugin as auto-dark-mode.nvim but available for all platforms.

[MultisampledNight]

Since the background option is set, and that's observable from Neovim, I suppose you could try to add an autocmd listening on OptionSet on any filetype. This wouldn't require any intervention on the Neovide side.

[mgax]

@falcucci that's interesting; I'm using catppuccin, which has support for both light and dark backgrounds, and flips between them automatically, but it makes sense that you may want more control. In that case @MultisampledNight's suggestion seems like the way to go.

From a purity perspective, auto-dark-mode.nvim polls the system to keep the theme in sync, which is a bit wasteful. The editor already receives events from the system when the theme changes, in a cross-platform compatible way, so it feels natural to automatically update the equivalent option in neovim.

[falcucci]

@MultisampledNight mine would work just fine because I would ignore the --theme option and keep my setup with auto-dark-mode.nvim, but for people using linux, windows they should adapt there configs to work properly as you said for any neovide applied changes as the background.

the feature itself is nice, I am just wondering about which side put it, neovim or neovide.

btw I'll test this PR to see if I get something.

[falcucci]

@mgax yeah but I think the polling is needed because who receives the event is the terminal (if not used within neovide), right?

[mgax]

@falcucci the implementation changed, it's no longer a command-line option (--theme), it's a neovim option, g:neovide_theme. I've updated the PR's description to reflect that.

but for people using linux, windows they should adapt there configs to work properly

FWIW, it looks like winit supports the ThemeChanged event on MacOS, Windows and Web. Linux support is discussed here: rust-windowing/winit#1549 (comment).

In any case, this patch doesn't change behaviour out of the box, it has to be enabled explicitly in the user's config.

[falcucci]

@mgax thank you for clarify it.

I just tried out the feature and want to point out that maybe we should put a note in the docs about the g:neovide_background_color to avoid confusion.

The background will be correctly applied only if it has no g:neovide_background_color or if they have the same value.

besides it, I dont see any issue for now.

[MultisampledNight]

Thank you!

[mgax]

@MultisampledNight @falcucci thank you both!