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.

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

Conversation

Projects
None yet
6 participants
@cmfcmf
Copy link
Contributor

cmfcmf commented Jan 3, 2019

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": []
        },
    ]
}

cmfcmf added some commits Jan 3, 2019

@msftclas

This comment has been minimized.

Copy link

msftclas commented Jan 3, 2019

CLA assistant check
All CLA requirements met.

@alexr00 alexr00 self-requested a review Jan 7, 2019

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

This comment has been minimized.

@alexr00

alexr00 Jan 7, 2019

Member

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.

This comment has been minimized.

@Tyriar

Tyriar Jan 7, 2019

Member

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).

This comment has been minimized.

@cmfcmf

cmfcmf Jan 7, 2019

Author Contributor

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

This comment has been minimized.

@alexr00

alexr00 Jan 8, 2019

Member

@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 alexr00 requested a review from Tyriar Jan 7, 2019

@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 7, 2019

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
Copy link
Member

alexr00 left a comment

See comments above.

cmfcmf added some commits Jan 7, 2019

@cmfcmf

This comment has been minimized.

Copy link
Contributor Author

cmfcmf commented Jan 7, 2019

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.

// 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 comment has been minimized.

@cmfcmf

cmfcmf Jan 7, 2019

Author Contributor

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?

export class LinkedMap<K, V> {

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

This comment has been minimized.

@alexr00

alexr00 Jan 8, 2019

Member

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.

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

This comment has been minimized.

@alexr00

alexr00 Jan 8, 2019

Member

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

This comment has been minimized.

@TwitchBronBron

TwitchBronBron Jan 15, 2019

@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
            }
        }
    ]
}

This comment has been minimized.

@TwitchBronBron

TwitchBronBron Jan 15, 2019

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

This comment has been minimized.

@cmfcmf

cmfcmf Jan 20, 2019

Author Contributor

I have now renamed terminalGroup to just group.

cmfcmf added some commits Jan 20, 2019

@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 21, 2019

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

@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 23, 2019

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

Tyriar approved these changes Jan 24, 2019

Copy link
Member

Tyriar left a comment

lgtm 👍

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

@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 24, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

cmfcmf commented Jan 24, 2019

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

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 24, 2019

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.
splitterminals

Only keep references to terminals in sameTaskTerminals and idleTaskTe…
…rminals if the terminal is not disposed
@cmfcmf

This comment has been minimized.

Copy link
Contributor Author

cmfcmf commented Jan 24, 2019

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:

if (prefersSameTerminal) {
let terminalId = this.sameTaskTerminals[taskKey];
if (terminalId) {
terminalToReuse = this.terminals[terminalId];
delete this.sameTaskTerminals[taskKey];
}
} else if (allowsSharedTerminal) {
let terminalId = this.idleTaskTerminals.remove(taskKey) || this.idleTaskTerminals.shift();
if (terminalId) {
terminalToReuse = this.terminals[terminalId];
}
}
if (terminalToReuse) {

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) {
@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 25, 2019

Nice feature, thank you @cmfcmf!

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

2 checks passed

VS Code #20190124.74 succeeded
Details
license/cla All CLA requirements met.
Details

@cmfcmf cmfcmf deleted the cmfcmf:feature/tasks/terminal-group branch Jan 25, 2019

aeschli added a commit that referenced this pull request Jan 25, 2019

Add terminalGroup to tasks to allow running them in split panes (#65973)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment