Fix the hyperlink detection when there are leading wide glyph in the row#16775
Conversation
|
Before: After: @lhecker
|
comzyh
changed the title
|
First of all, I deeply apologize for breaking this. I'm astonished you figured out how to fix this! This is rather deep in our code base after all. If you ever have any questions, please feel free to ask us. 🙂 I've edited your PR a bit to remove the images, since the commit may be archived for a long time and GitHub may not be around anymore at that point. |
lhecker
added
Area-Output
|
Thanks. terminal/src/buffer/out/Row.cpp Lines 115 to 117 in de0f702 you can try:
|
lhecker
added
the
AutoMerge
|
I've merged your PR as I'm planning to add some explanatory comments on top of your changes. Thanks for letting me know about the other issue you've found. I'll try to fix that with my next PR. I'm hoping to get to that later today or tomorrow. 🙂 |
|
I've figured it out. It's really that I forgot to use the terminal/src/buffer/out/Row.cpp Lines 115 to 117 in bb5f56e In other words, this is correct: if (offset < _charOffsets[col])
{
for (; offset < _charOffsets[col]; --col)
{
}
}
// ...It immediately makes way more sense this way, since the if condition and for loop now match perfectly. But looking at it again, I now realize that the entire code is way too complex. I've simplified it massively to just this: til::CoordType CharToColumnMapper::GetLeadingColumnAt(ptrdiff_t targetOffset) noexcept
{
targetOffset = clamp(targetOffset, 0, _lastCharOffset);
auto col = _currentColumn;
auto currentOffset = _charOffsets[col];
while (targetOffset > (currentOffset & CharOffsetsMask))
{
currentOffset = _charOffsets[++col];
}
while (targetOffset < currentOffset)
{
currentOffset = _charOffsets[--col];
}
_currentColumn = col;
return col;
}That's it. Just 2 loops. Makes it way more robust and faster too. I'll submit a PR soon. Thank you so much again for finding these bugs! |
Aside from overall simplifying `CharToColumnMapper` this fixes 2 bugs: * The backward search loop may have iterated 1 column too far, because it didn't stop at `*current <= *target`, but rather at `*(current - 1) <= *target`. This issue was only apparent when surrogate pairs were being used in a row. * When the target offset is that of a trailing surrogate pair the forward search loop may have iterated 1 column too far. It's somewhat unlikely for this to happen since this code is only used through ICU, but you never know. This is a continuation of PR #16775.
Aside from overall simplifying `CharToColumnMapper` this fixes 2 bugs: * The backward search loop may have iterated 1 column too far, because it didn't stop at `*current <= *target`, but rather at `*(current - 1) <= *target`. This issue was only apparent when surrogate pairs were being used in a row. * When the target offset is that of a trailing surrogate pair the forward search loop may have iterated 1 column too far. It's somewhat unlikely for this to happen since this code is only used through ICU, but you never know. This is a continuation of PR #16775. (cherry picked from commit 043d5cd) Service-Card-Id: 91955569 Service-Version: 1.20
Aside from overall simplifying `CharToColumnMapper` this fixes 2 bugs: * The backward search loop may have iterated 1 column too far, because it didn't stop at `*current <= *target`, but rather at `*(current - 1) <= *target`. This issue was only apparent when surrogate pairs were being used in a row. * When the target offset is that of a trailing surrogate pair the forward search loop may have iterated 1 column too far. It's somewhat unlikely for this to happen since this code is only used through ICU, but you never know. This is a continuation of PR #16775. (cherry picked from commit 043d5cd) Service-Card-Id: 91955617 Service-Version: 1.19
Aside from overall simplifying `CharToColumnMapper` this fixes 2 bugs: * The backward search loop may have iterated 1 column too far, because it didn't stop at `*current <= *target`, but rather at `*(current - 1) <= *target`. This issue was only apparent when surrogate pairs were being used in a row. * When the target offset is that of a trailing surrogate pair the forward search loop may have iterated 1 column too far. It's somewhat unlikely for this to happen since this code is only used through ICU, but you never know. This is a continuation of PR #16775. (cherry picked from commit 043d5cd) Service-Card-Id: 91955617 Service-Version: 1.19
3 participants
Summary of the Pull Request
URL detection was broken again in #15858. When the regex matched, we calculate the column(cell) by its offset, we use forward or backward iteration of the column to find the correct column that displays the glyphs of
_chars[offset].terminal/src/buffer/out/Row.cpp
Lines 95 to 104 in abf5d94
However, when calculating the
currentOffsetwe forget that MSB of_charOffsets[col]could be1, or col is pointing to another glyph in preceding column.terminal/src/buffer/out/Row.hpp
Lines 223 to 226 in abf5d94