Fixes #5275. Clear stale TextView OSC 8 hyperlinks#5276
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code reviewFound 3 issues:
Terminal.Gui/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs Lines 478 to 496 in ad1127d
Terminal.Gui/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs Lines 310 to 315 in ad1127d
Terminal.Gui/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs Lines 64 to 82 in ad1127d 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- Map char offsets back to columns in SyncAutoUrlsForRowCore so that multi-codepoint graphemes (ZWJ emoji, base + combining mark) before a URL no longer shift the auto-link metadata onto the wrong cells. - Null out _explicitUrlMap and _autoUrlMap in ClearContentsCore (and when SyncAutoUrlsForRowCore leaves _autoUrlMap empty) so GetCellUrl's null fast-path stays effective and RowContainsUrls doesn't acquire the contents lock per cell on URL-free buffers. Bump a UrlStateVersion counter so OutputBase can detect resets. - Track buffer reference, dimensions, and UrlStateVersion in OutputBase; drop _rowsWithUrls when any of those change so resize/clear no longer leaves stale row indices that would emit a spurious OSC 8 close at the start of the next render. - Refresh the Write XML summary that still mentioned WrapOsc8. - Add regression tests for the grapheme alignment, post-resize state, and GetCellUrl fast-path restoration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…acking The condition added in 56eef0c ("emit OSC 8 close at row start whenever the row had URLs previously") was a defensive workaround for the apparent floating-underline symptom on Warp/Windows Terminal. Investigation of WT source shows the bug is in ControlCore::_updateHoveredCell()'s stale _lastHoveredCell cache (no callback fires when buffer content changes under a stationary cursor) — not in Terminal.Gui's emission. The cells themselves get _hyperlinkId = 0 correctly via AdaptDispatch::EndHyperlink when we emit `OSC 8 ; ; ST` followed by new cell content. Restoring the original (rowHadUrlsPreviously && \!rowHasUrlsNow) condition avoids a redundant escape on every row that still contains a URL after redraw. Also drops the regression test that pinned the redundant behavior. Separately, fix a real bookkeeping bug: _rowsWithUrls.Add/Remove was placed after the empty-builder early-exit at the end of the per-row block. Rows whose dirty cells were entirely flushed mid-loop via WriteToConsole (leaving the builder empty and _lastUrl null) skipped the row-tracking update, leaving stale entries that trigger spurious row-start OSC 8 closes on subsequent frames. Move the Add/Remove before the early-exit, and add a regression test that fails without the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I've extensively tested with multiple agents and models trying various angles to fix the two root issues observed in #5275:
They did find and fix several small bugs related to OSC 8 handling that should make Terminal.Gui as compliant as possible. But were not able to fix the root issue, and after testing with various terminals and AI agent analysis of the Windows.Terminal codebase, I believe the root cause is a Windows Terminal bug. Observations:
If anyone has other terminals they can test with, I'd appreciate it. I am going to file a bug report in the Microsoft Terminal project with repro details. The other OSC8 related bug fixes in this PR can be reviewed and merged. |
Summary
Fixes the stale OSC 8 hyperlink state in
TextViewwhen links come from plain-text URL auto-detection rather than explicitLinkviews.Changes
Osc8UrlLinkerURL-range detection to map auto-detected URLs into per-cell output state before rendering.Testing
dotnet build --no-restoredotnet test --project Tests/UnitTestsParallelizable --no-buildTo pull down this PR locally: