Skip to content

Fix marking of new selection start - #9727

Merged
1 commit merged into
microsoft:mainfrom
Don-Vito:9725-fix-select-on-drag
Apr 6, 2021
Merged

Fix marking of new selection start#9727
1 commit merged into
microsoft:mainfrom
Don-Vito:9725-fix-select-on-drag

Conversation

@Don-Vito

@Don-Vito Don-Vito commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

PR Checklist

Validation Steps Performed

  • 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
  • Mouse mode: Shift single-click = defines start point
  • Mouse mode: second Shift single-click = defines end point
  • Mouse mode: Shift double-click = selects entire word
  • Mouse mode: Shift triple-click = selects entire line
  • Mouse mode: Shift double-click and drag = selects entire word, drag selects whole words
  • With existing selection: single-click outside the selection and drag = establishes a new selection starting from the click point

@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) 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. Severity-Blocking We won't ship a release like this! No-siree. labels Apr 6, 2021
@Don-Vito

Don-Vito commented Apr 6, 2021

Copy link
Copy Markdown
Contributor Author

Mike Griese (@zadjii-msft) - sorry 🤦Do we have a task for UT coverage? I volunteer to do it (the moment I get a week with more than 5 minutes to write code).

@zadjii-msft

Mike Griese (zadjii-msft) commented Apr 6, 2021

Copy link
Copy Markdown
Member

sorry 🤦

Haha, don't worry about it. This is just one of the absolute worst bits of spaghetti code in the entire Terminal.

Do we have a task for UT coverage? I volunteer to do it (the moment I get a week with more than 5 minutes to write code).

I actually caught this because I'm currently working on unittests over in #6842. The whole TermControl is getting split into 3 pieces, with the bottom two being fully unit testable. An unfortunate side effect is that I'll absolutely be blowing up any in-flight PRs that touch that area of the code, so maybe it's best to hold off for a bit.

Hopefully, I haven't regressed things too bad. I'll definitely be adding the list here to my "to check" list, but my initial PR will probably have tests for only a few of them. I'll add the rest to #7001 I'm hoping to get that submitted for review at the start of 1.9 and spend the entire cycle reviewing and iterating on anything that might have regressed.

@DHowett

Dustin L. Howett (DHowett) commented Apr 6, 2021

Copy link
Copy Markdown
Member

Thanks 😄

@DHowett

Dustin L. Howett (DHowett) commented Apr 6, 2021

Copy link
Copy Markdown
Member

@msftbot merge this in 2 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 6, 2021
@ghost

Deleted user (ghost) commented Apr 6, 2021

Copy link
Copy Markdown

Hello Dustin L. Howett (@DHowett)!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Tue, 06 Apr 2021 19:17:05 GMT, which is in 2 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost
ghost merged commit e80e9b9 into microsoft:main Apr 6, 2021
@zadjii-msft Mike Griese (zadjii-msft) mentioned this pull request Apr 6, 2021
Closed
Dustin L. Howett (DHowett) pushed a commit that referenced this pull request Apr 7, 2021
@Don-Vito @DHowett
Fix marking of new selection start (#9727)
7b6625d 
## PR Checklist
* [x] Closes #9725
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

## 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
* [x] With existing selection: single-click outside the selection and drag = establishes a new selection starting from the click point

(cherry picked from commit e80e9b9)
@DHowett

Dustin L. Howett (DHowett) commented Apr 7, 2021

Copy link
Copy Markdown
Member

You're probably going to hate me, but... this use case fails:

  • click-drag (set selection), shift-click-drag (extend selection while dragging)

Right now, shift-click-drag resets the start point when the drag happens (but successfully extends when the click happens!)

@ghost

Deleted user (ghost) commented Apr 14, 2021

Copy link
Copy Markdown

🎉Windows Terminal v1.7.1033.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Apr 14, 2021
Closed
@ghost

Deleted user (ghost) commented Apr 14, 2021

Copy link
Copy Markdown

🎉Windows Terminal Preview v1.8.1032.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

Reviewers

@DHowett Dustin L. Howett (DHowett) DHowett approved these changes
+1 more reviewer
@carlos-zamora Carlos Zamora (carlos-zamora) carlos-zamora approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met 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. Severity-Blocking We won't ship a release like this! No-siree.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Don-Vito @zadjii-msft @DHowett @carlos-zamora