From 8448512143e43c7c6e4789e11bd52893fd2afd73 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Fri, 7 Jun 2019 11:41:33 -0700 Subject: [PATCH 01/21] Use readonly arrays for the vscode.DiagnosticCollection api ## Problem The diagnostic collection object is set up so that it does not mutate the arrays of diagnostics you pass to it. It also does not expect or allow mutation of diagnostics that it returns. However it it currently typed using normal arrays. This means that if an extension (such as JS/TS) wishes to use readonly diagnostics intnernally, it cannot do so without casting. ## Proposed Fix Use `ReadonlyArray` in diagnostic collection. This should be a safe change for the `set` type methods. The changes to `get` and `forEach` have the risk of breaking the typing of some extensions, but `get` already returned a frozen array of diagnostic so trying to mutate the array itself would have resulted in runtime error. --- .../src/features/diagnostics.ts | 4 ++-- src/vs/vscode.d.ts | 8 +++---- .../api/common/extHostDiagnostics.ts | 22 +++++++++---------- .../api/extHostDiagnostics.test.ts | 6 ++--- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/extensions/typescript-language-features/src/features/diagnostics.ts b/extensions/typescript-language-features/src/features/diagnostics.ts index d3d40ac97f4..327573e8c18 100644 --- a/extensions/typescript-language-features/src/features/diagnostics.ts +++ b/extensions/typescript-language-features/src/features/diagnostics.ts @@ -208,7 +208,7 @@ export class DiagnosticsManager extends Disposable { public configFileDiagnosticsReceived( file: vscode.Uri, - diagnostics: vscode.Diagnostic[] + diagnostics: ReadonlyArray ): void { this._currentDiagnostics.set(file, diagnostics); } @@ -218,7 +218,7 @@ export class DiagnosticsManager extends Disposable { this._diagnostics.delete(resource); } - public getDiagnostics(file: vscode.Uri): vscode.Diagnostic[] { + public getDiagnostics(file: vscode.Uri): ReadonlyArray { return this._currentDiagnostics.get(file) || []; } diff --git a/src/vs/vscode.d.ts b/src/vs/vscode.d.ts index d03f3edf9a2..5668c4f1381 100644 --- a/src/vs/vscode.d.ts +++ b/src/vs/vscode.d.ts @@ -4354,7 +4354,7 @@ declare module 'vscode' { * @param uri A resource identifier. * @param diagnostics Array of diagnostics or `undefined` */ - set(uri: Uri, diagnostics: Diagnostic[] | undefined): void; + set(uri: Uri, diagnostics: ReadonlyArray | undefined): void; /** * Replace all entries in this collection. @@ -4366,7 +4366,7 @@ declare module 'vscode' { * * @param entries An array of tuples, like `[[file1, [d1, d2]], [file2, [d3, d4, d5]]]`, or `undefined`. */ - set(entries: [Uri, Diagnostic[] | undefined][]): void; + set(entries: ReadonlyArray<[Uri, ReadonlyArray | undefined]>): void; /** * Remove all diagnostics from this collection that belong @@ -4388,7 +4388,7 @@ declare module 'vscode' { * @param callback Function to execute for each entry. * @param thisArg The `this` context used when invoking the handler function. */ - forEach(callback: (uri: Uri, diagnostics: Diagnostic[], collection: DiagnosticCollection) => any, thisArg?: any): void; + forEach(callback: (uri: Uri, diagnostics: ReadonlyArray, collection: DiagnosticCollection) => any, thisArg?: any): void; /** * Get the diagnostics for a given resource. *Note* that you cannot @@ -4397,7 +4397,7 @@ declare module 'vscode' { * @param uri A resource identifier. * @returns An immutable array of [diagnostics](#Diagnostic) or `undefined`. */ - get(uri: Uri): Diagnostic[] | undefined; + get(uri: Uri): ReadonlyArray | undefined; /** * Check if this collection contains diagnostics for a diff --git a/src/vs/workbench/api/common/extHostDiagnostics.ts b/src/vs/workbench/api/common/extHostDiagnostics.ts index 8a54b7e405c..60dae259b05 100644 --- a/src/vs/workbench/api/common/extHostDiagnostics.ts +++ b/src/vs/workbench/api/common/extHostDiagnostics.ts @@ -47,9 +47,9 @@ export class DiagnosticCollection implements vscode.DiagnosticCollection { return this._name; } - set(uri: vscode.Uri, diagnostics: vscode.Diagnostic[]): void; - set(entries: [vscode.Uri, vscode.Diagnostic[]][]): void; - set(first: vscode.Uri | [vscode.Uri, vscode.Diagnostic[]][], diagnostics?: vscode.Diagnostic[]) { + set(uri: vscode.Uri, diagnostics: ReadonlyArray): void; + set(entries: ReadonlyArray<[vscode.Uri, ReadonlyArray]>): void; + set(first: vscode.Uri | ReadonlyArray<[vscode.Uri, ReadonlyArray]>, diagnostics?: ReadonlyArray) { if (!first) { // this set-call is a clear-call @@ -167,7 +167,7 @@ export class DiagnosticCollection implements vscode.DiagnosticCollection { this._proxy.$clear(this._owner); } - forEach(callback: (uri: URI, diagnostics: vscode.Diagnostic[], collection: DiagnosticCollection) => any, thisArg?: any): void { + forEach(callback: (uri: URI, diagnostics: ReadonlyArray, collection: DiagnosticCollection) => any, thisArg?: any): void { this._checkDisposed(); this._data.forEach((value, key) => { const uri = URI.parse(key); @@ -175,11 +175,11 @@ export class DiagnosticCollection implements vscode.DiagnosticCollection { }); } - get(uri: URI): vscode.Diagnostic[] { + get(uri: URI): ReadonlyArray { this._checkDisposed(); const result = this._data.get(uri.toString()); if (Array.isArray(result)) { - return Object.freeze(result.slice(0)); + return >Object.freeze(result.slice(0)); } return []; } @@ -278,10 +278,10 @@ export class ExtHostDiagnostics implements ExtHostDiagnosticsShape { return result; } - getDiagnostics(resource: vscode.Uri): vscode.Diagnostic[]; - getDiagnostics(): [vscode.Uri, vscode.Diagnostic[]][]; - getDiagnostics(resource?: vscode.Uri): vscode.Diagnostic[] | [vscode.Uri, vscode.Diagnostic[]][]; - getDiagnostics(resource?: vscode.Uri): vscode.Diagnostic[] | [vscode.Uri, vscode.Diagnostic[]][] { + getDiagnostics(resource: vscode.Uri): ReadonlyArray; + getDiagnostics(): ReadonlyArray<[vscode.Uri, ReadonlyArray]>; + getDiagnostics(resource?: vscode.Uri): ReadonlyArray | ReadonlyArray<[vscode.Uri, ReadonlyArray]>; + getDiagnostics(resource?: vscode.Uri): ReadonlyArray | ReadonlyArray<[vscode.Uri, ReadonlyArray]> { if (resource) { return this._getDiagnostics(resource); } else { @@ -302,7 +302,7 @@ export class ExtHostDiagnostics implements ExtHostDiagnosticsShape { } } - private _getDiagnostics(resource: vscode.Uri): vscode.Diagnostic[] { + private _getDiagnostics(resource: vscode.Uri): ReadonlyArray { let res: vscode.Diagnostic[] = []; this._collections.forEach(collection => { if (collection.has(resource)) { diff --git a/src/vs/workbench/test/electron-browser/api/extHostDiagnostics.test.ts b/src/vs/workbench/test/electron-browser/api/extHostDiagnostics.test.ts index 38c627a64c5..4eca9307e09 100644 --- a/src/vs/workbench/test/electron-browser/api/extHostDiagnostics.test.ts +++ b/src/vs/workbench/test/electron-browser/api/extHostDiagnostics.test.ts @@ -91,18 +91,18 @@ suite('ExtHostDiagnostics', () => { new Diagnostic(new Range(0, 0, 1, 1), 'message-2') ]); - let array = collection.get(URI.parse('foo:bar')); + let array = collection.get(URI.parse('foo:bar')) as Diagnostic[]; assert.throws(() => array.length = 0); assert.throws(() => array.pop()); assert.throws(() => array[0] = new Diagnostic(new Range(0, 0, 0, 0), 'evil')); - collection.forEach((uri, array) => { + collection.forEach((uri, array: Diagnostic[]) => { assert.throws(() => array.length = 0); assert.throws(() => array.pop()); assert.throws(() => array[0] = new Diagnostic(new Range(0, 0, 0, 0), 'evil')); }); - array = collection.get(URI.parse('foo:bar')); + array = collection.get(URI.parse('foo:bar')) as Diagnostic[]; assert.equal(array.length, 2); collection.dispose(); From e7a5f9a5e2e9a06d3265d42e599499f4116a8bb9 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 11:17:42 -0700 Subject: [PATCH 02/21] Remove the vscode-core-resource scheme This is no longer required and complicates loading of resources. Use the standard `vscode-resource` scheme instead --- src/vs/workbench/api/browser/mainThreadWebview.ts | 2 +- .../extensions/electron-browser/extensionEditor.ts | 10 +++++++--- .../update/electron-browser/releaseNotesEditor.ts | 12 +++++++++--- src/vs/workbench/contrib/webview/common/webview.ts | 2 ++ .../webview/electron-browser/webviewElement.ts | 14 +++----------- .../webview/electron-browser/webviewProtocols.ts | 6 +----- 6 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/vs/workbench/api/browser/mainThreadWebview.ts b/src/vs/workbench/api/browser/mainThreadWebview.ts index c7b1f6462c5..f41c0327ae3 100644 --- a/src/vs/workbench/api/browser/mainThreadWebview.ts +++ b/src/vs/workbench/api/browser/mainThreadWebview.ts @@ -339,7 +339,7 @@ export class MainThreadWebviews extends Disposable implements MainThreadWebviews - + ${localize('errorMessage', "An error occurred while restoring view:{0}", viewType)} `; diff --git a/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts b/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts index abd20f5b63a..8dd0f511fb4 100644 --- a/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts +++ b/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts @@ -51,14 +51,15 @@ import { IExtensionService } from 'vs/workbench/services/extensions/common/exten import { getDefaultValue } from 'vs/platform/configuration/common/configurationRegistry'; import { isUndefined } from 'vs/base/common/types'; import { IWorkbenchThemeService } from 'vs/workbench/services/themes/common/workbenchThemeService'; +import { URI } from 'vs/base/common/uri'; function renderBody(body: string): string { - const styleSheetPath = require.toUrl('./media/markdown.css').replace('file://', 'vscode-core-resource://'); + const styleSheetPath = require.toUrl('./media/markdown.css').replace('file://', 'vscode-resource://'); return ` - + @@ -538,7 +539,10 @@ export class ExtensionEditor extends BaseEditor { enableFindWidget: true, }, { - svgWhiteList: this.extensionsWorkbenchService.allowedBadgeProviders + svgWhiteList: this.extensionsWorkbenchService.allowedBadgeProviders, + localResourceRoots: [ + URI.parse(require.toUrl('./media')) + ] }); webviewElement.mountTo(this.content); this.contentDisposables.push(webviewElement.onDidFocus(() => this.fireOnDidFocus())); diff --git a/src/vs/workbench/contrib/update/electron-browser/releaseNotesEditor.ts b/src/vs/workbench/contrib/update/electron-browser/releaseNotesEditor.ts index 0fa86467f43..95c3f16b960 100644 --- a/src/vs/workbench/contrib/update/electron-browser/releaseNotesEditor.ts +++ b/src/vs/workbench/contrib/update/electron-browser/releaseNotesEditor.ts @@ -32,13 +32,13 @@ function renderBody( body: string, css: string ): string { - const styleSheetPath = require.toUrl('./media/markdown.css').replace('file://', 'vscode-core-resource://'); + const styleSheetPath = require.toUrl('./media/markdown.css').replace('file://', 'vscode-resource://'); return ` - + @@ -95,7 +95,13 @@ export class ReleaseNotesManager { 'releaseNotes', title, { group: ACTIVE_GROUP, preserveFocus: false }, - { tryRestoreScrollPosition: true, enableFindWidget: true }, + { + tryRestoreScrollPosition: true, + enableFindWidget: true, + localResourceRoots: [ + URI.parse(require.toUrl('./media')) + ] + }, undefined, { onDidClickLink: uri => this.onDidClickLink(uri), onDispose: () => { this._currentReleaseNotes = undefined; } diff --git a/src/vs/workbench/contrib/webview/common/webview.ts b/src/vs/workbench/contrib/webview/common/webview.ts index dd99355dcb4..0117b141c77 100644 --- a/src/vs/workbench/contrib/webview/common/webview.ts +++ b/src/vs/workbench/contrib/webview/common/webview.ts @@ -30,6 +30,8 @@ export interface IWebviewService { ): Webview; } +export const WebviewResourceScheme = 'vscode-resource'; + export interface WebviewOptions { readonly allowSvgs?: boolean; readonly extension?: { diff --git a/src/vs/workbench/contrib/webview/electron-browser/webviewElement.ts b/src/vs/workbench/contrib/webview/electron-browser/webviewElement.ts index b7838d14377..b86e3120d00 100644 --- a/src/vs/workbench/contrib/webview/electron-browser/webviewElement.ts +++ b/src/vs/workbench/contrib/webview/electron-browser/webviewElement.ts @@ -21,8 +21,8 @@ import { REMOTE_HOST_SCHEME } from 'vs/platform/remote/common/remoteHosts'; import { ITunnelService, RemoteTunnel } from 'vs/platform/remote/common/tunnel'; import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry'; import { ITheme, IThemeService } from 'vs/platform/theme/common/themeService'; -import { Webview, WebviewContentOptions, WebviewOptions } from 'vs/workbench/contrib/webview/common/webview'; -import { registerFileProtocol, WebviewProtocol } from 'vs/workbench/contrib/webview/electron-browser/webviewProtocols'; +import { Webview, WebviewContentOptions, WebviewOptions, WebviewResourceScheme } from 'vs/workbench/contrib/webview/common/webview'; +import { registerFileProtocol } from 'vs/workbench/contrib/webview/electron-browser/webviewProtocols'; import { areWebviewInputOptionsEqual } from '../browser/webviewEditorService'; import { WebviewFindWidget } from '../browser/webviewFindWidget'; import { getWebviewThemeData } from 'vs/workbench/contrib/webview/common/themeing'; @@ -116,7 +116,6 @@ class WebviewProtocolProvider extends Disposable { webview: Electron.WebviewTag, private readonly _extensionLocation: URI | undefined, private readonly _getLocalResourceRoots: () => ReadonlyArray, - private readonly _environmentService: IEnvironmentService, private readonly _fileService: IFileService, ) { super(); @@ -134,13 +133,7 @@ class WebviewProtocolProvider extends Disposable { return; } - const appRootUri = URI.file(this._environmentService.appRoot); - - registerFileProtocol(contents, WebviewProtocol.CoreResource, this._fileService, undefined, () => [ - appRootUri - ]); - - registerFileProtocol(contents, WebviewProtocol.VsCodeResource, this._fileService, this._extensionLocation, () => + registerFileProtocol(contents, WebviewResourceScheme, this._fileService, this._extensionLocation, () => this._getLocalResourceRoots() ); } @@ -420,7 +413,6 @@ export class WebviewElement extends Disposable implements Webview { this._webview, this._options.extension ? this._options.extension.location : undefined, () => (this.content.options.localResourceRoots || []), - environmentService, fileService)); this._register(new WebviewPortMappingProvider( diff --git a/src/vs/workbench/contrib/webview/electron-browser/webviewProtocols.ts b/src/vs/workbench/contrib/webview/electron-browser/webviewProtocols.ts index 2d54b962046..b10b22c053b 100644 --- a/src/vs/workbench/contrib/webview/electron-browser/webviewProtocols.ts +++ b/src/vs/workbench/contrib/webview/electron-browser/webviewProtocols.ts @@ -12,10 +12,6 @@ import { getWebviewContentMimeType } from 'vs/workbench/contrib/webview/common/m type BufferProtocolCallback = (buffer?: Buffer | electron.MimeTypedBuffer | { error: number }) => void; -export const enum WebviewProtocol { - CoreResource = 'vscode-core-resource', - VsCodeResource = 'vscode-resource', -} function resolveContent(fileService: IFileService, resource: URI, mime: string, callback: BufferProtocolCallback): void { fileService.readFile(resource).then(contents => { @@ -31,7 +27,7 @@ function resolveContent(fileService: IFileService, resource: URI, mime: string, export function registerFileProtocol( contents: electron.WebContents, - protocol: WebviewProtocol, + protocol: string, fileService: IFileService, extensionLocation: URI | undefined, getRoots: () => ReadonlyArray From 01f7276b7eb02fe07802e66bc44ba9a3d8ab35ce Mon Sep 17 00:00:00 2001 From: Micah Smith Date: Wed, 12 Jun 2019 14:30:54 -0400 Subject: [PATCH 03/21] Fix for issue #35245 --- .../src/features/documentLinkProvider.ts | 6 +++--- .../src/test/documentLinkProvider.test.ts | 8 ++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/extensions/markdown-language-features/src/features/documentLinkProvider.ts b/extensions/markdown-language-features/src/features/documentLinkProvider.ts index 548205566af..9ae4bcfed29 100644 --- a/extensions/markdown-language-features/src/features/documentLinkProvider.ts +++ b/extensions/markdown-language-features/src/features/documentLinkProvider.ts @@ -79,9 +79,9 @@ function extractDocumentLink( } export default class LinkProvider implements vscode.DocumentLinkProvider { - private readonly linkPattern = /(\[((!\[[^\]]*?\]\(\s*)([^\s\(\)]+?)\s*\)\]|[^\]]*\])\(\s*)(([^\s\(\)]|\(\S*?\))+)\s*(".*?")?\)/g; - private readonly referenceLinkPattern = /(\[([^\]]+)\]\[\s*?)([^\s\]]*?)\]/g; - private readonly definitionPattern = /^([\t ]*\[([^\]]+)\]:\s*)(\S+)/gm; + private readonly linkPattern = /(\[((!\[[^\]]*?\]\(\s*)([^\s\(\)]+?)\s*\)\]|(?:\\\]|[^\]])*\])\(\s*)(([^\s\(\)]|\(\S*?\))+)\s*(".*?")?\)/g; + private readonly referenceLinkPattern = /(\[((?:\\\]|[^\]])+)\]\[\s*?)([^\s\]]*?)\]/g; + private readonly definitionPattern = /^([\t ]*\[((?:\\\]|[^\]])+)\]:\s*)(\S+)/gm; public provideDocumentLinks( document: vscode.TextDocument, diff --git a/extensions/markdown-language-features/src/test/documentLinkProvider.test.ts b/extensions/markdown-language-features/src/test/documentLinkProvider.test.ts index f6309a027d3..ba6cfa2567f 100644 --- a/extensions/markdown-language-features/src/test/documentLinkProvider.test.ts +++ b/extensions/markdown-language-features/src/test/documentLinkProvider.test.ts @@ -72,6 +72,14 @@ suite('markdown.DocumentLinkProvider', () => { assertRangeEqual(link.range, new vscode.Range(0, 6, 0, 25)); }); + // #35245 + test('Should handle links with escaped characters in name', () => { + const links = getLinksForFile('a [b\\]](./file)'); + assert.strictEqual(links.length, 1); + const [link] = links; + assertRangeEqual(link.range, new vscode.Range(0, 8, 0, 14)); + }); + test('Should handle links with balanced parens', () => { { From 33ba72fef9099c4b58ad812585babc3fbacb9427 Mon Sep 17 00:00:00 2001 From: Sandeep Somavarapu Date: Wed, 12 Jun 2019 20:49:56 +0200 Subject: [PATCH 04/21] Fix #75343 --- .../services/keybinding/electron-browser/keybindingService.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/services/keybinding/electron-browser/keybindingService.ts b/src/vs/workbench/services/keybinding/electron-browser/keybindingService.ts index 99d5f9cad60..973bf52e9c4 100644 --- a/src/vs/workbench/services/keybinding/electron-browser/keybindingService.ts +++ b/src/vs/workbench/services/keybinding/electron-browser/keybindingService.ts @@ -46,6 +46,7 @@ import { IFileService, FileChangesEvent, FileChangeType } from 'vs/platform/file import { dirname, isEqual } from 'vs/base/common/resources'; import { parse } from 'vs/base/common/json'; import * as objects from 'vs/base/common/objects'; +import { isArray } from 'vs/base/common/types'; export class KeyboardMapperFactory { public static readonly INSTANCE = new KeyboardMapperFactory(); @@ -632,7 +633,8 @@ class UserKeybindings extends Disposable { const existing = this._keybindings; try { const content = await this.fileService.readFile(this.keybindingsResource); - this._keybindings = parse(content.value.toString()); + const value = parse(content.value.toString()); + this._keybindings = isArray(value) ? value : []; } catch (e) { this._keybindings = []; } From b6919c804ca8ab4282268db1014d47a96ec09194 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 11:28:52 -0700 Subject: [PATCH 05/21] MarkdownRenderer is disposable It registers an emitter so it should be disposed of --- src/vs/editor/contrib/markdown/markdownRenderer.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/vs/editor/contrib/markdown/markdownRenderer.ts b/src/vs/editor/contrib/markdown/markdownRenderer.ts index bd3a38b421c..5825a919bf5 100644 --- a/src/vs/editor/contrib/markdown/markdownRenderer.ts +++ b/src/vs/editor/contrib/markdown/markdownRenderer.ts @@ -13,16 +13,16 @@ import { tokenizeToString } from 'vs/editor/common/modes/textToHtmlTokenizer'; import { ICodeEditor } from 'vs/editor/browser/editorBrowser'; import { optional } from 'vs/platform/instantiation/common/instantiation'; import { Event, Emitter } from 'vs/base/common/event'; -import { IDisposable, DisposableStore } from 'vs/base/common/lifecycle'; +import { IDisposable, DisposableStore, Disposable } from 'vs/base/common/lifecycle'; import { TokenizationRegistry } from 'vs/editor/common/modes'; export interface IMarkdownRenderResult extends IDisposable { element: HTMLElement; } -export class MarkdownRenderer { +export class MarkdownRenderer extends Disposable { - private _onDidRenderCodeBlock = new Emitter(); + private _onDidRenderCodeBlock = this._register(new Emitter()); readonly onDidRenderCodeBlock: Event = this._onDidRenderCodeBlock.event; constructor( @@ -30,6 +30,7 @@ export class MarkdownRenderer { @IModeService private readonly _modeService: IModeService, @optional(IOpenerService) private readonly _openerService: IOpenerService | null = NullOpenerService, ) { + super(); } private getOptions(disposeables: DisposableStore): RenderOptions { From 89d258cd2843d9a9dc5c8f3e1823e5f975ea50b0 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 11:29:06 -0700 Subject: [PATCH 06/21] Use MutableDisposable for parameterHintsWidget --- .../parameterHints/parameterHintsWidget.ts | 35 +++++++------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/src/vs/editor/contrib/parameterHints/parameterHintsWidget.ts b/src/vs/editor/contrib/parameterHints/parameterHintsWidget.ts index f7eae70292e..53bf31abcc5 100644 --- a/src/vs/editor/contrib/parameterHints/parameterHintsWidget.ts +++ b/src/vs/editor/contrib/parameterHints/parameterHintsWidget.ts @@ -8,7 +8,7 @@ import { domEvent, stop } from 'vs/base/browser/event'; import * as aria from 'vs/base/browser/ui/aria/aria'; import { DomScrollableElement } from 'vs/base/browser/ui/scrollbar/scrollableElement'; import { Event } from 'vs/base/common/event'; -import { IDisposable, Disposable, DisposableStore } from 'vs/base/common/lifecycle'; +import { IDisposable, Disposable, DisposableStore, MutableDisposable } from 'vs/base/common/lifecycle'; import 'vs/css!./parameterHints'; import { ContentWidgetPositionPreference, ICodeEditor, IContentWidget, IContentWidgetPosition } from 'vs/editor/browser/editorBrowser'; import { IConfigurationChangedEvent } from 'vs/editor/common/config/editorOptions'; @@ -31,7 +31,7 @@ export class ParameterHintsWidget extends Disposable implements IContentWidget, private readonly markdownRenderer: MarkdownRenderer; private readonly renderDisposeables = this._register(new DisposableStore()); - private model: ParameterHintsModel | null; + private readonly model = this._register(new MutableDisposable()); private readonly keyVisible: IContextKey; private readonly keyMultipleSignatures: IContextKey; private element: HTMLElement; @@ -52,13 +52,13 @@ export class ParameterHintsWidget extends Disposable implements IContentWidget, @IModeService modeService: IModeService, ) { super(); - this.markdownRenderer = new MarkdownRenderer(editor, modeService, openerService); - this.model = new ParameterHintsModel(editor); + this.markdownRenderer = this._register(new MarkdownRenderer(editor, modeService, openerService)); + this.model.value = new ParameterHintsModel(editor); this.keyVisible = Context.Visible.bindTo(contextKeyService); this.keyMultipleSignatures = Context.MultipleSignatures.bindTo(contextKeyService); this.visible = false; - this._register(this.model.onChangedHints(newParameterHints => { + this._register(this.model.value.onChangedHints(newParameterHints => { if (newParameterHints) { this.show(); this.render(newParameterHints); @@ -282,22 +282,22 @@ export class ParameterHintsWidget extends Disposable implements IContentWidget, } next(): void { - if (this.model) { + if (this.model.value) { this.editor.focus(); - this.model.next(); + this.model.value.next(); } } previous(): void { - if (this.model) { + if (this.model.value) { this.editor.focus(); - this.model.previous(); + this.model.value.previous(); } } cancel(): void { - if (this.model) { - this.model.cancel(); + if (this.model.value) { + this.model.value.cancel(); } } @@ -310,8 +310,8 @@ export class ParameterHintsWidget extends Disposable implements IContentWidget, } trigger(context: TriggerContext): void { - if (this.model) { - this.model.trigger(context, 0); + if (this.model.value) { + this.model.value.trigger(context, 0); } } @@ -319,15 +319,6 @@ export class ParameterHintsWidget extends Disposable implements IContentWidget, const height = Math.max(this.editor.getLayoutInfo().height / 4, 250); this.element.style.maxHeight = `${height}px`; } - - dispose(): void { - super.dispose(); - - if (this.model) { - this.model.dispose(); - this.model = null; - } - } } registerThemingParticipant((theme, collector) => { From ac364bb726cf88a905fa2cbcd33614be37c41224 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 11:39:29 -0700 Subject: [PATCH 07/21] Also track disposables created from `toDisposable` and `combinedDisposable` --- src/vs/base/common/lifecycle.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index 0d13f5179aa..3a3f196619d 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -30,9 +30,9 @@ function markTracked(x: T): void { } } -function trackDisposable(x: T): void { +function trackDisposable(x: T): T { if (!TRACK_DISPOSABLES) { - return; + return x; } const stack = new Error().stack!; @@ -41,6 +41,7 @@ function trackDisposable(x: T): void { console.log(stack); } }, 3000); + return x; } export interface IDisposable { @@ -76,11 +77,11 @@ export function dispose(disposables: T | T[] | undefined) export function combinedDisposable(...disposables: IDisposable[]): IDisposable { disposables.forEach(markTracked); - return { dispose: () => dispose(disposables) }; + return trackDisposable({ dispose: () => dispose(disposables) }); } export function toDisposable(fn: () => void): IDisposable { - return { dispose: fn }; + return trackDisposable({ dispose: fn }); } export class DisposableStore implements IDisposable { From 4f59729b87a16cf5ddfda6dd7cd71f6d8c0cde1f Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 11:49:11 -0700 Subject: [PATCH 08/21] Use DisposableStore in extensionEditor --- .../electron-browser/extensionEditor.ts | 65 +++++++++---------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts b/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts index 8dd0f511fb4..86949eb39a1 100644 --- a/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts +++ b/src/vs/workbench/contrib/extensions/electron-browser/extensionEditor.ts @@ -13,7 +13,7 @@ import { Event, Emitter } from 'vs/base/common/event'; import { Cache, CacheResult } from 'vs/base/common/cache'; import { Action } from 'vs/base/common/actions'; import { isPromiseCanceledError } from 'vs/base/common/errors'; -import { IDisposable, dispose, toDisposable, Disposable } from 'vs/base/common/lifecycle'; +import { dispose, toDisposable, Disposable, DisposableStore } from 'vs/base/common/lifecycle'; import { domEvent } from 'vs/base/browser/event'; import { append, $, addClass, removeClass, finalHandler, join, toggleClass, hide, show } from 'vs/base/browser/dom'; import { BaseEditor } from 'vs/workbench/browser/parts/editor/baseEditor'; @@ -177,9 +177,8 @@ export class ExtensionEditor extends BaseEditor { private extensionManifest: Cache | null; private layoutParticipants: ILayoutParticipant[] = []; - private contentDisposables: IDisposable[] = []; - private transientDisposables: IDisposable[] = []; - private disposables: IDisposable[]; + private readonly contentDisposables = this._register(new DisposableStore()); + private readonly transientDisposables = this._register(new DisposableStore()); private activeElement: IActiveElement | null; private editorLoadComplete: boolean = false; @@ -199,7 +198,6 @@ export class ExtensionEditor extends BaseEditor { ) { super(ExtensionEditor.ID, telemetryService, themeService, storageService); - this.disposables = []; this.extensionReadme = null; this.extensionChangelog = null; this.extensionManifest = null; @@ -257,18 +255,18 @@ export class ExtensionEditor extends BaseEditor { this.subtext = append(this.subtextContainer, $('.subtext')); this.ignoreActionbar = new ActionBar(this.subtextContainer, { animated: false }); - this.disposables.push(this.extensionActionBar); - this.disposables.push(this.ignoreActionbar); + this._register(this.extensionActionBar); + this._register(this.ignoreActionbar); - Event.chain(this.extensionActionBar.onDidRun) + this._register(Event.chain(this.extensionActionBar.onDidRun) .map(({ error }) => error) .filter(error => !!error) - .on(this.onError, this, this.disposables); + .on(this.onError, this)); - Event.chain(this.ignoreActionbar.onDidRun) + this._register(Event.chain(this.ignoreActionbar.onDidRun) .map(({ error }) => error) .filter(error => !!error) - .on(this.onError, this, this.disposables); + .on(this.onError, this)); const body = append(root, $('.body')); this.navbar = new NavBar(body); @@ -285,7 +283,7 @@ export class ExtensionEditor extends BaseEditor { this.editorLoadComplete = false; const extension = input.extension; - this.transientDisposables = dispose(this.transientDisposables); + this.transientDisposables.clear(); this.extensionReadme = new Cache(() => createCancelablePromise(token => extension.getReadme(token))); this.extensionChangelog = new Cache(() => createCancelablePromise(token => extension.getChangelog(token))); @@ -384,7 +382,9 @@ export class ExtensionEditor extends BaseEditor { this.extensionActionBar.clear(); this.extensionActionBar.push(actions, { icon: true, label: true }); - this.transientDisposables.push(...[...actions, ...widgets, extensionContainers]); + for (const disposable of [...actions, ...widgets, extensionContainers]) { + this.transientDisposables.add(disposable); + } this.setSubText(extension, reloadAction); this.content.innerHTML = ''; // Clear content before setting navbar actions. @@ -431,7 +431,8 @@ export class ExtensionEditor extends BaseEditor { this.ignoreActionbar.clear(); this.ignoreActionbar.push([ignoreAction, undoIgnoreAction], { icon: true, label: true }); - this.transientDisposables.push(ignoreAction, undoIgnoreAction); + this.transientDisposables.add(ignoreAction); + this.transientDisposables.add(undoIgnoreAction); const extRecommendations = this.extensionTipsService.getAllRecommendationsWithReason(); if (extRecommendations[extension.identifier.id.toLowerCase()]) { @@ -464,7 +465,7 @@ export class ExtensionEditor extends BaseEditor { } }); - this.transientDisposables.push(reloadAction.onDidChange(e => { + this.transientDisposables.add(reloadAction.onDidChange(e => { if (e.tooltip) { this.subtext.textContent = reloadAction.tooltip; show(this.subtextContainer); @@ -505,7 +506,7 @@ export class ExtensionEditor extends BaseEditor { this.telemetryService.publicLog('extensionEditor:navbarChange', assign(extension.telemetryData, { navItem: id })); } - this.contentDisposables = dispose(this.contentDisposables); + this.contentDisposables.clear(); this.content.innerHTML = ''; this.activeElement = null; this.open(id, extension) @@ -545,12 +546,12 @@ export class ExtensionEditor extends BaseEditor { ] }); webviewElement.mountTo(this.content); - this.contentDisposables.push(webviewElement.onDidFocus(() => this.fireOnDidFocus())); + this.contentDisposables.add(webviewElement.onDidFocus(() => this.fireOnDidFocus())); const removeLayoutParticipant = arrays.insert(this.layoutParticipants, webviewElement); - this.contentDisposables.push(toDisposable(removeLayoutParticipant)); + this.contentDisposables.add(toDisposable(removeLayoutParticipant)); webviewElement.html = body; - this.contentDisposables.push(webviewElement.onDidClickLink(link => { + this.contentDisposables.add(webviewElement.onDidClickLink(link => { if (!link) { return; } @@ -559,7 +560,7 @@ export class ExtensionEditor extends BaseEditor { this.openerService.open(link); } }, null, this.contentDisposables)); - this.contentDisposables.push(webviewElement); + this.contentDisposables.add(webviewElement); return webviewElement; }) .then(undefined, () => { @@ -589,7 +590,7 @@ export class ExtensionEditor extends BaseEditor { const layout = () => scrollableContent.scanDomNode(); const removeLayoutParticipant = arrays.insert(this.layoutParticipants, { layout }); - this.contentDisposables.push(toDisposable(removeLayoutParticipant)); + this.contentDisposables.add(toDisposable(removeLayoutParticipant)); const renders = [ this.renderSettings(content, manifest, layout), @@ -613,7 +614,7 @@ export class ExtensionEditor extends BaseEditor { append(this.content, content); } else { append(this.content, scrollableContent.getDomNode()); - this.contentDisposables.push(scrollableContent); + this.contentDisposables.add(scrollableContent); } return content; }, () => { @@ -632,7 +633,7 @@ export class ExtensionEditor extends BaseEditor { const content = $('div', { class: 'subcontent' }); const scrollableContent = new DomScrollableElement(content, {}); append(this.content, scrollableContent.getDomNode()); - this.contentDisposables.push(scrollableContent); + this.contentDisposables.add(scrollableContent); const dependenciesTree = this.instantiationService.createInstance(ExtensionsTree, new ExtensionData(extension, null, extension => extension.dependencies || [], this.extensionsWorkbenchService), content); const layout = () => { @@ -641,9 +642,9 @@ export class ExtensionEditor extends BaseEditor { dependenciesTree.layout(scrollDimensions.height); }; const removeLayoutParticipant = arrays.insert(this.layoutParticipants, { layout }); - this.contentDisposables.push(toDisposable(removeLayoutParticipant)); + this.contentDisposables.add(toDisposable(removeLayoutParticipant)); - this.contentDisposables.push(dependenciesTree); + this.contentDisposables.add(dependenciesTree); scrollableContent.scanDomNode(); return Promise.resolve({ focus() { dependenciesTree.domFocus(); } }); } @@ -652,7 +653,7 @@ export class ExtensionEditor extends BaseEditor { const content = $('div', { class: 'subcontent' }); const scrollableContent = new DomScrollableElement(content, {}); append(this.content, scrollableContent.getDomNode()); - this.contentDisposables.push(scrollableContent); + this.contentDisposables.add(scrollableContent); const extensionsPackTree = this.instantiationService.createInstance(ExtensionsTree, new ExtensionData(extension, null, extension => extension.extensionPack || [], this.extensionsWorkbenchService), content); const layout = () => { @@ -661,9 +662,9 @@ export class ExtensionEditor extends BaseEditor { extensionsPackTree.layout(scrollDimensions.height); }; const removeLayoutParticipant = arrays.insert(this.layoutParticipants, { layout }); - this.contentDisposables.push(toDisposable(removeLayoutParticipant)); + this.contentDisposables.add(toDisposable(removeLayoutParticipant)); - this.contentDisposables.push(extensionsPackTree); + this.contentDisposables.add(extensionsPackTree); scrollableContent.scanDomNode(); return Promise.resolve({ focus() { extensionsPackTree.domFocus(); } }); } @@ -1078,7 +1079,7 @@ export class ExtensionEditor extends BaseEditor { const onDone = () => removeClass(this.content, 'loading'); result.promise.then(onDone, onDone); - this.contentDisposables.push(toDisposable(() => result.dispose())); + this.contentDisposables.add(toDisposable(() => result.dispose())); return result.promise; } @@ -1094,12 +1095,6 @@ export class ExtensionEditor extends BaseEditor { this.notificationService.error(err); } - - dispose(): void { - this.transientDisposables = dispose(this.transientDisposables); - this.disposables = dispose(this.disposables); - super.dispose(); - } } class ShowExtensionEditorFindCommand extends Command { From e6f9ffe8f31b025e1e537ceadec41a62c2fd0925 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 11:52:01 -0700 Subject: [PATCH 09/21] Extend disposable This helps with lifecycle tracking --- .../browser/controller/pointerHandler.ts | 22 ++++++------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/vs/editor/browser/controller/pointerHandler.ts b/src/vs/editor/browser/controller/pointerHandler.ts index f744847b48f..e4a4a03146f 100644 --- a/src/vs/editor/browser/controller/pointerHandler.ts +++ b/src/vs/editor/browser/controller/pointerHandler.ts @@ -5,7 +5,7 @@ import * as dom from 'vs/base/browser/dom'; import { EventType, Gesture, GestureEvent } from 'vs/base/browser/touch'; -import { IDisposable } from 'vs/base/common/lifecycle'; +import { IDisposable, Disposable } from 'vs/base/common/lifecycle'; import { IPointerHandlerHelper, MouseHandler } from 'vs/editor/browser/controller/mouseHandler'; import { IMouseTarget } from 'vs/editor/browser/editorBrowser'; import { EditorMouseEvent } from 'vs/editor/browser/editorDom'; @@ -195,11 +195,6 @@ class TouchHandler extends MouseHandler { this._register(dom.addDisposableListener(this.viewHelper.linesContentDomNode, EventType.Tap, (e) => this.onTap(e))); this._register(dom.addDisposableListener(this.viewHelper.linesContentDomNode, EventType.Change, (e) => this.onChange(e))); this._register(dom.addDisposableListener(this.viewHelper.linesContentDomNode, EventType.Contextmenu, (e: MouseEvent) => this._onContextMenu(new EditorMouseEvent(e, this.viewHelper.viewDomNode), false))); - - } - - public dispose(): void { - super.dispose(); } private onTap(event: GestureEvent): void { @@ -219,26 +214,23 @@ class TouchHandler extends MouseHandler { } } -export class PointerHandler implements IDisposable { +export class PointerHandler extends Disposable { private readonly handler: MouseHandler; constructor(context: ViewContext, viewController: ViewController, viewHelper: IPointerHandlerHelper) { + super(); if (window.navigator.msPointerEnabled) { - this.handler = new MsPointerHandler(context, viewController, viewHelper); + this.handler = this._register(new MsPointerHandler(context, viewController, viewHelper)); } else if ((window).TouchEvent) { - this.handler = new TouchHandler(context, viewController, viewHelper); + this.handler = this._register(new TouchHandler(context, viewController, viewHelper)); } else if (window.navigator.pointerEnabled || (window).PointerEvent) { - this.handler = new StandardPointerHandler(context, viewController, viewHelper); + this.handler = this._register(new StandardPointerHandler(context, viewController, viewHelper)); } else { - this.handler = new MouseHandler(context, viewController, viewHelper); + this.handler = this._register(new MouseHandler(context, viewController, viewHelper)); } } public getTargetAtClientPoint(clientX: number, clientY: number): IMouseTarget | null { return this.handler.getTargetAtClientPoint(clientX, clientY); } - - public dispose(): void { - this.handler.dispose(); - } } From c244a40e5a0316367e03ad5bc7370a4022cd6011 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 14:00:32 -0700 Subject: [PATCH 10/21] Use MutableDisposable for managing listeners --- .../browser/parts/statusbar/statusbarPart.ts | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts b/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts index 654c846e5b7..0286df6d404 100644 --- a/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts +++ b/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts @@ -6,7 +6,7 @@ import 'vs/css!./media/statusbarpart'; import * as nls from 'vs/nls'; import { toErrorMessage } from 'vs/base/common/errorMessage'; -import { dispose, IDisposable, Disposable, toDisposable } from 'vs/base/common/lifecycle'; +import { dispose, IDisposable, Disposable, toDisposable, MutableDisposable } from 'vs/base/common/lifecycle'; import { OcticonLabel } from 'vs/base/browser/ui/octiconLabel/octiconLabel'; import { Registry } from 'vs/platform/registry/common/platform'; import { ICommandService } from 'vs/platform/commands/common/commands'; @@ -636,10 +636,10 @@ class StatusbarEntryItem extends Disposable { private labelContainer: HTMLElement; private label: OcticonLabel; - private foregroundListener: IDisposable | undefined; - private backgroundListener: IDisposable | undefined; + private readonly foregroundListener = this._register(new MutableDisposable()); + private readonly backgroundListener = this._register(new MutableDisposable()); - private commandListener: IDisposable | undefined; + private readonly commandListener = this._register(new MutableDisposable()); constructor( private container: HTMLElement, @@ -692,11 +692,10 @@ class StatusbarEntryItem extends Disposable { // Update: Command if (!this.entry || entry.command !== this.entry.command) { - dispose(this.commandListener); - this.commandListener = undefined; + this.commandListener.clear(); if (entry.command) { - this.commandListener = addDisposableListener(this.labelContainer, EventType.CLICK, () => this.executeCommand(entry.command!, entry.arguments)); + this.commandListener.value = addDisposableListener(this.labelContainer, EventType.CLICK, () => this.executeCommand(entry.command!, entry.arguments)); removeClass(this.labelContainer, 'disabled'); } else { @@ -759,11 +758,9 @@ class StatusbarEntryItem extends Disposable { let colorResult: string | null = null; if (isBackground) { - dispose(this.backgroundListener); - this.backgroundListener = undefined; + this.backgroundListener.clear(); } else { - dispose(this.foregroundListener); - this.foregroundListener = undefined; + this.foregroundListener.clear(); } if (color) { @@ -781,9 +778,9 @@ class StatusbarEntryItem extends Disposable { }); if (isBackground) { - this.backgroundListener = listener; + this.backgroundListener.value = listener; } else { - this.foregroundListener = listener; + this.foregroundListener.value = listener; } } else { colorResult = color; From ba9266d1da6a44483621f25c20f9f4c1493457b3 Mon Sep 17 00:00:00 2001 From: Matt Bierner Date: Wed, 12 Jun 2019 16:04:20 -0700 Subject: [PATCH 11/21] `isNonEmptyArray` should return readonly arrays for readonly array input --- src/vs/base/common/arrays.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/vs/base/common/arrays.ts b/src/vs/base/common/arrays.ts index 8a3bb5bdbc0..c4db37f2134 100644 --- a/src/vs/base/common/arrays.ts +++ b/src/vs/base/common/arrays.ts @@ -336,7 +336,9 @@ export function isFalsyOrEmpty(obj: any): boolean { /** * @returns True if the provided object is an array and has at least one element. */ -export function isNonEmptyArray(obj: ReadonlyArray | undefined | null): obj is Array { +export function isNonEmptyArray(obj: T[] | undefined | null): obj is T[]; +export function isNonEmptyArray(obj: readonly T[] | undefined | null): obj is readonly T[]; +export function isNonEmptyArray(obj: T[] | readonly T[] | undefined | null): obj is T[] | readonly T[] { return Array.isArray(obj) && obj.length > 0; } From 86aa7aef36650fabe45dc551478c3e2a0acf3ea1 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 16:29:02 -0700 Subject: [PATCH 12/21] Allow terminals to launch using clean env on Mac and Linux Fixes #70248 --- .../api/node/extHostTerminalService.ts | 5 +- .../terminal/browser/terminal.contribution.ts | 5 ++ .../contrib/terminal/browser/terminal.ts | 1 + .../browser/terminalInstanceService.ts | 5 ++ .../browser/terminalProcessManager.ts | 3 +- .../contrib/terminal/common/terminal.ts | 1 + .../terminal/common/terminalEnvironment.ts | 5 +- .../terminalInstanceService.ts | 55 ++++++++++++++++++- 8 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/api/node/extHostTerminalService.ts b/src/vs/workbench/api/node/extHostTerminalService.ts index 59caecef265..a1de46cfdf5 100644 --- a/src/vs/workbench/api/node/extHostTerminalService.ts +++ b/src/vs/workbench/api/node/extHostTerminalService.ts @@ -500,7 +500,10 @@ export class ExtHostTerminalService implements ExtHostTerminalServiceShape { variableResolver, isWorkspaceShellAllowed, pkg.version, - terminalConfig.get('setLocaleVariables', false) + terminalConfig.get('setLocaleVariables', false), + // Always inherit the environment as we need to be running in a login shell, this may + // change when macOS servers are supported + true ); // Fork the process and listen for messages diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts b/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts index 25b79bc85a6..0904438ef80 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts @@ -228,6 +228,11 @@ configurationRegistry.registerConfiguration({ }, default: [] }, + 'terminal.integrated.inheritEnv': { + markdownDescription: nls.localize('terminal.integrated.inheritEnv', "TODO"), + type: 'boolean', + default: true + }, 'terminal.integrated.env.osx': { markdownDescription: nls.localize('terminal.integrated.env.osx', "Object with environment variables that will be added to the VS Code process to be used by the terminal on macOS. Set to `null` to delete the environment variable."), type: 'object', diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index 06295c9666a..59c773a8f7b 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -26,6 +26,7 @@ export interface ITerminalInstanceService { createWindowsShellHelper(shellProcessId: number, instance: ITerminalInstance, xterm: XTermTerminal): IWindowsShellHelper; createTerminalProcess(shellLaunchConfig: IShellLaunchConfig, cwd: string, cols: number, rows: number, env: IProcessEnvironment, windowsEnableConpty: boolean): ITerminalChildProcess; getDefaultShell(p: Platform): string; + getMainProcessParentEnv(): Promise; } export interface IBrowserTerminalConfigHelper extends ITerminalConfigHelper { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstanceService.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstanceService.ts index b9bd8954a3c..eb28c172632 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstanceService.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstanceService.ts @@ -8,6 +8,7 @@ import { IWindowsShellHelper, ITerminalChildProcess } from 'vs/workbench/contrib import { Terminal as XTermTerminal } from 'xterm'; import { WebLinksAddon as XTermWebLinksAddon } from 'xterm-addon-web-links'; import { SearchAddon as XTermSearchAddon } from 'xterm-addon-search'; +import { IProcessEnvironment } from 'vs/base/common/platform'; let Terminal: typeof XTermTerminal; let WebLinksAddon: typeof XTermWebLinksAddon; @@ -50,4 +51,8 @@ export class TerminalInstanceService implements ITerminalInstanceService { public getDefaultShell(): string { throw new Error('Not implemented'); } + + public async getMainProcessParentEnv(): Promise { + return {}; + } } \ No newline at end of file diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts index 0a3d373f178..51ed69c3793 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts @@ -171,7 +171,8 @@ export class TerminalProcessManager implements ITerminalProcessManager { const lastActiveWorkspace = activeWorkspaceRootUri ? this._workspaceContextService.getWorkspaceFolder(activeWorkspaceRootUri) : null; const envFromConfigValue = this._workspaceConfigurationService.inspect(`terminal.integrated.env.${platformKey}`); const isWorkspaceShellAllowed = this._configHelper.checkWorkspaceShellPermissions(); - const env = terminalEnvironment.createTerminalEnvironment(shellLaunchConfig, lastActiveWorkspace, envFromConfigValue, this._configurationResolverService, isWorkspaceShellAllowed, this._productService.version, this._configHelper.config.setLocaleVariables); + this._terminalInstanceService.getMainProcessParentEnv(); + const env = terminalEnvironment.createTerminalEnvironment(shellLaunchConfig, lastActiveWorkspace, envFromConfigValue, this._configurationResolverService, isWorkspaceShellAllowed, this._productService.version, this._configHelper.config.setLocaleVariables, this._configHelper.config.inheritEnv); const useConpty = this._configHelper.config.windowsEnableConpty; return this._terminalInstanceService.createTerminalProcess(shellLaunchConfig, initialCwd, cols, rows, env, useConpty); diff --git a/src/vs/workbench/contrib/terminal/common/terminal.ts b/src/vs/workbench/contrib/terminal/common/terminal.ts index 5fd9248c115..bce06565e11 100644 --- a/src/vs/workbench/contrib/terminal/common/terminal.ts +++ b/src/vs/workbench/contrib/terminal/common/terminal.ts @@ -94,6 +94,7 @@ export interface ITerminalConfiguration { cwd: string; confirmOnExit: boolean; enableBell: boolean; + inheritEnv: boolean; env: { linux: { [key: string]: string }; osx: { [key: string]: string }; diff --git a/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts b/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts index 813bd746af3..7d260cd9418 100644 --- a/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts +++ b/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts @@ -198,7 +198,8 @@ export function createTerminalEnvironment( configurationResolverService: IConfigurationResolverService | undefined, isWorkspaceShellAllowed: boolean, version: string | undefined, - setLocaleVariables: boolean + setLocaleVariables: boolean, + inheritEnv: boolean ): platform.IProcessEnvironment { // Create a terminal environment based on settings, launch config and permissions let env: platform.IProcessEnvironment = {}; @@ -207,7 +208,7 @@ export function createTerminalEnvironment( mergeNonNullKeys(env, shellLaunchConfig.env); } else { // Merge process env with the env from config and from shellLaunchConfig - mergeNonNullKeys(env, process.env); + mergeNonNullKeys(env, inheritEnv ? process.env : {}); // const platformKey = platform.isWindows ? 'windows' : (platform.isMacintosh ? 'osx' : 'linux'); // const envFromConfigValue = this._workspaceConfigurationService.inspect(`terminal.integrated.env.${platformKey}`); diff --git a/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts b/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts index e0e69912cbc..45a5971aad9 100644 --- a/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts +++ b/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts @@ -7,12 +7,14 @@ import { ITerminalInstanceService } from 'vs/workbench/contrib/terminal/browser/ import { ITerminalInstance, IWindowsShellHelper, IShellLaunchConfig, ITerminalChildProcess } from 'vs/workbench/contrib/terminal/common/terminal'; import { WindowsShellHelper } from 'vs/workbench/contrib/terminal/node/windowsShellHelper'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; -import { IProcessEnvironment, Platform } from 'vs/base/common/platform'; +import { IProcessEnvironment, Platform, isLinux, isMacintosh, isWindows } from 'vs/base/common/platform'; import { TerminalProcess } from 'vs/workbench/contrib/terminal/node/terminalProcess'; import { getDefaultShell } from 'vs/workbench/contrib/terminal/node/terminal'; import { Terminal as XTermTerminal } from 'xterm'; import { WebLinksAddon as XTermWebLinksAddon } from 'xterm-addon-web-links'; import { SearchAddon as XTermSearchAddon } from 'xterm-addon-search'; +import { readFile } from 'vs/base/node/pfs'; +import { basename } from 'vs/base/common/path'; let Terminal: typeof XTermTerminal; let WebLinksAddon: typeof XTermWebLinksAddon; @@ -21,6 +23,8 @@ let SearchAddon: typeof XTermSearchAddon; export class TerminalInstanceService implements ITerminalInstanceService { public _serviceBrand: any; + private _mainProcessParentEnv: IProcessEnvironment | undefined; + constructor( @IInstantiationService private readonly _instantiationService: IInstantiationService ) { @@ -58,4 +62,53 @@ export class TerminalInstanceService implements ITerminalInstanceService { public getDefaultShell(p: Platform): string { return getDefaultShell(p); } + + public async getMainProcessParentEnv(): Promise { + if (this._mainProcessParentEnv) { + return this._mainProcessParentEnv; + } + + // For Linux use /proc//status to get the parent of the main process and then fetch its + // env using /proc//environ. + if (isLinux) { + const mainProcessId = process.ppid; + const codeProcessName = basename(process.argv[0]); + let pid: number = 0; + let ppid: number = mainProcessId; + let name: string = codeProcessName; + do { + pid = ppid; + const status = await readFile(`/proc/${pid}/status`, 'utf8'); + const splitByLine = status.split('\n'); + splitByLine.forEach(line => { + if (line.indexOf('Name:') === 0) { + name = line.replace(/^Name:\s+/, ''); + } + if (line.indexOf('PPid:') === 0) { + ppid = parseInt(line.replace(/^PPid:\s+/, '')); + } + }); + } while (name === codeProcessName); + const rawEnv = await readFile(`/proc/${pid}/environ`, 'utf8'); + const env = {}; + rawEnv.split('\0').forEach(e => { + const i = e.indexOf('='); + env[e.substr(0, i)] = e.substr(i + 1); + }); + this._mainProcessParentEnv = env; + } + + // For macOS just return the root environment which seems to always be {}, this is the + // parent of the main process when code is launched from the dock. + if (isMacintosh) { + this._mainProcessParentEnv = {}; + } + + // TODO: Windows should return a fresh environment block, might need native code? + if (isWindows) { + this._mainProcessParentEnv = process.env as IProcessEnvironment; + } + + return this._mainProcessParentEnv!; + } } \ No newline at end of file From e3609a9576b74b9d5b79763a11b7e5b65ce53992 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 16:35:29 -0700 Subject: [PATCH 13/21] Hook up new method --- src/vs/workbench/api/node/extHostTerminalService.ts | 2 +- .../contrib/terminal/browser/terminalProcessManager.ts | 4 ++-- .../workbench/contrib/terminal/common/terminalEnvironment.ts | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/api/node/extHostTerminalService.ts b/src/vs/workbench/api/node/extHostTerminalService.ts index a1de46cfdf5..2aad0a4c38f 100644 --- a/src/vs/workbench/api/node/extHostTerminalService.ts +++ b/src/vs/workbench/api/node/extHostTerminalService.ts @@ -503,7 +503,7 @@ export class ExtHostTerminalService implements ExtHostTerminalServiceShape { terminalConfig.get('setLocaleVariables', false), // Always inherit the environment as we need to be running in a login shell, this may // change when macOS servers are supported - true + process.env as platform.IProcessEnvironment ); // Fork the process and listen for messages diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts index 51ed69c3793..8a3b9b1e9e1 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts @@ -171,8 +171,8 @@ export class TerminalProcessManager implements ITerminalProcessManager { const lastActiveWorkspace = activeWorkspaceRootUri ? this._workspaceContextService.getWorkspaceFolder(activeWorkspaceRootUri) : null; const envFromConfigValue = this._workspaceConfigurationService.inspect(`terminal.integrated.env.${platformKey}`); const isWorkspaceShellAllowed = this._configHelper.checkWorkspaceShellPermissions(); - this._terminalInstanceService.getMainProcessParentEnv(); - const env = terminalEnvironment.createTerminalEnvironment(shellLaunchConfig, lastActiveWorkspace, envFromConfigValue, this._configurationResolverService, isWorkspaceShellAllowed, this._productService.version, this._configHelper.config.setLocaleVariables, this._configHelper.config.inheritEnv); + const baseEnv = this._configHelper.config.inheritEnv ? process.env as platform.IProcessEnvironment : this._terminalInstanceService.getMainProcessParentEnv(); + const env = terminalEnvironment.createTerminalEnvironment(shellLaunchConfig, lastActiveWorkspace, envFromConfigValue, this._configurationResolverService, isWorkspaceShellAllowed, this._productService.version, this._configHelper.config.setLocaleVariables, baseEnv); const useConpty = this._configHelper.config.windowsEnableConpty; return this._terminalInstanceService.createTerminalProcess(shellLaunchConfig, initialCwd, cols, rows, env, useConpty); diff --git a/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts b/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts index 7d260cd9418..0e49495815a 100644 --- a/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts +++ b/src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts @@ -199,7 +199,7 @@ export function createTerminalEnvironment( isWorkspaceShellAllowed: boolean, version: string | undefined, setLocaleVariables: boolean, - inheritEnv: boolean + baseEnv: platform.IProcessEnvironment ): platform.IProcessEnvironment { // Create a terminal environment based on settings, launch config and permissions let env: platform.IProcessEnvironment = {}; @@ -208,7 +208,7 @@ export function createTerminalEnvironment( mergeNonNullKeys(env, shellLaunchConfig.env); } else { // Merge process env with the env from config and from shellLaunchConfig - mergeNonNullKeys(env, inheritEnv ? process.env : {}); + mergeNonNullKeys(env, baseEnv); // const platformKey = platform.isWindows ? 'windows' : (platform.isMacintosh ? 'osx' : 'linux'); // const envFromConfigValue = this._workspaceConfigurationService.inspect(`terminal.integrated.env.${platformKey}`); From 941b5ac526e890eeb53862714f032d794591fe12 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 16:37:56 -0700 Subject: [PATCH 14/21] Make parts of terminal process launching async --- .../contrib/terminal/browser/terminalProcessManager.ts | 10 +++++----- src/vs/workbench/contrib/terminal/common/terminal.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts index 8a3b9b1e9e1..f5c58368380 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts @@ -96,11 +96,11 @@ export class TerminalProcessManager implements ITerminalProcessManager { } } - public createProcess( + public async createProcess( shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number - ): void { + ): Promise { const forceExtHostProcess = (this._configHelper.config as any).extHostProcess; if (shellLaunchConfig.cwd && typeof shellLaunchConfig.cwd === 'object') { this.remoteAuthority = getRemoteAuthority(shellLaunchConfig.cwd); @@ -126,7 +126,7 @@ export class TerminalProcessManager implements ITerminalProcessManager { const activeWorkspaceRootUri = this._historyService.getLastActiveWorkspaceRoot(); this._process = this._instantiationService.createInstance(TerminalProcessExtHostProxy, this._terminalId, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, this._configHelper); } else { - this._process = this._launchProcess(shellLaunchConfig, cols, rows); + this._process = await this._launchProcess(shellLaunchConfig, cols, rows); } this.processState = ProcessState.LAUNCHING; @@ -159,7 +159,7 @@ export class TerminalProcessManager implements ITerminalProcessManager { }, LAUNCHING_DURATION); } - private _launchProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number): ITerminalChildProcess { + private async _launchProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number): Promise { if (!shellLaunchConfig.executable) { this._configHelper.mergeDefaultShellPathAndArgs(shellLaunchConfig, this._terminalInstanceService.getDefaultShell(platform.platform)); } @@ -171,7 +171,7 @@ export class TerminalProcessManager implements ITerminalProcessManager { const lastActiveWorkspace = activeWorkspaceRootUri ? this._workspaceContextService.getWorkspaceFolder(activeWorkspaceRootUri) : null; const envFromConfigValue = this._workspaceConfigurationService.inspect(`terminal.integrated.env.${platformKey}`); const isWorkspaceShellAllowed = this._configHelper.checkWorkspaceShellPermissions(); - const baseEnv = this._configHelper.config.inheritEnv ? process.env as platform.IProcessEnvironment : this._terminalInstanceService.getMainProcessParentEnv(); + const baseEnv = this._configHelper.config.inheritEnv ? process.env as platform.IProcessEnvironment : await this._terminalInstanceService.getMainProcessParentEnv(); const env = terminalEnvironment.createTerminalEnvironment(shellLaunchConfig, lastActiveWorkspace, envFromConfigValue, this._configurationResolverService, isWorkspaceShellAllowed, this._productService.version, this._configHelper.config.setLocaleVariables, baseEnv); const useConpty = this._configHelper.config.windowsEnableConpty; diff --git a/src/vs/workbench/contrib/terminal/common/terminal.ts b/src/vs/workbench/contrib/terminal/common/terminal.ts index bce06565e11..f1091ed5d1c 100644 --- a/src/vs/workbench/contrib/terminal/common/terminal.ts +++ b/src/vs/workbench/contrib/terminal/common/terminal.ts @@ -690,7 +690,7 @@ export interface ITerminalProcessManager extends IDisposable { readonly onProcessExit: Event; dispose(immediate?: boolean): void; - createProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number): void; + createProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number): Promise; write(data: string): void; setDimensions(cols: number, rows: number): void; From bc2758f7663cd9d6fa8b7ca1ab8982727acb1136 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 16:39:31 -0700 Subject: [PATCH 15/21] Fix tests --- .../test/electron-browser/terminalLinkHandler.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/test/electron-browser/terminalLinkHandler.test.ts b/src/vs/workbench/contrib/terminal/test/electron-browser/terminalLinkHandler.test.ts index b5fc5e78e8a..2ed2188c6d0 100644 --- a/src/vs/workbench/contrib/terminal/test/electron-browser/terminalLinkHandler.test.ts +++ b/src/vs/workbench/contrib/terminal/test/electron-browser/terminalLinkHandler.test.ts @@ -49,8 +49,9 @@ class MockTerminalInstanceService implements ITerminalInstanceService { getDefaultShell(p: Platform): string { throw new Error('Method not implemented.'); } - - + getMainProcessParentEnv(): any { + throw new Error('Method not implemented.'); + } } interface LinkFormatInfo { From f6fbe764655ff2a38a00772de46c3a02540694a6 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 16:41:46 -0700 Subject: [PATCH 16/21] Add docs --- .../workbench/contrib/terminal/browser/terminal.contribution.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts b/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts index 0904438ef80..866494f1d8b 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts @@ -229,7 +229,7 @@ configurationRegistry.registerConfiguration({ default: [] }, 'terminal.integrated.inheritEnv': { - markdownDescription: nls.localize('terminal.integrated.inheritEnv', "TODO"), + markdownDescription: nls.localize('terminal.integrated.inheritEnv', "Whether new shells should inherit their environment from VS Code."), type: 'boolean', default: true }, From fa4071952433e1af764b18aac445df8a8a7d1499 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 17:06:20 -0700 Subject: [PATCH 17/21] Typos --- src/vs/code/node/shellEnv.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vs/code/node/shellEnv.ts b/src/vs/code/node/shellEnv.ts index eea6922afbe..fee0648ea13 100644 --- a/src/vs/code/node/shellEnv.ts +++ b/src/vs/code/node/shellEnv.ts @@ -96,10 +96,10 @@ export function getShellEnvironment(logService: ILogService, environmentService: logService.trace('getShellEnvironment: disable-user-env-probe set, skipping'); _shellEnv = Promise.resolve({}); } else if (isWindows) { - logService.trace('getShellEnvironment: runing on windows, skipping'); + logService.trace('getShellEnvironment: running on Windows, skipping'); _shellEnv = Promise.resolve({}); } else if (process.env['VSCODE_CLI'] === '1') { - logService.trace('getShellEnvironment: runing on CLI, skipping'); + logService.trace('getShellEnvironment: running on CLI, skipping'); _shellEnv = Promise.resolve({}); } else { logService.trace('getShellEnvironment: running on Unix'); From 224399eadcb6848d4a3855c31939b6b39302e746 Mon Sep 17 00:00:00 2001 From: SteVen Batten Date: Wed, 12 Jun 2019 17:56:54 -0700 Subject: [PATCH 18/21] update distro --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 99db23062ff..567cab0694f 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "code-oss-dev", "version": "1.36.0", - "distro": "060a874751adc68d68777085e88c99a8f04ba5da", + "distro": "fa58edd879968b01e012f8cd56860787765aff8a", "author": { "name": "Microsoft Corporation" }, From ce84b4afab9671a96bfb58ddf81b07ceb3ceddfc Mon Sep 17 00:00:00 2001 From: SteVen Batten Date: Wed, 12 Jun 2019 17:57:37 -0700 Subject: [PATCH 19/21] remove equal sign params from web scripts --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 567cab0694f..64210383cec 100644 --- a/package.json +++ b/package.json @@ -25,8 +25,8 @@ "download-builtin-extensions": "node build/lib/builtInExtensions.js", "monaco-compile-check": "tsc -p src/tsconfig.monaco.json --noEmit", "strict-initialization-watch": "tsc --watch -p src/tsconfig.json --noEmit --strictPropertyInitialization", - "web": "node resources/server/bin-dev/code-web.js --port=9888", - "web-selfhost": "node resources/server/bin-dev/code-web.js --port=9777 --selfhost --folder .", + "web": "node resources/server/bin-dev/code-web.js --port 9888", + "web-selfhost": "node resources/server/bin-dev/code-web.js --port 9777 --selfhost --folder .", "install-server": "git fetch distro && git checkout distro/distro -- src/vs/server resources/server && git reset HEAD src/vs/server resources/server" }, "dependencies": { From 78dfd041de4b4ec052a9cdbcea8b11a46c0201c7 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 18:07:41 -0700 Subject: [PATCH 20/21] Get mac launching with mostly root env Part of #70248 --- .../terminalInstanceService.ts | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts b/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts index 45a5971aad9..e22c8b92540 100644 --- a/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts +++ b/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts @@ -98,10 +98,29 @@ export class TerminalInstanceService implements ITerminalInstanceService { this._mainProcessParentEnv = env; } - // For macOS just return the root environment which seems to always be {}, this is the - // parent of the main process when code is launched from the dock. + // For macOS it doesn't appear to be possible to get the "root" environment as + // `ps eww -o command` for PID 1 (the parent of the main process when launched from the + // dock/finder) returns no environment, because of this we will fill in the root environment + // using a whitelist of environment variables that we have. if (isMacintosh) { this._mainProcessParentEnv = {}; + // This list was generated by diffing launching a terminal with {} and the system + // terminal launched from finder. + const rootEnvVars = [ + 'SHELL', + 'SSH_AUTH_SOCK', + 'Apple_PubSub_Socket_Render', + 'XPC_FLAGS', + 'XPC_SERVICE_NAME', + 'HOME', + 'LOGNAME', + 'TMPDIR' + ]; + rootEnvVars.forEach(k => { + if (process.env[k]) { + this._mainProcessParentEnv![k] = process.env[k]!; + } + }); } // TODO: Windows should return a fresh environment block, might need native code? From 1988f12757c03ee032fcb0cc07eecc546ad35c55 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 12 Jun 2019 18:15:08 -0700 Subject: [PATCH 21/21] Clarify issue in comment --- .../terminal/electron-browser/terminalInstanceService.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts b/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts index e22c8b92540..885f01b4e4f 100644 --- a/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts +++ b/src/vs/workbench/contrib/terminal/electron-browser/terminalInstanceService.ts @@ -98,10 +98,11 @@ export class TerminalInstanceService implements ITerminalInstanceService { this._mainProcessParentEnv = env; } - // For macOS it doesn't appear to be possible to get the "root" environment as - // `ps eww -o command` for PID 1 (the parent of the main process when launched from the - // dock/finder) returns no environment, because of this we will fill in the root environment - // using a whitelist of environment variables that we have. + // For macOS we want the "root" environment as shells by default run as login shells. It + // doesn't appear to be possible to get the "root" environment as `ps eww -o command` for + // PID 1 (the parent of the main process when launched from the dock/finder) returns no + // environment, because of this we will fill in the root environment using a whitelist of + // environment variables that we have. if (isMacintosh) { this._mainProcessParentEnv = {}; // This list was generated by diffing launching a terminal with {} and the system