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.

Sign up for GitHub

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

Jump to bottom

Add implementation copy text on selection for terminal #6536

Merged
merged 1 commit into from
Nov 14, 2019
Merged

Add implementation copy text on selection for terminal #6536

merged 1 commit into from
Nov 14, 2019

Conversation

AndrienkoAleksandr
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr commented Nov 12, 2019 •
edited
Loading

What it does

Add implementation copy text on selection for terminal and add option to enable/disable feature to align with VSCode. Works with Google Chrome, Firefox.
With Safary seems an issue: https://bugs.webkit.org/show_bug.cgi?id=156529

Referenced issue:

eclipse-che/che#15106

How to test

  1. Open terminal preferences, find option terminal.integrated.copyOnSelection, set value true to enable feature.
  2. Open new terminal select some text(Don't use Ctrl +C).
  3. Open some text editor, gedit, atom or so on, and paste text by Ctrl + V.

Review checklist

Reminder for reviewers

Signed-off-by: Oleksandr Andriienko oandriie@redhat.com

@akosyakov akosyakov added clipboard issues related to the clipboard terminal issues related to the terminal webviews issues related to webviews and removed webviews issues related to webviews labels Nov 13, 2019
azatsarynnyy
azatsarynnyy previously approved these changes Nov 13, 2019
View reviewed changes
Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

I'd merge this PR with further improvement of ClipboardService in a separate PR.

@akosyakov
Copy link
Member

I'd merge this PR with further improvement of ClipboardService in a separate PR.

ok, please fix at least naming and unnecessary creation of listeners for each terminal, one has to be enough

Also add to #6209 that TerminalCopyOnSelectionHander has to be replaced with ClipboardService and copy on select for terminals has to be tested as a part of this issue.

@azatsarynnyy azatsarynnyy dismissed their stale review November 13, 2019 17:08

Some not related commits have been added to the PR, so dismissing my approval

@azatsarynnyy
Copy link
Member

@AndrienkoAleksandr looks like there are several non-related commits are added to the PR. Could you check it, please?

@azatsarynnyy azatsarynnyy mentioned this pull request Nov 14, 2019
Open
@azatsarynnyy
Copy link
Member

Also add to #6209 that TerminalCopyOnSelectionHander has to be replaced with ClipboardService and copy on select for terminals has to be tested as a part of this issue

@akosyakov done

@AndrienkoAleksandr
Add implementation copy text on selection for terminal and add option…
b69f53a 
… to enable/disable feature.

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr AndrienkoAleksandr force-pushed the implementTerminalCopyOnSelectionOption branch from 281b46a to b69f53a Compare November 14, 2019 08:35
@AndrienkoAleksandr
Copy link
Contributor Author

@azatsarynnyy looks like there are several non-related commits are added to the PR. Could you check it, please?

Sorry, it was wrong merge, I fixed.

akosyakov
akosyakov approved these changes Nov 14, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code-wise looks good, thank you!

Please rely for testing on @azatsarynnyy review

azatsarynnyy
azatsarynnyy approved these changes Nov 14, 2019
View reviewed changes
Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

I've tested it in Chrome and FF. Works well 👍
I'm ok to merge it with further improvement of ClipboardService, see #6209

@AndrienkoAleksandr AndrienkoAleksandr merged commit 7dc3c98 into master Nov 14, 2019
@AndrienkoAleksandr AndrienkoAleksandr deleted the implementTerminalCopyOnSelectionOption branch November 14, 2019 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov approved these changes

@azatsarynnyy azatsarynnyy azatsarynnyy approved these changes

@vince-fugnitto vince-fugnitto Awaiting requested review from vince-fugnitto

@mmorhun mmorhun Awaiting requested review from mmorhun

@fangnx fangnx Awaiting requested review from fangnx

Assignees
No one assigned
Labels
clipboard issues related to the clipboard terminal issues related to the terminal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndrienkoAleksandr @akosyakov @azatsarynnyy