Add Terminal preferences and allow ctrl+c and ctrl+v on terminal if it has a selection #3469

uniibu · 2018-11-13T12:09:36Z

Background:
Since theia let most key bindings be handled by xtermjs, the usual
copy&paste key shortcuts no longer works.

Theia did this intentionally to have key bindings such as ctrl+c to
propagate into xterm, which then is used to send a kill signal SIGINT.

About:
This PR adds Terminal to preferences with enableCopy and enablePaste as initial options.

enableCopy [Boolean] - Enables ctrl-c or cmd-c(for OSX) to copy selected texts from the terminal.
If there is no text selected, the command will propagate normally to xterm.
enablePaste [Boolean] - Enabled ctrl-v or cmd-v(for OSX) to paste from clipboard,

This PR uses xterm's attachCustomKeyEventHandler to listen for key strokes and the selection is returned by xterm's hasSelection function.

Signed-off-by: Uni Sayo [email protected]

packages/terminal/src/browser/terminal-widget-impl.ts

simark · 2018-11-13T20:20:59Z

I think it's a useful heuristic for the ctrl-c, since you are still able to send a ctrl-c to your program. But it blocks ctrl-v all the time, how would a user send a ctrl-v to the program?

kittaakos · 2018-11-14T08:08:11Z

✅ ECA

There is a valid ECA on file for [email protected].

FYI: ^C (SIGINT ) does not work on OS X when I have anything selected in the terminal. const ctrlCmd = event.ctrlKey || event.metaKey; seems to be incorrect on OS X.

uniibu · 2018-11-14T14:08:07Z

@AndrienkoAleksandr Thanks for all the input. I've updated this PR with latest commit fd0a08d which uses KeyCode from @theia/core/lib/browser to detect keybindings.

@kittaakos This now also detects user's system if it is an OSX, and only allows cmd+c and cmd+v, while ctrl+c and ctrl+v propagates normally to xterm. Further information on the commit message. Cheers!

kittaakos · 2018-11-14T14:09:00Z

@uniibu, please make sure you have squashed your commits before handing it over for the review. Thank you!

simark · 2018-11-14T14:14:36Z

I think it's a useful heuristic for the ctrl-c, since you are still able to send a ctrl-c to your program. But it blocks ctrl-v all the time, how would a user send a ctrl-v to the program?

This has not been addressed. With this patch, it's not possible to send ctrl-v to the program anymore...

AndrienkoAleksandr · 2018-11-14T14:56:41Z

I think it's a useful heuristic for the ctrl-c, since you are still able to send a ctrl-c to your program. But it blocks ctrl-v all the time, how would a user send a ctrl-v to the program?

This has not been addressed. With this patch, it's not possible to send ctrl-v to the program anymore...

@simark @uniibu Maybe we should create a command to activate/deactivate custom keyDownHandler?

uniibu · 2018-11-14T15:20:20Z

This has not been addressed. With this patch, it's not possible to send ctrl-v to the program anymore...

@simark The patch still allows ctrl+v but only for non-osx systems. And for osx systems it uses cmd+v

Demo on Windwos 10/chrome:

@AndrienkoAleksandr I initially planned to add a preference for terminal where a user can enable/disable the keybindings such as ctrl+c or ctrl+v.. So that could be an option.

@kittaakos commits squashed.

simark · 2018-11-14T15:25:27Z

@simark The patch still allows ctrl+v but only for non-osx systems. And for osx systems it uses cmd+v

I mean, if you have bound something to ctrl-v in your program which runs in the terminal, you can't use that anymore since it will paste text instead.

packages/terminal/src/browser/terminal-preferences.ts

packages/terminal/src/browser/terminal-widget-impl.ts

uniibu · 2018-11-14T23:20:25Z

As per @simark's concern, I've added terminal to the preferences with enableCopy and enablePaste as initial options and both defaults to false.

This further opens up the terminal for further front-end user configurations, such as cursorStyle, cursorBlink etc.

uniibu · 2018-11-15T00:00:39Z

Demo on Windows 10/Chrome:

Should be ready for review. Cheers!

packages/terminal/src/browser/terminal-preferences.ts

simark · 2018-11-15T13:06:51Z

packages/terminal/src/browser/terminal-preferences.ts

+        'terminal.enableCopy': {
+            type: 'boolean',
+            description: 'Enable ctrl-c (cmd-c on macOS) to copy selected text',
+            default: false


Note that I wouldn't be against setting these to true by default, since it's probably more common for people to use copy/paste than to use ctrl-v.

+1 to set up true.

If this gets merged, i'd be happy to have the default to true as well. +1

Could you update it before merge?

Since theia let most key bindings be handled by xterm, the usual copy&paste key shortcuts no longer works. Theia did this intentionally to have key bindings such as ctrl+c to propagate into xterm, which then is used to send a kill signal SIGINT. This PR, allows the user to copy texts on the terminal via ctrl+c or cmd+c(mac), but only if there is a selected/highlighted text on the terminal. This also allows the user to use ctrl+v or cmd+v to paste from clipboard Signed-off-by: Uni Sayo <[email protected]> add missing semicolon and remove trailing whitespace Signed-off-by: Uni Sayo <[email protected]> use KeyCode from core to detect keybindings, ignore ctrl+c/v if osx As suggested, this commit simplifies detection of the keybindings by using KeyCode class from theia/core. It also detects if the user system is OSX. For non-osx systems, ctrl+c with selection and ctrl+v will work. And for osx systems, cmd+c with selection and cmd+v will work, while ctrl+c and ctrl+v will propagate to xterm normally. Signed-off-by: Uni Sayo <[email protected]> Add Terminal Preferences with enableCopy and enablePaste Adding terminal preferences gives the user an option to enable ctrl/cmd+c and ctrl/cmd+v keyboard commands. This also opens up the terminal for other user configurations. Signed-off-by: Uni Sayo <[email protected]> remove preferencechange listener Signed-off-by: Uni Sayo <[email protected]> reword descriptions Signed-off-by: Uni Sayo <[email protected]> remove clearSelection on copy Signed-off-by: Uni Sayo <[email protected]> Change copyright to reflect my company Signed-off-by: Uni Sayo <[email protected]> set default value to true Signed-off-by: Uni Sayo <[email protected]>

uniibu · 2018-11-16T08:37:51Z

Updated and set defaults to true
Commits squashed

uniibu force-pushed the copy-paste branch from a85ac37 to 79c1598 Compare November 13, 2018 12:17

benoitf requested a review from AndrienkoAleksandr November 13, 2018 14:02

svenefftinge requested a review from simark November 13, 2018 16:48

AndrienkoAleksandr reviewed Nov 13, 2018

View reviewed changes

packages/terminal/src/browser/terminal-widget-impl.ts Outdated Show resolved Hide resolved

AndrienkoAleksandr reviewed Nov 13, 2018

View reviewed changes

packages/terminal/src/browser/terminal-widget-impl.ts Outdated Show resolved Hide resolved

uniibu force-pushed the copy-paste branch from fd0a08d to 6c95e39 Compare November 14, 2018 15:24

uniibu force-pushed the copy-paste branch from b10b745 to b9a5ec1 Compare November 14, 2018 23:04

uniibu changed the title ~~Allow ctrl+c and ctrl+v on terminal if it has a selection~~ Add Terminal preferences and allow ctrl+c and ctrl+v on terminal if it has a selection Nov 14, 2018

simark reviewed Nov 14, 2018

View reviewed changes

packages/terminal/src/browser/terminal-preferences.ts Outdated Show resolved Hide resolved

packages/terminal/src/browser/terminal-widget-impl.ts Outdated Show resolved Hide resolved

uniibu force-pushed the copy-paste branch from 26748ed to 6bd76a6 Compare November 14, 2018 23:20

uniibu force-pushed the copy-paste branch 2 times, most recently from 2982ce7 to 84cdd7e Compare November 14, 2018 23:49

kittaakos reviewed Nov 15, 2018

View reviewed changes

packages/terminal/src/browser/terminal-preferences.ts Outdated Show resolved Hide resolved

uniibu force-pushed the copy-paste branch from 48785cf to 383b490 Compare November 15, 2018 07:53

simark reviewed Nov 15, 2018

View reviewed changes

AndrienkoAleksandr approved these changes Nov 16, 2018

View reviewed changes

uniibu force-pushed the copy-paste branch from fecbddf to 147553a Compare November 16, 2018 08:36

svenefftinge merged commit b01110a into eclipse-theia:master Dec 10, 2018

kittaakos mentioned this pull request Dec 21, 2018

[terminal] Copy/paste on keyboard doesn't work in terminal #2216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Terminal preferences and allow ctrl+c and ctrl+v on terminal if it has a selection #3469

Add Terminal preferences and allow ctrl+c and ctrl+v on terminal if it has a selection #3469

uniibu commented Nov 13, 2018 •

edited

Loading

simark commented Nov 13, 2018

kittaakos commented Nov 14, 2018

uniibu commented Nov 14, 2018

kittaakos commented Nov 14, 2018

simark commented Nov 14, 2018

AndrienkoAleksandr commented Nov 14, 2018 •

edited

Loading

uniibu commented Nov 14, 2018 •

edited

Loading

simark commented Nov 14, 2018

uniibu commented Nov 14, 2018 •

edited

Loading

uniibu commented Nov 15, 2018

simark Nov 15, 2018

AndrienkoAleksandr Nov 15, 2018

uniibu Nov 15, 2018

svenefftinge Nov 16, 2018

uniibu commented Nov 16, 2018

Add Terminal preferences and allow ctrl+c and ctrl+v on terminal if it has a selection #3469

Add Terminal preferences and allow ctrl+c and ctrl+v on terminal if it has a selection #3469

Conversation

uniibu commented Nov 13, 2018 • edited Loading

simark commented Nov 13, 2018

kittaakos commented Nov 14, 2018

uniibu commented Nov 14, 2018

kittaakos commented Nov 14, 2018

simark commented Nov 14, 2018

AndrienkoAleksandr commented Nov 14, 2018 • edited Loading

uniibu commented Nov 14, 2018 • edited Loading

simark commented Nov 14, 2018

uniibu commented Nov 14, 2018 • edited Loading

uniibu commented Nov 15, 2018

simark Nov 15, 2018

Choose a reason for hiding this comment

AndrienkoAleksandr Nov 15, 2018

Choose a reason for hiding this comment

uniibu Nov 15, 2018

Choose a reason for hiding this comment

svenefftinge Nov 16, 2018

Choose a reason for hiding this comment

uniibu commented Nov 16, 2018

uniibu commented Nov 13, 2018 •

edited

Loading

AndrienkoAleksandr commented Nov 14, 2018 •

edited

Loading

uniibu commented Nov 14, 2018 •

edited

Loading

uniibu commented Nov 14, 2018 •

edited

Loading