Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.14] Fix keyboard selection and copyOnSelect interaction #13360

Merged
merged 4 commits into from
Jun 30, 2022
Merged

[1.14] Fix keyboard selection and copyOnSelect interaction #13360

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0b0a6c
[1.14] Fix keyboard selection and copyOnSelect interaction
carlos-zamora Jun 22, 2022
c4f37fc
remove const
carlos-zamora Jun 23, 2022
d9cfe31
Merge branch 'release-1.14' into dev/cazamor/1.14/bugfix-copyOnSelect
carlos-zamora Jun 30, 2022
9dc99a9
Update src/cascadia/TerminalCore/TerminalSelection.cpp
DHowett Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,13 +933,14 @@ 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
// if we should defer which formats are copied to the global setting
// - clearSelection: if true, clear the selection. Used for CopyOnSelect.
bool ControlCore::CopySelectionToClipboard(bool singleLine,
const Windows::Foundation::IReference<CopyFormat>& formats)
const Windows::Foundation::IReference<CopyFormat>& formats,
bool clearSelection)
{
// no selection --> nothing to copy
if (!_terminal->IsSelectionActive())
Expand Down Expand Up @@ -979,7 +980,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
bgColor) :
"";

if (!_settings->CopyOnSelect())
if (clearSelection)
{
_terminal->ClearSelection();
_renderer->TriggerSelection();
Expand All @@ -1001,6 +1002,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_renderer->TriggerSelection();
}

const bool ControlCore::IsInQuickEditMode() const noexcept
{
return _terminal->IsInQuickEditMode();
}

// Method Description:
// - Pre-process text pasted (presumably from the clipboard)
// before sending it over the terminal's connection.
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalControl/ControlCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void SendInput(const winrt::hstring& wstr);
void PasteText(const winrt::hstring& hstr);
bool CopySelectionToClipboard(bool singleLine, const Windows::Foundation::IReference<CopyFormat>& formats);
bool CopySelectionToClipboard(bool singleLine, const Windows::Foundation::IReference<CopyFormat>& formats, bool clearSelection = true);
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
void SelectAll();
const bool IsInQuickEditMode() const noexcept;

void GotFocus();
void LostFocus();
Expand Down
30 changes: 23 additions & 7 deletions src/cascadia/TerminalControl/ControlInteractivity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,14 @@ 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
// if we should defer which formats are copied to the global setting
// - clearSelection: if true, clear the selection after copying it. Used for CopyOnSelect.
bool ControlInteractivity::CopySelectionToClipboard(bool singleLine,
const Windows::Foundation::IReference<CopyFormat>& formats)
const Windows::Foundation::IReference<CopyFormat>& formats,
bool clearSelection)
{
if (_core)
{
Expand All @@ -164,7 +165,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Mark the current selection as copied
_selectionNeedsToBeCopied = false;

return _core->CopySelectionToClipboard(singleLine, formats);
return _core->CopySelectionToClipboard(singleLine, formats, clearSelection);
}

return false;
Expand Down Expand Up @@ -257,15 +258,27 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
else if (WI_IsFlagSet(buttonState, MouseButtonState::IsRightButtonDown))
{
// CopyOnSelect right click always pastes
if (_core->CopyOnSelect() || !_core->HasSelection())
if (_core->CopyOnSelect())
{
// CopyOnSelect:
// 1. keyboard selection? --> copy the new content first
// 2. right click always pastes!
if (_core->IsInQuickEditMode())
{
CopySelectionToClipboard(shiftEnabled, nullptr);
Copy link
Member

@lhecker lhecker Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're CopyOnSelect() == true wouldn't we have already copied the selection at this point in ControlInteractivity::PointerReleased?

When is IsInQuickEditMode() == false? Whenever we do a keyboard selection? Would IsInMouseSelectionMode (or inversely IsInKeyboardSelectionMode) not be a better (more descriptive and modern) function name then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're CopyOnSelect() == true wouldn't we have already copied the selection at this point in ControlInteractivity::PointerReleased?

Yes and no.
Yes, because that covers the scenario of (1) create a mouse selection then (2) releasing the mouse button. The content immediately gets copied to your clipboard. When you right-click, you paste that content, so this code isn't called at all because we're not in "quick edit mode".

No, because "quick edit mode" is specifically when you add these steps to the scenario above... (3) shift+right to extend the selection a bit. Then, when you right-click, we already copied the wrong contents! We copied the data when you let go, not when you're right-clicking. So this extra code just detects when you've done step 3 and we need to copy the contents again.

When is IsInQuickEditMode() == false? Whenever we do a keyboard selection? Would IsInMouseSelectionMode (or inversely IsInKeyboardSelectionMode) not be a better (more descriptive and modern) function name then?

QuickEditMode == true --> you made a selection with the mouse, then modified it with the keyboard
MarkMode == true --> you toggled mark mode manually (created a selection at the cursor)

They're mutually exclusive. After a quick chat with Dustin, here's a new approach...

enum class SelectionMode
{
   Mouse = 0,
   Keyboard,
   Mark
};

We just have 3 modes in increasing order. All mutually exclusive, but all useful to know when to show the selection markers and what trickery could occur. That also lets me combine IsInMarkMode and IsInQuickEditMode. AND it also gets rid of the historical term of "quick edit mode".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this approach only applies to 1.15+

}
RequestPasteTextFromClipboard();
}
else
else if (_core->HasSelection())
{
// copy selected text
CopySelectionToClipboard(shiftEnabled, nullptr);
}
else
{
// no selection --> paste
RequestPasteTextFromClipboard();
}
}
}

Expand Down Expand Up @@ -383,7 +396,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
isLeftMouseRelease &&
_selectionNeedsToBeCopied)
{
CopySelectionToClipboard(false, nullptr);
// IMPORTANT!
// Set clearSelection to false here!
// Otherwise, the selection will be cleared immediately after you make it.
CopySelectionToClipboard(false, nullptr, /*clearSelection*/ false);
}

_singleClickTouchdownPos = std::nullopt;
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalControl/ControlInteractivity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
#pragma endregion

bool CopySelectionToClipboard(bool singleLine,
const Windows::Foundation::IReference<CopyFormat>& formats);
const Windows::Foundation::IReference<CopyFormat>& formats,
bool clearSelection = true);
void RequestPasteTextFromClipboard();
void SetEndSelectionPoint(const Core::Point pixelPosition);
bool ManglePathsForWsl();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Terminal::Terminal() :
_snapOnInput{ true },
_altGrAliasing{ true },
_blockSelection{ false },
_quickEditMode{ false },
_selection{ std::nullopt },
_taskbarState{ 0 },
_taskbarProgress{ 0 },
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalCore/Terminal.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ class Microsoft::Terminal::Core::Terminal final :
void SetBlockSelection(const bool isEnabled) noexcept;
void UpdateSelection(SelectionDirection direction, SelectionExpansion mode);
void SelectAll();
const bool IsInQuickEditMode() const noexcept;
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved

using UpdateSelectionParams = std::optional<std::pair<SelectionDirection, SelectionExpansion>>;
static UpdateSelectionParams ConvertKeyEventToUpdateSelectionParams(const ControlKeyStates mods, const WORD vkey);
Expand Down Expand Up @@ -313,6 +314,7 @@ class Microsoft::Terminal::Core::Terminal final :
bool _blockSelection;
std::wstring _wordDelimiters;
SelectionExpansion _multiClickSelectionMode;
bool _quickEditMode;
#pragma endregion

std::unique_ptr<TextBuffer> _mainBuffer;
Expand Down
9 changes: 8 additions & 1 deletion src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ const bool Terminal::IsBlockSelection() const noexcept
return _blockSelection;
}

const bool Terminal::IsInQuickEditMode() const noexcept
{
return _quickEditMode;
}

// Method Description:
// - Perform a multi-click selection at viewportPos expanding according to the expansionMode
// Arguments:
Expand Down Expand Up @@ -314,9 +319,10 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion
targetPos = bottomRightInclusive;
}

// 3. Actually modify the selection
// 3. Actually modify the selection state
// NOTE: targetStart doesn't matter here
auto targetStart = false;
_quickEditMode = true;
std::tie(_selection->start, _selection->end) = _PivotSelection(targetPos, targetStart);

// 4. Scroll (if necessary)
Expand Down Expand Up @@ -482,6 +488,7 @@ void Terminal::_MoveByBuffer(SelectionDirection direction, COORD& pos)
void Terminal::ClearSelection()
{
_selection = std::nullopt;
_quickEditMode = false;
}

// Method Description:
Expand Down