Skip to content

Add 'Focus to terminal X' action#20862

Merged
Tyriar merged 11 commits intomicrosoft:masterfrom
hun1ahpu:focusTerminal
Feb 21, 2017
Merged

Add 'Focus to terminal X' action#20862
Tyriar merged 11 commits intomicrosoft:masterfrom
hun1ahpu:focusTerminal

Conversation

@hun1ahpu
Copy link
Copy Markdown
Contributor

@hun1ahpu hun1ahpu commented Feb 18, 2017 •
edited by Tyriar
Loading

Fixes #20133

@mention-bot
Copy link
Copy Markdown

mention-bot commented Feb 18, 2017

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

Tyriar
Tyriar suggested changes Feb 18, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

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

src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts Outdated
actionRegistry.registerWorkbenchAction(new SyncActionDescriptor(FocusActiveTerminalAction, FocusActiveTerminalAction.ID, FocusActiveTerminalAction.LABEL), 'Terminal: Focus Active Terminal', category);
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
Copy Markdown
Contributor

@Tyriar Tyriar Feb 18, 2017

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
Copy Markdown
Contributor Author

@hun1ahpu hun1ahpu Feb 18, 2017

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
hun1ahpu
hun1ahpu commented Feb 18, 2017
View reviewed changes
src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts
CopyTerminalSelectionAction.ID,
KillTerminalAction.ID,
FocusTerminalAction.ID,
FocusActiveTerminalAction.ID,
Copy link
Copy Markdown
Contributor Author

@hun1ahpu hun1ahpu Feb 18, 2017

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
Copy Markdown
Contributor

@Tyriar Tyriar Feb 18, 2017

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
Copy Markdown
Contributor

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.

Tyriar
Tyriar reviewed Feb 21, 2017
View reviewed changes
src/vs/workbench/parts/terminal/electron-browser/terminalActions.ts Outdated
}

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

@Tyriar Tyriar Feb 21, 2017

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
Copy Markdown
Contributor

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.

Reviewers

1 more reviewer

@Tyriar Tyriar Tyriar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

March 2017

Development

Successfully merging this pull request may close these issues.

Change focus to terminal panes via number keyboard shortcut

4 participants

@hun1ahpu @mention-bot @Tyriar @msftclas