Improve notebook colors and UX (#3914)

This was reviewed / worked on with Smitha and will be signed off on by PM via mail.
1 thing left (make run button look better when not selected) will be one in separate review.

Changes
- Add top/bottom padding to editor so it's not cramped
- Added an (on by default) setting `notebook.overrideEditorTheming`. This controls whether new colors etc. are used for notebook editors or if users should see vanilla UI like in standard editor. Settings under this flag are:
  - When unselected, editor has same color as toolbar. On selection it goes back to regular editor view so colors work "right"
  - In standard light/dark themes we now use a filled in background color instead of border box.

extensions/notebook/package.json

@@ -26,6 +26,11 @@
 					"type": "boolean",
 					"default": false,
 					"description": "%notebook.sqlKernelEnabled.description%"
+				},
+				"notebook.overrideEditorTheming": {
+					"type": "boolean",
+					"default": true,
+					"description": "%notebook.overrideEditorTheming.description%"
 				}
 			}
 		},

extensions/notebook/package.nls.json

@@ -4,6 +4,7 @@
 	"notebook.configuration.title": "Notebook configuration",
 	"notebook.pythonPath.description": "Local path to python installation used by Notebooks.",
 	"notebook.sqlKernelEnabled.description": "Enable SQL kernel in notebook editor (Preview). Requires reloading this window to take effect",
+	"notebook.overrideEditorTheming.description": "Override editor default settings in the Notebook editor. Settings include background color, current line color and border",
 	"notebook.command.new": "New Notebook",
 	"notebook.command.open": "Open Notebook",
 	"notebook.analyzeJupyterNotebook": "Analyze in Notebook",

src/sql/parts/notebook/cellViews/code.css

@@ -41,6 +41,9 @@ code-component .toolbarIconStop {
 	background-image: url('../media/dark/stop_cell_inverse.svg');
 }
 
+code-component .editor {
+	padding: 5px 0px 5px 0px
+}
 /* overview ruler */
 code-component .monaco-editor .decorationsOverviewRuler {
 	visibility: hidden;

src/sql/parts/notebook/notebook.component.ts

@@ -3,8 +3,6 @@
 *  Licensed under the Source EULA. See License.txt in the project root for license information.
 *--------------------------------------------------------------------------------------------*/
 
-import 'sql/parts/notebook/notebookStyles';
-
 import { OnInit, Component, Inject, forwardRef, ElementRef, ChangeDetectorRef, ViewChild, OnDestroy } from '@angular/core';
 
 import { IColorTheme, IWorkbenchThemeService } from 'vs/workbench/services/themes/common/workbenchThemeService';

src/sql/parts/notebook/notebookStyles.ts

@@ -5,13 +5,18 @@
 import 'vs/css!./notebook';
 
 import { registerThemingParticipant, ITheme, ICssStyleCollector } from 'vs/platform/theme/common/themeService';
-import { SIDE_BAR_BACKGROUND } from 'vs/workbench/common/theme';
+import { SIDE_BAR_BACKGROUND, SIDE_BAR_SECTION_HEADER_BACKGROUND, EDITOR_GROUP_HEADER_TABS_BACKGROUND } from 'vs/workbench/common/theme';
-import { activeContrastBorder, contrastBorder, buttonBackground, textLinkForeground } from 'vs/platform/theme/common/colorRegistry';
+import { activeContrastBorder, contrastBorder, buttonBackground, textLinkForeground, editorBackground } from 'vs/platform/theme/common/colorRegistry';
+import { IDisposable } from 'vscode-xterm';
+import { editorLineHighlight, editorLineHighlightBorder } from 'vs/editor/common/view/editorColorRegistry';
 
-registerThemingParticipant((theme: ITheme, collector: ICssStyleCollector) => {
+export function registerNotebookThemes(overrideEditorThemeSetting: boolean): IDisposable {
+	return registerThemingParticipant((theme: ITheme, collector: ICssStyleCollector) => {
 
 		let lightBoxShadow = '0px 4px 6px 0px rgba(0,0,0,0.14)';
 		let darkBoxShadow = '0 4px 6px 0px rgba(0, 0, 0, 1)';
+		let addBorderToInactiveCodeCells = true;
+
 		// Active border
 		const activeBorder = theme.getColor(buttonBackground);
 		if (activeBorder) {
@@ -50,10 +55,62 @@ registerThemingParticipant((theme: ITheme, collector: ICssStyleCollector) => {
 			}
 		`);
 
+		const inactiveBorder = theme.getColor(SIDE_BAR_BACKGROUND);
+		const sidebarColor = theme.getColor(SIDE_BAR_SECTION_HEADER_BACKGROUND);
+		const notebookLineHighlight = theme.getColor(EDITOR_GROUP_HEADER_TABS_BACKGROUND);
+		// Code editor style overrides - only applied if user chooses this as preferred option
+		if (overrideEditorThemeSetting) {
+			let lineHighlight = theme.getColor(editorLineHighlight);
+			if (!lineHighlight || lineHighlight.isTransparent()) {
+				// Use notebook color override
+				lineHighlight = notebookLineHighlight;
+				if (lineHighlight) {
+					collector.addRule(`code-component .monaco-editor .view-overlays .current-line { background-color: ${lineHighlight}; border: 0px; }`);
+				}
+			} // else do nothing as current theme's line highlight will work
+
+			if (theme.defines(editorLineHighlightBorder) && theme.type !== 'hc') {
+				// We need to clear out the border because we do not want to show it for notebooks
+				// Override values only for the children of code-component so regular editors aren't affected
+				collector.addRule(`code-component .monaco-editor .view-overlays .current-line { border: 0px; }`);
+			}
+
+			// Override code editor background if color is defined
+			let codeBackground = inactiveBorder; // theme.getColor(EDITOR_GROUP_HEADER_TABS_BACKGROUND);
+			if (codeBackground) {
+				// Main background
+				collector.addRule(`.notebook-cell:not(.active) code-component { background-color: ${codeBackground}; }`);
+				collector.addRule(`
+					.notebook-cell:not(.active) code-component .monaco-editor,
+					.notebook-cell:not(.active) code-component .monaco-editor-background,
+					.notebook-cell:not(.active) code-component .monaco-editor .inputarea.ime-input
+					{
+						background-color: ${codeBackground};
+					}`);
+				// Margin background will be the same (may override some styles)
+				collector.addRule(`.notebook-cell:not(.active) code-component .monaco-editor .margin { background-color: ${codeBackground}; }`);
+				addBorderToInactiveCodeCells = false;
+			}
+		}
 
 		// Inactive border
-	const inactiveBorder = theme.getColor(SIDE_BAR_BACKGROUND);
 		if (inactiveBorder) {
+			// Standard notebook cell behavior
+			collector.addRule(`
+				.notebookEditor .notebook-cell {
+					border-color: ${inactiveBorder};
+					border-width: 0px;
+				}
+				.notebookEditor .notebook-cell.active {
+					border-width: 1px;
+				}
+				.notebookEditor .notebook-cell:hover {
+					border-width: 1px;
+				}
+			`);
+
+			if (addBorderToInactiveCodeCells) {
+				// Sets a border for the editor component if we don't have a custom color for editor instead
 				collector.addRule(`
 					.notebookEditor .notebook-cell code-component {
 						border-color: ${inactiveBorder};
@@ -69,25 +126,17 @@ registerThemingParticipant((theme: ITheme, collector: ICssStyleCollector) => {
 						border-width: 0px 0px 1px 0px;
 						border-radius: 0px;
 					}
-			.notebookEditor .notebook-cell {
-				border-color: ${inactiveBorder};
-				border-width: 0px;
-			}
-			.notebookEditor .notebook-cell.active {
-				border-width: 1px;
-			}
-			.notebookEditor .notebook-cell:hover {
-				border-width: 1px;
-			}
-		`);
-		// toolbar color set only when active
-		collector.addRule(`
-			code-component .toolbar {
-				background-color: ${inactiveBorder};
-			}
 				`);
 			}
 
+		}
+
+		// Sidebar and cell outline toolbar color set only when active
+		collector.addRule(`
+			.notebook-cell.active code-component .toolbar {
+				background-color: ${sidebarColor};
+			}
+		`);
 		// Styling with Outline color (e.g. high contrast theme)
 		const outline = theme.getColor(activeContrastBorder);
 		const hcOutline = theme.getColor(contrastBorder);
@@ -125,4 +174,5 @@ registerThemingParticipant((theme: ITheme, collector: ICssStyleCollector) => {
 			}
 			`);
 		}
-});
+	});
+}

src/sql/workbench/services/notebook/common/notebookServiceImpl.ts

@@ -24,7 +24,7 @@ import { Memento } from 'vs/workbench/common/memento';
 import { IStorageService } from 'vs/platform/storage/common/storage';
 import { IExtensionService } from 'vs/workbench/services/extensions/common/extensions';
 import { IExtensionManagementService, IExtensionIdentifier } from 'vs/platform/extensionManagement/common/extensionManagement';
-import { Disposable } from 'vs/base/common/lifecycle';
+import { Disposable, IDisposable } from 'vs/base/common/lifecycle';
 import { getIdFromLocalExtensionId } from 'vs/platform/extensionManagement/common/extensionManagementUtil';
 import { Deferred } from 'sql/base/common/promise';
 import { SqlSessionManager } from 'sql/workbench/services/notebook/common/sqlSessionManager';
@@ -35,6 +35,11 @@ import { NotebookEditorVisibleContext } from 'sql/workbench/services/notebook/co
 import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
 import { NotebookEditor } from 'sql/parts/notebook/notebookEditor';
 import { IEditorGroupsService } from 'vs/workbench/services/group/common/editorGroupsService';
+import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
+import { IThemeService } from 'vs/platform/theme/common/themeService';
+import { registerNotebookThemes } from 'sql/parts/notebook/notebookStyles';
+
+const OVERRIDE_EDITOR_THEMING_SETTING = 'notebook.overrideEditorTheming';
 
 export interface NotebookProviderProperties {
 	provider: string;
@@ -89,6 +94,8 @@ export class NotebookService extends Disposable implements INotebookService {
 	private _registrationComplete = new Deferred<void>();
 	private _isRegistrationComplete = false;
 	private notebookEditorVisible: IContextKey<boolean>;
+	private _themeParticipant: IDisposable;
+	private _overrideEditorThemeSetting: boolean;
 
 	constructor(
 		@IStorageService private _storageService: IStorageService,
@@ -97,7 +104,9 @@ export class NotebookService extends Disposable implements INotebookService {
 		@IInstantiationService private _instantiationService: IInstantiationService,
 		@IContextKeyService private _contextKeyService: IContextKeyService,
 		@IEditorService private readonly _editorService: IEditorService,
-		@IEditorGroupsService private readonly _editorGroupsService: IEditorGroupsService
+		@IEditorGroupsService private readonly _editorGroupsService: IEditorGroupsService,
+		@IConfigurationService private readonly _configurationService: IConfigurationService,
+		@IThemeService private readonly _themeService: IThemeService
 	) {
 		super();
 		this._register(notebookRegistry.onNewRegistration(this.updateRegisteredProviders, this));
@@ -114,9 +123,17 @@ export class NotebookService extends Disposable implements INotebookService {
 			this._register(extensionManagementService.onDidUninstallExtension(({ identifier }) => this.removeContributedProvidersFromCache(identifier, extensionService)));
 		}
 		this.hookContextKeyListeners();
+		this.hookNotebookThemesAndConfigListener();
 	}
 
-	private hookContextKeyListeners() {
+	public dispose(): void {
+		super.dispose();
+		if (this._themeParticipant) {
+			this._themeParticipant.dispose();
+		}
+	}
+
+	private hookContextKeyListeners(): void {
 		const updateEditorContextKeys = () => {
 			const visibleEditors = this._editorService.visibleControls;
 			this.notebookEditorVisible.set(visibleEditors.some(control => control.getId() === NotebookEditor.ID));
@@ -132,6 +149,30 @@ export class NotebookService extends Disposable implements INotebookService {
 		}
 	}
 
+	private hookNotebookThemesAndConfigListener(): void {
+		if(this._configurationService) {
+			this.updateNotebookThemes();
+			this._register(this._configurationService.onDidChangeConfiguration(e => {
+				if (e.affectsConfiguration(OVERRIDE_EDITOR_THEMING_SETTING)) {
+					this.updateNotebookThemes();
+				}
+			}));
+		}
+	}
+
+	private updateNotebookThemes() {
+		let overrideEditorSetting = this._configurationService.getValue<boolean>(OVERRIDE_EDITOR_THEMING_SETTING);
+		if (overrideEditorSetting !== this._overrideEditorThemeSetting) {
+			// Re-add the participant since this will trigger update of theming rules, can't just
+			// update something and ask to change
+			if (this._themeParticipant) {
+				this._themeParticipant.dispose();
+			}
+			this._overrideEditorThemeSetting = overrideEditorSetting;
+			this._themeParticipant = registerNotebookThemes(overrideEditorSetting);
+		}
+	}
+
 	private updateRegisteredProviders(p: { id: string; registration: NotebookProviderRegistration; }) {
 		let registration = p.registration;