Add selection marker overlays for keyboard selection #10865

carlos-zamora · 2021-08-03T22:50:51Z

Summary of the Pull Request

This introduces a selection marker overlay that tells the user which endpoint is currently being moved by the keyboard. The selection markers are respect font size changes and cursor color.

References

#715 - Keyboard Selection
#2840 - Keyboard Selection Spec
#5804 - Mark Mode Spec

Detailed Description of the Pull Request / Additional comments

TermControl layer:
- Use a canvas (similar to the one used for hyperlinks) to be able to draw the selection markers.
- If we are notified that the selection changed, update the selection markers appropriately.
- UpdateSelectionMarkersEventArgs lets us distinguish between mouse and keyboard selections. ClearMarkers is set to true in the following cases...
  1. Mouse selection, via SetEndSelectionPoint
  2. LeftClickOnTerminal, pretty self-explanatory
  3. a selection created from searching for text
ControlCore layer:
- Responsible for notifying TermControl to update the selection markers when a selection has changed.
- Transfers info (the selection endpoint positions and which endpoint we're moving) from the terminal core to the term control.
TerminalCore layer:
- Provides the viewport position of the selection endpoints.

Validation Steps Performed

mouse selection (w/ and w/out shift) --> no markers
keyboard selection --> markers
markers update appropriately when we pivot the selection
markers scroll when you hit a boundary
markers take the color of the cursor color setting
markers are resized when the font size changes

carlos-zamora · 2022-06-02T17:44:57Z

Finished rebasing the PR! 🎉
This still isn't working right with mark mode, so I'll have to polish this quite a bit.
If you are interested in reviewing this, you're welcome to do so, but I suggest waiting until I take this out of draft.

EDIT: things to do...

Bug: markMode doesn't switch the markers appropriately
Bug: marker is still displayed when you scroll down and it's outside of the viewport
Task: replace markers with y-beam provided by Kayla
Question: selection endpoints are currently inclusive. It's out of scope to make "end" exclusive and "start" inclusive. But should I try to fake it in this PR? It would be pretty important for when we're in mark mode and just moving the cursor around. 🤔 EDIT: NO

carlos-zamora · 2022-06-03T16:21:27Z

src/cascadia/TerminalCore/TerminalSelection.cpp

@@ -318,10 +343,9 @@ Terminal::UpdateSelectionParams Terminal::ConvertKeyEventToUpdateSelectionParams
 void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion mode, ControlKeyStates mods)


Changes to this function may seem a little odd and unrelated, but they're necessary. Now that we're displaying the relevant y-beam end based on which endpoint we're moving, we need to make sure we're moving the correct one. Before, we could cheat it a bit since we didn't really have a concept of "start" and "end" (more just point 1 and 2).

oh this is much easier to read

src/cascadia/TerminalCore/TerminalSelection.cpp

carlos-zamora · 2022-06-07T17:14:00Z

A few things I want to highlight (heh)...

the selection markers match the color of the selection background
(not shown) the selection markers change in size when the font size changes
both markers are shown when a single cell selection is present
one marker is shown when moving one side and the appropriate one is shown when the selection pivots

zadjii-msft

this sparks joy

zadjii-msft · 2022-06-10T11:30:29Z

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1052,7 +1047,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        if (_terminal->IsSelectionActive())
        {
            _terminal->SetBlockSelection(!_terminal->IsBlockSelection());


idle thought: maybe we file a follow up to change the shape of the markers when we're in block selection mode? idk.

Oh yeah. Dustin and I were chatting about that the other day. it would be cool if we used some kind of 'L' shape for that.

Definitely a follow-up and something to mention/show in the spec.

src/cascadia/TerminalControl/ControlCore.cpp

DHowett · 2022-06-10T19:41:02Z

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1012,7 +1038,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
    {
        auto lock = _terminal->LockForWriting();
        _terminal->SelectAll();
-        _renderer->TriggerSelection();
+        _updateSelection(false);


I'm not too sure I like this actually! I can't divine rhyme or reason as to why we would clear sometimes and not others. Is there a guiding principle you're using?

yeah, the general intent is...

are you using your keyboard? --> show markers

using a mouse? --> clear markers

search? --> clear

toggle block selection? --> probably don't mess with it? (I'm adding this in the next commit)

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalCore/TerminalSelection.cpp

src/cascadia/TerminalControl/ControlCore.h

DHowett · 2022-06-13T23:25:12Z

Additional notes from bug bash

Looks like some things like Search and command palette trigger the flags to show up
DPI is still an issue
Make sure we aren't spending too much time scaling them on zoom (just in case it is slow)

carlos-zamora · 2022-06-14T23:07:52Z

Make sure we aren't spending too much time scaling them on zoom (just in case it is slow)

Did a perf trace in VS. It doesn't even appear in the call tree. Relevant changes are here. Looks like the scale transform is cheap.

NOTE: later iteration adds dpi scaling, which is hardly different tbh. That's the one I profiled.

carlos-zamora · 2022-06-15T21:25:05Z

carlos-zamora · 2022-06-15T21:26:33Z

@DHowett this is ready for review now :)

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalControl/ControlCore.idl

ghost · 2022-06-20T22:39:38Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2022-07-06T18:10:06Z

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

Handy links:

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

carlos-zamora marked this pull request as draft August 4, 2021 16:58

carlos-zamora force-pushed the dev/cazamor/keyboard-selection branch from 5b4dd33 to b2a7a19 Compare August 18, 2021 23:20

carlos-zamora force-pushed the dev/cazamor/keyboard-selection branch from 5297b80 to 0cc9bd0 Compare September 2, 2021 22:04

carlos-zamora force-pushed the dev/cazamor/keyboard-selection branch 2 times, most recently from e498dea to 118d1cd Compare September 17, 2021 02:58

carlos-zamora force-pushed the dev/cazamor/ks/copy-mode branch from 2c52439 to ad26780 Compare September 23, 2021 19:23

Base automatically changed from dev/cazamor/keyboard-selection to main September 23, 2021 19:24

zadjii-msft mentioned this pull request Feb 25, 2022

Shift modifer conficts between "disable mouse input" and "extend selection" #9608

Open

zadjii-msft mentioned this pull request Mar 14, 2022

Scenario: Keyboard Text Selection #4993

Open

11 tasks

zadjii-msft added this to the Terminal v1.15 milestone Apr 12, 2022

carlos-zamora mentioned this pull request May 9, 2022

Introduce Mark Mode #13053

Merged

10 tasks

carlos-zamora force-pushed the dev/cazamor/ks/copy-mode branch from ad26780 to 25d14ae Compare June 2, 2022 17:41

microsoft deleted a comment from github-actions bot Jun 2, 2022

This comment was marked as resolved.

Sign in to view

carlos-zamora force-pushed the dev/cazamor/ks/copy-mode branch from 25d14ae to 85dafbb Compare June 2, 2022 17:49

carlos-zamora added 2 commits June 2, 2022 17:07

Add selection marker overlays for keyboard selection

76e77de

simplify logic a bit

214d3af

carlos-zamora force-pushed the dev/cazamor/ks/copy-mode branch from 85dafbb to 214d3af Compare June 3, 2022 00:11

carlos-zamora commented Jun 3, 2022

View reviewed changes

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

carlos-zamora added 2 commits June 6, 2022 14:27

add markers as paths; fix selectAll/toggleMarkMode

f9baadc

MovingStart-->MovingEnd

aa2ff6b

Merge branch 'main' into dev/cazamor/ks/copy-mode

23dc59f

zadjii-msft approved these changes Jun 10, 2022

View reviewed changes

zadjii-msft added the Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) label Jun 10, 2022

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

DHowett requested changes Jun 10, 2022

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 Jun 10, 2022

This comment was marked as outdated.

Sign in to view

Merge branch 'main' into dev/cazamor/ks/copy-mode

365cfcf

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 10, 2022

This comment was marked as resolved.

Sign in to view

address Dustin's feedback

659230a

This comment was marked as resolved.

Sign in to view

flip markers at side boundaries

2007e18

carlos-zamora assigned DHowett Jun 16, 2022

This comment was marked as resolved.

Sign in to view

fix unfocused appearance bug

4623371

carlos-zamora commented Jun 16, 2022

View reviewed changes

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

DHowett reviewed Jun 16, 2022

View reviewed changes

src/cascadia/TerminalControl/ControlCore.idl Outdated Show resolved Hide resolved

carlos-zamora added 2 commits June 17, 2022 11:28

update names for marker data

5eb1717

change marker color to cursor color

326dadf

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 20, 2022

DHowett approved these changes Jun 20, 2022

View reviewed changes

DHowett merged commit 6436e71 into main Jun 20, 2022

DHowett deleted the dev/cazamor/ks/copy-mode branch June 20, 2022 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add selection marker overlays for keyboard selection #10865

Add selection marker overlays for keyboard selection #10865

carlos-zamora commented Aug 3, 2021 •

edited

Loading

This comment was marked as outdated.

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

carlos-zamora commented Jun 2, 2022 •

edited

Loading

carlos-zamora Jun 3, 2022

zadjii-msft Jun 9, 2022

carlos-zamora commented Jun 7, 2022 •

edited

Loading

zadjii-msft left a comment

zadjii-msft Jun 10, 2022

carlos-zamora Jun 10, 2022

This comment was marked as resolved.

This comment was marked as outdated.

DHowett Jun 10, 2022

carlos-zamora Jun 10, 2022

This comment was marked as outdated.

This comment was marked as resolved.

DHowett commented Jun 13, 2022 •

edited by carlos-zamora

Loading

carlos-zamora commented Jun 14, 2022

This comment was marked as resolved.

carlos-zamora commented Jun 15, 2022

carlos-zamora commented Jun 15, 2022

This comment was marked as resolved.

ghost commented Jun 20, 2022

ghost commented Jul 6, 2022

		@@ -318,10 +343,9 @@ Terminal::UpdateSelectionParams Terminal::ConvertKeyEventToUpdateSelectionParams
		void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion mode, ControlKeyStates mods)

Add selection marker overlays for keyboard selection #10865

Add selection marker overlays for keyboard selection #10865

Conversation

carlos-zamora commented Aug 3, 2021 • edited Loading

Summary of the Pull Request

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

This comment was marked as outdated.

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

carlos-zamora commented Jun 2, 2022 • edited Loading

carlos-zamora Jun 3, 2022

Choose a reason for hiding this comment

zadjii-msft Jun 9, 2022

Choose a reason for hiding this comment

carlos-zamora commented Jun 7, 2022 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Jun 10, 2022

Choose a reason for hiding this comment

carlos-zamora Jun 10, 2022

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

DHowett Jun 10, 2022

Choose a reason for hiding this comment

carlos-zamora Jun 10, 2022

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as resolved.

DHowett commented Jun 13, 2022 • edited by carlos-zamora Loading

carlos-zamora commented Jun 14, 2022

This comment was marked as resolved.

carlos-zamora commented Jun 15, 2022

carlos-zamora commented Jun 15, 2022

This comment was marked as resolved.

ghost commented Jun 20, 2022

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 6, 2022

carlos-zamora commented Aug 3, 2021 •

edited

Loading

carlos-zamora commented Jun 2, 2022 •

edited

Loading

carlos-zamora commented Jun 7, 2022 •

edited

Loading

DHowett commented Jun 13, 2022 •

edited by carlos-zamora

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.