Miscellaneous bug fixes for Mark Mode #13358

carlos-zamora · 2022-06-22T00:10:59Z

Summary of the Pull Request

[copy on select] when manually copying text (i.e. kbd or right-click) while in mark/quick-edit mode, we now dismiss the selection.
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.
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.
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.
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.

carlos-zamora · 2022-06-22T19:41:59Z

Discussion points

How do we feel about Enter being bound to copy() by default?
- It's consistent with conhost, and worst-case, people can disable it. I don't expect people to bind Enter to anything nowadays anyways.
Should we backport a fix for the copy on select stuff?
- It's a decently sized fix and nobody's complained about it. Ethically, we should, but there is some risk due to the size. I'd basically copy over anything that references IsInQuickEditMode().
Also found another "bug?" where if you select empty text while copy on select is enabled, we end up copying literally nothing. I'm guessing this is because we switched the default copy() behavior to trimming whitespace. Should that be a part of this PR?
- I vote no. I'm also not sure if we should even commit to fixing this for 1.14 and 1.15. But I could 100% be wrong. Either way, bringing it to your attention.

CC @DHowett as my favorite copyOnSelect user who definitely has opinions on this stuff haha

carlos-zamora

While working on the 1.14 backport, found a few things I could clean up

src/cascadia/TerminalCore/TerminalSelection.cpp

src/cascadia/TerminalCore/Terminal.hpp

src/cascadia/TerminalControl/ControlCore.h

ofek · 2022-06-22T22:33:35Z

Nice! So Enter would only copy when there is a selection right?

carlos-zamora · 2022-06-22T22:42:11Z

Nice! So Enter would only copy when there is a selection right?

Correct! Just like how we handle ctrl+c today 😊

ofek · 2022-06-22T23:03:30Z

Just like how we handle ctrl+c today

Hmm, not for me. Try:

{ "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+c" }

then go to a previous command, press home to go to the start of it, then shift+end to select it, then finally ctrl+c. For me it interrupts rather than copies

…soft/terminal into dev/cazamor/kbd-sln/polish

carlos-zamora · 2022-06-23T16:54:58Z

Just like how we handle ctrl+c today

Hmm, not for me. Try:
{ "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+c" }
then go to a previous command, press home to go to the start of it, then shift+end to select it, then finally ctrl+c. For me it interrupts rather than copies

This seems to work fine on my branch and our internal 1.15 build. @ofek could you file a big report here and be sure to include your settings.json file (if you don't feel comfortable with that, the main things I'd want to check are (1) copy on select and (2) the copy keybindings being used).

DHowett · 2022-06-23T16:59:33Z

I don't understand why this is conflicting with main when I try to merge it...!

DHowett · 2022-06-23T17:00:41Z

Oh, because I ran git merge without actually telling it what ref to merge. WTF?

ofek · 2022-06-23T17:53:55Z

could you file a bug report

#13369

zadjii-msft · 2022-06-23T19:17:51Z

src/cascadia/TerminalSettingsModel/defaults.json

@@ -382,6 +382,7 @@
        // Clipboard Integration
        { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+shift+c" },
        { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+insert" },
+        { "command": { "action": "copy", "singleLine": false }, "keys": "enter" },


This is potentially a spicy change 🌶️

I know conhost did this in the past. I feel like we've discissed this in the past on the repl, but I can't find the discussion if we specifically said we'd add this to the defaults or not.

We may also want to consider something like #13115 - what does that even mean? Is that something separate from a text selection?

We may also want to consider something like #13115 - what does that even mean? Is that something separate from a text selection?

My vision for #13115 (which isn't even specced tbh), is that it's a way to navigate within mark mode. It might even be something as simple as [shift]+tab to move to the next/previous hyperlink. That 100% needs to be specc'd though.

This is potentially a spicy change 🌶️

Yeah, I don't remember the discussion either :/. The paths forward I see are...

bind copy() to Enter in defaults.json (configurable)

make Enter copy the text when in mark mode (non-configurable)

don't even consider Enter entirely. If the user wants it, the user can bind it

Dustin seemed to be a little confused when using mark mode saying that there's no clear way to copy the selection. I think this stems from him using copyOnSelect, which seems fair because you never really had to think about copying your selection before. But we can't copy the selection every time the selection changes in mark mode. That would totally mess up clipboard history for people who use it. :/

We could pull this part out and discuss it at sync on Monday? It's a one-liner with no risk. Really just "political" reasons to decide to add it in or not.

Funny enough, now I'm thinking that we don't add this as a keybinding in defaults.json. Maybe we should make it non-configurable to allow for this.

That way, Enter copies when in mark mode, but opens the URL when it's selected/found by tabbing through he buffer in mark mode. It would then go back to copying if the user modified the selection.

Putting it in defaults.json allows the user to unbind it from both behaviors, but still gives us the freedom to change it (as opposed to inserting it in usersettings.json). We don't need to make the decision now, because defaults.json is entirely under our control across app updates.

Have we considered adding a mode option to key bindings that would only apply when enabled? They could correspond to this enum #13360 (comment)

Alacritty has the same concept (vi mode is mark mode) https://github.com/alacritty/alacritty/blob/master/alacritty/src/config/bindings.rs

Adding modal key bindings has been hotly debated, we just don't have the cycles to do it right now :)

DHowett · 2022-06-29T17:24:34Z

src/cascadia/TerminalSettingsModel/defaults.json

@@ -382,6 +382,7 @@
        // Clipboard Integration
        { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+shift+c" },
        { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+insert" },
+        { "command": { "action": "copy", "singleLine": false }, "keys": "enter" },


Adding modal key bindings has been hotly debated, we just don't have the cycles to do it right now :)

src/cascadia/TerminalControl/ControlCore.cpp

src/cascadia/TerminalControl/ControlInteractivity.cpp

src/cascadia/TerminalCore/Terminal.cpp

src/cascadia/TerminalCore/TerminalSelection.cpp

A port of commit 0c5a1e9 from #13358. This fixes an oversight from #13353. That ended up not fixing when trying to move past the top boundary. This is also a better fix overall. By moving the fix to `_MoveByChar`, we have consistency between all the `_MoveByX` functions in that they automatically clamp to be within the scroll area.

## Summary of the Pull Request ⚠️This targets release-1.14⚠️ Backport of #13358 This PR fixes a number of interactions between `copyOnSelect` and keyboard selection. Sometimes the selection just wouldn't be cleared or copied appropriately. I wrote 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

src/cascadia/TerminalControl/ControlCore.cpp

lhecker · 2022-06-30T23:53:52Z

src/cascadia/TerminalControl/ControlCore.cpp

+            const auto isInMarkMode = _terminal->SelectionMode() == ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode::Mark;
+            if (isInMarkMode && modifiers.IsCtrlPressed() && vkey == 'A')


The namespace qualifier seems overly specific. FYI this should work as expected from other languages:

using ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode;

lhecker · 2022-07-01T00:00:38Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // DO NOT call _updateSelectionUI() here.
+            // We don't want to show the markers so manually tell it to clear it.


I personally find this comment a bit hard to understand. Why do we not want to show them?

lhecker · 2022-07-01T00:04:15Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

@@ -257,15 +256,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        }
        else if (WI_IsFlagSet(buttonState, MouseButtonState::IsRightButtonDown))
        {
-            // CopyOnSelect right click always pastes
-            if (_core->CopyOnSelect() || !_core->HasSelection())
+            // Try to copy the text and clear the selection


It could be helpful to mention here that CopySelectionToClipboard returns false if the selection is empty, which results in our "unique" right-click behavior (copy if something's selected, paste if it isn't).

DHowett

Don't automerge this!

## Summary of the Pull Request Introduces the `switchSelectionEndpoint` action which switches whichever selection endpoint is targeted when a selection is present. For example, if you are targeting "start", `switchSelectionEndpoint` makes it so that now you are targeting "end". This also updates the selection markers appropriately. ## References Spec - #5804 #13358 Closes #3663 ## Detailed Description of the Pull Request / Additional comments Most of the code is just standard code of adding a new action. Other than that, we have... - if there is no selection, the action fails and the keybinding is passed through (similar to `copy()`) - when we update the selection endpoint, we need to also update the "pivot". This ensures that future calls of `UpdateSelection()` respect this swap. - [Corner Case] if the cursor is being moved, we make it so that you basically "anchored" an endpoint and you don't have to hold shift anymore.

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

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

Handy links:

Miscellaneous bug fixes for Mark Mode

cdbe030

This comment was marked as outdated.

Sign in to view

fix copyOnSelect interactions with keyboard selection

654bb08

carlos-zamora marked this pull request as ready for review June 22, 2022 19:42

carlos-zamora commented Jun 22, 2022

View reviewed changes

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

src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved

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

Fix a few method signatures

0b54345

carlos-zamora mentioned this pull request Jun 22, 2022

[1.14] Fix keyboard selection and copyOnSelect interaction #13360

Merged

carlos-zamora added 3 commits June 23, 2022 09:44

bugfix: correctly show markers when one cell selected

06a39f8

Merge branch 'dev/cazamor/kbd-sln/polish' of https://github.com/micro…

40ee74e

…soft/terminal into dev/cazamor/kbd-sln/polish

spell fix

408fca2

Merge branch 'main' into dev/cazamor/kbd-sln/polish

a2a346b

carlos-zamora mentioned this pull request Jun 23, 2022

Introduce switchSelectionEndpoint action #13370

Merged

zadjii-msft reviewed Jun 23, 2022

View reviewed changes

zadjii-msft added this to the Terminal v1.15 milestone Jun 23, 2022

carlos-zamora added 2 commits June 23, 2022 17:36

combine mark mode and quick edit mode

32ca437

fix clamping the selection at top/bottom properly

0c5a1e9

carlos-zamora mentioned this pull request Jun 24, 2022

[1.14] Properly fix keyboard sln at top/bot #13372

Merged

add toggleBlockSelection to schema

c78818a

This was referenced Jun 26, 2022

Document keyboard selection MicrosoftDocs/terminal#552

Merged

Scenario: Keyboard Text Selection #4993

Open

DHowett requested changes Jun 29, 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 29, 2022

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

address Dustin's feedback

ce6c87e

split copy and clear sln; simplify logic

1a84bc6

DHowett reviewed Jun 30, 2022

View reviewed changes

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

remove the bool from _updateSelectionUI

b8b2d76

lhecker approved these changes Jul 1, 2022

View reviewed changes

DHowett approved these changes Jul 1, 2022

View reviewed changes

DHowett merged commit 2cd2351 into main Jul 1, 2022

DHowett deleted the dev/cazamor/kbd-sln/polish branch July 1, 2022 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous bug fixes for Mark Mode #13358

Miscellaneous bug fixes for Mark Mode #13358

carlos-zamora commented Jun 22, 2022 •

edited

Loading

This comment was marked as outdated.

carlos-zamora commented Jun 22, 2022

carlos-zamora left a comment

ofek commented Jun 22, 2022

carlos-zamora commented Jun 22, 2022

ofek commented Jun 22, 2022

carlos-zamora commented Jun 23, 2022

DHowett commented Jun 23, 2022

DHowett commented Jun 23, 2022

ofek commented Jun 23, 2022

zadjii-msft Jun 23, 2022

carlos-zamora Jun 23, 2022

carlos-zamora Jun 28, 2022

DHowett Jun 28, 2022

ofek Jun 28, 2022

DHowett Jun 29, 2022

DHowett Jun 29, 2022

lhecker Jun 30, 2022

lhecker Jul 1, 2022

lhecker Jul 1, 2022

DHowett left a comment

ghost commented Jul 6, 2022

		const auto isInMarkMode = _terminal->SelectionMode() == ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode::Mark;
		if (isInMarkMode && modifiers.IsCtrlPressed() && vkey == 'A')

		// DO NOT call _updateSelectionUI() here.
		// We don't want to show the markers so manually tell it to clear it.

Miscellaneous bug fixes for Mark Mode #13358

Miscellaneous bug fixes for Mark Mode #13358

Conversation

carlos-zamora commented Jun 22, 2022 • edited Loading

Summary of the Pull Request

References

Validation Steps Performed

This comment was marked as outdated.

carlos-zamora commented Jun 22, 2022

Discussion points

carlos-zamora left a comment

Choose a reason for hiding this comment

ofek commented Jun 22, 2022

carlos-zamora commented Jun 22, 2022

ofek commented Jun 22, 2022

carlos-zamora commented Jun 23, 2022

DHowett commented Jun 23, 2022

DHowett commented Jun 23, 2022

ofek commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Jul 6, 2022

carlos-zamora commented Jun 22, 2022 •

edited

Loading