From 0008fc6b45827fbfc0c5e5825ee2a538ee0283a3 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Tue, 22 Sep 2020 12:23:00 -0700 Subject: [PATCH 1/3] Hash the URI as part of the hyperlink ID It turns out that we missed part of the OSC 8 spec which indicated that _hyperlinks with the same ID but different URIs are logically distinct._ > Character cells that have the same target URI and the same nonempty id > are always underlined together on mouseover. > The same id is only used for connecting character cells whose URIs is > also the same. Character cells pointing to different URIs should never > be underlined together when hovering over. This pull request fixes that oversight by appending the (hashed) URI to the generated ID. When Terminal receives one of these links over ConPTY, it will hash the URL a second time and therefore append a second hashed ID. This is taken as an acceptable cost. Fixes #7698 --- src/buffer/out/textBuffer.cpp | 17 +++++---- src/buffer/out/textBuffer.hpp | 2 +- src/cascadia/TerminalCore/TerminalApi.cpp | 2 +- .../TerminalApiTest.cpp | 35 +++++++++++++++++-- src/host/getset.cpp | 2 +- src/host/ut_host/ScreenBufferTests.cpp | 32 +++++++++++++++-- src/host/ut_host/TextBufferTests.cpp | 6 ++-- 7 files changed, 79 insertions(+), 17 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 294813a3f55..cac3718f432 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2279,32 +2279,35 @@ std::wstring TextBuffer::GetHyperlinkUriFromId(uint16_t id) const // - The user-defined id // Return value: // - The internal hyperlink ID -uint16_t TextBuffer::GetHyperlinkId(std::wstring_view params) +uint16_t TextBuffer::GetHyperlinkId(std::wstring_view uri, std::wstring_view id) { - uint16_t id = 0; - if (params.empty()) + uint16_t numericId = 0; + if (id.empty()) { // no custom id specified, return our internal count - id = _currentHyperlinkId; + numericId = _currentHyperlinkId; ++_currentHyperlinkId; } else { // assign _currentHyperlinkId if the custom id does not already exist - const auto result = _hyperlinkCustomIdMap.emplace(params, _currentHyperlinkId); + std::wstring newId{ id }; + // hash the URL and add it to the custom ID - GH#7698 + newId += L"%" + std::to_wstring(std::hash()(uri)); + const auto result = _hyperlinkCustomIdMap.emplace(newId, _currentHyperlinkId); if (result.second) { // the custom id did not already exist ++_currentHyperlinkId; } - id = (*(result.first)).second; + numericId = (*(result.first)).second; } // _currentHyperlinkId could overflow, make sure its not 0 if (_currentHyperlinkId == 0) { ++_currentHyperlinkId; } - return id; + return numericId; } // Method Description: diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 56a3c59f3b0..c72de24517f 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -143,7 +143,7 @@ class TextBuffer final void AddHyperlinkToMap(std::wstring_view uri, uint16_t id); std::wstring GetHyperlinkUriFromId(uint16_t id) const; - uint16_t GetHyperlinkId(std::wstring_view params); + uint16_t GetHyperlinkId(std::wstring_view uri, std::wstring_view id); void RemoveHyperlinkFromMap(uint16_t id); std::wstring GetCustomIdFromId(uint16_t id) const; void CopyHyperlinkMaps(const TextBuffer& OtherBuffer); diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index 9a7e896fe95..9fa3a847310 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -574,7 +574,7 @@ CATCH_LOG_RETURN_FALSE() bool Terminal::AddHyperlink(std::wstring_view uri, std::wstring_view params) noexcept { auto attr = _buffer->GetCurrentAttributes(); - const auto id = _buffer->GetHyperlinkId(params); + const auto id = _buffer->GetHyperlinkId(uri, params); attr.SetHyperlinkId(id); _buffer->SetCurrentAttributes(attr); _buffer->AddHyperlinkToMap(uri, id); diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp index fc3db8a8164..08a58ef60d3 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp @@ -37,6 +37,7 @@ namespace TerminalCoreUnitTests TEST_METHOD(AddHyperlink); TEST_METHOD(AddHyperlinkCustomId); + TEST_METHOD(AddHyperlinkCustomIdDifferentUri); }; }; @@ -296,15 +297,45 @@ void TerminalCoreUnitTests::TerminalApiTest::AddHyperlinkCustomId() stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c"); VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url"); - VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); // Send any other text stateMachine.ProcessString(L"Hello World"); VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url"); - VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); // Process the closing osc 8 sequences stateMachine.ProcessString(L"\x1b]8;;\x9c"); VERIFY_IS_FALSE(tbi.GetCurrentAttributes().IsHyperlink()); } + +void TerminalCoreUnitTests::TerminalApiTest::AddHyperlinkCustomIdDifferentUri() +{ + // This is a nearly literal copy-paste of ScreenBufferTests::TestAddHyperlinkCustomId, adapted for the Terminal + + Terminal term; + DummyRenderTarget emptyRT; + term.Create({ 100, 100 }, 0, emptyRT); + + auto& tbi = *(term._buffer); + auto& stateMachine = *(term._stateMachine); + + // Process the opening osc 8 sequence + stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c"); + VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url"); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + + const auto oldAttributes{ tbi.GetCurrentAttributes() }; + + // Send any other text + stateMachine.ProcessString(L"\x1b]8;id=myId;other.url\x9c"); + VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"other.url"); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"other.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + + // This second URL should not change the URL of the original ID! + VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(oldAttributes.GetHyperlinkId()), L"test.url"); + VERIFY_ARE_NOT_EQUAL(oldAttributes.GetHyperlinkId(), tbi.GetCurrentAttributes().GetHyperlinkId()); +} diff --git a/src/host/getset.cpp b/src/host/getset.cpp index b7c8e254a0e..7a088afb638 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -1567,7 +1567,7 @@ void DoSrvAddHyperlink(SCREEN_INFORMATION& screenInfo, const std::wstring_view params) { auto attr = screenInfo.GetAttributes(); - const auto id = screenInfo.GetTextBuffer().GetHyperlinkId(params); + const auto id = screenInfo.GetTextBuffer().GetHyperlinkId(uri, params); attr.SetHyperlinkId(id); screenInfo.GetTextBuffer().SetCurrentAttributes(attr); screenInfo.GetTextBuffer().AddHyperlinkToMap(uri, id); diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 1e559505914..d324def26d1 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -213,6 +213,7 @@ class ScreenBufferTests TEST_METHOD(TestAddHyperlink); TEST_METHOD(TestAddHyperlinkCustomId); + TEST_METHOD(TestAddHyperlinkCustomIdDifferentUri); TEST_METHOD(UpdateVirtualBottomWhenCursorMovesBelowIt); @@ -5956,19 +5957,46 @@ void ScreenBufferTests::TestAddHyperlinkCustomId() stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c"); VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url"); - VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); // Send any other text stateMachine.ProcessString(L"Hello World"); VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url"); - VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); // Process the closing osc 8 sequences stateMachine.ProcessString(L"\x1b]8;;\x9c"); VERIFY_IS_FALSE(tbi.GetCurrentAttributes().IsHyperlink()); } +void ScreenBufferTests::TestAddHyperlinkCustomIdDifferentUri() +{ + auto& g = ServiceLocator::LocateGlobals(); + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& tbi = si.GetTextBuffer(); + auto& stateMachine = si.GetStateMachine(); + + // Process the opening osc 8 sequence with a custom id + stateMachine.ProcessString(L"\x1b]8;id=myId;test.url\x9c"); + VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"test.url"); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"test.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + + const auto oldAttributes{ tbi.GetCurrentAttributes() }; + + // Send any other text + stateMachine.ProcessString(L"\x1b]8;id=myId;other.url\x9c"); + VERIFY_IS_TRUE(tbi.GetCurrentAttributes().IsHyperlink()); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(tbi.GetCurrentAttributes().GetHyperlinkId()), L"other.url"); + VERIFY_ARE_EQUAL(tbi.GetHyperlinkId(L"other.url", L"myId"), tbi.GetCurrentAttributes().GetHyperlinkId()); + + // This second URL should not change the URL of the original ID! + VERIFY_ARE_EQUAL(tbi.GetHyperlinkUriFromId(oldAttributes.GetHyperlinkId()), L"test.url"); + VERIFY_ARE_NOT_EQUAL(oldAttributes.GetHyperlinkId(), tbi.GetCurrentAttributes().GetHyperlinkId()); +} + void ScreenBufferTests::UpdateVirtualBottomWhenCursorMovesBelowIt() { auto& g = ServiceLocator::LocateGlobals(); diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 1ee79447b08..90c9576197a 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -2464,7 +2464,7 @@ void TextBufferTests::HyperlinkTrim() // Set a hyperlink id in the first row and add a hyperlink to our map const COORD pos{ 70, 0 }; - const auto id = _buffer->GetHyperlinkId(customId); + const auto id = _buffer->GetHyperlinkId(url, customId); TextAttribute newAttr{ 0x7f }; newAttr.SetHyperlinkId(id); _buffer->GetRowByOffset(pos.Y).GetAttrRow().SetAttrToEnd(pos.X, newAttr); @@ -2472,7 +2472,7 @@ void TextBufferTests::HyperlinkTrim() // Set a different hyperlink id somewhere else in the buffer const COORD otherPos{ 70, 5 }; - const auto otherId = _buffer->GetHyperlinkId(otherCustomId); + const auto otherId = _buffer->GetHyperlinkId(otherUrl, otherCustomId); newAttr.SetHyperlinkId(otherId); _buffer->GetRowByOffset(otherPos.Y).GetAttrRow().SetAttrToEnd(otherPos.X, newAttr); _buffer->AddHyperlinkToMap(otherUrl, otherId); @@ -2505,7 +2505,7 @@ void TextBufferTests::NoHyperlinkTrim() // Set a hyperlink id in the first row and add a hyperlink to our map const COORD pos{ 70, 0 }; - const auto id = _buffer->GetHyperlinkId(customId); + const auto id = _buffer->GetHyperlinkId(url, customId); TextAttribute newAttr{ 0x7f }; newAttr.SetHyperlinkId(id); _buffer->GetRowByOffset(pos.Y).GetAttrRow().SetAttrToEnd(pos.X, newAttr); From dc444e61ff8b5a519399e4ea0b22d2f4ef6cbd66 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Fri, 16 Oct 2020 12:45:39 -0700 Subject: [PATCH 2/3] un-dumb the tests --- src/host/ut_host/TextBufferTests.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 90c9576197a..be27bea5adf 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -2480,14 +2480,17 @@ void TextBufferTests::HyperlinkTrim() // Increment the circular buffer _buffer->IncrementCircularBuffer(); + const auto finalCustomId = fmt::format(L"{}%{}", customId, std::hash{}(url)); + const auto finalOtherCustomId = fmt::format(L"{}%{}", otherCustomId, std::hash{}(otherUrl)); + // The hyperlink reference that was only in the first row should be deleted from the map VERIFY_ARE_EQUAL(_buffer->_hyperlinkMap.find(id), _buffer->_hyperlinkMap.end()); // Since there was a custom id, that should be deleted as well - VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap.find(customId), _buffer->_hyperlinkCustomIdMap.end()); + VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap.find(finalCustomId), _buffer->_hyperlinkCustomIdMap.end()); // The other hyperlink reference should not be deleted VERIFY_ARE_EQUAL(_buffer->_hyperlinkMap[otherId], otherUrl); - VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[otherCustomId], otherId); + VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[finalOtherCustomId], otherId); } // This tests that when we increment the circular buffer, non-obsolete hyperlink references @@ -2518,7 +2521,9 @@ void TextBufferTests::NoHyperlinkTrim() // Increment the circular buffer _buffer->IncrementCircularBuffer(); + const auto finalCustomId = fmt::format(L"{}%{}", customId, std::hash{}(url)); + // The hyperlink reference should not be deleted from the map since it is still present in the buffer VERIFY_ARE_EQUAL(_buffer->GetHyperlinkUriFromId(id), url); - VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[customId], id); + VERIFY_ARE_EQUAL(_buffer->_hyperlinkCustomIdMap[finalCustomId], id); } From 39d467e45c50262992e3cf0dbd105364d286b6ae Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Fri, 16 Oct 2020 12:50:13 -0700 Subject: [PATCH 3/3] Update src/buffer/out/textBuffer.cpp --- src/buffer/out/textBuffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index cac3718f432..244709723f4 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2293,7 +2293,7 @@ uint16_t TextBuffer::GetHyperlinkId(std::wstring_view uri, std::wstring_view id) // assign _currentHyperlinkId if the custom id does not already exist std::wstring newId{ id }; // hash the URL and add it to the custom ID - GH#7698 - newId += L"%" + std::to_wstring(std::hash()(uri)); + newId += L"%" + std::to_wstring(std::hash{}(uri)); const auto result = _hyperlinkCustomIdMap.emplace(newId, _currentHyperlinkId); if (result.second) {