Skip to content

Commit

Permalink
Miscellaneous bug fixes for Mark Mode (#13358)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
carlos-zamora authored Jul 1, 2022
1 parent 843e61a commit 2cd2351
Show file tree
Hide file tree
Showing 10 changed files with 146 additions and 89 deletions.
1 change: 1 addition & 0 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@
"switchToTab",
"tabSearch",
"toggleAlwaysOnTop",
"toggleBlockSelection",
"toggleFocusMode",
"selectAll",
"setFocusMode",
Expand Down
58 changes: 29 additions & 29 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -417,11 +417,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
vkey != VK_SNAPSHOT &&
keyDown)
{
if (_terminal->IsInMarkMode() && modifiers.IsCtrlPressed() && vkey == 'A')
const auto isInMarkMode = _terminal->SelectionMode() == ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode::Mark;
if (isInMarkMode && modifiers.IsCtrlPressed() && vkey == 'A')
{
auto lock = _terminal->LockForWriting();
_terminal->SelectAll();
_updateSelection();
_updateSelectionUI();
return true;
}

Expand All @@ -430,15 +431,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
auto lock = _terminal->LockForWriting();
_terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second, modifiers);
_updateSelection();
_updateSelectionUI();
return true;
}

// GH#8791 - don't dismiss selection if Windows key was also pressed as a key-combination.
if (!modifiers.IsWinPressed())
{
_terminal->ClearSelection();
_updateSelection();
_updateSelectionUI();
}

// When there is a selection active, escape should clear it and NOT flow through
Expand Down Expand Up @@ -950,8 +951,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const auto end{ _terminal->SelectionEndForRendering() };
info.EndPos = { end.X, end.Y };

info.MovingEnd = _terminal->MovingEnd();
info.MovingCursor = _terminal->MovingCursor();
info.Endpoint = static_cast<SelectionEndpointTarget>(_terminal->SelectionEndpointTarget());

const auto bufferSize{ _terminal->GetTextBuffer().GetSize() };
info.StartAtLeftBoundary = _terminal->GetSelectionAnchor().x == bufferSize.Left();
Expand Down Expand Up @@ -981,11 +981,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

// save location (for rendering) + render
_terminal->SetSelectionEnd(terminalPosition);
_renderer->TriggerSelection();

// this is used for mouse dragging,
// so hide the markers
_UpdateSelectionMarkersHandlers(*this, winrt::make<implementation::UpdateSelectionMarkersEventArgs>(true));
_updateSelectionUI();
}

// Called when the Terminal wants to set something to the clipboard, i.e.
Expand All @@ -998,7 +994,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Method Description:
// - Given a copy-able selection, get the selected text from the buffer and send it to the
// Windows Clipboard (CascadiaWin32:main.cpp).
// - CopyOnSelect does NOT clear the selection
// Arguments:
// - singleLine: collapse all of the text to one line
// - formats: which formats to copy (defined by action's CopyFormatting arg). nullptr
Expand Down Expand Up @@ -1044,12 +1039,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
bgColor) :
"";

if (!_settings->CopyOnSelect())
{
_terminal->ClearSelection();
_updateSelection();
}

// send data up for clipboard
_CopyToClipboardHandlers(*this,
winrt::make<CopyToClipboardEventArgs>(winrt::hstring{ textData },
Expand All @@ -1063,7 +1052,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
auto lock = _terminal->LockForWriting();
_terminal->SelectAll();
_updateSelection();
_updateSelectionUI();
}

void ControlCore::ClearSelection()
{
auto lock = _terminal->LockForWriting();
_terminal->ClearSelection();
_updateSelectionUI();
}

bool ControlCore::ToggleBlockSelection()
Expand All @@ -1085,12 +1081,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
auto lock = _terminal->LockForWriting();
_terminal->ToggleMarkMode();
_updateSelection();
_updateSelectionUI();
}

bool ControlCore::IsInMarkMode() const
Control::SelectionInteractionMode ControlCore::SelectionMode() const
{
return _terminal->IsInMarkMode();
return static_cast<Control::SelectionInteractionMode>(_terminal->SelectionMode());
}

// Method Description:
Expand All @@ -1100,7 +1096,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
_terminal->WritePastedText(hstr);
_terminal->ClearSelection();
_updateSelection();
_updateSelectionUI();
_terminal->TrySnapOnInput();
}

Expand Down Expand Up @@ -1420,10 +1416,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
_terminal->SetBlockSelection(false);
search.Select();
_renderer->TriggerSelection();

// this is used for search,
// so hide the markers
// DO NOT call _updateSelectionUI() here.
// We don't want to show the markers so manually tell it to clear it.
_renderer->TriggerSelection();
_UpdateSelectionMarkersHandlers(*this, winrt::make<implementation::UpdateSelectionMarkersEventArgs>(true));
}

Expand Down Expand Up @@ -1625,14 +1622,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_terminal->MultiClickSelection(terminalPosition, mode);
selectionNeedsToBeCopied = true;
}
_updateSelection();
_updateSelectionUI();
}

void ControlCore::_updateSelection()
// Method Description:
// - Updates the renderer's representation of the selection as well as the selection marker overlay in TermControl
void ControlCore::_updateSelectionUI()
{
_renderer->TriggerSelection();
const bool clearMarkers{ !_terminal->IsSelectionActive() };
_UpdateSelectionMarkersHandlers(*this, winrt::make<implementation::UpdateSelectionMarkersEventArgs>(clearMarkers));
// only show the markers if we're doing a keyboard selection or in mark mode
const bool showMarkers{ _terminal->SelectionMode() >= ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode::Keyboard };
_UpdateSelectionMarkersHandlers(*this, winrt::make<implementation::UpdateSelectionMarkersEventArgs>(!showMarkers));
}

void ControlCore::AttachUiaEngine(::Microsoft::Console::Render::IRenderEngine* const pEngine)
Expand Down
5 changes: 3 additions & 2 deletions src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void PasteText(const winrt::hstring& hstr);
bool CopySelectionToClipboard(bool singleLine, const Windows::Foundation::IReference<CopyFormat>& formats);
void SelectAll();
void ClearSelection();
bool ToggleBlockSelection();
void ToggleMarkMode();
bool IsInMarkMode() const;
Control::SelectionInteractionMode SelectionMode() const;

void GotFocus();
void LostFocus();
Expand Down Expand Up @@ -271,7 +272,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
bool _setFontSizeUnderLock(int fontSize);
void _updateFont(const bool initialUpdate = false);
void _refreshSizeUnderLock();
void _updateSelection();
void _updateSelectionUI();

void _sendInputToConnection(std::wstring_view wstr);

Expand Down
21 changes: 18 additions & 3 deletions src/cascadia/TerminalControl/ControlCore.idl
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,26 @@ namespace Microsoft.Terminal.Control
All
};

enum SelectionInteractionMode
{
None,
Mouse,
Keyboard,
Mark
};

[flags]
enum SelectionEndpointTarget
{
Start = 0x1,
End = 0x2
};

struct SelectionData
{
Microsoft.Terminal.Core.Point StartPos;
Microsoft.Terminal.Core.Point EndPos;
Boolean MovingEnd;
Boolean MovingCursor;
SelectionEndpointTarget Endpoint;
Boolean StartAtLeftBoundary;
Boolean EndAtRightBoundary;
};
Expand Down Expand Up @@ -75,10 +89,10 @@ namespace Microsoft.Terminal.Control
void SendInput(String text);
void PasteText(String text);
void SelectAll();
void ClearSelection();
Boolean ToggleBlockSelection();
void ToggleMarkMode();
void ClearBuffer(ClearBufferType clearType);
Boolean IsInMarkMode();

void SetHoveredCell(Microsoft.Terminal.Core.Point terminalPosition);
void ClearHoveredCell();
Expand All @@ -101,6 +115,7 @@ namespace Microsoft.Terminal.Control
Boolean HasSelection { get; };
IVector<String> SelectedText(Boolean trimTrailingWhitespace);
SelectionData SelectionInfo { get; };
SelectionInteractionMode SelectionMode();

String HoveredUriText { get; };
Windows.Foundation.IReference<Microsoft.Terminal.Core.Point> HoveredCell { get; };
Expand Down
16 changes: 9 additions & 7 deletions src/cascadia/TerminalControl/ControlInteractivity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Method Description:
// - Given a copy-able selection, get the selected text from the buffer and send it to the
// Windows Clipboard (CascadiaWin32:main.cpp).
// - CopyOnSelect does NOT clear the selection
// Arguments:
// - singleLine: collapse all of the text to one line
// - formats: which formats to copy (defined by action's CopyFormatting arg). nullptr
Expand Down Expand Up @@ -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
const auto successfulCopy = CopySelectionToClipboard(shiftEnabled, nullptr);
_core->ClearSelection();
if (_core->CopyOnSelect() || !successfulCopy)
{
// CopyOnSelect: right click always pastes!
// Otherwise: no selection --> paste
RequestPasteTextFromClipboard();
}
else
{
CopySelectionToClipboard(shiftEnabled, nullptr);
}
}
}

Expand Down Expand Up @@ -383,6 +382,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
isLeftMouseRelease &&
_selectionNeedsToBeCopied)
{
// IMPORTANT!
// DO NOT clear the selection here!
// Otherwise, the selection will be cleared immediately after you make it.
CopySelectionToClipboard(false, nullptr);
}

Expand Down
19 changes: 11 additions & 8 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1188,7 +1188,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
// Manually show the cursor when a key is pressed. Restarting
// the timer prevents flickering.
_core.CursorOn(!_core.IsInMarkMode());
_core.CursorOn(_core.SelectionMode() != SelectionInteractionMode::Mark);
_cursorTimer->Start();
}

Expand Down Expand Up @@ -1659,7 +1659,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (_cursorTimer)
{
// When the terminal focuses, show the cursor immediately
_core.CursorOn(!_core.IsInMarkMode());
_core.CursorOn(_core.SelectionMode() != SelectionInteractionMode::Mark);
_cursorTimer->Start();
}

Expand Down Expand Up @@ -1902,7 +1902,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return false;
}

return _interactivity.CopySelectionToClipboard(singleLine, formats);
const auto successfulCopy = _interactivity.CopySelectionToClipboard(singleLine, formats);
_core.ClearSelection();
return successfulCopy;
}

// Method Description:
Expand Down Expand Up @@ -2830,9 +2832,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation

// show/update selection markers
// figure out which endpoint to move, get it and the relevant icon (hide the other icon)
const auto selectionAnchor{ markerData.MovingEnd ? markerData.EndPos : markerData.StartPos };
const auto& marker{ markerData.MovingEnd ? SelectionEndMarker() : SelectionStartMarker() };
const auto& otherMarker{ markerData.MovingEnd ? SelectionStartMarker() : SelectionEndMarker() };
const auto movingEnd{ WI_IsFlagSet(markerData.Endpoint, SelectionEndpointTarget::End) };
const auto selectionAnchor{ movingEnd ? markerData.EndPos : markerData.StartPos };
const auto& marker{ movingEnd ? SelectionEndMarker() : SelectionStartMarker() };
const auto& otherMarker{ movingEnd ? SelectionStartMarker() : SelectionEndMarker() };
if (selectionAnchor.Y < 0 || selectionAnchor.Y >= _core.ViewHeight())
{
// if the endpoint is outside of the viewport,
Expand All @@ -2841,7 +2844,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
otherMarker.Visibility(Visibility::Collapsed);
co_return;
}
else if (markerData.MovingCursor)
else if (WI_AreAllFlagsSet(markerData.Endpoint, SelectionEndpointTarget::Start | SelectionEndpointTarget::End))
{
// display both markers
displayMarker(true);
Expand All @@ -2851,7 +2854,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
// display one marker,
// but hide the other
displayMarker(markerData.MovingEnd);
displayMarker(movingEnd);
otherMarker.Visibility(Visibility::Collapsed);
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ Terminal::Terminal() :
_snapOnInput{ true },
_altGrAliasing{ true },
_blockSelection{ false },
_markMode{ false },
_selectionMode{ SelectionInteractionMode::None },
_selection{ std::nullopt },
_selectionEndpoint{ static_cast<SelectionEndpoint>(0) },
_taskbarState{ 0 },
_taskbarProgress{ 0 },
_trimBlockSelection{ false },
Expand Down Expand Up @@ -1369,7 +1370,7 @@ void Terminal::SetCursorOn(const bool isOn)
bool Terminal::IsCursorBlinkingAllowed() const noexcept
{
const auto& cursor = _activeBuffer().GetCursor();
return !_markMode && cursor.IsBlinkingAllowed();
return _selectionMode != SelectionInteractionMode::Mark && cursor.IsBlinkingAllowed();
}

// Method Description:
Expand Down
Loading

0 comments on commit 2cd2351

Please sign in to comment.