Skip to content

Commit fe6ebab

Browse files
authored
fix(windows): pin a screenshot window's size when it is built, not from inside its layout pass (#3092)
1 parent 45a8c1f commit fe6ebab

5 files changed

Lines changed: 172 additions & 24 deletions

File tree

TablePro/Core/Services/Infrastructure/TabWindowController.swift

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,14 @@ internal final class TabWindowController: NSWindowController, NSWindowDelegate {
9696
/// `adopting` carries a connection that is moving here from another window, whole. Everything
9797
/// the user has in it lives on that object, so the window takes it rather than building a
9898
/// second one around the same connection.
99+
///
100+
/// `pinnedWindowSize` is the screenshot size, passed in so a test can build the real window at
101+
/// one without the UI test sandbox.
99102
internal init(
100103
payload: EditorTabPayload,
101104
sessionState: SessionStateFactory.SessionState? = nil,
102105
autoConnect: Bool = false,
106+
pinnedWindowSize: CGSize? = ScreenshotEnvironment.windowSize,
103107
adopting workspace: ConnectionWorkspace? = nil
104108
) {
105109
self.payload = payload
@@ -123,18 +127,9 @@ internal final class TabWindowController: NSWindowController, NSWindowDelegate {
123127
super.init(window: window)
124128

125129
window.isReleasedWhenClosed = false
130+
Self.placeInitialFrame(of: window, pinnedSize: pinnedWindowSize)
126131
window.delegate = self
127132

128-
if !window.setFrameUsingName(Self.frameAutosaveName) {
129-
let visibleSize = (window.screen ?? NSScreen.main)?.visibleFrame.size
130-
?? NSSize(width: 1_440, height: 900)
131-
window.setContentSize(NSSize(
132-
width: min(1_200, visibleSize.width),
133-
height: min(800, visibleSize.height)
134-
))
135-
window.center()
136-
}
137-
138133
Self.lifecycleLogger.info(
139134
"[open] TabWindowController.init payloadId=\(payload.id, privacy: .public) connId=\(payload.connectionId, privacy: .public) controllerId=\(self.controllerId, privacy: .public) eagerToolbar=\(sessionState != nil)"
140135
)
@@ -145,6 +140,36 @@ internal final class TabWindowController: NSWindowController, NSWindowDelegate {
145140
fatalError("TabWindowController does not support NSCoder init")
146141
}
147142

143+
/// Every frame the window starts with is settled here, before it is shown or laid out. A resize
144+
/// issued from inside the window's own layout pass lands under `_layoutSubtreeWithOldSize:`,
145+
/// which has already captured the old size and resizes the content view by the difference a
146+
/// second time. Measured on macOS 27 with a 1200x800 window pinned to 1512x861 from a SwiftUI
147+
/// `viewDidMoveToWindow`, which is where the screenshot pin used to run: the split view came out
148+
/// 1824x922 at y -61, so the sidebar's top row sat under the titlebar and the inspector divider
149+
/// stood 312pt past the window's edge, taking the toolbar's trailing items into the overflow
150+
/// menu. Activating the window afterwards did not repair it.
151+
///
152+
/// Placed before the controller becomes the window's delegate, so the starting frame is not
153+
/// filed under `frameAutosaveName` as if the user had chosen it. A pinned size is not theirs, and
154+
/// a unit test building this window would otherwise write over the real saved frame, which is
155+
/// not namespaced outside the UI test sandbox. The frame is still saved on close.
156+
private static func placeInitialFrame(of window: NSWindow, pinnedSize: CGSize?) {
157+
let visibleFrame = (window.screen ?? NSScreen.main)?.visibleFrame
158+
if let pinnedSize {
159+
let pinned = visibleFrame.map { ScreenshotEnvironment.pinnedFrame(size: pinnedSize, in: $0) }
160+
?? NSRect(origin: window.frame.origin, size: pinnedSize)
161+
window.setFrame(pinned, display: false)
162+
return
163+
}
164+
guard !window.setFrameUsingName(frameAutosaveName) else { return }
165+
let visibleSize = visibleFrame?.size ?? NSSize(width: 1_440, height: 900)
166+
window.setContentSize(NSSize(
167+
width: min(1_200, visibleSize.width),
168+
height: min(800, visibleSize.height)
169+
))
170+
window.center()
171+
}
172+
148173
/// The one place an editor window's chrome is configured, so a test can hold the whole shape.
149174
internal static func makeEditorWindow() -> NSWindow {
150175
let window = EditorWindow(

TablePro/Core/Testing/ScreenshotEnvironment.swift

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -53,17 +53,17 @@ internal enum ScreenshotEnvironment {
5353
/// Centred on the visible frame rather than placed at a fixed origin. `screencapture` reads the
5454
/// window's own bounds, so a window hanging off the screen edge comes back clipped by the
5555
/// display instead of failing.
56-
@MainActor
57-
internal static func pinWindowSize(_ window: NSWindow) {
58-
guard let size = windowSize else { return }
59-
guard let screen = window.screen ?? NSScreen.main else { return }
60-
61-
let visible = screen.visibleFrame
62-
let origin = NSPoint(
63-
x: visible.midX - size.width / 2,
64-
y: visible.midY - size.height / 2
56+
///
57+
/// Only a frame. Where it is applied is the caller's decision, and there is one right answer:
58+
/// see `TabWindowController.placeInitialFrame(of:pinnedSize:)`.
59+
internal static func pinnedFrame(size: CGSize, in visibleFrame: NSRect) -> NSRect {
60+
NSRect(
61+
origin: NSPoint(
62+
x: visibleFrame.midX - size.width / 2,
63+
y: visibleFrame.midY - size.height / 2
64+
),
65+
size: size
6566
)
66-
window.setFrame(NSRect(origin: origin, size: size), display: true)
6767
}
6868

6969
private static func sandboxedValue(of variable: String) -> String? {

TablePro/Views/Main/Extensions/MainContentView+Setup.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,6 @@ extension MainContentView {
279279
splitVC.pointToolbar(at: coordinator)
280280
}
281281

282-
ScreenshotEnvironment.pinWindowSize(window)
283282
MainContentView.lifecycleLogger.info(
284283
"[open] configureWindow done windowId=\(windowId, privacy: .public) isPreview=\(isPreview) elapsedMs=\(Int(Date().timeIntervalSince(start) * 1_000))"
285284
)

TableProTests/Core/Testing/ScreenshotEnvironmentTests.swift

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,24 @@
33
// TableProTests
44
//
55

6+
import CoreGraphics
67
import Foundation
78
@testable import TablePro
89
import Testing
910

10-
@Suite("ScreenshotEnvironment frame parsing")
11+
@Suite("ScreenshotEnvironment frame")
1112
struct ScreenshotEnvironmentTests {
1213
@Test("Reads the size the marketing shots are cut to")
1314
func readsWidthAndHeight() throws {
1415
let size = try #require(ScreenshotEnvironment.size(from: "1512x861"))
15-
#expect(size.width == 1512)
16+
#expect(size.width == 1_512)
1617
#expect(size.height == 861)
1718
}
1819

1920
@Test("Accepts an upper case separator")
2021
func acceptsUpperCaseSeparator() throws {
2122
let size = try #require(ScreenshotEnvironment.size(from: "1512X861"))
22-
#expect(size.width == 1512)
23+
#expect(size.width == 1_512)
2324
}
2425

2526
/// Every one of these has to come back nil rather than a default. A default would open the
@@ -32,4 +33,15 @@ struct ScreenshotEnvironmentTests {
3233
func refusesMalformedInput(_ raw: String) {
3334
#expect(ScreenshotEnvironment.size(from: raw) == nil)
3435
}
36+
37+
@Test("The pinned frame is centred on the screen's visible frame")
38+
func pinnedFrameIsCentred() {
39+
let visible = NSRect(x: 0, y: 25, width: 2_560, height: 1_410)
40+
41+
let frame = ScreenshotEnvironment.pinnedFrame(size: CGSize(width: 1_512, height: 861), in: visible)
42+
43+
#expect(frame.size == CGSize(width: 1_512, height: 861))
44+
#expect(frame.midX == visible.midX)
45+
#expect(frame.midY == visible.midY)
46+
}
3547
}
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
//
2+
// EditorWindowInitialFrameTests.swift
3+
// TableProTests
4+
//
5+
6+
import AppKit
7+
import SwiftUI
8+
import Testing
9+
10+
@testable import TablePro
11+
12+
/// A connection window opened in the background under a pinned screenshot size came up with its
13+
/// sidebar's top row under the titlebar and its toolbar's trailing items in the overflow menu, and
14+
/// stayed that way. The pin resized the window from a `WindowAccessor` callback, which runs inside
15+
/// the window's layout pass.
16+
@Suite("Editor window initial frame", .serialized)
17+
@MainActor
18+
struct EditorWindowInitialFrameTests {
19+
private let pinnedSize = CGSize(width: 1_000, height: 700)
20+
21+
/// Built through the initializer every connection window comes from, so moving the pin anywhere
22+
/// after construction fails here: the window then starts at its content's own size.
23+
@Test("A connection window starts at its pinned size, before it is ever shown")
24+
func connectionWindowIsBuiltAtItsPinnedSize() throws {
25+
try withConnectionWindow { window in
26+
#expect(!window.isVisible)
27+
#expect(window.frame.size == pinnedSize)
28+
}
29+
}
30+
31+
/// The window's content view is the split view every pane hangs from, so it has to span exactly
32+
/// the window: the sidebar's top inset, the inspector divider and the toolbar sections that
33+
/// track both are all measured against it.
34+
@Test("A connection window's split view spans the pinned window once it is laid out")
35+
func splitViewSpansThePinnedWindow() throws {
36+
try withConnectionWindow { window in
37+
window.layoutIfNeeded()
38+
39+
#expect(window.contentView?.frame.size == pinnedSize)
40+
#expect(window.contentView?.frame.origin == .zero)
41+
}
42+
}
43+
44+
/// Why the pin cannot live anywhere a view reports its window from. The connection's content
45+
/// arrives in a pane that is already on the window, the way `refreshPanes` hands it over, and
46+
/// SwiftUI mounts its `WindowAccessor` while it renders inside the window's layout pass. A
47+
/// resize from there is applied to the content view twice. If this starts failing, AppKit has
48+
/// changed and the reasoning in `TabWindowController.placeInitialFrame(of:pinnedSize:)` should
49+
/// be measured again.
50+
@Test("A resize from a WindowAccessor callback leaves the content view out of step with the window")
51+
func resizingFromALayoutCallbackOvershootsTheContent() {
52+
let window = TabWindowController.makeEditorWindow()
53+
window.isReleasedWhenClosed = false
54+
defer { window.close() }
55+
let detail = Self.installSplit(in: window)
56+
window.layoutIfNeeded()
57+
let initialSize = window.frame.size
58+
let target = pinnedSize
59+
60+
detail.rootView = AnyView(Color.clear.background(WindowAccessor { accessed in
61+
accessed.setFrame(NSRect(origin: accessed.frame.origin, size: target), display: true)
62+
}))
63+
window.layoutIfNeeded()
64+
65+
#expect(window.frame.size == target)
66+
#expect(window.contentView?.frame.width == target.width + (target.width - initialSize.width))
67+
#expect(window.contentView?.frame.height == target.height + (target.height - initialSize.height))
68+
}
69+
70+
// MARK: - Helpers
71+
72+
/// No session and a workspace handed in whole, so nothing reaches the connection store. The
73+
/// window is never shown or closed: closing runs the controller's own teardown, which saves the
74+
/// frame into the real defaults and cancels connects.
75+
private func withConnectionWindow(_ body: (NSWindow) throws -> Void) throws {
76+
let connection = TestFixtures.makeConnection(name: "Pinned frame")
77+
let workspace = ConnectionWorkspace(
78+
connectionId: connection.id,
79+
payload: nil,
80+
autoConnect: false,
81+
payloadConnection: connection,
82+
session: nil,
83+
sessionState: nil,
84+
trailingPaneState: nil,
85+
phase: .connecting
86+
)
87+
let controller = TabWindowController(
88+
payload: EditorTabPayload(connectionId: connection.id),
89+
pinnedWindowSize: pinnedSize,
90+
adopting: workspace
91+
)
92+
let window = try #require(controller.window)
93+
defer {
94+
window.delegate = nil
95+
window.contentViewController = nil
96+
workspace.teardown()
97+
}
98+
try body(window)
99+
}
100+
101+
private static func installSplit(in window: NSWindow) -> NSHostingController<AnyView> {
102+
let split = NSSplitViewController()
103+
let sidebar = NSViewController()
104+
sidebar.view = NSView()
105+
split.addSplitViewItem(NSSplitViewItem(sidebarWithViewController: sidebar))
106+
let detail = NSHostingController(rootView: AnyView(Color.clear))
107+
detail.sizingOptions = []
108+
split.addSplitViewItem(NSSplitViewItem(viewController: detail))
109+
window.contentViewController = split
110+
return detail
111+
}
112+
}

0 commit comments

Comments
 (0)