Trouble porting Gruvbox theme to Oni

[parkerault]

I'm attempting to port the Gruvbox theme to Oni, and it mostly works, but some of the chrome colors are getting borked. You can see in the following screenshots; I have vscode open on the side so you can see the actual hex colors, and the colors as rendered by Oni. It looks to me like Oni is overriding the colors set in the json with its best guess colorscheme import method. I've put the theme on github here: https://github.com/parkerault/onivim-theme-gruvbox

Dark theme: mostly looks okay but the normal mode background color is wrong:

Light theme: the window chrome background is definitely not the color specified in the json...

Any ideas?

[bryphe]

Porting over some comments from discord 😄

Just cloned this and checked it out:

1) One thing that threw me off initially was that its listed in the package.json as gruvboxdark instead of gruvbox_dark - I changed both of those entries to match the name in the gruvbox_dark.json and gruvbox_light.json

2) The baseVimTheme in the colors.json has to match the colorscheme that Vim reports, otherwise it thinks its just a random colorscheme and uses the inferred colors like you mentioned in the bug So I changed the baseVimTheme to gruvbox and at least got the chrome colors working (I switched some to #FFFFFF and #000000 just as a sanity check)

I think we need better logging around both these bullet points!

Also... it seems like what I tried for 2 isn't a complete fix - since it is a light background by default.

I think we might want to add a flag like mode: "light" or background: "light" - then, when we set the theme, we can set the background too. That would make it so that you don't need to split out the gruvbox_dark.vim and gruvbox_light.vim files - you'd still need the two json files, but the baseVimTheme would be gruvbox for both, and we'd just change that mode/background setting. That would hopefully make it less awkward and more explicit when bringing over existing themes - what do you think about that?

Otherwise, the theme + colors look awesome! Nice work on the styling. This seems like another good candidate to bring in as a first-class packaged theme 😄

[parkerault]

Doh, Of course! I think I was doing an over-eager find and replace. So now there's just one issue; the sidebar icons for explorer, etc are all rendering as white and they should be... what property sets the foreground color for those icons?

[parkerault]

@bryphe actually, I think the original version was correct since there is a gruvbox_dark.vim and gruvbox_light.vim in the colors directory; this is based on the solarized theme by @Akin909. It should set the background value appropriately and source the original theme file... Maybe this is where the error is?

https://github.com/parkerault/onivim-theme-gruvbox/blob/master/colors/gruvbox_dark.vim

[parkerault]

@bryphe Maybe the theme file could optionally pass "colors.dark" and "colors.light" for themes that have a light and dark mode, and we could use a naming convention for the theme; like if the theme string in the config is "gruvbox.dark", Oni can run "set background=dark" before sourcing the vim theme? This would be backwards compatible with existing themes and wouldn't require an additional configuration option. Just a thought. Since @Akin909 has already dealt with this maybe he has some ideas?

{
  "name": "gruvbox",
  "baseVimTheme": "gruvbox",
  "colors.dark": {
    "background": "#282828",
    "foreground": "#ebdbb2",
    ...
  }
  "colors.light": {
    "background": "#ebdbb2",
    "foreground": "#282828",
    ...
  }
}

[bryphe]

FYI - I posted a PR of what I was thinking about - #2083

This lets the theme explicitly call out whether the background is dark or light, so in your theme you could use:

{
  "name": "gruvbox_dark",
  "baseVimTheme": "gruvbox",
  "baseVimBackground": "dark"
}

This would help us to no longer require those wrapper VimL files. I tried out the PR with your theme (with the modifications above), and it worked! 👍 The wrapper VimL files are unfortunately fragile as you called out - they don't set the reported colorscheme (so even if we're running gruvbox_dark.vim, Vim still reports it as gruvbox - we'd need to additional work... It's preferable IMO to just get out of that business).

The PR actually exposed a bug, too - the solarized8_light.json actually has a bunch of dark colors - but because the solarized8 (colorscheme name) didn't match the solarized8_light in the solarized8_light.json, it would use the _inferred_colors, so that was tucked away.

Maybe the theme file could optionally pass "colors.dark" and "colors.light" for themes that have a light and dark mode, and we could use a naming convention for the theme; like if the theme string in the config is "gruvbox.dark",

Hmm, I think this might be nice ergonomically (to avoid have two json files, and to implicitly specify what the 'background' setting should be without the explicit baseVimBackground), but it seems orthogonal (in other words, I don't think it would solve the root problem).

[parkerault]

Sounds great! I will update the theme json and wait until #2083 is merged. Thanks for the quick action!

[bryphe]

Thanks @parkerault for your work on this! I'm assuming this is good now, since we have gruvbox and hybrid in master now! 💯

[parkerault]

Actually there's still the issue with the icons and light themes... If the file explorer etc are using font awesome for their icons you should be able to just change the CSS for those depending on the background setting. Not sure about the seti icons though...