Fix detecting partially visible URLs#19878
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Ah whoops, I thought I had already reviewed it last week and just re-approved it. Dejavu? |
| // GH#18177: Scan extra rows beyond the viewport so that URLs | ||
| // wrapping across the viewport boundary are matched in full | ||
| const auto bufferSize = _activeBuffer().GetSize(); | ||
| const auto beg = std::max<til::CoordType>(0, visStart - viewportHeight); | ||
| const auto end = std::min(bufferSize.BottomInclusive(), visEnd + viewportHeight); |
There was a problem hiding this comment.
Should we trim the results? For instance, we could pass both a start/end range for search, as well as a start/end range for filtering the search to _getPatterns.
There was a problem hiding this comment.
Eh. _patternIntervalTree can definitely be improved to include/expose the buffer range it's looking at and that would make some of the other parts of the code a bit cleaner. That said, I think that's a separate refactor outside of this PR and I don't think we necessarily need that right now.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. These words are not needed and should be removedlto TlggForbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: Should be
|
| const auto visibleViewport = _GetVisibleViewport(); | ||
| if (visibleViewport.IsInBounds(bufferPos)) | ||
| // Check cached interval tree (covers visible viewport +/- viewport height) | ||
| if (const auto results = _patternIntervalTree.findOverlapping({ bufferPos.x + 1, bufferPos.y }, bufferPos); !results.empty()) |
Summary of the Pull Request
Fixes a bug where a partially visible URL would not be detected. This is fixed by expanding the search space by 1 viewport height in both directions.
The
_patternIntervalTreenow operates in the absolute-buffer space as opposed to the viewport-relative space. It's a bit of an annoying change, but the alternative would be to keep track of the offset used by the method above, which I find more annoying, personally. As a part of this change, I made it a bit more clear when something is viewport-relative vs buffer-absolute.Regarding mark mode hyperlink navigation, now that everything is in the absolute-buffer space, I'm able to fix some of the issues in #13854. I removed
_selectionIsTargetingUrland fixed/validated navigating to hyperlinks that are partially visible.Validation Steps Performed
Detects URL that is...
✅ fully visible
✅ partially cropped off the top
✅ partially cropped off the bottom
✅ Above scenarios work with mark mode hyperlink navigation
✅Tests added
Closes #18177
Closes #13854