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

Right click to copy selection and paste on Windows #17496

Merged
merged 5 commits into from
Dec 21, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/vs/workbench/parts/terminal/common/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ export interface ITerminalConfiguration {
osx: string[],
windows: string[]
},
rightClickAction: {
linux: string,
osx: string,
windows: string
},
cursorBlinking: boolean,
fontFamily: string,
fontLigatures: boolean,
Expand All @@ -62,6 +67,7 @@ export interface ITerminalConfigHelper {
getFont(): ITerminalFont;
getFontLigaturesEnabled(): boolean;
getCursorBlink(): boolean;
getRightClickAction(): string;
getCommandsToSkipShell(): string[];
getScrollback(): number;
getCwd(): string;
Expand Down Expand Up @@ -146,11 +152,21 @@ export interface ITerminalInstance {
*/
dispose(): void;

/**
* Check if anything is selected in terminal.
*/
hasSelection(): boolean;

/**
* Copies the terminal selection to the clipboard.
*/
copySelection(): void;

/**
* Clear current selection.
*/
clearSelection(): void;

/**
* Focuses the terminal instance.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ configurationRegistry.registerConfiguration({
},
'default': []
},
'terminal.integrated.rightClickAction.linux': {
'description': nls.localize('terminal.integrated.shell.rightClickAction.linux', "Defines action on right mouse click on Linux terminal."),
'type': 'string',
enum: ['copyPaste', 'contextMenu'],
'default': 'contextMenu'
},
'terminal.integrated.shell.osx': {
'description': nls.localize('terminal.integrated.shell.osx', "The path of the shell that the terminal uses on OS X."),
'type': 'string',
Expand All @@ -59,6 +65,12 @@ configurationRegistry.registerConfiguration({
},
'default': []
},
'terminal.integrated.rightClickAction.osx': {
'description': nls.localize('terminal.integrated.shell.rightClickAction.osx', "Defines action on right mouse click on the OS X terminal."),
'type': 'string',
enum: ['copyPaste', 'contextMenu'],
'default': 'contextMenu'
},
'terminal.integrated.shell.windows': {
'description': nls.localize('terminal.integrated.shell.windows', "The path of the shell that the terminal uses on Windows. When using shells shipped with Windows (cmd, PowerShell or Bash on Ubuntu), prefer C:\\Windows\\sysnative over C:\\Windows\\System32 to use the 64-bit versions."),
'type': 'string',
Expand All @@ -72,6 +84,12 @@ configurationRegistry.registerConfiguration({
},
'default': []
},
'terminal.integrated.rightClickAction.windows': {
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking instead of having 3 settings for each platform and using strings, instead having a single setting for all platforms that just turns on rightClickCopy/Paste. Maybe terminal.integrated.rightClickCopyPaste? Then the default could be true when platform.isWindows (put the default in terminal.ts and import it into this file).

'description': nls.localize('terminal.integrated.shell.rightClickAction.windows', "Defines action on right mouse click on the Windows terminal."),
'type': 'string',
enum: ['copyPaste', 'contextMenu'],
'default': 'copyPaste'
},
'terminal.integrated.fontFamily': {
'description': nls.localize('terminal.integrated.fontFamily', "Controls the font family of the terminal, this defaults to editor.fontFamily's value."),
'type': 'string'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ export class KillTerminalAction extends Action {
}

/**
* Copies the terminal selection. Note that since the command palette takes focus from the terminal,
* this can only be triggered via a keybinding.
* Copies the terminal selection.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment seems obsolete to me. Let me know if i'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

The comment is still relevant as you can't trigger it through the command palette. This is what it should probably say though:

/**
 * Copies the terminal selection. Note that since the command palette takes focus from the terminal,
 * this cannot be triggered through the command palette.
 */

*/
export class CopyTerminalSelectionAction extends Action {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,22 @@ export class TerminalConfigHelper implements ITerminalConfigHelper {
return terminalConfig.cursorBlinking;
}

public getRightClickAction(): string {
let config = this._configurationService.getConfiguration<ITerminalConfiguration>();
const integrated = config && config.terminal && config.terminal.integrated;
let rightClickAction: string = '';

if (this._platform === Platform.Windows) {
rightClickAction = integrated.rightClickAction.windows;
} else if (this._platform === Platform.Mac) {
rightClickAction = integrated.rightClickAction.osx;
} else if (this._platform === Platform.Linux) {
rightClickAction = integrated.rightClickAction.linux;
}

return rightClickAction;
}

public getShell(): IShell {
let config = this._configurationService.getConfiguration<ITerminalConfiguration>();
let shell: IShell = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,10 @@ export class TerminalInstance implements ITerminalInstance {
this.updateConfig();
}

public hasSelection(): boolean {
return !document.getSelection().isCollapsed;
}

public copySelection(): void {
if (document.activeElement.classList.contains('xterm')) {
document.execCommand('copy');
Expand All @@ -203,6 +207,10 @@ export class TerminalInstance implements ITerminalInstance {
}
}

public clearSelection(): void {
document.getSelection().empty();
}

public dispose(): void {
this._isExiting = true;

Expand Down
44 changes: 27 additions & 17 deletions src/vs/workbench/parts/terminal/electron-browser/terminalPanel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,25 +149,35 @@ export class TerminalPanel extends Panel {
// occurs on the selection itself.
this._terminalService.getActiveInstance().focus();
} else if (event.which === 3) {
// Trigger the context menu on right click
let anchor: HTMLElement | { x: number, y: number } = this._parentDomElement;
if (event instanceof MouseEvent) {
const standardEvent = new StandardMouseEvent(event);
anchor = { x: standardEvent.posx, y: standardEvent.posy };
}
let rightClickAction: string = this._terminalService.configHelper.getRightClickAction();
if (rightClickAction === 'copyPaste') {
Copy link
Contributor Author

@hun1ahpu hun1ahpu Dec 18, 2016

Choose a reason for hiding this comment

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

I was thinking of adding enum for rightClickAction but not sure it's worth it.

let terminal = this._terminalService.getActiveInstance();
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that this is actually my code haha, as long as the context menu appears in the correct spot after this change (when rightClickCopyPaste is false) then this was fine to remove 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it still works with the change

if (terminal.hasSelection()) {
terminal.copySelection();
terminal.clearSelection();
} else {
terminal.paste();
}
} else if (rightClickAction === 'contextMenu') {
let anchor: HTMLElement | { x: number, y: number } = this._parentDomElement;
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer if these were 2 different variables (if you need to store this._parentDomElement at all)

if (event instanceof MouseEvent) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't event guaranteed to be a MouseEvent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like. Didn't dare to touch this part of the code. Will remove unnecessary check. Thanks.

const standardEvent = new StandardMouseEvent(event);
anchor = { x: standardEvent.posx, y: standardEvent.posy };
}

this._contextMenuService.showContextMenu({
getAnchor: () => anchor,
getActions: () => TPromise.as(this._getContextMenuActions()),
getActionsContext: () => this._parentDomElement,
getKeyBinding: (action) => {
const opts = this._keybindingService.lookupKeybindings(action.id);
if (opts.length > 0) {
return opts[0]; // only take the first one
this._contextMenuService.showContextMenu({
getAnchor: () => anchor,
getActions: () => TPromise.as(this._getContextMenuActions()),
getActionsContext: () => this._parentDomElement,
getKeyBinding: (action) => {
const opts = this._keybindingService.lookupKeybindings(action.id);
if (opts.length > 0) {
return opts[0]; // only take the first one
}
return null;
}
return null;
}
});
});
}
}
}));
this._register(DOM.addDisposableListener(this._parentDomElement, 'click', (event) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ suite('Workbench - TerminalConfigHelper', () => {
},
shellArgs: {
linux: []
},
rightClickAction: {
Copy link
Member

Choose a reason for hiding this comment

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

These can be removed now that the setting changed.

linux: 'contextMenu'
}
}
}
Expand All @@ -181,6 +184,9 @@ suite('Workbench - TerminalConfigHelper', () => {
},
shellArgs: {
osx: []
},
rightClickAction: {
osx: 'contextMenu'
}
}
}
Expand All @@ -197,6 +203,9 @@ suite('Workbench - TerminalConfigHelper', () => {
},
shellArgs: {
windows: []
},
rightClickAction: {
windows: 'copyPaste'
}
}
}
Expand Down