From 2ec30ef270788e26e71a504e685f69f397addd18 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Jan 2023 12:01:12 -0600 Subject: [PATCH 1/3] this is a basic copyOnSelect test, for reference, for starting more tests --- .../ControlInteractivityTests.cpp | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index 5a1225d8335..53af702b8bb 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -43,6 +43,8 @@ namespace ControlUnitTests TEST_METHOD(GetMouseEventsInTest); TEST_METHOD(AltBufferClampMouse); + TEST_METHOD(CopyOnSelectSimple); + TEST_CLASS_SETUP(ClassSetup) { winrt::init_apartment(winrt::apartment_type::single_threaded); @@ -966,4 +968,85 @@ namespace ControlUnitTests cursorPosition1.to_core_point()); VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); } + + void ControlInteractivityTests::CopyOnSelectSimple() + { + WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{}; + + 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) }); + } + + std::optional 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); + }); + // Start checking output + // std::deque expectedOutput{}; + // auto validateDrained = _addInputCallback(conn, expectedOutput); + + const auto originalViewport{ term.GetViewport() }; + VERIFY_ARE_EQUAL(originalViewport.Width(), 30); + + // Log::Comment(L" --- Enable mouse mode ---"); + // term.Write(L"\x1b[?1000h"); + + // Log::Comment(L" --- Click on the terminal ---"); + // // Recall: + // // + // // > ! specifies the value 1. The upper left character position on + // // > the terminal is denoted as 1,1 + // // + // // So 5 in our buffer is 32+5+1 = '&' + // expectedOutput.push_back(L"\x1b[M &&"); + // For this test, don't use any modifiers + const auto modifiers = ControlKeyStates(); + const auto leftMouseDown{ Control::MouseButtonState::IsLeftButtonDown }; + 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_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); + 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()); + } } From 45b3e8d4f8649390e43e703d91abd54de7753caf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Jan 2023 13:51:26 -0600 Subject: [PATCH 2/3] some tests that fail, intentionally --- .../ControlInteractivityTests.cpp | 127 +++++++++++++++--- 1 file changed, 110 insertions(+), 17 deletions(-) diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index 53af702b8bb..abd2ca30837 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -44,6 +44,7 @@ namespace ControlUnitTests TEST_METHOD(AltBufferClampMouse); TEST_METHOD(CopyOnSelectSimple); + TEST_METHOD(CopyOnSelectAltBuffer); TEST_CLASS_SETUP(ClassSetup) { @@ -971,8 +972,6 @@ namespace ControlUnitTests void ControlInteractivityTests::CopyOnSelectSimple() { - WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{}; - auto [settings, conn] = _createSettingsAndConnection(); settings->CopyOnSelect(true); auto [core, interactivity] = _createCoreAndInteractivity(*settings, *conn); @@ -985,6 +984,8 @@ namespace ControlUnitTests 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 expectedCopyContents{ std::nullopt }; core->CopyToClipboard([&](auto&&, auto& args) { VERIFY_IS_TRUE(expectedCopyContents.has_value()); @@ -992,27 +993,104 @@ namespace ControlUnitTests const std::wstring actual{ args.Text().c_str() }; VERIFY_ARE_EQUAL(expected, actual); }); - // Start checking output - // std::deque expectedOutput{}; - // auto validateDrained = _addInputCallback(conn, expectedOutput); + 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 --- "); + 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() + { + 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 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); - // Log::Comment(L" --- Enable mouse mode ---"); - // term.Write(L"\x1b[?1000h"); - - // Log::Comment(L" --- Click on the terminal ---"); - // // Recall: - // // - // // > ! specifies the value 1. The upper left character position on - // // > the terminal is denoted as 1,1 - // // - // // So 5 in our buffer is 32+5+1 = '&' - // expectedOutput.push_back(L"\x1b[M &&"); // 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 }; @@ -1027,7 +1105,6 @@ namespace ControlUnitTests 0, // timestamp modifiers, cursorPosition0.to_core_point()); - // VERIFY_ARE_EQUAL(0u, expectedOutput.size(), L"Validate we drained all the expected output"); VERIFY_IS_FALSE(core->HasSelection()); Log::Comment(L" --- Drag to 5,5 ---"); @@ -1048,5 +1125,21 @@ namespace ControlUnitTests 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()); } } From ca658ba331ef641830dabe57f99198e2857197b1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 5 Jan 2023 14:19:28 -0600 Subject: [PATCH 3/3] Fix the bug --- .../TerminalControl/ControlInteractivity.cpp | 17 ++++++++++++++--- .../ControlInteractivityTests.cpp | 6 ++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 7eaf4275535..8521d6e835c 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -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) + { + // 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 diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index abd2ca30837..f9b018f4d62 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -1041,6 +1041,9 @@ namespace ControlUnitTests 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, @@ -1053,6 +1056,9 @@ namespace ControlUnitTests 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);