Skip to content

Add terminal.newHere command#79863

Merged
Tyriar merged 5 commits intomicrosoft:masterfrom
jeanp413:terminal-here-command
Sep 13, 2019
Merged

Add terminal.newHere command#79863
Tyriar merged 5 commits intomicrosoft:masterfrom
jeanp413:terminal-here-command

Conversation

@jeanp413
Copy link
Contributor

Fixes #79133
Let me know if there's some missing validation for the cwd command argument.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Instead of a new command, workbench.action.terminal.new should accept an optional arg cwd so keybindings can be setup for different cwds

@jeanp413
Copy link
Contributor Author

jeanp413 commented Sep 6, 2019

@Tyriar I looked into that at first but workbench.action.terminal.new is an action and action arguments are not generic, they just use a from property when invoked through a keybinding

try {
const from = args && args.from || 'keybinding';
await actionInstance.run(undefined, { from });
} finally {

also when creating actions, it doesn't seem to be a way to specify the args schema like this

args: [{
  name: 'cwd',
  schema: {
      'type': 'string'
  }
}]

Should this PR contain changes to allow actions with generic arguments?

@Tyriar
Copy link
Member

Tyriar commented Sep 6, 2019

Can workbench.action.terminal.new be converted to extend Command then?

@jeanp413
Copy link
Contributor Author

jeanp413 commented Sep 9, 2019

Not sure about that, it needs to be an action because it's used in getActions() and _getContextMenuActions() from terminalPanel.
Do you know if there is a way for workbench.action.terminal.new to be an Action and also a Command?

@Tyriar
Copy link
Member

Tyriar commented Sep 9, 2019

Good point, can we do what SwitchSideBarViewAction does here?

@jeanp413
Copy link
Contributor Author

Are you referring to the parameter offset? that's just a constant value provided by the child classes that inherit from SwitchSideBarViewAction when calling the parent method

export class PreviousSideBarViewAction extends SwitchSideBarViewAction {
...
	run(): Promise<any> {
		return super.run(-1);
	}
}

export class NextSideBarViewAction extends SwitchSideBarViewAction {
...
	run(): Promise<any> {
		return super.run(1);
	}
}

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

@jeanp413 thanks for your patience, I tried to do what i was thinking and it ended up not working out. I changed the name of the setting to newWithCwd and also for the actual cwd to be wrapped in an object and gave it a description:

Screen Shot 2019-09-13 at 11 07 49 AM

Here's an example of a keybinding now:

{
    "key": "cmd+shift+h",
    "command": "workbench.action.terminal.newWithCwd",
    "args": {
        "cwd": "${fileDirname}"
    }
}

@Tyriar Tyriar added this to the September 2019 milestone Sep 13, 2019
@Tyriar Tyriar merged commit 1963239 into microsoft:master Sep 13, 2019
@jeanp413 jeanp413 deleted the terminal-here-command branch September 13, 2019 19:14
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

workbench.action.terminal.new should accept cwd as an argument

2 participants