Reserve scroller inset so terminal text isn't clipped on the right #3001
+108
−7
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import XCTest | ||
| import AppKit | ||
|
|
||
| #if canImport(cmux_DEV) | ||
| @testable import cmux_DEV | ||
| #elseif canImport(cmux) | ||
| @testable import cmux | ||
| #endif | ||
|
|
||
| /// Regression tests for https://github.com/manaflow-ai/cmux/issues/1082 and | ||
| /// https://github.com/manaflow-ai/cmux/issues/2997. A persistent vertical | ||
| /// scroller rendered on top of the rightmost terminal column because the | ||
| /// surface width passed to libghostty did not account for the scroller gutter. | ||
| /// The terminal reserves the inset only when macOS is going to render a | ||
| /// persistent (legacy) scroller; transient overlay scrollers that fade out | ||
| /// keep the full terminal width. | ||
| final class TerminalScrollerInsetTests: XCTestCase { | ||
| func testReservesNothingWhenScrollerIsAbsent() { | ||
| let inset = GhosttySurfaceScrollView.verticalScrollerInsetWidth( | ||
| hasVerticalScroller: false, | ||
| preferredScrollerStyle: .legacy, | ||
| scrollerWidth: 15 | ||
| ) | ||
| XCTAssertEqual(inset, 0) | ||
| } | ||
|
|
||
| func testReservesNothingForTransientOverlayScrollers() { | ||
| // Trackpad / transient-overlay case: scroller fades out when idle, so | ||
| // briefly overlapping content during scroll is acceptable in exchange | ||
| // for keeping the full terminal width. | ||
| let inset = GhosttySurfaceScrollView.verticalScrollerInsetWidth( | ||
| hasVerticalScroller: true, | ||
| preferredScrollerStyle: .overlay, | ||
| scrollerWidth: 15 | ||
| ) | ||
| XCTAssertEqual(inset, 0) | ||
| } | ||
|
|
||
| func testReservesScrollerWidthForPersistentLegacyScrollers() { | ||
| // macOS reports legacy when "Show scroll bars" is Always, or Automatic | ||
| // with a mouse attached — in both cases the scroller is permanently | ||
| // visible, so we must shrink the terminal surface by its width. | ||
| let inset = GhosttySurfaceScrollView.verticalScrollerInsetWidth( | ||
| hasVerticalScroller: true, | ||
| preferredScrollerStyle: .legacy, | ||
| scrollerWidth: 15 | ||
| ) | ||
| XCTAssertEqual(inset, 15) | ||
| } | ||
|
|
||
| func testClampsNegativeScrollerWidthToZero() { | ||
| let inset = GhosttySurfaceScrollView.verticalScrollerInsetWidth( | ||
| hasVerticalScroller: true, | ||
| preferredScrollerStyle: .legacy, | ||
| scrollerWidth: -4 | ||
| ) | ||
| XCTAssertEqual(inset, 0) | ||
| } | ||
|
Comment on lines
+51
to
+58
Contributor
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scrollerWidthquery for overlay styleNSScroller.scrollerWidth(for: .regular, scrollerStyle: style)is called unconditionally, but the result is only used whenstyle == .legacy. Whenstyle == .overlaythe static helper returns0before readingscrollerWidth, so thescrollerWidthcall is dead work. Consider querying the width only when needed:This also removes the indirection through the static helper for the hot-path call and makes the guard order match the static helper's logic explicitly.