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

Don't copy before pasting in copyOnSelect mode #14635

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Don't copy before pasting in copyOnSelect mode #14635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ec30ef
this is a basic copyOnSelect test, for reference, for starting more t…
zadjii-msft Jan 5, 2023
45b3e8d
some tests that fail, intentionally
zadjii-msft Jan 5, 2023
ca658ba
Fix the bug
zadjii-msft Jan 5, 2023
eaf7215
Merge remote-tracking branch 'origin/main' into dev/migrie/b/14464-co…
zadjii-msft Jan 13, 2023
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
17 changes: 14 additions & 3 deletions src/cascadia/TerminalControl/ControlInteractivity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
else if (WI_IsFlagSet(buttonState, MouseButtonState::IsRightButtonDown))
{
// Try to copy the text and clear the selection
const auto successfulCopy = CopySelectionToClipboard(shiftEnabled, nullptr);
const auto copyOnSelect{ _core->CopyOnSelect() };
bool successfulCopy = false;

// Don't try to copy if we're in copyOnSelect mode and have already
// copied this selection. GH#14464 demonstrates a scenario where the
// buffer contents might have changed since the selection was made,
// and copying here would cause weirdness.
if (_selectionNeedsToBeCopied || !copyOnSelect)
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite get why this depends on CopyOnSelect... When would we ever want to copy text more than once?

{
// Try to copy the text and clear the selection
successfulCopy = CopySelectionToClipboard(shiftEnabled, nullptr);
}
_core->ClearSelection();
if (_core->CopyOnSelect() || !successfulCopy)

if (copyOnSelect || !successfulCopy)
{
// CopyOnSelect: right click always pastes!
// Otherwise: no selection --> paste
Expand Down
182 changes: 182 additions & 0 deletions src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ namespace ControlUnitTests
TEST_METHOD(GetMouseEventsInTest);
TEST_METHOD(AltBufferClampMouse);

TEST_METHOD(CopyOnSelectSimple);
TEST_METHOD(CopyOnSelectAltBuffer);

TEST_CLASS_SETUP(ClassSetup)
{
winrt::init_apartment(winrt::apartment_type::single_threaded);
Expand Down Expand Up @@ -966,4 +969,183 @@ namespace ControlUnitTests
cursorPosition1.to_core_point());
VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output");
}

void ControlInteractivityTests::CopyOnSelectSimple()
{
auto [settings, conn] = _createSettingsAndConnection();
settings->CopyOnSelect(true);
auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn);
_standardInit(core, interactivity);
auto& term{ *core->_terminal };

// Output some text
for (auto i = 0; i < core->ViewHeight() / 2; ++i)
{
conn->WriteInput(winrt::hstring{ fmt::format(L"line {}\r\n", i) });
}

// A callback for checking Copy events. expectedCopyContents=nullopt
// indicates that we're not expecting any copy events.
std::optional<std::wstring> expectedCopyContents{ std::nullopt };
core->CopyToClipboard([&](auto&&, auto& args) {
VERIFY_IS_TRUE(expectedCopyContents.has_value());
const std::wstring expected{ expectedCopyContents->c_str() };
const std::wstring actual{ args.Text().c_str() };
VERIFY_ARE_EQUAL(expected, actual);
});
bool expectedPaste = false;
interactivity->PasteFromClipboard([&](auto&&, auto&&) {
VERIFY_IS_TRUE(expectedPaste);
});

const auto originalViewport{ term.GetViewport() };
VERIFY_ARE_EQUAL(originalViewport.Width(), 30);

// For this test, don't use any modifiers
const auto modifiers = ControlKeyStates();
const auto leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown };
const auto rightMouseDown{ Control::MouseButtonState::IsRightButtonDown };
const Control::MouseButtonState noMouseDown{};
const til::size fontSize{ 9, 21 };
const til::point terminalPosition0{ 0, 5 };
const til::point terminalPosition1{ 5, 5 };
const auto cursorPosition0{ terminalPosition0 * fontSize };
const auto cursorPosition1{ terminalPosition1 * fontSize };

Log::Comment(L" --- Click on the terminal at 0,5 ---");

interactivity->PointerPressed(leftMouseDown,
WM_LBUTTONDOWN, //pointerUpdateKind
0, // timestamp
modifiers,
cursorPosition0.to_core_point());
VERIFY_IS_FALSE(core->HasSelection());

Log::Comment(L" --- Drag to 5,5 ---");

interactivity->PointerMoved(leftMouseDown,
WM_LBUTTONDOWN, //pointerUpdateKind
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true); // pointerPressedInBounds
VERIFY_IS_TRUE(core->HasSelection());
// expectedCopyContents being nullopt here will ensure that we don't send the copy event till the pointer released

Log::Comment(L" --- Release the mouse --- ");
expectedCopyContents = L"line 5";
interactivity->PointerReleased(noMouseDown,
WM_LBUTTONUP, //pointerUpdateKind
modifiers,
cursorPosition1.to_core_point());
VERIFY_IS_TRUE(core->HasSelection());

Log::Comment(L" --- Right-click to paste --- ");
// Note from GH#14464: we don't want to copy _again_ at this point. The
// copy occured when the selection was made, we shouldn't stealth-update
// the clipboard again.
expectedCopyContents = std::nullopt;
expectedPaste = true;
interactivity->PointerPressed(rightMouseDown,
WM_RBUTTONDOWN, //pointerUpdateKind
0, // timestamp
modifiers,
cursorPosition1.to_core_point());
VERIFY_IS_FALSE(core->HasSelection());
}

void ControlInteractivityTests::CopyOnSelectAltBuffer()
{
// This test was inspired by GH#14464. Ultimately, it's similar to the
// CopyOnSelectSimple, just with an alt buffer, and outputting text
// after the selection was made.
auto [settings, conn] = _createSettingsAndConnection();
settings->CopyOnSelect(true);
auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn);
_standardInit(core, interactivity);
auto& term{ *core->_terminal };

Log::Comment(L" --- Switch to alt buffer ---");
term.Write(L"\x1b[?1049h");
auto returnToMain = wil::scope_exit([&]() { term.Write(L"\x1b[?1049h"); });

// Output some text
auto i = 0;
for (i = 0; i < core->ViewHeight() - 1; ++i)
{
conn->WriteInput(winrt::hstring{ fmt::format(L"line {}\r\n", i) });
}

// A callback for checking Copy events. expectedCopyContents=nullopt
// indicates that we're not expecting any copy events.
std::optional<std::wstring> expectedCopyContents{ std::nullopt };
core->CopyToClipboard([&](auto&&, auto& args) {
VERIFY_IS_TRUE(expectedCopyContents.has_value());
const std::wstring expected{ expectedCopyContents->c_str() };
const std::wstring actual{ args.Text().c_str() };
VERIFY_ARE_EQUAL(expected, actual);
});
bool expectedPaste = false;
interactivity->PasteFromClipboard([&](auto&&, auto&&) {
VERIFY_IS_TRUE(expectedPaste);
});

const auto originalViewport{ term.GetViewport() };
VERIFY_ARE_EQUAL(originalViewport.Width(), 30);

// For this test, don't use any modifiers
const auto modifiers = ControlKeyStates();
const auto leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown };
const auto rightMouseDown{ Control::MouseButtonState::IsRightButtonDown };
const Control::MouseButtonState noMouseDown{};
const til::size fontSize{ 9, 21 };
const til::point terminalPosition0{ 0, 5 };
const til::point terminalPosition1{ 5, 5 };
const auto cursorPosition0{ terminalPosition0 * fontSize };
const auto cursorPosition1{ terminalPosition1 * fontSize };

Log::Comment(L" --- Click on the terminal at 0,5 ---");

interactivity->PointerPressed(leftMouseDown,
WM_LBUTTONDOWN, //pointerUpdateKind
0, // timestamp
modifiers,
cursorPosition0.to_core_point());
VERIFY_IS_FALSE(core->HasSelection());

Log::Comment(L" --- Drag to 5,5 ---");

interactivity->PointerMoved(leftMouseDown,
WM_LBUTTONDOWN, //pointerUpdateKind
modifiers,
true, // focused,
cursorPosition1.to_core_point(),
true); // pointerPressedInBounds
VERIFY_IS_TRUE(core->HasSelection());
// expectedCopyContents being nullopt here will ensure that we don't send the copy event till the pointer released

Log::Comment(L" --- Release the mouse --- ");
expectedCopyContents = L"line 5";
interactivity->PointerReleased(noMouseDown,
WM_LBUTTONUP, //pointerUpdateKind
modifiers,
cursorPosition1.to_core_point());
VERIFY_IS_TRUE(core->HasSelection());

// Output some text
for (; i < 5; ++i)
{
conn->WriteInput(winrt::hstring{ fmt::format(L"line {}\r\n", i) });
}

Log::Comment(L" --- Right-click to paste --- ");
expectedCopyContents = std::nullopt;
expectedPaste = true;
interactivity->PointerPressed(rightMouseDown,
WM_RBUTTONDOWN, //pointerUpdateKind
0, // timestamp
modifiers,
cursorPosition1.to_core_point());
VERIFY_IS_FALSE(core->HasSelection());
}
}