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

Duplicate copyOnSelect actions part 2 #4740

Closed
maguiresf opened this issue Feb 28, 2020 · 5 comments · Fixed by #4819
Closed

Duplicate copyOnSelect actions part 2 #4740

maguiresf opened this issue Feb 28, 2020 · 5 comments · Fixed by #4819
Assignees
@leonMSFT
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-DataLoss A bug that causes the user's data to be lost, unintentionally
Milestone
Terminal v1.0

Comments

@maguiresf
Copy link

maguiresf commented Feb 28, 2020 •
edited by carlos-zamora
Loading

Environment

Platform ServicePack Version VersionString

Win32NT 10.0.18363.0 Microsoft Windows NT 10.0.18363.0

Windows Terminal Version 0.9.433.0

Steps to reproduce

Please note, this isn't a duplicate of issue #4255 which I reported previously, it's a variation of the same issue which is still present in the latest version where as 4255 is now fixed.

  1. Open WSL Ubuntu terminal
  2. Split pane in two: pane_1 and pane_2
  3. Select a block on text with copyOnSelect enabled (for example "TEST1") on pane_1
  4. Ensure block of text remains highlighted
  5. Make pane_2 the active pane
  6. Switch to notepad instance
  7. Paste selected text into Notepad
  8. Make some change to text, (for example change string to "TEST999")
  9. Copy string inside Notepad
  10. Highlight Windows Terminal, clicking into the last active pane (pane_2)
  11. Paste string into Windows Terminal
  12. Now highlight the other pane (pane_1) that still has the original string highlighted
  13. Paste again

Expected behavior

In both steps 11 and 13, the last copied string, "TEST999", is pasted

Actual behavior

In step 11, TEST999 is pasted
In step 13, TEST1 is pasted
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 28, 2020
@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Feb 28, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 28, 2020
@zadjii-msft
Copy link
Member

Thanks for the great repro steps!

@carlos-zamora carlos-zamora added Severity-DataLoss A bug that causes the user's data to be lost, unintentionally Priority-0 Bugs that we consider release-blocking/recall-class (P0) labels Mar 2, 2020
@carlos-zamora carlos-zamora added this to the Terminal v1.0 milestone Mar 2, 2020
@leonMSFT
Copy link
Contributor

leonMSFT commented Mar 4, 2020 •
edited
Loading

@maguiresf Just to see if I'm understanding the steps correctly, in Step 12, you're simply refocusing pane_1 by single clicking on it right? In addition, throughout all the steps, TEST1 is still highlighted on pane_1?

If what I described is correct, the behavior we've coded for is to only copy to clipboard the first time a selection is made, and so since "TEST1" was selected for the first time on Step 3, that's the only time "TEST1" will be copied to the clipboard. Refocusing on a pane with a selection active should not trigger a copy on select on that selection. This is the behavior that was expected in #4255. So, in step 12, refocusing pane_1 will not recopy "TEST1" to the clipboard, which is why "TEST999" (which was copied from notepad) is pasted.

@DHowett-MSFT
Copy link
Contributor

Nah, this does repro for me. The right-click paste in pane 2 causes a copy of pane 2's selection.

Should we just suppress right-click copy in CoS cases?

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 4, 2020
@leonMSFT
Copy link
Contributor

leonMSFT commented Mar 4, 2020

Argh you're right, it's the right click that repros, I think it's reasonable to suppress right-click copy in this case then.

@leonMSFT leonMSFT mentioned this issue Mar 6, 2020
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Mar 6, 2020
@maguiresf
Copy link
Author

Ah, apologies, I should have mentioned I was using right-click to paste.

I'd just found another variation of this involving switching between tabs and panes but it again only appears to be triggered when using right-click to paste so it looks like it must be the same issue.

@ghost ghost closed this as completed in #4819 Mar 10, 2020
@ghost ghost removed the In-PR This issue has a related PR label Mar 10, 2020
ghost pushed a commit that referenced this issue Mar 10, 2020
@leonMSFT
Ignore right-click copy when copy on select is enabled (#4819)
dd8813e 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Right clicking on a focused tab while Copy On Select is active currently copies any active selection. This is because `PointerReleasedHandler` doesn't check for the mouse button that was released. 
During a mouse button release, only the left mouse button release should be doing anything.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #4740
* [x] CLA signed.
* [x] Tests added/passed

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
These are the scenarios I've tested. They're a combination of in focus/out of focus, Copy On Select on/off, left/right click pressed and their move and release variants.

From Out of Focus:
- Left Click = Focus
- Left Click Move = Focus + Selection
- Left Click Release
  - CoS on = Copy
  - CoS off = Nothing
- Shift Left Click = Focus
- Right Click 
  - Focus 
  - CoS on = Paste
  - CoS off = Copy if Active Selection, Paste if not.
- Right Click Move = Nothing
- Right Click Release = Nothing

From In Focus
- Left Click = Selection if CoS off
- Left Click Move = Selection
- Left Click Release
  - CoS on = Copy
  - CoS off = Nothing
- Shift Left Click = Set Selection Anchor
- Right Click 
  - CoS on = Paste
  - CoS off = Copy if Active Selection, Paste if not.
- Right Click Move = Nothing
- Right Click Release = Nothing
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Mar 10, 2020
abhijeetviswam pushed a commit to abhijeetviswam/terminal that referenced this issue Mar 12, 2020
@leonMSFT @abhijeetviswam
Ignore right-click copy when copy on select is enabled (microsoft#4819)
1c52585 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Right clicking on a focused tab while Copy On Select is active currently copies any active selection. This is because `PointerReleasedHandler` doesn't check for the mouse button that was released. 
During a mouse button release, only the left mouse button release should be doing anything.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes microsoft#4740
* [x] CLA signed.
* [x] Tests added/passed

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
These are the scenarios I've tested. They're a combination of in focus/out of focus, Copy On Select on/off, left/right click pressed and their move and release variants.

From Out of Focus:
- Left Click = Focus
- Left Click Move = Focus + Selection
- Left Click Release
  - CoS on = Copy
  - CoS off = Nothing
- Shift Left Click = Focus
- Right Click 
  - Focus 
  - CoS on = Paste
  - CoS off = Copy if Active Selection, Paste if not.
- Right Click Move = Nothing
- Right Click Release = Nothing

From In Focus
- Left Click = Selection if CoS off
- Left Click Move = Selection
- Left Click Release
  - CoS on = Copy
  - CoS off = Nothing
- Shift Left Click = Set Selection Anchor
- Right Click 
  - CoS on = Paste
  - CoS off = Copy if Active Selection, Paste if not.
- Right Click Move = Nothing
- Right Click Release = Nothing
DHowett-MSFT pushed a commit that referenced this issue Mar 24, 2020
@DHowett
Rework and simplify selection in TermControl
53080d3 
This commit rewrites selection handling at the TermControl layer.
Previously, we were keeping track of a number of redundant variables
that were easy to get out of sync.

The new selection model is as follows:

* A single left click will always begin a _pending_ selection operation
* A single left click will always clear a selection (#4477)
* A double left click will always begin a word selection
* A triple left click will always begin a line selection
* A selection will only truly start when the cursor moves a quarter of
  the smallest dimension of a cell (usually its width) in any direction
  _This eliminates the selection of a single cell on one click._
  (#4282, #5082)
* We now keep track of whether the selection has been "copied", or
  "updated" since it was last copied. If an endpoint moves, it is
  updated. For copy-on-select, it is only copied if it's updated.
  (#4740)

Because of this, we can stop tracking the position of the focus-raising
click, and whether it was part of click-drag operation. All clicks can
_become_ part of a click-drag operation if the user drags.

We can also eliminate the special handling of single cell selection at
the TerminalCore layer: since TermControl determines when to begin a
selection, TerminalCore no longer needs to know whether copy on select
is enabled _or_ whether the user has started and then backtracked over a
single cell. This is now implicit in TermControl.

Fixes #4082; Fixes #4477
@DHowett-MSFT DHowett-MSFT mentioned this issue Mar 24, 2020
Merged
ghost pushed a commit that referenced this issue Mar 25, 2020
@DHowett
Rework and simplify selection in TermControl (#5096)
499f24a 
This commit rewrites selection handling at the TermControl layer.
Previously, we were keeping track of a number of redundant variables
that were easy to get out of sync.

The new selection model is as follows:

* A single left click will always begin a _pending_ selection operation
* A single left click will always clear a selection (#4477)
* A double left click will always begin a word selection
* A triple left click will always begin a line selection
* A selection will only truly start when the cursor moves a quarter of
  the smallest dimension of a cell (usually its width) in any direction
  _This eliminates the selection of a single cell on one click._
  (#4282, #5082)
* We now keep track of whether the selection has been "copied", or
  "updated" since it was last copied. If an endpoint moves, it is
  updated. For copy-on-select, it is only copied if it's updated.
  (#4740)

Because of this, we can stop tracking the position of the focus-raising
click, and whether it was part of click-drag operation. All clicks can
_become_ part of a click-drag operation if the user drags.

We can also eliminate the special handling of single cell selection at
the TerminalCore layer: since TermControl determines when to begin a
selection, TerminalCore no longer needs to know whether copy on select
is enabled _or_ whether the user has started and then backtracked over a
single cell. This is now implicit in TermControl.

Fixes #5082; Fixes #4477
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@leonMSFT leonMSFT

Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-DataLoss A bug that causes the user's data to be lost, unintentionally
Projects
None yet
Milestone
Terminal v1.0
Development

Successfully merging a pull request may close this issue.

Ignore right-click copy when copy on select is enabled microsoft/terminal
5 participants
@maguiresf @carlos-zamora @DHowett-MSFT @zadjii-msft @leonMSFT