Rework and simplify selection in TermControl #5096

DHowett-MSFT · 2020-03-24T06:15:50Z

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 ([ui] Difficult to get rid of selection area #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.
(Behaviour of click-and-drag text selection depends inconsistently on prior pane and application focus state #4282, Determine proper behavior for single-cell selection; only highlight on click + threshold-drag? #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.
(Duplicate copyOnSelect actions part 2 #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 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

carlos-zamora · 2020-03-24T16:52:46Z

A single left click will always clear a selection ([ui] Difficult to get rid of selection area #4477)

This one is a bit concerning. Really only because ConHost does not do this. I hate to say it, but it makes me wonder if we should make this particular setting a pointer binding in #1553

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.
(Behaviour of click-and-drag text selection depends inconsistently on prior pane and application focus state #4282, Determine proper behavior for single-cell selection; only highlight on click + threshold-drag? #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.
(Duplicate copyOnSelect actions part 2 #4740)

[Paraphrasing:] we can stop tracking the position of the focus-raising
click, and whether it was part of click-drag operation.
We can also eliminate the special handling of single cell selection at
the TerminalCore layer.

These are all things I really like and want. I think the only reason we needed copyOnSelect in TermCore was for the single cell selection. But if we can differentiate between a "tap" pending a selection action vs a "drag" actually selecting, then this model looks a lot cleaner.

Side note: #4082 is probably not the issue you want to link here?

DHowett-MSFT · 2020-03-24T16:55:07Z

Yeah, I meant #5082. Let’s talk about selection clearing: why do we want to keep the selection when you click somewhere else? All other text buffer applications clear it to start another selection. Right now there’s no way to even abort a selection with the mouse (see complaints in 4477)

DHowett-MSFT · 2020-03-24T16:58:07Z

I legit think “because conhost did it” is an okay reason for some things, but Terminal also presents a chance to change the things conhost did that don’t make sense. Right now, conhost always starts and completes a selection on a single click so there isn’t a clear state—all you can do is move to another single cell selection, which we’ve established isn’t useful 😄

If people are using single cell selection to pause output, that’s a failing on our part for us having not provided any other way to pause.

carlos-zamora · 2020-03-24T17:06:22Z

I legit think “because conhost did it” is an okay reason for some things, but Terminal also presents a chance to change the things conhost did that don’t make sense. Right now, conhost always starts and completes a selection on a single click so there isn’t a clear state—all you can do is move to another single cell selection, which we’ve established isn’t useful 😄

If people are using single cell selection to pause output, that’s a failing on our part for us having not provided any other way to pause.

I agree and I think this updated behavior is more correct. Rather, my only concern is that we'll have a number of users coming from ConHost expecting it to work somewhat similarly. That said, we could do what we've done with other features and introduce this new behavior as the default. Then if somebody submits an issue expecting it to work the other way, we then address it by adding it in as a customization option.

I genuinely think what you are proposing is the right behavior. I just don't want people familiar with Windows-isms to suddenly get confused. Even though I think this is one of those bad Windows-isms.

DHowett-MSFT · 2020-03-24T17:26:34Z

Also, this diff is +35 -120 😄 you know how much I love deleting code

carlos-zamora

I know you're still in draft, but I added some comments since I'll have to review this at some point anyways.

src/cascadia/TerminalControl/TermControl.h

carlos-zamora · 2020-03-24T18:26:23Z

src/cascadia/TerminalControl/TermControl.cpp

+                    auto& touchdownPoint{ *_singleClickTouchdownPos };
+                    auto distance{ std::sqrtf(std::powf(cursorPosition.X - touchdownPoint.X, 2) + std::powf(cursorPosition.Y - touchdownPoint.Y, 2)) };
+                    const auto fontSize{ _actualFont.GetSize() };
+                    if (distance >= (std::min(fontSize.X, fontSize.Y) / 4.f))


could/should we cache this? Also, isn't X always smaller than Y?

X isn't always smaller than Y; some fonts are strange. Consider that caching requires cache invalidation, which is a lot to ask for a few simple floating point operations.

carlos-zamora · 2020-03-24T18:27:39Z

src/cascadia/TerminalControl/TermControl.h

-        std::optional<winrt::Windows::Foundation::Point> _focusRaisedClickPos;
-        bool _clickDrag;
+        std::optional<winrt::Windows::Foundation::Point> _singleClickTouchdownPos;
+        bool _selectionUpdatedThisCycle;


Could you add a description here or somewhere for this new bool?

leonMSFT · 2020-03-25T17:15:28Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -980,7 +975,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
            else if (point.Properties().IsRightButtonPressed())
            {
                // CopyOnSelect right click always pastes
-                if (_terminal->IsCopyOnSelectActive() || !_terminal->IsSelectionActive())
+                if (!_selectionUpdatedThisCycle || !_terminal->IsSelectionActive())


I'm having a bit of trouble understanding why the change in this if statement is still equivalent to the comment above of "CopyOnSelect right click always pastes", can you explain?

Iiiii might have forgotten to push the commit that changed the condition here back to CoS 😅

carlos-zamora

Played with it a bit. Looks great. Just add the comment in too please. And Leon's comment.

DHowett-MSFT · 2020-03-25T19:56:01Z

@leonMSFT after playing with it, any strong feelings on whether focusing a window with a selection should preserve or destroy the selection? Notepad/word destroy it; xterm destroys it; putty destroys it

leonMSFT · 2020-03-25T20:07:13Z

I think it's fine to clear selection when focusing, makes the click-to-clear selection behavior more consistent 👍

zadjii-msft

Yea this looks good to me, and feels good too

ghost · 2020-03-25T21:04:39Z

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

aybiss · 2020-03-25T21:23:29Z

@DHowett-MSFT hey so I'm stoked you implemented my idea so thanks!

When will this make its way into a release? Do I just wait for the app to update from the store or what? Sorry I've literally used the MS store only once before this! :-)

DHowett-MSFT · 2020-03-25T21:30:59Z

@aybiss thanks! So, we're trying to release approximately monthly, and since this is a behavior change we're probably going to hold it until the next preview build when we change the version number. It should not be too long 😄

…on (#5374) ## Summary of the Pull Request This pull request ports #5096 to WpfTerminalControl, bringing it in line with the selection mechanics in Terminal. It also introduces double- and triple-click selection and makes sure we clear the selection when we resize. Please read #5096 for more details. ## Detailed Description of the Pull Request / Additional comments This code is, largely, copy-and-pasted from TermControl with some updates to use `std::chrono` and `til::point`. I love `til::point`. A lot. ## Validation Steps Performed Lots of manual selection.

ghost · 2020-04-22T17:14:48Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

This test dies with TerminalCore's knowledge of CoS

975622d

carlos-zamora reviewed Mar 24, 2020

View reviewed changes

DHowett-MSFT changed the title ~~SUBMITTED FOR DISCUSSION: Rework and simplify selection in TermControl~~ Rework and simplify selection in TermControl Mar 24, 2020

DHowett-MSFT marked this pull request as ready for review March 24, 2020 22:12

leonMSFT approved these changes Mar 25, 2020

View reviewed changes

carlos-zamora requested changes Mar 25, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 25, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 25, 2020

PR feedback, comment

67092d4

Move CopyOnSelect to Control settings, complete the trifecta

9cfb0d5

carlos-zamora approved these changes Mar 25, 2020

View reviewed changes

zadjii-msft approved these changes Mar 25, 2020

View reviewed changes

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 25, 2020

ghost merged commit 499f24a into master Mar 25, 2020

ghost deleted the dev/duhowett/selection branch March 25, 2020 21:09

DHowett-MSFT pushed a commit that referenced this pull request Apr 16, 2020

wpf: port selection changes from TermControl in #5096

8d18cae

DHowett-MSFT mentioned this pull request Apr 16, 2020

Clicking on window without dragging causes a selection #2636

Closed

DHowett-MSFT pushed a commit that referenced this pull request Apr 16, 2020

wpf: port selection changes from TermControl in #5096

883567d

DHowett-MSFT mentioned this pull request Apr 16, 2020

wpf: port selection changes from TermControl, add multi-click selection #5374

Merged

This was referenced Apr 22, 2020

[ui] Difficult to get rid of selection area #4477

Closed

Determine proper behavior for single-cell selection; only highlight on click + threshold-drag? #5082

Closed

zadjii-msft mentioned this pull request Aug 24, 2021

Reduce sensitivity of automatic copy selection #11027

Closed

zadjii-msft mentioned this pull request Sep 9, 2022

Mouse selection should begin and end at nearest cell boundary #5099

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework and simplify selection in TermControl #5096

Rework and simplify selection in TermControl #5096

DHowett-MSFT commented Mar 24, 2020 •

edited

Loading

carlos-zamora commented Mar 24, 2020

DHowett-MSFT commented Mar 24, 2020

DHowett-MSFT commented Mar 24, 2020

carlos-zamora commented Mar 24, 2020

DHowett-MSFT commented Mar 24, 2020

carlos-zamora left a comment

carlos-zamora Mar 24, 2020

DHowett-MSFT Mar 24, 2020

carlos-zamora Mar 24, 2020

leonMSFT Mar 25, 2020

DHowett-MSFT Mar 25, 2020

carlos-zamora left a comment

DHowett-MSFT commented Mar 25, 2020

leonMSFT commented Mar 25, 2020

zadjii-msft left a comment

ghost commented Mar 25, 2020

aybiss commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

ghost commented Apr 22, 2020

Rework and simplify selection in TermControl #5096

Rework and simplify selection in TermControl #5096

Conversation

DHowett-MSFT commented Mar 24, 2020 • edited Loading

carlos-zamora commented Mar 24, 2020

DHowett-MSFT commented Mar 24, 2020

DHowett-MSFT commented Mar 24, 2020

carlos-zamora commented Mar 24, 2020

DHowett-MSFT commented Mar 24, 2020

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Mar 24, 2020

Choose a reason for hiding this comment

DHowett-MSFT Mar 24, 2020

Choose a reason for hiding this comment

carlos-zamora Mar 24, 2020

Choose a reason for hiding this comment

leonMSFT Mar 25, 2020

Choose a reason for hiding this comment

DHowett-MSFT Mar 25, 2020

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 25, 2020

leonMSFT commented Mar 25, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Mar 25, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

aybiss commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

ghost commented Apr 22, 2020

DHowett-MSFT commented Mar 24, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.