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

Chunk Selection Expansion for Double/Triple Click Selection #2184

Merged
merged 11 commits into from
Aug 14, 2019

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Jul 31, 2019

Summary of the Pull Request

Double/Triple click create a selection expanding beyond one cell. This PR makes it so that when you're dragging your mouse to expand the selection, you expand to the next delimiter defined by double/triple click.

So, double click expands by doubleClickDelimiter ranges. Triple click expands by line.

When you double/triple click, a word/line is selected. When you drag, that word/line will remain selected after the expansion occurs.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Rather than resizing the selection when the mouse event occurs, I figured I'd do what I did with wide glyph selection: expand at render time.

We needed an enum multiClickSelectionMode to keep track of which expansion mode we're in.

Minor modifications to _ExpandDoubleClickSelection*(COORD) had to be made so that we can re-use them.

Actual expansion occurs in _GetSelectionRects()

Validation Steps Performed

  • generic double click test
    • dir or ls
    • double click a word
    • drag up
    • Works! ✔
  • double click on delimiter test
    • dir or ls
    • double click a word delimiter (i.e.: space between words)
    • drag up
    • Works! ✔
  • generic triple click test
    • dir or ls
    • triple click a line
    • drag up
    • Works! ✔
  • ALT + double click test
    • dir or ls
    • hold ALT
    • double click a word
    • drag up
    • Works! ✔

repeat above tests in following scenarios:

  • when at top of scrollback
  • drag down instead of up

@carlos-zamora carlos-zamora self-assigned this Jul 31, 2019
@@ -63,6 +63,19 @@ std::vector<SMALL_RECT> Terminal::_GetSelectionRects() const
selectionRow.Right = (row == lowerCoord.Y) ? lowerCoord.X : bufferSize.RightInclusive();
}

// expand selection for Double/Triple Click
if (multiClickSelectionMode == selectionExpansionMode::Word && _selectionAnchor != _endSelectionPosition)
Copy link
Member Author

Choose a reason for hiding this comment

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

so, this little section here: _selectionAnchor != _endSelectionPosition is something that I can call a function in in #2152

I'll be sure to clean it up if (a) that PR goes in first or (b) after I do a general cleanup to address #1327

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.

I think I'd really love to see some extra tests added for this in the TerminalCore tests.

src/cascadia/TerminalCore/TerminalSelection.cpp Outdated Show resolved Hide resolved
@carlos-zamora
Copy link
Member Author

carlos-zamora commented Aug 1, 2019

BUG REPORT:
- double click
- drag to leftmost highlighted cell

Expected Behavior: entire word stays highlighted

Actual Behavior: selection changes to a 1x1 cell selection

#Resolved

@carlos-zamora carlos-zamora added the Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) label Aug 13, 2019
src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved
{
return viewportPos;
THROW_HR(E_INVALIDARG);
Copy link
Contributor

Choose a reason for hiding this comment

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

y new except

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically, we should never hit this case. But, if somebody somehow did a double click OUTSIDE of the terminal, I think it makes sense to clamp it to the nearest valid buffer cell. So how does this sound?

positionWithOffsets.X = std::clamp(viewportPos.X, static_cast<SHORT>(0), _buffer->GetSize().RightInclusive());
positionWithOffsets.Y = std::clamp(viewportPos.Y, static_cast<SHORT>(0), _buffer->GetSize().BottomInclusive());

Note that this is explicitly should not be clamped to the viewport to allow for selections outside of the region we can see. This mainly affects scroll interactions (i.e.: auto scroll, scrolling into an existing selection, etc...).

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.

Double/Triple Click testing

image

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 14, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 14, 2019
@DHowett-MSFT
Copy link
Contributor

SA is out of disk space.

@DHowett-MSFT DHowett-MSFT merged commit 1f41fd3 into master Aug 14, 2019
@DHowett-MSFT DHowett-MSFT deleted the dev/cazamor/chunk-selection branch August 14, 2019 23:41
@ghost
Copy link

ghost commented Aug 27, 2019

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

Handy links:

@ghost ghost mentioned this pull request Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request - Chunk Selection
3 participants