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

copy from terminal doesn't work #31902 #37290

Merged
merged 5 commits into from
Dec 27, 2017
Merged

Conversation

warpdesign
Copy link
Contributor

@warpdesign warpdesign commented Oct 31, 2017

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

// 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

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

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

@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

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

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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