microsoft/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2025-12-11 04:38:24 -06:00
Code Issues Packages Projects Releases 138 Wiki Activity

Hash the URI as part of the hyperlink ID (#7940)

Browse Source 
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
This commit is contained in:
Dustin L. Howett 2020-10-16 15:08:59 -07:00 committed by GitHub
parent 743283e434
commit df7c3ccc3b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 87 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
src/buffer/out/textBuffer.cpp
View File

@ -2281,32 +2281,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<std::wstring_view>{}(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:

2
src/buffer/out/textBuffer.hpp
View File

@ -143,7 +143,7 @@ public:
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);

2
src/cascadia/TerminalCore/TerminalApi.cpp
View File

@ -582,7 +582,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);

35
src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp
View File

@ -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());
}

2
src/host/getset.cpp
View File

@ -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);

32
src/host/ut_host/ScreenBufferTests.cpp
View File

@ -212,6 +212,7 @@ class ScreenBufferTests
TEST_METHOD(TestAddHyperlink);
TEST_METHOD(TestAddHyperlinkCustomId);
TEST_METHOD(TestAddHyperlinkCustomIdDifferentUri);
TEST_METHOD(UpdateVirtualBottomWhenCursorMovesBelowIt);
@ -5932,19 +5933,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();

17
src/host/ut_host/TextBufferTests.cpp
View File

@ -2546,7 +2546,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);
@ -2554,7 +2554,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);
@ -2562,14 +2562,17 @@ void TextBufferTests::HyperlinkTrim()
// Increment the circular buffer
_buffer->IncrementCircularBuffer();
const auto finalCustomId = fmt::format(L"{}%{}", customId, std::hash<std::wstring_view>{}(url));
const auto finalOtherCustomId = fmt::format(L"{}%{}", otherCustomId, std::hash<std::wstring_view>{}(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
@ -2587,7 +2590,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);
@ -2600,7 +2603,9 @@ void TextBufferTests::NoHyperlinkTrim()
// Increment the circular buffer
_buffer->IncrementCircularBuffer();
const auto finalCustomId = fmt::format(L"{}%{}", customId, std::hash<std::wstring_view>{}(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);
}