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

Fix selection logic with shift on multi-click and in mouse mode #9384

Closed

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Mar 5, 2021

PR Checklist

Detailed Description of the Pull Request / Additional comments

Currently, if selection is active, all clicks with shift pressed extend it,
i.e., shift+click, shift+double-click, shift+triple-click extend the selection
from anchor start to the current position.

While we can argue the correctness of this UX in general
(hint the terminals I checked usually do something else),
it is fully broken by #8611, that introduced cell selection on shift+click.
Currently, upon shift+double-click:

  • the first click sets the selection
  • the second click simply extends it
    So instead of the word we get only partial selection.

To address this I suggest to change the selection logic to:
extend only on a single-click.
I.e., shift+double-click will select a word rather than
extending an existing selection.

This is how it works in ConEmu.
However MinTTY have even smarter solution
where shift+double-click adds a word to existing selection.

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 5, 2021

ShifDoubleClick

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.

Huh, I think the MinTTY approach makes more sense to me. In my mind, holding shift extends the selection. So shift+double-click should extend the selection to the end of the word, whereas shift+triple-click should extend the selection to the end of the line (unless you're going up from the first selection anchor, in which case it should always go to the beginning).

Like, if you wanted to select the word as shown in your gif, just don't hold shift.

#9382 sounds more like a combination of 2 problems.

First, we get a conflict of...

  1. shift means we should select, rather than send VT mouse input
  2. shift means we should extend the selection

Second (easier to fix), shift+double-clicking a word when no selection is present only expands the selection to the left rather than expanding both selection anchors.

I think if we just fix the second one, we can close the bug.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 5, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 5, 2021

I think if we just fix the second one, we can close the bug.

Actually, this is how I tried to fix it 😄
The problem is that knowing there was no selection prior to double-click is more complex (as after the first click there is a selection already).
I can do something else: if the selection anchor is current cell, then double-click and triple-click will behave as there is no selection.
WDYT?

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 5, 2021
@carlos-zamora
Copy link
Member

I can do something else: if the selection anchor is current cell, then double-click and triple-click will behave as there is no selection.
WDYT?

That sounds right to me! Let's do it! 😊

@Don-Vito Don-Vito changed the title Fix selection not to extend on multi-click Fix selection logic with shift on multi-click and in mouse mode Mar 7, 2021
@Don-Vito Don-Vito closed this Mar 7, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Mar 7, 2021

Closed in favor of #9403 that takes better approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants