Mac: repaint the rows where screen updates actually land while scrolled back - #603
Mac: repaint the rows where screen updates actually land while scrolled back#603bones7456 wants to merge 2 commits into
Conversation
…to 1.3.6 SwiftTerm's updateDisplay invalidated live-screen rows as if the view were always scrolled to the bottom, so with the view scrolled back the echoed characters landed on rows that never repainted (the caret, positioned with the correct offset, kept moving). Fixed in the fork (upstream PR migueldeicaza/SwiftTerm#603) and pinned Notchy to the new notchy-branch tip.
|
I had codex review the changes, just as a safety check: [P2] Preserve pending Metal dirty rows. updateDisplay replaces metalDirtyRange. This can discard a link-highlight row queued at lines 923–930 before Metal draws it. Link highlights do not change the buffer-line generation, so the renderer does not recover the lost update. Merge the terminal range with the existing dirty range. [P2] Apply the full-screen rule to Metal. The CoreGraphics path treats refresh(0, rows - 1) as a visible-viewport refresh. The Metal path instead translates that range from yBase. When scrolled back two rows, my test got 31...43 instead of the visible 29...41. Use visibleStart...visibleEnd for the same full-screen condition and add a Metal version of the regression test. |
|
Thanks — both hold up, and the second one is a regression I introduced. Fixed in 5134c24 (added on top rather than squashed, so the delta is reviewable). Preserving pending Metal dirty rows. Confirmed the mechanism: I left Applying the full-screen rule to Metal. Your numbers reproduce exactly. Also worth flagging that this case was right before this PR by accident: Rather than special-casing the full-screen condition twice, the Metal branch now derives its absolute rows from the view rows the CoreGraphics path already computes ( Tests. Four Metal cases; three of them fail against 4335830 (the full-screen one with precisely your One thing worth knowing if you write Metal tests later: my first attempt used That Full suite (460 tests) passes, and the package still builds for iOS. |
…ed back The update range reported by Terminal.getUpdateRange() is relative to the live screen (yBase), but updateDisplay's CoreGraphics path used those rows directly as view rows, which is only correct when the view is scrolled to the bottom (yDisp == yBase). With the view scrolled back N rows, a change on live-screen row r is displayed on view row r + N — or below the viewport entirely — so the invalidated rect repainted unchanged scrollback while the changed row stayed stale. Typing into a shell while scrolled back moved the caret (positioned by updateCursorPosition, which does account for the offset) but the echoed characters stayed invisible until something forced a full redraw, such as scrolling back to the bottom. Translate the update range into view rows before invalidating, skip the invalidation when the changed rows sit entirely below the viewport, and keep full-screen refreshes covering the whole viewport since they are used for things that also affect visible scrollback (palette changes and the like). Also align the two other users of that coordinate space: - The Metal branch computed the dirty rows as yDisp + row; the true absolute row is yBase + row. The per-line generation check in the renderer masked most of the impact, but the range was wrong. - invalidateLinkHighlightRow fed yDisp-relative rows into updateRange, compensating for the CG bug and breaking once it is fixed; a highlight row can live in the scrollback, which the yBase-relative update range cannot express. Invalidate the displayed row directly instead — the highlight is view state, no buffer line changes. Add regression tests asserting that the invalidated region follows the displayed position of the changed rows in all four cases: at the bottom, scrolled back, pushed below the viewport, and a full-screen refresh.
Two review findings on the previous commit: - updateDisplay translated the update range from yBase for the Metal renderer, so a full-screen refresh while scrolled back marked yBase...yBase+rows-1 dirty instead of the visible yDisp...yDisp+rows-1. The visible scrollback rows then stayed stale for changes that do not bump a line's generation, a palette change being the reason the CoreGraphics path treats a full-screen refresh as "everything visible" in the first place. Derive the Metal range from the view rows that path already computes, so the two agree on where changed rows are displayed, on what a full-screen refresh means, and on leaving rows below the viewport alone. - The Metal branch replaced metalDirtyRange, which discards a link-highlight row queued by invalidateLinkHighlightRow before the renderer got to draw it. Highlights are view state, so the buffer line's generation does not change and the renderer keeps its cached row — the update is not recoverable. Merge into the pending range instead, through the new addMetalDirtyRows, and use it on the iOS path too. Four Metal regression tests cover both, plus the yBase-relative placement of a partial update while scrolled back. They attach a bare MTKView rather than calling setUseMetal(true): the range bookkeeping never draws, so the tests need no device, renderer or shader library and run everywhere. Three of them fail against the previous commit. Full suite (460 tests) passes, and the package still builds for iOS.
5134c24 to
8d586fc
Compare
|
Rebased. The conflict wasn't textual — #620 landed the same diagnosis meanwhile — so to be explicit about what this still changes: #620 invalidates the whole view when scrolled back. This narrows it to the rows that changed, which is the case its own message calls out: a TUI redrawing a status block while you read scrollback repaints one band instead of the entire view on every update. The rest is what #620 doesn't reach: the Metal dirty range and its full-screen rule, the link-highlight invalidation, and the bidi dependency lookup added since, which asks If you'd rather leave #620's behavior alone, I'm happy to drop the CG narrowing and keep only the Metal, highlight and bidi fixes. |
2 participants
Rebased on top of #620, which fixed the CoreGraphics half of this while the PR was open by invalidating the whole view when
yDisp != yBase.What this changes:
yDisp + rowwhere the absolute row isyBase + row. Now derived from the same view rows as the CG path, so the full-screen rule matches too — it was marking31...43dirty for a visible29...41.renderingDependencyRangeis asked aboutyDisp + rowStart, the same conflation a third time. NowyBase-relative, unioned into the view rows so both paths inherit it.metalDirtyRangewas replaced, not merged, so a link-highlight row queued before the renderer drew was dropped. Highlights don't bump the line generation, so the renderer's cache check can't recover it.invalidateLinkHighlightRowno longer routes throughupdateRange, which can't express a scrollback row at all.8 tests, 4 per path. The Metal ones attach a bare
MTKView(device: nil)instead of callingsetUseMetal(true)— the range bookkeeping never draws, so no GPU is needed and they actually run on CI. Gating on a real device makes them silently skip underswift test: the shader bundle sits next to the.xctestbundle, whichcandidateBundles()doesn't probe, sosetUseMetal(true)throwsshaderSourceMissing.#620's test passes unchanged. Full suite (690 tests) passes, iOS builds.