From a2c73771349e0cbe6e11523cd7688396f37e302f Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Tue, 5 Feb 2019 17:51:42 -0800 Subject: [PATCH] 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 | 5 + extensions/notebook/package.nls.json | 1 + src/sql/parts/notebook/cellViews/code.css | 3 + src/sql/parts/notebook/notebook.component.ts | 2 - src/sql/parts/notebook/notebookStyles.ts | 262 +++++++++++------- .../notebook/common/notebookServiceImpl.ts | 47 +++- 6 files changed, 209 insertions(+), 111 deletions(-) diff --git a/extensions/notebook/package.json b/extensions/notebook/package.json index a968bc20a6..c54fe1c24b 100644 --- a/extensions/notebook/package.json +++ b/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%" } } }, diff --git a/extensions/notebook/package.nls.json b/extensions/notebook/package.nls.json index 98351cfd86..b362246006 100644 --- a/extensions/notebook/package.nls.json +++ b/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", diff --git a/src/sql/parts/notebook/cellViews/code.css b/src/sql/parts/notebook/cellViews/code.css index 4048326fc6..38a2e8b0a1 100644 --- a/src/sql/parts/notebook/cellViews/code.css +++ b/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; diff --git a/src/sql/parts/notebook/notebook.component.ts b/src/sql/parts/notebook/notebook.component.ts index 9763b8a3e9..4d0a52e5ce 100644 --- a/src/sql/parts/notebook/notebook.component.ts +++ b/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'; diff --git a/src/sql/parts/notebook/notebookStyles.ts b/src/sql/parts/notebook/notebookStyles.ts index 45f4fa6deb..a73c3b2ce2 100644 --- a/src/sql/parts/notebook/notebookStyles.ts +++ b/src/sql/parts/notebook/notebookStyles.ts @@ -5,124 +5,174 @@ import 'vs/css!./notebook'; import { registerThemingParticipant, ITheme, ICssStyleCollector } from 'vs/platform/theme/common/themeService'; -import { SIDE_BAR_BACKGROUND } from 'vs/workbench/common/theme'; -import { activeContrastBorder, contrastBorder, buttonBackground, textLinkForeground } from 'vs/platform/theme/common/colorRegistry'; +import { SIDE_BAR_BACKGROUND, SIDE_BAR_SECTION_HEADER_BACKGROUND, EDITOR_GROUP_HEADER_TABS_BACKGROUND } from 'vs/workbench/common/theme'; +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)'; - // Active border - const activeBorder = theme.getColor(buttonBackground); - if (activeBorder) { + 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) { + collector.addRule(` + .notebookEditor .notebook-cell.active { + border-color: ${activeBorder}; + border-width: 1px; + } + `); + } + + // Box shadow handling collector.addRule(` .notebookEditor .notebook-cell.active { - border-color: ${activeBorder}; - border-width: 1px; + box-shadow: ${lightBoxShadow}; + } + + .vs-dark .notebookEditor .notebook-cell.active { + box-shadow: ${darkBoxShadow}; + } + + .hc-black .notebookEditor .notebook-cell.active { + box-shadow: 0; + } + + .notebookEditor .notebook-cell:hover:not(.active) { + box-shadow: ${lightBoxShadow}; + } + + .vs-dark .notebookEditor .notebook-cell:hover:not(.active) { + box-shadow: ${darkBoxShadow}; + } + + .hc-black .notebookEditor .notebook-cell:hover:not(.active) { + box-shadow: 0; } `); - } - // Box shadow handling - collector.addRule(` - .notebookEditor .notebook-cell.active { - box-shadow: ${lightBoxShadow}; + 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; + } } - .vs-dark .notebookEditor .notebook-cell.active { - box-shadow: ${darkBoxShadow}; + // Inactive border + 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}; + border-width: 1px; + border-style: solid; + border-radius: 3px 3px 3px 3px; + } + .notebookEditor .notebook-cell.active code-component { + border-width: 0px 0px 1px 0px; + border-radius: 0px; + } + .notebookEditor .notebook-cell:hover code-component { + border-width: 0px 0px 1px 0px; + border-radius: 0px; + } + `); + } + } - .hc-black .notebookEditor .notebook-cell.active { - box-shadow: 0; - } - - .notebookEditor .notebook-cell:hover:not(.active) { - box-shadow: ${lightBoxShadow}; - } - - .vs-dark .notebookEditor .notebook-cell:hover:not(.active) { - box-shadow: ${darkBoxShadow}; - } - - .hc-black .notebookEditor .notebook-cell:hover:not(.active) { - box-shadow: 0; - } - `); - - - // Inactive border - const inactiveBorder = theme.getColor(SIDE_BAR_BACKGROUND); - if (inactiveBorder) { + // Sidebar and cell outline toolbar color set only when active collector.addRule(` - .notebookEditor .notebook-cell code-component { - border-color: ${inactiveBorder}; - border-width: 1px; - border-style: solid; - border-radius: 3px 3px 3px 3px; - } - .notebookEditor .notebook-cell.active code-component { - border-width: 0px 0px 1px 0px; - border-radius: 0px; - } - .notebookEditor .notebook-cell:hover code-component { - 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; + .notebook-cell.active code-component .toolbar { + background-color: ${sidebarColor}; } `); - // toolbar color set only when active - collector.addRule(` - code-component .toolbar { - background-color: ${inactiveBorder}; - } - `); - } - - // Styling with Outline color (e.g. high contrast theme) - const outline = theme.getColor(activeContrastBorder); - const hcOutline = theme.getColor(contrastBorder); - if (outline) { - collector.addRule(` - .hc-black .notebookEditor .notebook-cell:not(.active) code-component { - border-color: ${hcOutline}; - border-width: 0px 0px 1px 0px; - } - .hc-black .notebookEditor .notebook-cell.active code-component { - border-color: ${outline}; - border-width: 0px 0px 1px 0px; - } - .hc-black .notebookEditor .notebook-cell:not(.active) { - outline-color: ${hcOutline}; - outline-width: 1px; - outline-style: solid; - } - .notebookEditor .notebook-cell.active { - outline-color: ${outline}; - outline-width: 1px; - outline-style: solid; - } - `); - } - - // Styling for all links in notebooks - const linkForeground = theme.getColor(textLinkForeground); - if (linkForeground) { - collector.addRule(` - .notebookEditor a:link { - text-decoration: none; - font-weight: bold; - color: ${linkForeground}; + // Styling with Outline color (e.g. high contrast theme) + const outline = theme.getColor(activeContrastBorder); + const hcOutline = theme.getColor(contrastBorder); + if (outline) { + collector.addRule(` + .hc-black .notebookEditor .notebook-cell:not(.active) code-component { + border-color: ${hcOutline}; + border-width: 0px 0px 1px 0px; + } + .hc-black .notebookEditor .notebook-cell.active code-component { + border-color: ${outline}; + border-width: 0px 0px 1px 0px; + } + .hc-black .notebookEditor .notebook-cell:not(.active) { + outline-color: ${hcOutline}; + outline-width: 1px; + outline-style: solid; + } + .notebookEditor .notebook-cell.active { + outline-color: ${outline}; + outline-width: 1px; + outline-style: solid; + } + `); } - `); - } -}); + + // Styling for all links in notebooks + const linkForeground = theme.getColor(textLinkForeground); + if (linkForeground) { + collector.addRule(` + .notebookEditor a:link { + text-decoration: none; + font-weight: bold; + color: ${linkForeground}; + } + `); + } + }); +} \ No newline at end of file diff --git a/src/sql/workbench/services/notebook/common/notebookServiceImpl.ts b/src/sql/workbench/services/notebook/common/notebookServiceImpl.ts index 6de8a2c4e6..b9013254d6 100644 --- a/src/sql/workbench/services/notebook/common/notebookServiceImpl.ts +++ b/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(); private _isRegistrationComplete = false; private notebookEditorVisible: IContextKey; + 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(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;