Specify optional 'baseVimBackground' for themes (#2083)

* Update solarized theme

* Fix background color for solarized

* Fix lint issues

* Add test

* Fix bug when setting colorscheme, where the colorscheme would not always be reported correctly

* Remove file accidentally committed

browser/src/Editor/NeovimEditor/NeovimEditor.tsx

@@ -61,7 +61,7 @@ import {
 } from "./../../Services/SyntaxHighlighting"
 
 import { MenuManager } from "./../../Services/Menu"
-import { ThemeManager } from "./../../Services/Themes"
+import { IThemeMetadata, ThemeManager } from "./../../Services/Themes"
 import { TypingPredictionManager } from "./../../Services/TypingPredictionManager"
 import { Workspace } from "./../../Services/Workspace"
 
@@ -122,6 +122,7 @@ export class NeovimEditor extends Editor implements IEditor {
     private _windowManager: NeovimWindowManager
 
     private _currentColorScheme: string = ""
+    private _currentBackground: string = ""
     private _isFirstRender: boolean = true
 
     private _lastBufferId: string = null
@@ -971,15 +972,17 @@ export class NeovimEditor extends Editor implements IEditor {
         this._themeManager.onThemeChanged.subscribe(() => {
             const newTheme = this._themeManager.activeTheme
 
-            if (newTheme.baseVimTheme && newTheme.baseVimTheme !== this._currentColorScheme) {
-                this._neovimInstance.command(":color " + newTheme.baseVimTheme)
+            if (
+                newTheme.baseVimTheme &&
+                (newTheme.baseVimTheme !== this._currentColorScheme ||
+                    newTheme.baseVimBackground !== this._currentBackground)
+            ) {
+                this.setColorSchemeFromTheme(newTheme)
             }
         })
 
         if (this._themeManager.activeTheme && this._themeManager.activeTheme.baseVimTheme) {
-            await this._neovimInstance.command(
-                ":color " + this._themeManager.activeTheme.baseVimTheme,
-            )
+            await this.setColorSchemeFromTheme(this._themeManager.activeTheme)
         }
 
         if (filesToOpen && filesToOpen.length > 0) {
@@ -998,6 +1001,18 @@ export class NeovimEditor extends Editor implements IEditor {
         this._scheduleRender()
     }
 
+    public async setColorSchemeFromTheme(theme: IThemeMetadata): Promise<void> {
+        if (
+            (theme.baseVimBackground === "dark" || theme.baseVimBackground === "light") &&
+            theme.baseVimBackground !== this._currentBackground
+        ) {
+            await this._neovimInstance.command(":set background=" + theme.baseVimBackground)
+            this._currentBackground = theme.baseVimBackground
+        }
+
+        await this._neovimInstance.command(":color " + theme.baseVimTheme)
+    }
+
     public getBuffers(): Array<Oni.Buffer | Oni.InactiveBuffer> {
         return this._bufferManager.getBuffers()
     }
@@ -1216,6 +1231,12 @@ export class NeovimEditor extends Editor implements IEditor {
 
     private async _onColorsChanged(): Promise<void> {
         const newColorScheme = await this._neovimInstance.eval<string>("g:colors_name")
+
+        // In error cases, the neovim API layer returns an array
+        if (typeof newColorScheme !== "string") {
+            return
+        }
+
         this._currentColorScheme = newColorScheme
         const backgroundColor = this._screen.backgroundColor
         const foregroundColor = this._screen.foregroundColor

browser/src/Plugins/Api/Oni.ts

@@ -60,7 +60,6 @@ export class Oni implements OniApi.Plugin.Api {
     private _dependencies: Dependencies
     private _ui: Ui
     private _services: Services
-    private _colors: Colors
 
     public get achievements(): any /* TODO: Promote to API */ {
         return getAchievementsInstance()
@@ -71,7 +70,7 @@ export class Oni implements OniApi.Plugin.Api {
     }
 
     public get colors(): Colors /* TODO: Promote to API */ {
-        return this._colors
+        return getColors()
     }
 
     public get commands(): OniApi.Commands.Api {
@@ -179,8 +178,6 @@ export class Oni implements OniApi.Plugin.Api {
     }
 
     constructor() {
-        this._colors = getColors()
-
         this._dependencies = new Dependencies()
         this._ui = new Ui(react)
         this._services = new Services()

browser/src/Services/Themes/ThemeManager.ts

@@ -283,9 +283,14 @@ export const DefaultThemeColors: IThemeColors = {
     "editor.tokenColors": [],
 }
 
+// Value used to determine whether the base Vim theme
+// should be set to 'dark' or 'light'
+export type VimBackground = "light" | "dark"
+
 export interface IThemeMetadata {
     name: string
     baseVimTheme?: string
+    baseVimBackground?: VimBackground
     colors: Partial<IThemeColors>
     tokenColors: TokenColor[]
 }

extensions/theme-solarized/colors/solarized8_dark.json

@@ -1,6 +1,7 @@
 {
     "name": "solarized8_dark",
-    "baseVimTheme": "solarized8_dark",
+    "baseVimTheme": "solarized8",
+    "baseVimBackground": "dark",
     "colors": {
         "background": "#073642",
         "foreground": "#839496",

extensions/theme-solarized/colors/solarized8_dark.vim

@@ -1,4 +0,0 @@
-let s:dir = expand('<sfile>:p:h').(!exists("+shellslash") || &shellslash ? '/' : '\')
-set background=dark
-execute "source" s:dir."solarized8.vim"
-unlet s:dir

extensions/theme-solarized/colors/solarized8_light.json

@@ -1,17 +1,18 @@
 {
     "name": "solarized8_light",
-    "baseVimTheme": "solarized8_light",
+    "baseVimTheme": "solarized8",
+    "baseVimBackground": "light",
     "colors": {
-        "background": "#073642",
+        "background": "#eee8d5",
         "foreground": "#839496",
 
-        "title.background": "#002b36",
+        "title.background": "#eee8d5",
         "title.foreground": "#839496",
 
-        "editor.background": "#002b36",
-        "editor.foreground": "#586e75",
+        "editor.background": "#eee8d5",
+        "editor.foreground": "#839496",
 
-        "tabs.background": "#002b36",
+        "tabs.background": "#eee8d5",
         "tabs.foreground": "#839496",
 
         "toolTip.background": "#002b36",

extensions/theme-solarized/colors/solarized8_light.vim

@@ -1,4 +0,0 @@
-let s:dir = expand('<sfile>:p:h').(!exists("+shellslash") || &shellslash ? '/' : '\')
-set background=light
-execute "source" s:dir."solarized8.vim"
-unlet s:dir

test/CiTests.ts

@@ -53,6 +53,8 @@ const CiTests = [
     "TextmateHighlighting.ScopesOnEnterTest",
     "TextmateHighlighting.TokenColorOverrideTest",
 
+    "Theming.LightAndDarkColorsTest",
+
     // This test occasionally hangs and breaks tests after - trying to move it later...
     "LargeFileTest",
 ]

test/ci/Theming.LightAndDarkColorsTest.ts

@@ -0,0 +1,46 @@
+/**
+ * Test script to validate themings are properly set for light/dark versions of theme
+ */
+
+import * as React from "react"
+
+import * as assert from "assert"
+import * as os from "os"
+import * as path from "path"
+
+import * as Oni from "oni-api"
+
+import { createNewFile } from "./Common"
+
+const getBackgroundColor = (oni: Oni.Plugin.Api): string => {
+    return oni.colors.getColor("editor.background")
+}
+
+export const test = async (oni: Oni.Plugin.Api) => {
+    await oni.automation.waitForEditors()
+
+    // TODO: Should we expose `request` as an API method?
+    const neovimAsAny: any = oni.editors.activeEditor.neovim
+
+    // Set theme to solarized light, validate background color
+    oni.configuration.setValues({ "ui.colorscheme": "solarized8_light" })
+    await oni.automation.waitFor(() => getBackgroundColor(oni) === "#eee8d5")
+
+    let background: string = await neovimAsAny.request("nvim_get_option", ["background"])
+    assert.strictEqual(background, "light")
+
+    // Switch back to dark, validate the color was changed
+    oni.configuration.setValues({ "ui.colorscheme": "solarized8_dark" })
+    await oni.automation.waitFor(() => getBackgroundColor(oni) === "#073642")
+
+    background = await neovimAsAny.request("nvim_get_option", ["background"])
+    assert.strictEqual(background, "dark")
+
+    // Switch back to light
+    oni.configuration.setValues({ "ui.colorscheme": "solarized8_light" })
+    await oni.automation.waitFor(() => getBackgroundColor(oni) === "#eee8d5")
+    background = await neovimAsAny.request("nvim_get_option", ["background"])
+    assert.strictEqual(background, "light")
+
+    assert.ok(true, "Color switches were successful!")
+}