Repository: microsoft/terminal (language: cpp)

Issue:
Mark Mode should preempt key bindings, and work with anything that causes scrolling...
### Windows Terminal version

1.15.1862.0

### Windows build number

10.0.19042.0

### Other Software

_No response_

### Steps to reproduce

I know the mark mode keyboard selection is still very early days. I just wanted to make sure the expected behaviors are all covered, and I didn't see this called out.

Any hotkey that pre-exists the mark mode currently interfere with selection. That is, currently:

- Mark mode seems to only works with the default windows text navigation keys: the arrows, PgUp/PgDn, home/end.
- **Selecting** requires turning on mark mode and then holding shift while pressing those keys.

If the user has an "Action" configured for `Ctrl+Shift+Home` or `Ctrl+Shift+PgUp` or whatever, that action will trigger **instead of** the selection.

Additionally, nothing else that scrolls the buffer causes the mark to move or select:

- Scrolling with the mouse
- Scrolling with scrollToMark
- Scrolling via the command palette (Ctrl+Shift+P)

On top of that, we can't **usefully** turn on mark mode _after_ scrolling, because it always appears at the bottom.

### Expected Behavior

Mark mode should probably be called "selection" mode, to disambiguate with the new "marks" ...

Mark mode should disregard actions that consume Arrows/PgUp/PgDn/Home/End  -- or possibly, should ignore all actions except selection-specific actions or Esc to cancel, changing all the hotkeys like a `vi` mode does.

The "mark" should follow the scroll buffer if the buffer is scrolled via other actions. Ideally, it should select when that happens with SHIFT pressed. 

The mark should always appear _within the visible buffer_ when it's invoked, **not** only at the bottom/prompt.

### Actual Behavior

The mark only ever appears at the prompt, and can only be used if you haven't configured actions over the default text selection keys.


Discussion / hints:
There's a lot to unpack here, I'll make sure this is at the top of the discussion order ~this afternoon~ EDIT: tomorrow due to some oofages. Thanks!
quick discussion notes:

> Mark mode should probably be called "selection" mode, to disambiguate with the new "marks" ...

* meh, I think we're more attached to calling this "Mark Mode" than we are to calling the scrollbar marks "marks".

> Mark mode should disregard actions that consume Arrows/PgUp/PgDn/Home/End -- or possibly, should ignore all actions except selection-specific actions or Esc to cancel, changing all the hotkeys like a vi mode does.

* yes - we think we have an issue for this. This gets more complicated with things like nextMatch/prevMatch, find in general. We'll noodle on this one. 

> The "mark" should follow the scroll buffer if the buffer is scrolled via other actions. Ideally, it should select when that happens with SHIFT pressed.

* maybe not? 

> The mark should always appear within the visible buffer when it's invoked, not only at the bottom/prompt.

* for sure

over to @carlos-zamora 
Ok, I added this to the main epic #4993. I also have a few notes on a few of these:

> Mark mode should disregard actions that consume Arrows/PgUp/PgDn/Home/End -- or possibly, should ignore all actions except selection-specific actions or Esc to cancel, changing all the hotkeys like a vi mode does.

I was wrong. No issue tracking this. So congrats, this issue is now tracking that haha.

> The "mark" should follow the scroll buffer if the buffer is scrolled via other actions. Ideally, it should select when that happens with SHIFT pressed.

_This one_ has an issue: #4985. I closed it because selecting by viewport automatically scrolls, which mostly satisfies the need. _That said_ since this is the second time it's been asked for (especially after mark mode is available), I'll reopen it.

Fix patch:
diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index 2ed9f2d7efe..d39393fb900 100644
--- a/src/cascadia/TerminalControl/ControlCore.cpp
+++ b/src/cascadia/TerminalControl/ControlCore.cpp
@@ -389,6 +389,72 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         return _terminal->SendCharEvent(ch, scanCode, modifiers);
     }
 
+    bool ControlCore::_shouldTryUpdateSelection(const WORD vkey)
+    {
+        // GH#6423 - don't update selection if the key that was pressed was a
+        // modifier key. We'll wait for a real keystroke to dismiss the
+        // GH #7395 - don't update selection when taking PrintScreen
+        // selection.
+        return HasSelection() && !KeyEvent::IsModifierKey(vkey) && vkey != VK_SNAPSHOT;
+    }
+
+    bool ControlCore::TryMarkModeKeybinding(const WORD vkey,
+                                            const ::Microsoft::Terminal::Core::ControlKeyStates mods)
+    {
+        if (_shouldTryUpdateSelection(vkey) && _terminal->SelectionMode() == ::Terminal::SelectionInteractionMode::Mark)
+        {
+            if (vkey == 'A' && !mods.IsAltPressed() && !mods.IsShiftPressed() && mods.IsCtrlPressed())
+            {
+                // Ctrl + A --> Select all
+                auto lock = _terminal->LockForWriting();
+                _terminal->SelectAll();
+                _updateSelectionUI();
+                return true;
+            }
+            else if (vkey == VK_TAB && !mods.IsAltPressed() && !mods.IsCtrlPressed() && _settings->DetectURLs())
+            {
+                // [Shift +] Tab --> next/previous hyperlink
+                auto lock = _terminal->LockForWriting();
+                const auto direction = mods.IsShiftPressed() ? ::Terminal::SearchDirection::Backward : ::Terminal::SearchDirection::Forward;
+                _terminal->SelectHyperlink(direction);
+                _updateSelectionUI();
+                return true;
+            }
+            else if (vkey == VK_RETURN && mods.IsCtrlPressed() && _terminal->IsTargetingUrl())
+            {
+                // Ctrl + Enter --> Open URL
+                auto lock = _terminal->LockForReading();
+                const auto uri = _terminal->GetHyperlinkAtBufferPosition(_terminal->GetSelectionAnchor());
+                _OpenHyperlinkHandlers(*this, winrt::make<OpenHyperlinkEventArgs>(winrt::hstring{ uri }));
+                return true;
+            }
+            else if (vkey == VK_RETURN && !mods.IsCtrlPressed() && !mods.IsAltPressed())
+            {
+                // [Shift +] Enter --> copy text
+                // Don't lock here! CopySelectionToClipboard already locks for you!
+                CopySelectionToClipboard(mods.IsShiftPressed(), nullptr);
+                _terminal->ClearSelection();
+                _updateSelectionUI();
+                return true;
+            }
+            else if (vkey == VK_ESCAPE)
+            {
+                _terminal->ClearSelection();
+                _updateSelectionUI();
+                return true;
+            }
+            else if (const auto updateSlnParams{ _terminal->ConvertKeyEventToUpdateSelectionParams(mods, vkey) })
+            {
+                // try to update the selection
+                auto lock = _terminal->LockForWriting();
+                _terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second, mods);
+                _updateSelectionUI();
+                return true;
+            }
+        }
+        return false;
+    }
+
     // Method Description:
     // - Send this particular key event to the terminal.
     //   See Terminal::SendKeyEvent for more information.
@@ -405,57 +471,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                                       const bool keyDown)
     {
         // Update the selection, if it's present
-        // GH#6423 - don't dismiss selection if the key that was pressed was a
-        // modifier key. We'll wait for a real keystroke to dismiss the
-        // GH #7395 - don't dismiss selection when taking PrintScreen
-        // selection.
         // GH#8522, GH#3758 - Only modify the selection on key _down_. If we
         // modify on key up, then there's chance that we'll immediately dismiss
         // a selection created by an action bound to a keydown.
-        if (HasSelection() &&
-            !KeyEvent::IsModifierKey(vkey) &&
-            vkey != VK_SNAPSHOT &&
-            keyDown)
+        if (_shouldTryUpdateSelection(vkey) && keyDown)
         {
-            const auto isInMarkMode = _terminal->SelectionMode() == ::Terminal::SelectionInteractionMode::Mark;
-            if (isInMarkMode)
-            {
-                if (modifiers.IsCtrlPressed() && vkey == 'A')
-                {
-                    // Ctrl + A --> Select all
-                    auto lock = _terminal->LockForWriting();
-                    _terminal->SelectAll();
-                    _updateSelectionUI();
-                    return true;
-                }
-                else if (vkey == VK_TAB && _settings->DetectURLs())
-                {
-                    // [Shift +] Tab --> next/previous hyperlink
-                    auto lock = _terminal->LockForWriting();
-                    const auto direction = modifiers.IsShiftPressed() ? ::Terminal::SearchDirection::Backward : ::Terminal::SearchDirection::Forward;
-                    _terminal->SelectHyperlink(direction);
-                    _updateSelectionUI();
-                    return true;
-                }
-                else if (vkey == VK_RETURN && modifiers.IsCtrlPressed() && _terminal->IsTargetingUrl())
-                {
-                    // Ctrl + Enter --> Open URL
-                    auto lock = _terminal->LockForReading();
-                    const auto uri = _terminal->GetHyperlinkAtBufferPosition(_terminal->GetSelectionAnchor());
-                    _OpenHyperlinkHandlers(*this, winrt::make<OpenHyperlinkEventArgs>(winrt::hstring{ uri }));
-                    return true;
-                }
-                else if (vkey == VK_RETURN)
-                {
-                    // [Shift +] Enter --> copy text
-                    // Don't lock here! CopySelectionToClipboard already locks for you!
-                    CopySelectionToClipboard(modifiers.IsShiftPressed(), nullptr);
-                    _terminal->ClearSelection();
-                    _updateSelectionUI();
-                    return true;
-                }
-            }
-
             // try to update the selection
             if (const auto updateSlnParams{ _terminal->ConvertKeyEventToUpdateSelectionParams(modifiers, vkey) })
             {
diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h
index c8ba9b88d7d..5efae0f81d5 100644
--- a/src/cascadia/TerminalControl/ControlCore.h
+++ b/src/cascadia/TerminalControl/ControlCore.h
@@ -87,6 +87,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         void ToggleMarkMode();
         Control::SelectionInteractionMode SelectionMode() const;
         bool SwitchSelectionEndpoint();
+        bool TryMarkModeKeybinding(const WORD vkey,
+                                   const ::Microsoft::Terminal::Core::ControlKeyStates modifiers);
 
         void GotFocus();
         void LostFocus();
@@ -275,6 +277,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         void _updateFont(const bool initialUpdate = false);
         void _refreshSizeUnderLock();
         void _updateSelectionUI();
+        bool _shouldTryUpdateSelection(const WORD vkey);
 
         void _sendInputToConnection(std::wstring_view wstr);
 
diff --git a/src/cascadia/TerminalControl/ControlCore.idl b/src/cascadia/TerminalControl/ControlCore.idl
index ec8230fa180..12ddf683e36 100644
--- a/src/cascadia/TerminalControl/ControlCore.idl
+++ b/src/cascadia/TerminalControl/ControlCore.idl
@@ -14,9 +14,7 @@ namespace Microsoft.Terminal.Control
     // !! LOAD BEARING !! If you make this a struct with Booleans (like they
     // make the most sense as), then the app will crash trying to toss one of
     // these across the process boundary. I haven't the damndest idea why.
-    [flags]
-    enum MouseButtonState
-    {
+    [flags] enum MouseButtonState {
         IsLeftButtonDown = 0x1,
         IsMiddleButtonDown = 0x2,
         IsRightButtonDown = 0x4
@@ -37,9 +35,7 @@ namespace Microsoft.Terminal.Control
         Mark
     };
 
-    [flags]
-    enum SelectionEndpointTarget
-    {
+    [flags] enum SelectionEndpointTarget {
         Start = 0x1,
         End = 0x2
     };
@@ -79,13 +75,15 @@ namespace Microsoft.Terminal.Control
         Double Opacity { get; };
         Boolean UseAcrylic { get; };
 
+        Boolean TryMarkModeKeybinding(Int16 vkey,
+                                      Microsoft.Terminal.Core.ControlKeyStates modifiers);
         Boolean TrySendKeyEvent(Int16 vkey,
-                             Int16 scanCode,
-                             Microsoft.Terminal.Core.ControlKeyStates modifiers,
-                             Boolean keyDown);
+                                Int16 scanCode,
+                                Microsoft.Terminal.Core.ControlKeyStates modifiers,
+                                Boolean keyDown);
         Boolean SendCharEvent(Char ch,
-                           Int16 scanCode,
-                           Microsoft.Terminal.Core.ControlKeyStates modifiers);
+                              Int16 scanCode,
+                              Microsoft.Terminal.Core.ControlKeyStates modifiers);
         void SendInput(String text);
         void PasteText(String text);
         void SelectAll();
diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp
index 943190cfaf2..5519b51a9b0 100644
--- a/src/cascadia/TerminalControl/TermControl.cpp
+++ b/src/cascadia/TerminalControl/TermControl.cpp
@@ -1085,7 +1085,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // keybindings on the keyUp, then we'll still send the keydown to the
         // connected terminal application, and something like ctrl+shift+T will
         // emit a ^T to the pipe.
-        if (!modifiers.IsAltGrPressed() && keyDown && _TryHandleKeyBinding(vkey, scanCode, modifiers))
+        if (!modifiers.IsAltGrPressed() &&
+            keyDown &&
+            _TryHandleKeyBinding(vkey, scanCode, modifiers))
         {
             e.Handled(true);
             return;
@@ -1111,6 +1113,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // - modifiers: The ControlKeyStates representing the modifier key states.
     bool TermControl::_TryHandleKeyBinding(const WORD vkey, const WORD scanCode, ::Microsoft::Terminal::Core::ControlKeyStates modifiers) const
     {
+        // Mark mode has a specific set of pre-defined key bindings.
+        // If we're in mark mode, we should be prioritizing those over
+        // the custom defined key bindings.
+        if (_core.TryMarkModeKeybinding(vkey, modifiers))
+        {
+            return true;
+        }
+
         // TODO: GH#5000
         // The Core owning the keybindings is weird. That's for sure. In the
         // future, we may want to pass the keybindings into the control


Test patch: