Skip to content

Commit 7d30ac7

Browse files
harderclaude
andcommitted
Revert #5276 over-eager OSC 8 close; fix _rowsWithUrls stale tracking
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>
1 parent 56eef0c commit 7d30ac7

2 files changed

Lines changed: 62 additions & 39 deletions

File tree

Terminal.Gui/Drivers/Output/OutputBase.cs

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public virtual void Write (IOutputBuffer buffer)
120120
outputStringBuilder.Clear ();
121121
_lastUrl = null; // Reset URL state at the start of each row
122122

123-
if (!IsLegacyConsole && rowHadUrlsPreviously)
123+
if (!IsLegacyConsole && rowHadUrlsPreviously && !rowHasUrlsNow)
124124
{
125125
outputStringBuilder.Append (EscSeqUtils.OSC_EndHyperlink ());
126126
}
@@ -199,6 +199,21 @@ public virtual void Write (IOutputBuffer buffer)
199199
}
200200
}
201201

202+
// Track row's URL status BEFORE the early-exit so _rowsWithUrls stays consistent
203+
// with the buffer state — even for rows whose cells were all flushed via WriteToConsole
204+
// during the inner loop (leaving outputStringBuilder empty at this point).
205+
if (!IsLegacyConsole)
206+
{
207+
if (rowHasUrlsNow)
208+
{
209+
_rowsWithUrls.Add (row);
210+
}
211+
else
212+
{
213+
_rowsWithUrls.Remove (row);
214+
}
215+
}
216+
202217
// Flush buffered output for row. Even when nothing remains buffered, an OSC 8 hyperlink
203218
// may still be open in the terminal because it was started in a prior batch flushed by
204219
// WriteToConsole and the row ended (or only clean cells followed) before any cell with
@@ -227,15 +242,6 @@ public virtual void Write (IOutputBuffer buffer)
227242
SetCursorPositionImpl (lastCol, row);
228243

229244
Write (outputStringBuilder);
230-
231-
if (rowHasUrlsNow)
232-
{
233-
_rowsWithUrls.Add (row);
234-
}
235-
else
236-
{
237-
_rowsWithUrls.Remove (row);
238-
}
239245
}
240246

241247
if (IsLegacyConsole)

Tests/UnitTestsParallelizable/Drivers/Output/OutputBaseTests.cs

Lines changed: 46 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -425,35 +425,6 @@ public void Write_AutoDetectedUrl_ThenSpaces_EmitsOsc8CloseBeforeClearingCells (
425425
Assert.True (endIdx >= 0 && endIdx < replacementIdx, "OSC 8 close must be emitted before replacement spaces");
426426
}
427427

428-
// Copilot - GPT-5.4
429-
// Regression coverage for the remaining floating-underline case when a row still contains
430-
// a URL after redraw, but cells that were formerly part of a URL are now plain text.
431-
// Warp appears to need an explicit OSC 8 close before those replacement cells are emitted;
432-
// otherwise the old hyperlink metadata can remain attached to the old screen columns.
433-
[Fact]
434-
public void Write_AutoDetectedUrl_MovedWithinSameRow_ClosesBeforePlainReplacement ()
435-
{
436-
AnsiOutput output = new ();
437-
IOutputBuffer buffer = output.GetLastBuffer ()!;
438-
buffer.SetSize (48, 1);
439-
440-
buffer.Move (0, 0);
441-
buffer.AddStr ("https://one.example then trailing text ");
442-
output.Write (buffer);
443-
444-
buffer.Move (0, 0);
445-
buffer.AddStr ("plain intro then https://two.example ");
446-
447-
output.Write (buffer);
448-
string result = output.GetLastOutput ();
449-
string end = EscSeqUtils.OSC_EndHyperlink ();
450-
int replacementIdx = result.IndexOf ("plain intro then ", StringComparison.Ordinal);
451-
int endIdx = result.IndexOf (end, StringComparison.Ordinal);
452-
453-
Assert.True (replacementIdx >= 0, "Replacement plain text was not emitted");
454-
Assert.True (endIdx >= 0 && endIdx < replacementIdx, "OSC 8 close must be emitted before the old linked cells are redrawn as plain text");
455-
}
456-
457428
// Claude - Opus 4.7
458429
// Regression coverage for the char-vs-column mismatch in SyncAutoUrlsForRowCore.
459430
// When a multi-codepoint grapheme (ZWJ emoji, base + combining mark) precedes a URL
@@ -552,6 +523,52 @@ public void GetCellUrl_AfterUrlSetThenCleared_RestoresNullFastPath ()
552523
Assert.True (buffer.UrlStateVersion > 0);
553524
}
554525

526+
// Claude - Opus 4.7
527+
// Regression coverage for _rowsWithUrls bookkeeping when the per-row flush happens entirely
528+
// via WriteToConsole (i.e. the end-of-row Write path takes the empty-builder early-exit).
529+
// Before the fix, the Add/Remove of the row index lived after the early-exit, so a row that
530+
// lost its URL via overwrite but had clean trailing cells (causing the builder to flush
531+
// mid-loop and end empty) kept a stale row-tracking entry — leading to a spurious row-start
532+
// OSC 8 close on subsequent frames.
533+
[Fact]
534+
public void Write_RowLosesUrl_BuilderFlushedMidLoop_RemovesRowTracking ()
535+
{
536+
// Arrange — buffer with a URL on row 0, then overwrite the URL cells with non-URL
537+
// content followed by clean cells (so the builder flushes mid-loop and the row exits
538+
// with an empty builder, taking the early-exit).
539+
AnsiOutput output = new ();
540+
IOutputBuffer buffer = output.GetLastBuffer ()!;
541+
buffer.SetSize (40, 1);
542+
543+
// Frame 1: write URL covering cols 0-18, trailing space cells stay as ' '.
544+
buffer.Move (0, 0);
545+
buffer.AddStr ("https://example.com");
546+
output.Write (buffer);
547+
548+
// Frame 2: overwrite the URL cells with Y characters. Cells 0-19 become dirty (19
549+
// Y's plus the adjacent-dirty mark on cell 19). Cells 20-39 stay clean — they will
550+
// trigger the WriteToConsole flush and leave the row with an empty builder.
551+
buffer.Move (0, 0);
552+
buffer.AddStr ("YYYYYYYYYYYYYYYYYYY");
553+
output.Write (buffer);
554+
555+
// Frame 3: trivial change. With the fix, row 0 has been removed from _rowsWithUrls,
556+
// so no OSC 8 close is emitted at the start of row 0. Without the fix, the stale
557+
// entry causes a spurious OSC 8 close.
558+
buffer.Move (0, 0);
559+
buffer.AddStr ("X");
560+
561+
// Act
562+
output.Write (buffer);
563+
string result = output.GetLastOutput ();
564+
string end = EscSeqUtils.OSC_EndHyperlink ();
565+
566+
// Assert — frame 3 must NOT emit an OSC 8 close because frame 2's row no longer
567+
// has any URL state, and _rowsWithUrls must reflect that even when the early-exit
568+
// path is taken.
569+
Assert.DoesNotContain (end, result);
570+
}
571+
555572
// Copilot
556573
[Fact]
557574
public void ToAnsi_LegacyConsole_NoOsc8 ()

0 commit comments

Comments
 (0)