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.

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 terminal.newHere command #79863

Merged
merged 5 commits into from Sep 13, 2019

Conversation

@jeanp413
Copy link
Contributor

commented Aug 27, 2019

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

Copy link
Member

left a comment

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

@jeanp413

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

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

@jeanp413

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Good point, can we do what SwitchSideBarViewAction does here?

@jeanp413

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

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);
	}
}
@Tyriar
Tyriar approved these changes Sep 13, 2019
Copy link
Member

left a comment

@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
1 of 2 checks passed
1 of 2 checks passed
VS Code in progress
Details
license/cla All CLA requirements met.
Details
@jeanp413 jeanp413 deleted the jeanp413:terminal-here-command branch Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.