Fix selection logic with shift on multi-click #9403
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
|
@carlos-zamora - reapplied the fix. I believe it is worth servicing. |
Don-Vito
changed the title
|
@zadjii-msft - please take a look at this as well. It is severe enough IMHO to go at least into 1.7. |
zadjii-msft
approved these changes
Mar 8, 2021
There was a problem hiding this comment.
Okay yea this looks right. Thanks!
<musing>
Man, I should really think about the fact that we need TermControl tests more. With the TermControl->{UwpTerminalControl, ControlInteractivity, ControlCore} work I'm doing, this is one of those things we're definitely going to want better tests on. This mostly seems like it's interactivity logic that would be bundled in the content process.
</musing>
34 tasks
6 tasks
carlos-zamora
approved these changes
Mar 8, 2021
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! Because this pull request has the 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 (
|
ghost
merged commit
carlos-zamora
added
zPreview-Service-Queued-1.13
zadjii-msft
reviewed
Mar 11, 2021
Comment on lines
+1374
to
+1389
| if (shiftEnabled) | ||
| { | ||
| if (_terminal->IsSelectionActive()) | ||
| { | ||
| // If there is a selection we extend it using the selection mode | ||
| // (expand the "end"selection point) | ||
| _terminal->SetSelectionEnd(terminalPosition, mode); | ||
| } | ||
| else | ||
| { | ||
| // If there is no selection we establish it using the selected mode | ||
| // (expand both "start" and "end" selection points) | ||
| _terminal->MultiClickSelection(terminalPosition, mode); | ||
| } | ||
| _selectionNeedsToBeCopied = true; | ||
| } |
There was a problem hiding this comment.
IN RETROSPECT: I don't think this is right. We might have lost the case where there's no selection, and you double/triple click on a word
@Don-Vito as an fyi
There was a problem hiding this comment.
I may stage a revert for this until we can get it in and get it right.
I'd like to see manual test runs for the following scenarios:
- single click = no selection
- single click and drag = selection starting from first point
- single click in unfocused pane and drag = focus pane, selection starting from first point
- double-click = selects a whole word
- triple-click = selects a whole line
- double-click and drag = selects a whole word, drag selects whole words
- triple-click and drag = selects a whole line, drag selects whole lines
- Shift single-click = defines start point
- second Shift single-click = defines end point
- Shift double-click = selects entire word
- Shift triple-click = selects entire line
- Shift double-click and drag = selects entire word, drag selects whole words
- Shift triple-click and drag = selects entire line, drag selects whole lines
17 tasks
ghost
pushed a commit
that referenced
this pull request
Mar 12, 2021
When working on #9403 I completely forgot that double-click and triple-click should work even without shift. Fixed it by allowing multi-selection even if not shift is pressed. Closes #9453 ## Validation Steps Performed * [x] single click = no selection * [x] single click and drag = selection starting from first point * [x] single click in unfocused pane and drag = focus pane, selection starting from first point * [x] double-click = selects a whole word * [x] triple-click = selects a whole line * [x] double-click and drag = selects a whole word, drag selects whole words * [x] triple-click and drag = selects a whole line, drag selects whole lines * [x] Shift single-click = defines start point * [x] second Shift single-click = defines end point * [x] Shift double-click = selects entire word * [x] Shift triple-click = selects entire line * [x] Shift double-click and drag = selects entire word, drag selects whole words * [x] Mouse mode: Shift single-click = defines start point * [x] Mouse mode: second Shift single-click = defines end point * [x] Mouse mode: Shift double-click = selects entire word * [x] Mouse mode: Shift triple-click = selects entire line * [x] Mouse mode: Shift double-click and drag = selects entire word, drag selects whole words
DHowett
pushed a commit
that referenced
this pull request
Mar 15, 2021
## PR Checklist * [x] Closes #9382 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. ## Detailed Description of the Pull Request / Additional comments The selection with shift is quite broken in 1.6. It started with #8611 that introduces cell selection on `shift+click`. This change resulted in the following defect: `shift+double-click`, `shift+triple-click` select only parts of the word. The reason for this is that the first `shift+click` establishes the selection, while the consequent clicks simply extend it to the relevant boundary (aka word / line boundary) However, the logic was broken even before #8611. For instance, `shift+triple-click` had exactly the same handicap: `shift+double-click` was establishing the selection and the third click was simply extending it to the line boundary. This PR addresses the both defects in the following manner: upon multi-click that starts new selection we establish a new selection on every consequent click using appropriate mode (cell/word/line) rather than trying to extend one. For this purpose we remember the position that started the selection. (cherry picked from commit 83f2a3b)
DHowett
pushed a commit
that referenced
this pull request
Mar 15, 2021
When working on #9403 I completely forgot that double-click and triple-click should work even without shift. Fixed it by allowing multi-selection even if not shift is pressed. Closes #9453 ## Validation Steps Performed * [x] single click = no selection * [x] single click and drag = selection starting from first point * [x] single click in unfocused pane and drag = focus pane, selection starting from first point * [x] double-click = selects a whole word * [x] triple-click = selects a whole line * [x] double-click and drag = selects a whole word, drag selects whole words * [x] triple-click and drag = selects a whole line, drag selects whole lines * [x] Shift single-click = defines start point * [x] second Shift single-click = defines end point * [x] Shift double-click = selects entire word * [x] Shift triple-click = selects entire line * [x] Shift double-click and drag = selects entire word, drag selects whole words * [x] Mouse mode: Shift single-click = defines start point * [x] Mouse mode: second Shift single-click = defines end point * [x] Mouse mode: Shift double-click = selects entire word * [x] Mouse mode: Shift triple-click = selects entire line * [x] Mouse mode: Shift double-click and drag = selects entire word, drag selects whole words (cherry picked from commit efc92de)
DHowett
removed
the
zStable-Service-Queued-1.12
DHowett
pushed a commit
that referenced
this pull request
Mar 15, 2021
## PR Checklist * [x] Closes #9382 * [x] CLA signed. * [ ] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. ## Detailed Description of the Pull Request / Additional comments The selection with shift is quite broken in 1.6. It started with #8611 that introduces cell selection on `shift+click`. This change resulted in the following defect: `shift+double-click`, `shift+triple-click` select only parts of the word. The reason for this is that the first `shift+click` establishes the selection, while the consequent clicks simply extend it to the relevant boundary (aka word / line boundary) However, the logic was broken even before #8611. For instance, `shift+triple-click` had exactly the same handicap: `shift+double-click` was establishing the selection and the third click was simply extending it to the line boundary. This PR addresses the both defects in the following manner: upon multi-click that starts new selection we establish a new selection on every consequent click using appropriate mode (cell/word/line) rather than trying to extend one. For this purpose we remember the position that started the selection. (cherry picked from commit 83f2a3b)
DHowett
pushed a commit
that referenced
this pull request
Mar 15, 2021
When working on #9403 I completely forgot that double-click and triple-click should work even without shift. Fixed it by allowing multi-selection even if not shift is pressed. Closes #9453 ## Validation Steps Performed * [x] single click = no selection * [x] single click and drag = selection starting from first point * [x] single click in unfocused pane and drag = focus pane, selection starting from first point * [x] double-click = selects a whole word * [x] triple-click = selects a whole line * [x] double-click and drag = selects a whole word, drag selects whole words * [x] triple-click and drag = selects a whole line, drag selects whole lines * [x] Shift single-click = defines start point * [x] second Shift single-click = defines end point * [x] Shift double-click = selects entire word * [x] Shift triple-click = selects entire line * [x] Shift double-click and drag = selects entire word, drag selects whole words * [x] Mouse mode: Shift single-click = defines start point * [x] Mouse mode: second Shift single-click = defines end point * [x] Mouse mode: Shift double-click = selects entire word * [x] Mouse mode: Shift triple-click = selects entire line * [x] Mouse mode: Shift double-click and drag = selects entire word, drag selects whole words (cherry picked from commit efc92de)
DHowett
removed
the
zPreview-Service-Queued-1.13
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Detailed Description of the Pull Request / Additional comments
The selection with shift is quite broken in 1.6.
It started with #8611 that introduces cell selection on
shift+click.This change resulted in the following defect:
shift+double-click,shift+triple-clickselect only parts of the word.The reason for this is that the first
shift+clickestablishes the selection,while the consequent clicks simply extend it to the relevant boundary
(aka word / line boundary)
However, the logic was broken even before #8611.
For instance,
shift+triple-clickhad exactly the same handicap:shift+double-clickwas establishing the selection and thethird click was simply extending it to the line boundary.
This PR addresses the both defects in the following manner:
upon multi-click that starts new selection we establish
a new selection on every consequent click using appropriate mode
(cell/word/line) rather than trying to extend one.
For this purpose we remember the position that started the selection.