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

Rework and simplify selection in TermControl #5096

Merged
4 commits merged into from
Mar 25, 2020
Merged

Conversation

DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented Mar 24, 2020

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:

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
Copy link
Member

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

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

std::optional<winrt::Windows::Foundation::Point> _focusRaisedClickPos;
bool _clickDrag;
std::optional<winrt::Windows::Foundation::Point> _singleClickTouchdownPos;
bool _selectionUpdatedThisCycle;
Copy link
Member

Choose a reason for hiding this comment

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

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

@DHowett-MSFT 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 DHowett-MSFT marked this pull request as ready for review March 24, 2020 22:12
@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

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

@ghost 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
@DHowett-MSFT
Copy link
Contributor Author

@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

@ghost 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
@leonMSFT
Copy link
Contributor

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

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Yea this looks good to me, and feels good too

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

ghost commented Mar 25, 2020

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.

@ghost ghost merged commit 499f24a into master Mar 25, 2020
@ghost ghost deleted the dev/duhowett/selection branch March 25, 2020 21:09
@aybiss
Copy link

aybiss commented Mar 25, 2020

@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
Copy link
Contributor Author

@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 😄

ghost pushed a commit that referenced this pull request Apr 17, 2020
…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
Copy link

ghost commented Apr 22, 2020

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
6 participants