Right click to copy selection and paste on Windows #17496

Merged
merged 5 commits into from Dec 21, 2016

Projects

None yet

3 participants

@hun1ahpu
Contributor

Proposal fix for #14202.
Added config setting 'rightClickAction' for all the platforms set to 'copyPaste' by default only on windows. Let me know if it makes sense at all to have this setting on other platforms or not.
Tested on Windows 10 only.

@hun1ahpu hun1ahpu Right click to copy selection and paste on Windows
dea01a9
@msftclas

Hi @hun1ahpu, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@@ -56,8 +56,7 @@ export class KillTerminalAction extends Action {
}
/**
- * Copies the terminal selection. Note that since the command palette takes focus from the terminal,
@hun1ahpu
hun1ahpu Dec 18, 2016 Contributor

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

- anchor = { x: standardEvent.posx, y: standardEvent.posy };
- }
+ let rightClickAction: string = this._terminalService.configHelper.getRightClickAction();
+ if (rightClickAction === 'copyPaste') {
@hun1ahpu
hun1ahpu Dec 18, 2016 edited Contributor

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

@Tyriar Tyriar was assigned by kieferrm Dec 19, 2016
@@ -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.
@Tyriar
Tyriar Dec 19, 2016 Member

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.
 */
@@ -72,6 +84,12 @@ configurationRegistry.registerConfiguration({
},
'default': []
},
+ 'terminal.integrated.rightClickAction.windows': {
@Tyriar
Tyriar Dec 19, 2016 Member

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

+ terminal.paste();
+ }
+ } else if (rightClickAction === 'contextMenu') {
+ let anchor: HTMLElement | { x: number, y: number } = this._parentDomElement;
@Tyriar
Tyriar Dec 19, 2016 Member

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

+ }
+ } else if (rightClickAction === 'contextMenu') {
+ let anchor: HTMLElement | { x: number, y: number } = this._parentDomElement;
+ if (event instanceof MouseEvent) {
@Tyriar
Tyriar Dec 19, 2016 Member

Isn't event guaranteed to be a MouseEvent?

@hun1ahpu
hun1ahpu Dec 20, 2016 Contributor

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

@Tyriar
Member
Tyriar commented Dec 19, 2016

@hun1ahpu thanks for looking into this 😃

@hun1ahpu hun1ahpu Addressed comments
fc5a3b5
@@ -165,6 +165,9 @@ suite('Workbench - TerminalConfigHelper', () => {
},
shellArgs: {
linux: []
+ },
+ rightClickAction: {
@Tyriar
Tyriar Dec 21, 2016 Member

These can be removed now that the setting changed.

@@ -62,6 +65,7 @@ export interface ITerminalConfigHelper {
getFont(): ITerminalFont;
getFontLigaturesEnabled(): boolean;
getCursorBlink(): boolean;
+ isRightClickCopyPaste(): boolean;
@Tyriar
Tyriar Dec 21, 2016 Member

Change this to get... like getCursorBlink

@@ -149,25 +149,30 @@ 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) {
@Tyriar
Tyriar Dec 21, 2016 Member

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 👍

@hun1ahpu
hun1ahpu Dec 21, 2016 Contributor

Yes it still works with the change

@@ -72,6 +72,11 @@ configurationRegistry.registerConfiguration({
},
'default': []
},
+ 'terminal.integrated.rightClickCopyPaste': {
+ 'description': nls.localize('terminal.integrated.rightClickCopyPaste', "Controls whether copy/paste happens on mouse right click in integrated terminal."),
@Tyriar
Tyriar Dec 21, 2016 Member

I think the description needs to be more detailed, how about this?

When set, this will prevent the context menu from appearing on right clicking in the terminal, instead it will copy when there is a selection and paste when there is no selection.

hun1ahpu and others added some commits Dec 21, 2016
@hun1ahpu hun1ahpu Addressed comments 2
f2c6556
@Tyriar Tyriar Merge remote-tracking branch 'upstream/master' into i14202 b8c9d2c
@Tyriar Tyriar Improve wording
ca0dd70
@Tyriar
Member
Tyriar commented Dec 21, 2016

Works great! The build is failing due to an unrelated issue so this is good to go. Thanks a lot for taking the effort to implement this 🎆

@Tyriar
Tyriar approved these changes Dec 21, 2016 View changes
@Tyriar Tyriar merged commit 651c27d into Microsoft:master Dec 21, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@Tyriar Tyriar added this to the January 2017 milestone Dec 21, 2016
@hun1ahpu hun1ahpu deleted the hun1ahpu:i14202 branch Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment