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

cmfcmf · 2019-01-03T15:32:52Z

This is an implementation of #47265, which is currently assigned to @dbaeumer. It adds a new field terminalGroup to the presentation option of the .vscode/tasks.json file. When supplied, all tasks of the same terminalGroup open as split panes in the same tab. Tasks without terminalGroup are not allowed to reuse terminals created by tasks which have a terminalGroup.

I haven't written tests yet, but can add them if you think the general direction of this PR is fine.

Example `tasks.json`

{
    // See https://go.microsoft.com/fwlink/?LinkId=733558
    // for the documentation about the tasks.json format
    "version": "2.0.0",
    "tasks": [
        {
            "label": "tasks",
            "dependsOn": ["task1", "task2", "task3", "task4", "task5", "task6"],
            "runOptions": {
                "runOn": "folderOpen"
            },
        },
        {
            "label": "task1",
            "type": "shell",
            "command": "echo Task1 && sleep 5",
            "presentation": {
                "terminalGroup": "group"
            },
            "problemMatcher": []
        },
        {
            "label": "task2",
            "type": "shell",
            "command": "echo Task2 && sleep 5",
            "presentation": {
                "terminalGroup": "group"
            },
            "problemMatcher": []
        },
        {
            "label": "task3",
            "type": "shell",
            "command": "echo Task3 && sleep 5",
            "presentation": {
                "terminalGroup": "group"
            },
            "problemMatcher": []
        },
        {
            "label": "task4",
            "type": "shell",
            "command": "echo Task4 && sleep 5",
            "presentation": {
                "terminalGroup": "group"
            },
            "problemMatcher": []
        },
        {
            "label": "task5",
            "type": "shell",
            "command": "echo Task5 && sleep 5",
            "presentation": {
                "terminalGroup": "group"
            },
            "problemMatcher": []
        },
        {
            "label": "task6",
            "type": "shell",
            "command": "echo Task6 && sleep 5",
            "presentation": {
                "terminalGroup": "group"
            },
            "problemMatcher": []
        },
    ]
}

msftclas · 2019-01-03T15:33:04Z

All CLA requirements met.

alexr00 · 2019-01-07T10:10:36Z

src/vs/vscode.d.ts

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


This will need to go into vscode.proposed.d.ts. Once the API change has gone through our review process I'll move it to vscode.d.ts.

I made a comment on the request about this #47265 (comment), I'd like to be in the API call when it happens (I should be in the next one).

I didn't even realize I was changing the API 😄 Moved it into vscode.proposed.d.ts as requested.

@Tyriar, sounds good. I'll bring it up for the next API call.
@cmfcmf, expect some more change requests sometime next week from the API discussion. I think you're heading in a good direction here though, and I don't expect any big functional changes.

alexr00 · 2019-01-07T11:59:41Z

I tried out this change with 2 tasks in terminal group a and 1 task in terminal group b. Then I did the following:

Ran the first task in group a
Ran the second task in group a
Both tasks finished and were split correctly
Reran the second task in group a and it correctly reused the same terminal
Ran the task in group b.
It didn't run. Instead, it gave an error about not being to create a terminal.

alexr00

See comments above.

src/vs/workbench/parts/terminal/common/terminalService.ts

https://github.com/Microsoft/vscode/pull/65973/files/42e3171a71ae4b6963e47318fd289a66eb48a96a#r245762395

cmfcmf · 2019-01-07T19:57:53Z

I tried out this change with 2 tasks in terminal group a and 1 task in terminal group b. Then I did the following:

* Ran the first task in group `a`

* Ran the second task in group `a`

* Both tasks finished and were split correctly

* Reran the second task in group `a` and it correctly reused the same terminal

* Ran the task in group `b`.

* It didn't run. Instead, it gave an error about not being to create a terminal.

Thank you for testing; I could reproduce the error. It was caused by me accidentally using idleTerminals[...] instead of idleTerminals.get(). Fixed in e397e02.

cmfcmf · 2019-01-07T20:03:16Z

src/vs/workbench/parts/tasks/electron-browser/terminalTaskSystem.ts

+				// Search for any idle terminal used previously by a task of the same terminalGroup
+				// (or, if the task has no terminalGroup, a terminal used by a task without terminalGroup).
+				for (const taskId of this.idleTaskTerminals.keys()) {
+					const idleTerminalId = this.idleTaskTerminals.get(taskId)!;


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; }

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.

alexr00 · 2019-01-08T09:15:25Z

src/vs/vscode.proposed.d.ts

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


I'm leaning more towards calling this group. That way we aren't tied to any specific presenter.

@alexr00 I don't think you can call it group. There's already a group property for tasks. It's used to specify if the task is the default build task or default test task. I'm assuming they would conflict? See this link for details,

{ // See https://go.microsoft.com/fwlink/?LinkId=733558 // for the documentation about the tasks.json format "version": "2.0.0", "tasks": [ { "type": "typescript", "tsconfig": "tsconfig.json", "problemMatcher": [ "$tsc" ], "group": { "kind": "build", "isDefault": true } } ] }

Nevermind...I skimmed the PR and hadn't realized the "group" property was placed into "presentation". Ignore me. :D

I have now renamed terminalGroup to just group.

…nal-group

alexr00 · 2019-01-21T11:27:39Z

@cmfcmf, thanks for the updates! I should have more API feedback for you this week.

alexr00 · 2019-01-23T11:37:09Z

Looks good to me! The API change is ok to merge into vscode.proposed.d.ts too. We will leave it there for a bit and make sure we don't want to make any changes to it before moving it to vscode.d.ts. @Tyriar do you have any feedback on the terminal changes before I merge?

Tyriar

lgtm 👍

alexr00 · 2019-01-24T09:29:51Z

@cmfcmf I did some more testing before merging and I found another issue. My test tasks:

{
	"version": "2.0.0",
	"tasks": [
		{
			"label": "Task A",
			"type": "shell",
			"command": "echo A && sleep 5000",
			"problemMatcher": [],
			"presentation": {
				"group": "groupA"
			}
		},
		{
			"label": "Task B",
			"type": "shell",
			"command": "echo B",
			"problemMatcher": [],
			"presentation": {
				"group": "groupA"
			}
		},
		{
			"label": "Task C",
			"type": "shell",
			"command": "echo C && sleep 5000",
			"problemMatcher": [],
			"presentation": {
				"group": "groupC"
			}
		},
		{
			"label": "Task D",
			"type": "shell",
			"command": "echo D && sleep 5000",
			"problemMatcher": [],
			"presentation": {
				"group": "groupC"
			}
		},
		{
			"label": "Task E",
			"type": "shell",
			"command": "echo E && sleep 5000",
			"problemMatcher": []
		},
		{
			"label": "Task F",
			"type": "shell",
			"command": "echo F && sleep 5000",
			"problemMatcher": []
		},
	]
}

I ran Task B. After it finished I left it's terminal open and ran Task A. Task A failed to run with a message about being unable to create a terminal.

cmfcmf · 2019-01-24T11:05:58Z

I ran Task B. After it finished I left it's terminal open and ran Task A. Task A failed to run with a message about being unable to create a terminal.

@alexr00 I'm afraid I can't reproduce the issue. I tried yours and a range of other possible combinations and it worked fine.
Could you copy the error message from the debugging console, if there is any? Are you sure you're on the latest commit and have yarn installed the dependencies (when I merged master 4 days ago I had to re-install the dependencies due to updates on master)?

alexr00 · 2019-01-24T15:07:46Z

I was on the correct commit, but I hadn't run yarn install. Looks like that was it since I can't repro now. However, I am seeing some other odd behavior:
I run Task A, then kill it with the trash can icon. Then I run Task B and it fails.

…rminals if the terminal is not disposed

cmfcmf · 2019-01-24T19:23:35Z

Good catch @alexr00! I forgot to test the trash can icon up until now and was able to reproduce the behavior and fix it in 708aa4c. This was actually a problem related to the existing code: When a terminal is killed / disposed using the trash icon, its id was still added to sameTaskTerminals/idleTaskTerminals. This didn't cause any problems up until this PR, because before using an id from sameTaskTerminals/idleTaskTerminals, terminalToReuse was always checked for truthiness:
https://github.com/Microsoft/vscode/blob/ac1812d748463ca1cb1e6777e7029134f4b56f2f/src/vs/workbench/parts/tasks/electron-browser/terminalTaskSystem.ts#L863-L875

This implicitly avoided accessing disposed terminals. I, however, made the assumption that all ids referenced in sameTaskTerminals/idleTaskTerminals are actually pointing to valid terminals, which caused this line from my PR to try to access .group on undefined:

if (this.terminals[idleTerminalId].group === group) {

src/vs/workbench/parts/tasks/electron-browser/terminalTaskSystem.ts

alexr00 · 2019-01-25T10:11:44Z

Nice feature, thank you @cmfcmf!

* Return splitted instance from ITerminalService::splitInstance * Add support for terminalGroup to tasks * Early-exit when the tab could not be split https://github.com/Microsoft/vscode/pull/65973/files/42e3171a71ae4b6963e47318fd289a66eb48a96a#r245762395 * Use .get()! instead of the unsupported [] to access LinkedMap * Move api changes into `vscode.proposed.d.ts` * Rename "terminalGroup" to "group" * Only keep references to terminals in sameTaskTerminals and idleTaskTerminals if the terminal is not disposed * Type result variable Fixes #47265

cmfcmf added 2 commits January 3, 2019 16:03

Return splitted instance from ITerminalService::splitInstance

c75984c

Add support for terminalGroup to tasks

42e3171

egamma assigned dbaeumer and alexr00 Jan 4, 2019

alexr00 self-requested a review January 7, 2019 10:08

alexr00 reviewed Jan 7, 2019

View reviewed changes

alexr00 requested a review from Tyriar January 7, 2019 10:21

alexr00 requested changes Jan 7, 2019

View reviewed changes

Tyriar mentioned this pull request Jan 7, 2019

Not loading images in Description page microsoft/vscode-pull-request-github#778

Closed

Tyriar reviewed Jan 7, 2019

View reviewed changes

src/vs/workbench/parts/terminal/common/terminalService.ts Outdated Show resolved Hide resolved

cmfcmf added 3 commits January 7, 2019 20:07

Early-exit when the tab could not be split

5637011

https://github.com/Microsoft/vscode/pull/65973/files/42e3171a71ae4b6963e47318fd289a66eb48a96a#r245762395

Use .get()! instead of the unsupported [] to access LinkedMap

e397e02

Move api changes into vscode.proposed.d.ts

32f4930

cmfcmf commented Jan 7, 2019

View reviewed changes

alexr00 reviewed Jan 8, 2019

View reviewed changes

cmfcmf added 2 commits January 20, 2019 14:29

Merge remote-tracking branch 'origin/master' into feature/tasks/termi…

b1778b2

…nal-group

Rename "terminalGroup" to "group"

4414683

alexr00 approved these changes Jan 23, 2019

View reviewed changes

Tyriar approved these changes Jan 24, 2019

View reviewed changes

Tyriar added this to the December/January 2019 milestone Jan 24, 2019

Only keep references to terminals in sameTaskTerminals and idleTaskTe…

708aa4c

…rminals if the terminal is not disposed

Tyriar reviewed Jan 24, 2019

View reviewed changes

src/vs/workbench/parts/tasks/electron-browser/terminalTaskSystem.ts Outdated Show resolved Hide resolved

Type result variable

0d72f88

alexr00 merged commit 7a9f7e5 into microsoft:master Jan 25, 2019

cmfcmf deleted the feature/tasks/terminal-group branch January 25, 2019 10:23

github-actions bot locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cmfcmf commented Jan 3, 2019 •

edited

Loading

msftclas commented Jan 3, 2019 •

edited

Loading

alexr00 Jan 7, 2019

Tyriar Jan 7, 2019

cmfcmf Jan 7, 2019

alexr00 Jan 8, 2019 •

edited

Loading

alexr00 commented Jan 7, 2019

alexr00 left a comment

cmfcmf commented Jan 7, 2019

cmfcmf Jan 7, 2019

alexr00 Jan 8, 2019

alexr00 Jan 8, 2019

TwitchBronBron Jan 15, 2019

TwitchBronBron Jan 15, 2019 •

edited

Loading

cmfcmf Jan 20, 2019

alexr00 commented Jan 21, 2019

alexr00 commented Jan 23, 2019

Tyriar left a comment

alexr00 commented Jan 24, 2019 •

edited

Loading

cmfcmf commented Jan 24, 2019

alexr00 commented Jan 24, 2019 •

edited

Loading

cmfcmf commented Jan 24, 2019

alexr00 commented Jan 25, 2019 •

edited

Loading

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

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

Conversation

cmfcmf commented Jan 3, 2019 • edited Loading

msftclas commented Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexr00 Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

alexr00 commented Jan 7, 2019

alexr00 left a comment

Choose a reason for hiding this comment

cmfcmf commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TwitchBronBron Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexr00 commented Jan 21, 2019

alexr00 commented Jan 23, 2019

Tyriar left a comment

Choose a reason for hiding this comment

alexr00 commented Jan 24, 2019 • edited Loading

cmfcmf commented Jan 24, 2019

alexr00 commented Jan 24, 2019 • edited Loading

cmfcmf commented Jan 24, 2019

alexr00 commented Jan 25, 2019 • edited Loading

cmfcmf commented Jan 3, 2019 •

edited

Loading

msftclas commented Jan 3, 2019 •

edited

Loading

alexr00 Jan 8, 2019 •

edited

Loading

TwitchBronBron Jan 15, 2019 •

edited

Loading

alexr00 commented Jan 24, 2019 •

edited

Loading

alexr00 commented Jan 24, 2019 •

edited

Loading

alexr00 commented Jan 25, 2019 •

edited

Loading