Introduce Mark Mode by carlos-zamora · Pull Request #13053 · microsoft/terminal

carlos-zamora · 2022-05-07T01:23:42Z

Summary of the Pull Request

Introduces a non-configurable version of mark mode to Windows Terminal. It has the following interactions defined:

ctrl+shift+m --> Enter Mark Mode
when in Mark Mode...
- ESC --> Exit Mark Mode
- arrow keys --> move "start"
- shift + arrow keys --> anchor "start", move "end"
- ctrl+a --> select all
when a selection is active...

When in mark mode, the cursor does not blink.

References

#4993 - [Epic] Keyboard Selection

PR Checklist

Closes Terminal: Support keyboard text selection & navigation #715
Provides a resolution for [Command Prompt] : User is not able to Select commands using 'Shift+ right/Left arrow keys' inside Command Prompt. #11985

Detailed Description of the Pull Request / Additional comments

TermControl:
- TermControl.cpp just adds logic to prevent the cursor from blinking when in mark mode
ControlCore
- in the same place we handle quick edit, we add an entry point to mark mode
TerminalCore
- this leverages UpdateSelection() and other quick edit functions to make mark mode happen

Validation Steps Performed

Make selection, split pane, close pane
- NOTE: A similar scenario caused a crash at one point. Really weird. Keep an eye on it.
Cursor is off when in mark mode
general movement/selection
general movement/selection that forces the viewport to move
In mark mode, selectAll...
- arrow keys --> move start
- shift + arrow keys --> move end
(regardless of mark mode) if selection active, enter --> copy to clipboard

carlos-zamora · 2022-05-07T01:24:25Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // TODO CARLOS: should we make this ctrl+shift+m instead?
+            if (modifiers.IsCtrlPressed() && vkey == 'M')


Whatcha think?

make this an action, default it to ctrl+shift+m. The keybindings w/in mark mode don't need to be configurable now, but the toggle should be.

That seems reasonable to me. Then your shift+left behavior from CMD will exist if you want it to.

Actually, you still won't be able to get that shift+left behavior. Binding markMode to that will make it so that you can't use shift+left to select towards the left. Right now, I have the markMode action work as a toggle...

if in mark mode --> exit mark mode

if not in mark mode (even if there's a mouse selection present) --> enter mark mode --> create selection at cursor

That said, I'm not too sure about this behavior. Definitely shippable, but open to changing it.

We could do this (but I'm concerned it may be overcomplicating it)...

if in mark mode --> ignore markMode action

if selection active (from mouse) --> ignore markMode action

if no selection (and not in mark mode) --> create selection at cursor
That should permit the shift+left behavior from CMD. Thoughts?

zadjii-msft

NAK on magic ctrl+m

zadjii-msft · 2022-05-09T15:10:38Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // TODO CARLOS: should we make this ctrl+shift+m instead?
+            if (modifiers.IsCtrlPressed() && vkey == 'M')


make this an action, default it to ctrl+shift+m. The keybindings w/in mark mode don't need to be configurable now, but the toggle should be.

zadjii-msft · 2022-05-09T15:13:04Z

src/cascadia/TerminalControl/TermControl.cpp

            // Manually show the cursor when a key is pressed. Restarting
            // the timer prevents flickering.
-            _core.CursorOn(true);
+            _core.CursorOn(!_core.IsInMarkMode());


is this right? On is literally the on/off blink state of the cursor (e.g. when it's drawn, it's on, then it blinks to off and is not drawn, then blinks back to on)

So we don't want the cursor to be shown at all when in mark mode?

Yeah, that's intentional (but definitely up for discussion on the design, actually). I'll just rapid fire random thoughts in my head for why I feel this is the desired behavior...

the cursor is a bit distracting when it's blinking if you're in mark mode

preventing the cursor from blinking makes it feel more clear that you're in a different mode (though we're looking to add the markers in Add selection marker overlays for keyboard selection #10865 )

when in mark mode, the "cursor" should really be the selection marker (just like in a text editor). So there's no reason to show the actual cursor.

[Accessibility] we do need to track the cursor movements for some things. Maybe we should actually be moving the cursor along with the selection? Idk. I'll have to test this out.

Ah right I forgot about the overlays. Good point.

src/cascadia/TerminalCore/TerminalSelection.cpp

zadjii-msft · 2022-05-09T21:11:47Z

Team discuss:

yea make mark mode an action
keys as-is is good, if someone really wants rebinding, we'll let them ask for it.
make a follow up for toggle endpoint as an action - having a separate mode for "copy mode" is silly
#shipit
The docs have updateSelection actions in them, probably remove those
Add a new top-level docs section to describe all this interaction

doc/cascadia/profiles.schema.json

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw

src/cascadia/TerminalControl/ControlCore.cpp

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw

zadjii-msft · 2022-05-16T18:42:59Z

src/cascadia/TerminalControl/TermControl.cpp

            // Manually show the cursor when a key is pressed. Restarting
            // the timer prevents flickering.
-            _core.CursorOn(true);
+            _core.CursorOn(!_core.IsInMarkMode());


Ah right I forgot about the overlays. Good point.

src/cascadia/TerminalCore/TerminalSelection.cpp

src/cascadia/TerminalControl/ControlCore.cpp

DHowett

blocking for time to look today

DHowett

okay, so some selections are selections and some selections are mark mode selections? I can get behind that. My only concern is that selection one, because it impacts specifically me and it is not configurable. I don't think it SHOULD be configurable, i just think that a PR adding mark mode should not sneak in any changes to the non-mark mode ;)

src/cascadia/TerminalControl/ControlCore.cpp

DHowett · 2022-05-18T22:41:31Z

src/cascadia/TerminalControl/ControlCore.cpp

+                auto lock = _terminal->LockForWriting();
+                _terminal->SelectAll();
+                _renderer->TriggerSelection();


I'm surprised we don't have a helper for this!

yeah, I think at one point I made (or witnessed) the decision that triggering the renderer and (un)locking would be handled at the control core layer consistently to ensure we don't deadlock on accident. It's a little annoying, but we could swing the pendulum the other way hahaha

ghost · 2022-05-20T23:04:42Z

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.

ofek · 2022-05-26T05:49:21Z

When will this be released? Also since there's not much documentation, can you please tell me if this will satisfy the use case I described here? #6528

ofek · 2022-05-26T05:55:05Z

or is this the final feature? #11868

zadjii-msft · 2022-05-26T11:12:49Z

@ofek We're planning on shipping this in Preview 1.15, the next Terminal release. Typically, these releases are about 6 weeks apart. We'll get to elaborating on the docs once we get closer to release.

Trying to parse everything there and translate it to what we've got so far:

"click and drag (highlight) somewhere with the mouse"->"press any arrow key while holding shift"
- I think this actually shipped in 1.13, IIRC. Definitely in 1.14.
"At this point you can release the left-click (1) or shift (2). ... and you can now move anywhere with just the arrow keys"
- @carlos-zamora would have to clarify this one for me. I think that at this point, just arrow keys won't move the cursor.
- HOWEVER, if you started the selection by pressing ctrl+shift+m (or whatever you bind mark mode to), then just arrow keys will move the cursor.
"To highlight you then hold shift while using arrow keys similar to the highlighting logic of holding left-click while moving the mouse."
- Yep, that's how mark mode works.
"Copying can happen in 2 ways:...."
- This I don't think we have today (again, Carlos correct me if I'm wrong). We may need to add an arg to copyText to specifically ask that the selection is not dismissed.

ofek · 2022-06-20T05:40:35Z

Hey! So I spent this weekend learning & configuring the terminal. It's amazing, and I think I can immediately switch over when this is released. I wanted to test to be sure though so I downloaded the drop artifact from the CI like in https://dev.azure.com/ms/terminal/_build/results?buildId=322645&view=artifacts&pathAsName=false&type=publishedArtifacts

The .\Release\x64\test\wt.exe doesn't do anything and the .\CascadiaPackage_0.0.1.0_x64.msix requires a certificate to install. Is building from source required to test development versions?

zadjii-msft · 2022-06-20T10:27:02Z

Is building from source required to test development versions?

Yep. We don't really have any sort of official nightly channel set up.

## Summary of the Pull Request 1. [copy on select] when manually copying text (i.e. kbd or right-click) while in mark/quick-edit mode, we now dismiss the selection. 2. `Enter` is now bound to copy by default. - This works very well with mark mode and provides a more consistent behavior with conhost's selection experience overall. - "why not hardcode `Enter` as a way to copy when in mark mode?" - In an effort to make this as configurable as possible, I decided to make it a configurable keybinding, but am open to suggestions. 3. selection markers a. we now hide the selection markers when multi-clicking the terminal. b. selection markers are now properly shown when a single cell selection exists - Prior to this PR, any single cell selection would display both markers. Now, we actually track which endpoint we're moving and display the appropriate one. 4. ensures that when you use keyboard selection to move past the top/bottom of the scroll area, we clamp it to the origin/bottom-right respectively. The fix is also better here in that it provides consistent behavior across all of the `_MoveByX` functions. 5. adds `toggleBlockSelection` to the schema ## References #13053 ## Validation Steps Performed Did a whole flowchart of expected behavior with copy on select: - enable `copyOnSelect` - make a selection with the mouse - ✅ right-click should copy the text --> clear the selection --> paste - use keyboard selection to quick-edit the existing selection - ✅ `copy` action should clear the selection - ✅ right-click should copy the text --> clear the selection --> paste Played with selection markers a bit in mark mode and quick edit mode. Markers are updating appropriately.

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

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

Handy links:

ofek · 2022-07-06T18:34:11Z

I'm now able to use Windows Terminal without fatigue from heavy mouse movement. Thank you very much!!!

carlos-zamora commented May 7, 2022

View reviewed changes

zadjii-msft added this to the Terminal v1.15 milestone May 9, 2022

zadjii-msft requested changes May 9, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 9, 2022

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label May 9, 2022

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label May 9, 2022

carlos-zamora commented May 10, 2022

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

carlos-zamora added 4 commits May 11, 2022 12:52

Introduce Mark Mode

2f4a1c5

make mark mode an action

262a6bb

update schema; fix deadlock

c9e28cb

fix a few nits

2d71c2c

carlos-zamora force-pushed the dev/cazamor/kbd-sln/classic-mark-mode branch from 099a8e2 to 2d71c2c Compare May 11, 2022 19:56

carlos-zamora commented May 11, 2022

View reviewed changes

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

remove 'Enter' --> copy in Mark Mode

75faa5d

zadjii-msft approved these changes May 16, 2022

View reviewed changes

address PR feedback

f302605

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label May 16, 2022

ghost requested review from PankajBhojwani and lhecker May 16, 2022 21:32

carlos-zamora mentioned this pull request May 16, 2022

Scenario: Keyboard Text Selection #4993

Open

11 tasks

ghost added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. labels May 16, 2022

lhecker reviewed May 18, 2022

View reviewed changes

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

apply Leonard's suggestions

4a19d6b

carlos-zamora assigned lhecker May 18, 2022

lhecker approved these changes May 18, 2022

View reviewed changes

DHowett requested changes May 18, 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 May 18, 2022

DHowett requested changes May 18, 2022

View reviewed changes

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

fix ctrl+a --> select all in mm

9564090

carlos-zamora assigned DHowett and unassigned lhecker May 19, 2022

DHowett approved these changes May 20, 2022

View reviewed changes

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

ghost merged commit bf41a90 into main May 20, 2022

ghost deleted the dev/cazamor/kbd-sln/classic-mark-mode branch May 20, 2022 23:05

ofek mentioned this pull request May 21, 2022

Introduce Mark Mode spec (add-on) #5804

Merged

1 task

This was referenced Jun 22, 2022

Miscellaneous bug fixes for Mark Mode #13358

Merged

Document keyboard selection MicrosoftDocs/terminal#552

Merged

ghost mentioned this pull request Jul 6, 2022

Terminal: Support keyboard text selection & navigation #715

Closed

This pull request was closed.

		// TODO CARLOS: should we make this ctrl+shift+m instead?
		if (modifiers.IsCtrlPressed() && vkey == 'M')

Conversation

carlos-zamora commented May 7, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zadjii-msft commented May 9, 2022 • edited by carlos-zamora 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 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.

Uh oh!

ofek commented May 26, 2022

Uh oh!

ofek commented May 26, 2022

Uh oh!

zadjii-msft commented May 26, 2022

Uh oh!

ofek commented Jun 20, 2022

Uh oh!

zadjii-msft commented Jun 20, 2022

Uh oh!

ghost commented Jul 6, 2022

Uh oh!

ofek commented Jul 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlos-zamora commented May 7, 2022 •

edited

Loading

zadjii-msft commented May 9, 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.