Skip to content

copy from terminal doesn't work #31902#37290

Merged
Tyriar merged 5 commits intomicrosoft:masterfrom
warpdesign:master
Dec 27, 2017
Merged

copy from terminal doesn't work #31902#37290
Tyriar merged 5 commits intomicrosoft:masterfrom
warpdesign:master

Conversation

@warpdesign
Copy link
Contributor

@warpdesign warpdesign commented Oct 31, 2017 •
edited by Tyriar
Loading

Copy from terminal panel didn't work if the mouse was released outside of the _xterm.element: in this case, the mouseup event wasn't triggered.

I fixed the problem by adding a boolean to track down when the mousedown event is triggered on _xterm.element. In this case, the selection is refreshed when the mouseup event bubbles up to the document.

Fixes #31902

@octref octref requested a review from Tyriar October 31, 2017 22:31
Tyriar
Tyriar requested changes Nov 2, 2017
View reviewed changes
src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts Outdated

// We need to listen to the mouseup event up to the document since the user may release the mouse button anywhere
// outside of _xterm.element.
this._instanceDisposables.push(dom.addDisposableListener(document, 'mouseup', (event: KeyboardEvent) => {
Copy link
Member

@Tyriar Tyriar Nov 2, 2017

Choose a reason for hiding this comment

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

I'd prefer if this listener was only active after a mousedown and disposed of itself on mouseup, otherwise this will run on every mouse click. You can probably do this all within the mousedown listener without the need to introduce more properties to TerminalInstance.

Also this listener will also make it very easy to implement #36271 👍

Copy link
Contributor Author

@warpdesign warpdesign Nov 3, 2017

Choose a reason for hiding this comment

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

Oh you're right: that's much better.

@warpdesign
Copy link
Contributor Author

warpdesign commented Dec 1, 2017

@Tyriar I think I made the necessary changes: could you review it again?

This bug is driving me crazy :)

@Tyriar Tyriar added this to the December 2017/January 2018 milestone Dec 23, 2017
@Tyriar
Copy link
Member

Tyriar commented Dec 23, 2017

And I'm back! I can't actually seem to reproduce, on mac at least. Am I missing something?

  1. Open terminal
  2. ll (generated a bunch of output)
  3. Scroll up a little
  4. Select some text, trigger mouseup when it's to the left of the terminal panel
  5. ctrl/cmd+c
  6. ctrl/cmd+v

@warpdesign
Copy link
Contributor Author

warpdesign commented Dec 23, 2017 •
edited
Loading

I can reproduce it with Windows 10: maybe this is Windows only ?

  1. downloaded insider build
  2. activated the option to copy on selection
  3. select text and release the mouse outside of terminal div
  4. paste: ctrl+v

Nothing is pasted. If I release the mouse inside the terminal div it works as expected.

Edit: that's the version that I installed:

Version 1.20.0-insider
Commit f9115349ef09cfef2bed669680d10ac58b490dce
Date 2017-12-21T16:36:58.030Z
Shell 1.7.9
Renderer 58.0.3029.110
Node 7.9.0
Architecture x64

@Tyriar
Copy link
Member

Tyriar commented Dec 25, 2017

@warpdesign yeah you're right, the problem is actually with the Windows only keybinding which is why I couldn't see it

This was referenced Dec 25, 2017
Closed
Closed
@Tyriar
Copy link
Member

Tyriar commented Dec 25, 2017

Made a few tweaks, will merge after CI is happy

@Tyriar Tyriar merged commit fe28c5c into microsoft:master Dec 27, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Tyriar Tyriar Tyriar approved these changes

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

January 2018

Development

Successfully merging this pull request may close these issues.

Copy from terminal doesn't work [Windows/Insiders]

2 participants

@warpdesign @Tyriar