Selection Code Cleanup and Double Click Delimiter Runs by carlos-zamora · Pull Request #2511 · microsoft/terminal

carlos-zamora · 2019-08-22T16:53:36Z

Summary of the Pull Request

Just some housekeeping work on our selection. Added a few more tests and refactored some of the functionality to improve code clarity.

Reenabled x86 tests. They work on my machine. I hope they work on CI?

Added support for double clicking delimiter runs.

Double Click Delimiter runs now operate as follows. Characters are separated into 3 classes:

class 0: spaces and control characters
class 1: delimiters imported from settings
class 2: all other characters
If you double click a cell, the selection will expand until the class of the next cell differs from the preceding one.

This is slightly different than before in that the following example will only highlight the ">" char instead of the ">" and all the surrounding spaces:

C:\Terminal>
           ^
double click here

References

N/A

PR Checklist

Closes Revisit TerminalSelection to add tests, clean up, proper selection mechanics #1327
Closes Bug Report: Failing Wide Glyph Tests (x86) #2261
Closes Bug Report: Double Click should select delimiter runs #2206
CLA signed
Tests added/passed
~~Requires documentation to be updated~~
I am a core contributor

Detailed Description of the Pull Request / Additional comments

Added some testing to ensure we don't crash on out-of-bounds selection anchors.
Consolidated the vertical offset for the anchors since they're always the same.
Broke up GetSelectionRects() into a more clear format with several helper methods.

Validation Steps Performed

All tests pass. I manually did some ALT+selections and regular selections w/ and w/out the scroll area and it still seems fine.

- selectionVerticalOffset - proper return values (const) - Break up GetSelectionRects()

src/cascadia/TerminalCore/TerminalSelection.cpp

More cleanup

carlos-zamora · 2019-08-26T20:58:58Z

NOTE: Double Click Delimiter runs in this isn't quite right yet. Dustin and I agreed on a new model:
handle delimiter runs as different delimiter classes where...

' ' --> class 0
delimiters --> class 1
otherwise --> class 2

double clicking a cell containing class I expands the selection to (exclusive) the first non-I class cell in both directions.

~~This will be implemented in a new branch that targets this one, BUT will (probably) go into master as two different commits.~~ This was easy. New commit covers this. Updating all other things about this PR.

Add test (caught a minor thing because of that)

src/cascadia/TerminalCore/TerminalSelection.cpp

miniksa · 2019-08-27T16:54:54Z

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp

            }
        }

+        TEST_METHOD(SelectToOutOfBounds)


Per some of my comments above, I'd love to see some tests exercising what happens when clearly invalid things are being put into your movement functions like moving left but the left is already left of the viewport.

(also some overflows, probably, given we're using SHORTs here and overflows aren't an impossibility.)

src/cascadia/TerminalCore/Terminal.hpp

src/cascadia/TerminalCore/TerminalSelection.cpp

DHowett-MSFT · 2019-08-28T01:56:13Z

Hey, x86 spontaneously failed! They're all in Griese's App tests though..

zadjii-msft · 2019-08-28T13:19:30Z

RPC_S_CALL_FAILED Yea I don't think that's something I caused...

zadjii-msft

Overall seems good, kinda minor nits from me

src/cascadia/TerminalCore/TerminalSelection.cpp

zadjii-msft

I'm definitely happier with this now

src/cascadia/TerminalCore/Terminal.hpp

src/cascadia/TerminalCore/TerminalSelection.cpp

remove some const return values that are unneccesary

dlong11 · 2019-09-10T02:23:42Z

src/cascadia/TerminalCore/TerminalSelection.cpp

+// - the selection row needed for rendering
+SMALL_RECT Terminal::_GetSelectionRow(const SHORT row, const COORD higherCoord, const COORD lowerCoord) const
+{
+    SMALL_RECT selectionRow;


nit: Even though each member of SMALL_RECT gets set currently it is best practice to always initialize a struct. I am not sure what the team's stance is on this though.

SMALL_RECT selectionRow{0};

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp

Add/Fix Tests

Also added overflow tests.

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp

miniksa

Approved with minor nits.

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp

Lots of changes.

ghost · 2019-09-24T16:26:45Z

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

Handy links:

carlos-zamora added 3 commits August 22, 2019 09:22

Test: out of bounds selection

d93816e

Clean up Selection code:

85fb29c

- selectionVerticalOffset - proper return values (const) - Break up GetSelectionRects()

reenable x86 tests

4e6d723

carlos-zamora self-assigned this Aug 22, 2019

DHowett-MSFT reviewed Aug 22, 2019

View reviewed changes

src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved

Double Click selects delimiter runs

e2e21aa

More cleanup

carlos-zamora changed the title ~~Add More Selection Testing and Proper Cleanup~~ Selection Code Cleanup and Double Click Delimiter Runs Aug 22, 2019

fix test

4dbfe59

Fix Delimiter Text Runs (#2552)

6fd44cf

Add test (caught a minor thing because of that)

miniksa reviewed Aug 27, 2019

View reviewed changes

PR Changes (need to add more tests though)

b277968

DHowett-MSFT previously requested changes Aug 28, 2019

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 Aug 28, 2019

zadjii-msft reviewed Aug 28, 2019

View reviewed changes

src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalCore/TerminalSelection.cpp Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 28, 2019

PR changes

7aaa8b8

DHowett-MSFT reviewed Aug 28, 2019

View reviewed changes

src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved

zadjii-msft approved these changes Aug 29, 2019

View reviewed changes

miniksa requested changes Sep 4, 2019

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 Sep 4, 2019

dlong11 reviewed Sep 7, 2019

View reviewed changes

src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 9, 2019

Address PR comments

5a7e22c

remove some const return values that are unneccesary

dlong11 reviewed Sep 10, 2019

View reviewed changes

fixed failing test (whoops!)

52f8028

carlos-zamora commented Sep 12, 2019

View reviewed changes

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp Outdated Show resolved Hide resolved

use iterator

2154b22

Add/Fix Tests

carlos-zamora added 2 commits September 16, 2019 10:32

Yo dawg, I heard you like lambda functions...

5effa41

Also added overflow tests.

consistent SHRT_MAX

886ebb7

miniksa reviewed Sep 16, 2019

View reviewed changes

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp Show resolved Hide resolved

miniksa reviewed Sep 16, 2019

View reviewed changes

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp Outdated Show resolved Hide resolved

miniksa approved these changes Sep 16, 2019

View reviewed changes

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp Show resolved Hide resolved

Added helper method and some log comments throughout tests

3af9640

DHowett-MSFT approved these changes Sep 17, 2019

View reviewed changes

DHowett-MSFT merged commit 4217bed into master Sep 17, 2019

DHowett-MSFT deleted the dev/cazamor/misc-selection-tasks branch September 17, 2019 17:39

This was referenced Sep 24, 2019

Bug Report: Double Click should select delimiter runs #2206

Closed

Bug Report: Failing Wide Glyph Tests (x86) #2261

Closed

Revisit TerminalSelection to add tests, clean up, proper selection mechanics #1327

Closed

carlos-zamora mentioned this pull request Nov 22, 2019

Enable Word Navigation in UiaTextRange #3659

Merged

6 tasks

Conversation

carlos-zamora commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Uh oh!

carlos-zamora commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

miniksa Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett-MSFT commented Aug 28, 2019

Uh oh!

zadjii-msft commented Aug 28, 2019

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlong11 Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Sep 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlos-zamora commented Aug 22, 2019 •

edited

Loading

carlos-zamora commented Aug 26, 2019 •

edited

Loading