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 'Focus to terminal X' action #20862

Merged
merged 11 commits into from
Feb 21, 2017
Merged

Conversation

hun1ahpu
Copy link
Contributor

@hun1ahpu hun1ahpu commented Feb 18, 2017

Fixes #20133

@mention-bot
Copy link

@hun1ahpu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tyriar and @bpasero to be potential reviewers.

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.

Looks great, just a minor comment 👍

actionRegistry.registerWorkbenchAction(new SyncActionDescriptor(FocusNextTerminalAction, FocusNextTerminalAction.ID, FocusNextTerminalAction.LABEL), 'Terminal: Focus Next Terminal', category);
actionRegistry.registerWorkbenchAction(new SyncActionDescriptor(FocusPreviousTerminalAction, FocusPreviousTerminalAction.ID, FocusPreviousTerminalAction.LABEL), 'Terminal: Focus Previous Terminal', category);
for (let i = 0; i < 10; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be 1 to 9, not 0 to 9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep thanks

@Tyriar Tyriar added this to the March 2017 milestone Feb 18, 2017
@@ -147,7 +147,7 @@ configurationRegistry.registerConfiguration({
CreateNewTerminalAction.ID,
CopyTerminalSelectionAction.ID,
KillTerminalAction.ID,
FocusTerminalAction.ID,
FocusActiveTerminalAction.ID,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if all the new commands should be added here as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes they should, if not the terminal may eat the keystrokes before they reach the vscode keybinding system.

@Tyriar
Copy link
Member

Tyriar commented Feb 19, 2017

@hun1ahpu tests are failing on Travis:

out-build/vs/workbench/parts/terminal/electron-browser/terminalActions.nls.js:13
	`Focus Terminal ${n}`,
	                  ^
ReferenceError: n is not defined
    at Object.<anonymous> (out-build/vs/workbench/parts/terminal/electron-browser/terminalActions.nls.js:13:20)
    at NodeScriptLoader._loadAndEvalScript (evalmachine.<anonymous>:696:15)
    at evalmachine.<anonymous>:643:31
    at tryToString (fs.js:455:3)
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:442:12)
The command "gulp optimize-vscode --silent --max_old_space_size=4096" exited with 1.

https://travis-ci.org/Microsoft/vscode/jobs/203133143#L3225

Maybe make it a regular static function instead of an variable storing an inline function? Also changing it to getLabel/getId is probably a good idea.

}

public static getLabel(n: number): string {
return nls.localize(`workbench.action.terminal.focusByNumber${n}`, `Focus Terminal ${n}`, n);
Copy link
Member

Choose a reason for hiding this comment

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

Almost there 😃 I think this should be:

return nls.localize('workbench.action.terminal.focusByNumber', 'Focus Terminal {0}', n);

That way our localization system will understand it.

@Tyriar Tyriar merged commit 09a9571 into microsoft:master Feb 21, 2017
@Tyriar
Copy link
Member

Tyriar commented Feb 21, 2017

Thanks @hun1ahpu!

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

Change focus to terminal panes via number keyboard shortcut
4 participants