Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add terminalGroup to tasks to allow running them in split panes #65973

Merged
merged 9 commits into from
Jan 25, 2019
Merged

Add terminalGroup to tasks to allow running them in split panes #65973

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c75984c
Return splitted instance from ITerminalService::splitInstance
cmfcmf Jan 3, 2019
42e3171
Add support for terminalGroup to tasks
cmfcmf Jan 3, 2019
5637011
Early-exit when the tab could not be split
cmfcmf Jan 7, 2019
e397e02
Use .get()! instead of the unsupported [] to access LinkedMap
cmfcmf Jan 7, 2019
32f4930
Move api changes into `vscode.proposed.d.ts`
cmfcmf Jan 7, 2019
b1778b2
Merge remote-tracking branch 'origin/master' into feature/tasks/termi…
cmfcmf Jan 20, 2019
4414683
Rename "terminalGroup" to "group"
cmfcmf Jan 20, 2019
708aa4c
Only keep references to terminals in sameTaskTerminals and idleTaskTe…
cmfcmf Jan 24, 2019
0d72f88
Type result variable
cmfcmf Jan 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/vs/vscode.proposed.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,5 +1121,13 @@ declare module 'vscode' {
}
//#endregion

//#region Tasks
export interface TaskPresentationOptions {
/**
* Controls whether the task is executed in a specific terminal group using split panes.
*/
group?: string;
}
//#endregion

}
1 change: 1 addition & 0 deletions src/vs/workbench/api/shared/tasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface TaskPresentationOptionsDTO {
panel?: number;
showReuseMessage?: boolean;
clear?: boolean;
group?: string;
}

export interface RunOptionsDTO {
Expand Down
5 changes: 5 additions & 0 deletions src/vs/workbench/parts/tasks/common/tasks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,11 @@ export interface PresentationOptions {
* Controls whether to clear the terminal before executing the task.
*/
clear: boolean;

/**
* Controls whether the task is executed in a specific terminal group using split panes.
*/
group?: string;
}

export namespace PresentationOptions {
Expand Down
6 changes: 5 additions & 1 deletion src/vs/workbench/parts/tasks/electron-browser/jsonSchema_v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,11 @@ const presentation: IJSONSchema = {
type: 'boolean',
default: false,
description: nls.localize('JsonSchema.tasks.presentation.clear', 'Controls whether the terminal is cleared before executing the task.')
}
},
group: {
type: 'string',
description: nls.localize('JsonSchema.tasks.presentation.group', 'Controls whether the task is executed in a specific terminal group using split panes.')
},
}
};

Expand Down
76 changes: 58 additions & 18 deletions src/vs/workbench/parts/tasks/electron-browser/terminalTaskSystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import * as Objects from 'vs/base/common/objects';
import * as Types from 'vs/base/common/types';
import * as Platform from 'vs/base/common/platform';
import * as Async from 'vs/base/common/async';
import { IStringDictionary } from 'vs/base/common/collections';
import { IStringDictionary, values } from 'vs/base/common/collections';
import { LinkedMap, Touch } from 'vs/base/common/map';
import Severity from 'vs/base/common/severity';
import { Event, Emitter } from 'vs/base/common/event';
Expand Down Expand Up @@ -42,6 +42,7 @@ import {
interface TerminalData {
terminal: ITerminalInstance;
lastTask: string;
group?: string;
}

interface ActiveTerminalData {
Expand Down Expand Up @@ -538,13 +539,16 @@ export class TerminalTaskSystem implements ITaskSystem {
let key = task.getMapKey();
delete this.activeTasks[key];
this._onDidStateChange.fire(TaskEvent.create(TaskEventKind.Changed));
switch (task.command.presentation!.panel) {
case PanelKind.Dedicated:
this.sameTaskTerminals[key] = terminal!.id.toString();
break;
case PanelKind.Shared:
this.idleTaskTerminals.set(key, terminal!.id.toString(), Touch.AsOld);
break;
if (exitCode !== undefined) {
// Only keep a reference to the terminal if it is not being disposed.
switch (task.command.presentation!.panel) {
case PanelKind.Dedicated:
this.sameTaskTerminals[key] = terminal!.id.toString();
break;
case PanelKind.Shared:
this.idleTaskTerminals.set(key, terminal!.id.toString(), Touch.AsOld);
break;
}
}
let reveal = task.command.presentation!.reveal;
if ((reveal === RevealKind.Silent) && ((exitCode !== 0) || (watchingProblemMatcher.numberOfMatches > 0) && watchingProblemMatcher.maxMarkerSeverity &&
Expand Down Expand Up @@ -601,13 +605,16 @@ export class TerminalTaskSystem implements ITaskSystem {
let key = task.getMapKey();
delete this.activeTasks[key];
this._onDidStateChange.fire(TaskEvent.create(TaskEventKind.Changed));
switch (task.command.presentation!.panel) {
case PanelKind.Dedicated:
this.sameTaskTerminals[key] = terminal!.id.toString();
break;
case PanelKind.Shared:
this.idleTaskTerminals.set(key, terminal!.id.toString(), Touch.AsOld);
break;
if (exitCode !== undefined) {
// Only keep a reference to the terminal if it is not being disposed.
switch (task.command.presentation!.panel) {
case PanelKind.Dedicated:
this.sameTaskTerminals[key] = terminal!.id.toString();
break;
case PanelKind.Shared:
this.idleTaskTerminals.set(key, terminal!.id.toString(), Touch.AsOld);
break;
}
}
let reveal = task.command.presentation!.reveal;
if (terminal && (reveal === RevealKind.Silent) && ((exitCode !== 0) || (startStopProblemMatcher.numberOfMatches > 0) && startStopProblemMatcher.maxMarkerSeverity &&
Expand Down Expand Up @@ -857,6 +864,7 @@ export class TerminalTaskSystem implements ITaskSystem {
}
let prefersSameTerminal = presentationOptions.panel === PanelKind.Dedicated;
let allowsSharedTerminal = presentationOptions.panel === PanelKind.Shared;
let group = presentationOptions.group;

let taskKey = task.getMapKey();
let terminalToReuse: TerminalData | undefined;
Expand All @@ -867,7 +875,20 @@ export class TerminalTaskSystem implements ITaskSystem {
delete this.sameTaskTerminals[taskKey];
}
} else if (allowsSharedTerminal) {
let terminalId = this.idleTaskTerminals.remove(taskKey) || this.idleTaskTerminals.shift();
// Always allow to reuse the terminal previously used by the same task.
let terminalId = this.idleTaskTerminals.remove(taskKey);
if (!terminalId) {
// There is no idle terminal which was used by the same task.
// Search for any idle terminal used previously by a task of the same group
// (or, if the task has no group, a terminal used by a task without group).
for (const taskId of this.idleTaskTerminals.keys()) {
const idleTerminalId = this.idleTaskTerminals.get(taskId)!;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop needs to access both key and value of the idleTaskTerminals LinkedMap. Maybe it is worth adding an entries(): [V, K][] method to the LinkedMap class to avoid usage of the ! operator? WDYT?
https://github.com/Microsoft/vscode/blob/ac2c2922dc1ce64e694c0b6d024b68af42333020/src/vs/base/common/map.ts#L499

	entries(): [V, K][] {
		let result: [V, K][] = [];
		let current = this._head;
		while (current) {
			result.push([current.value, current.key]);
			current = current.next;
		}
		return result;
	}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea but I can't justify it. I don't see that there would be a noticable performance gain with adding entries and in my opinion it wouldn't significantly improve the readability either.

if (this.terminals[idleTerminalId].group === group) {
terminalId = this.idleTaskTerminals.remove(taskId);
break;
}
}
}
if (terminalId) {
terminalToReuse = this.terminals[terminalId];
}
Expand All @@ -880,7 +901,26 @@ export class TerminalTaskSystem implements ITaskSystem {
return [terminalToReuse.terminal, commandExecutable, undefined];
}

const result = this.terminalService.createTerminal(this.currentTask.shellLaunchConfig);
let result;
cmfcmf marked this conversation as resolved.
Show resolved Hide resolved
if (group) {
// Try to find an existing terminal to split.
// Even if an existing terminal is found, the split can fail if the terminal width is too small.
for (const terminal of values(this.terminals)) {
if (terminal.group === group) {
const originalInstance = terminal.terminal;
const config = this.currentTask.shellLaunchConfig;
result = this.terminalService.splitInstance(originalInstance, config);
if (result) {
break;
}
}
}
}
if (!result) {
// Either no group is used, no terminal with the group exists or splitting an existing terminal failed.
result = this.terminalService.createTerminal(this.currentTask.shellLaunchConfig);
}

const terminalKey = result.id.toString();
result.onDisposed((terminal) => {
let terminalData = this.terminals[terminalKey];
Expand All @@ -895,7 +935,7 @@ export class TerminalTaskSystem implements ITaskSystem {
delete this.activeTasks[task.getMapKey()];
}
});
this.terminals[terminalKey] = { terminal: result, lastTask: taskKey };
this.terminals[terminalKey] = { terminal: result, lastTask: taskKey, group };
return [result, commandExecutable, undefined];
}

Expand Down
13 changes: 11 additions & 2 deletions src/vs/workbench/parts/tasks/node/taskConfiguration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ export interface PresentationOptionsConfig {
* Controls whether the terminal should be cleared before running the task.
*/
clear?: boolean;

/**
* Controls whether the task is executed in a specific terminal group using split panes.
*/
group?: string;
}

export interface RunOptionsConfig {
Expand Down Expand Up @@ -794,7 +799,7 @@ namespace CommandOptions {
namespace CommandConfiguration {

export namespace PresentationOptions {
const properties: MetaData<Tasks.PresentationOptions, void>[] = [{ property: 'echo' }, { property: 'reveal' }, { property: 'focus' }, { property: 'panel' }, { property: 'showReuseMessage' }, { property: 'clear' }];
const properties: MetaData<Tasks.PresentationOptions, void>[] = [{ property: 'echo' }, { property: 'reveal' }, { property: 'focus' }, { property: 'panel' }, { property: 'showReuseMessage' }, { property: 'clear' }, { property: 'group' }];

interface PresentationOptionsShape extends LegacyCommandProperties {
presentation?: PresentationOptionsConfig;
Expand All @@ -807,6 +812,7 @@ namespace CommandConfiguration {
let panel: Tasks.PanelKind;
let showReuseMessage: boolean;
let clear: boolean;
let group: string | undefined;
let hasProps = false;
if (Types.isBoolean(config.echoCommand)) {
echo = config.echoCommand;
Expand Down Expand Up @@ -836,12 +842,15 @@ namespace CommandConfiguration {
if (Types.isBoolean(presentation.clear)) {
clear = presentation.clear;
}
if (Types.isString(presentation.group)) {
group = presentation.group;
}
hasProps = true;
}
if (!hasProps) {
return undefined;
}
return { echo: echo!, reveal: reveal!, focus: focus!, panel: panel!, showReuseMessage: showReuseMessage!, clear: clear! };
return { echo: echo!, reveal: reveal!, focus: focus!, panel: panel!, showReuseMessage: showReuseMessage!, clear: clear!, group };
}

export function assignProperties(target: Tasks.PresentationOptions, source: Tasks.PresentationOptions | undefined): Tasks.PresentationOptions | undefined {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/parts/terminal/common/terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ export interface ITerminalService {
setActiveInstance(terminalInstance: ITerminalInstance): void;
setActiveInstanceByIndex(terminalIndex: number): void;
getActiveOrCreateInstance(wasNewTerminalAction?: boolean): ITerminalInstance;
splitInstance(instance: ITerminalInstance, shell?: IShellLaunchConfig): void;
splitInstance(instance: ITerminalInstance, shell?: IShellLaunchConfig): ITerminalInstance | null;

getActiveTab(): ITerminalTab | null;
setActiveTabToNext(): void;
Expand Down
18 changes: 10 additions & 8 deletions src/vs/workbench/parts/terminal/common/terminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,21 +264,23 @@ export abstract class TerminalService implements ITerminalService {
this.setActiveTabByIndex(newIndex);
}

public splitInstance(instanceToSplit: ITerminalInstance, shellLaunchConfig: IShellLaunchConfig = {}): void {
public splitInstance(instanceToSplit: ITerminalInstance, shellLaunchConfig: IShellLaunchConfig = {}): ITerminalInstance | null {
const tab = this._getTabForInstance(instanceToSplit);
if (!tab) {
return;
return null;
}

const instance = tab.split(this._terminalFocusContextKey, this.configHelper, shellLaunchConfig);
if (instance) {
this._initInstanceListeners(instance);
this._onInstancesChanged.fire();

this._terminalTabs.forEach((t, i) => t.setVisible(i === this._activeTabIndex));
} else {
if (!instance) {
this._showNotEnoughSpaceToast();
return null;
}

this._initInstanceListeners(instance);
this._onInstancesChanged.fire();

this._terminalTabs.forEach((t, i) => t.setVisible(i === this._activeTabIndex));
return instance;
}

protected _initInstanceListeners(instance: ITerminalInstance): void {
Expand Down