Repository navigation
Resync terminal portals after sidebar changes #1253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
8ec410f
Add regression test for portal ancestor shifts
lawrencecchen 1ddda0e
Resync terminal portals after sidebar changes
lawrencecchen 2e2b4e1
Restore safeHelp view helper
lawrencecchen 2f847cc
Fix portal geometry regression test harness
lawrencecchen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11861,6 +11861,80 @@ final class TerminalWindowPortalLifecycleTests: XCTestCase { | |||||||||||||||||||
| portal.synchronizeHostedViewForAnchor(anchor) | ||||||||||||||||||||
| XCTAssertFalse(hosted.isHidden, "Portal should unhide after geometry is usable") | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func testScheduledExternalGeometrySyncRefreshesAncestorLayoutShift() { | ||||||||||||||||||||
| let window = NSWindow( | ||||||||||||||||||||
| contentRect: NSRect(x: 0, y: 0, width: 700, height: 420), | ||||||||||||||||||||
| styleMask: [.titled, .closable], | ||||||||||||||||||||
| backing: .buffered, | ||||||||||||||||||||
| defer: false | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| defer { | ||||||||||||||||||||
| NotificationCenter.default.post(name: NSWindow.willCloseNotification, object: window) | ||||||||||||||||||||
| window.orderOut(nil) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| realizeWindowLayout(window) | ||||||||||||||||||||
| guard let contentView = window.contentView else { | ||||||||||||||||||||
| XCTFail("Expected content view") | ||||||||||||||||||||
| return | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let shiftedContainer = NSView(frame: NSRect(x: 120, y: 60, width: 220, height: 160)) | ||||||||||||||||||||
| contentView.addSubview(shiftedContainer) | ||||||||||||||||||||
| let anchor = NSView(frame: NSRect(x: 24, y: 28, width: 72, height: 56)) | ||||||||||||||||||||
| shiftedContainer.addSubview(anchor) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let surface = TerminalSurface( | ||||||||||||||||||||
| tabId: UUID(), | ||||||||||||||||||||
| context: GHOSTTY_SURFACE_CONTEXT_SPLIT, | ||||||||||||||||||||
| configTemplate: nil, | ||||||||||||||||||||
| workingDirectory: nil | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| let hosted = surface.hostedView | ||||||||||||||||||||
| TerminalWindowPortalRegistry.bind( | ||||||||||||||||||||
| hostedView: hosted, | ||||||||||||||||||||
| to: anchor, | ||||||||||||||||||||
| visibleInUI: true, | ||||||||||||||||||||
| expectedSurfaceId: surface.id, | ||||||||||||||||||||
| expectedGeneration: surface.portalBindingGeneration() | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| TerminalWindowPortalRegistry.synchronizeForAnchor(anchor) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let anchorCenter = NSPoint(x: anchor.bounds.midX, y: anchor.bounds.midY) | ||||||||||||||||||||
| let originalWindowPoint = anchor.convert(anchorCenter, to: nil) | ||||||||||||||||||||
| XCTAssertNotNil( | ||||||||||||||||||||
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window), | ||||||||||||||||||||
| "Initial hit-testing should resolve the portal-hosted terminal at its original window position" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
Comment on lines
+11906
to
+11909
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: This regression test now only checks for a non-nil terminal view, so it can pass without proving hit-testing resolves the bound surface. Assert the returned terminal’s surface ID matches Prompt for AI agents
Suggested change
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| shiftedContainer.frame.origin.x += 96 | ||||||||||||||||||||
| contentView.layoutSubtreeIfNeeded() | ||||||||||||||||||||
| window.displayIfNeeded() | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let shiftedWindowPoint = anchor.convert(anchorCenter, to: nil) | ||||||||||||||||||||
| XCTAssertNotEqual(originalWindowPoint.x, shiftedWindowPoint.x, accuracy: 0.5) | ||||||||||||||||||||
| XCTAssertNil( | ||||||||||||||||||||
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(shiftedWindowPoint, in: window), | ||||||||||||||||||||
| "Ancestor-only layout shifts should leave the portal stale until an external geometry sync runs" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| XCTAssertNotNil( | ||||||||||||||||||||
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window), | ||||||||||||||||||||
| "Before the external geometry sync, hit-testing should still point at the stale portal location" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| TerminalWindowPortalRegistry.scheduleExternalGeometrySynchronizeForAllWindows() | ||||||||||||||||||||
| RunLoop.current.run(until: Date().addingTimeInterval(0.05)) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| XCTAssertNil( | ||||||||||||||||||||
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(originalWindowPoint, in: window), | ||||||||||||||||||||
| "The stale portal position should be cleared after the scheduled external geometry sync" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| XCTAssertNotNil( | ||||||||||||||||||||
| TerminalWindowPortalRegistry.terminalViewAtWindowPoint(shiftedWindowPoint, in: window), | ||||||||||||||||||||
| "The scheduled external geometry sync should move the portal-hosted terminal to the anchor's new window position" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @MainActor | ||||||||||||||||||||
|
|
||||||||||||||||||||
Oops, something went wrong.
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.
Missing
MainActor.assumeIsolatedinDispatchQueue.main.asyncclosureThe
DispatchQueue.main.asyncclosure accesses@MainActor-isolated static properties (Self.hasPendingExternalGeometrySyncForAllWindowsandSelf.portalsByWindowId) without asserting main-actor isolation. The existinginstallWindowCloseObserverIfNeededfunction in the same file already establishes the correct pattern: even when dispatching to.main, the compiler cannot statically prove the closure inherits@MainActorisolation, soMainActor.assumeIsolated { }is required.Without it, in Swift 6 strict concurrency mode these accesses are unisolated captures of
@MainActor-guarded state, which can generate errors. The fix mirrors the existing pattern used at line 1672: