refactor(window): collapse 3 NSHostingControllers to 1 via NavigationSplitView#1032
Closed
refactor(window): collapse 3 NSHostingControllers to 1 via NavigationSplitView#1032
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ng-root-window # Conflicts: # TablePro/Core/Services/Infrastructure/MainSplitViewController.swift
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Architectural cleanup: replace
MainSplitViewController(NSSplitViewController + 3 separate NSHostingControllers for sidebar/detail/inspector) with a singleNSHostingController<MainEditorRootView>whose body isNavigationSplitView { sidebar } detail: { detail } .inspector(...). The single SwiftUI rendering root replaces three independent ones.This is option β from the perf refactor planning thread. Ships in one PR. Window/tab/route management (TabWindowController + EditorWindow + WindowManager) is intentionally untouched — that's the WindowGroup migration which is a separate, larger, runtime-test-required PR.
What changes
Deleted:
MainSplitViewController.swift(~497 lines) — NSSplitViewController shell with 3 split itemsSidebarContainerViewController.swift(~132 lines) — NSViewController wrapper that hosted an NSSearchField above an NSHostingControllerInspectorVisibilityProxyprotocol uses (now redundant)Added:
MainEditorWindowState.swift—@Observableclass that ownscurrentSession,sessionState,rightPanelState,inspectorPresented,sidebarColumnVisibility,windowTitle. Replaces the state-owning portion of MainSplitViewController. HandlesconnectionStatusDidChangeobservation.MainEditorRootView.swift— SwiftUI root.NavigationSplitView { sidebar } detail: { MainContentView } .inspector(isPresented:). Sidebar search uses.searchable(text:placement:.sidebar)— replaces SidebarContainerViewController's NSSearchField.Modified:
TabWindowController.swift— instantiatesMainEditorWindowState+NSHostingController<MainEditorRootView>instead ofMainSplitViewController. Owns theMainWindowToolbardirectly (was previously installed by the split VC). ObserveswindowState.sessionStateso the toolbar can be installed when a delayed connection completes.MainContentCoordinator.swift—inspectorProxy: InspectorVisibilityProxy?andsplitViewController: MainSplitViewController?weak refs replaced witheditorWindowState: MainEditorWindowState?. Inspector toggle, sidebar tab switch, etc. all flow through this object.MainWindowToolbar.swift—coordinator.splitViewController?.setSidebarTab(...)becomescoordinator.editorWindowState?.setSidebarTab(...). NSSplitView KVO observer (which depended on the deletedMainSplitViewController.splitView) replaced by adding_ = coordinator.editorWindowState?.sidebarColumnVisibilityto the existingwithObservationTrackingloop. Cleaner — observation framework instead of NSNotification fan-out.MainContentView.swift/MainContentCommandActions.swift/MainContentView+Setup.swift— updated 3 call sites to use the neweditorWindowStateAPI.Net code: -202 lines (522 added, 724 deleted across 10 files).
Why this is the right scope
Per WWDC 2020 "Use SwiftUI with AppKit" and Apple's apps (Xcode, Pages, Numbers): the canonical pattern is
NSWindowController+ a singleNSHostingControllercontaining the entire SwiftUI tree. Multiple NSHostingControllers per window is a known performance footgun — each is its own SwiftUI rendering root with independentViewGraph,AttributeGraph, layout engine, and AppKit constraint negotiation.The previous architecture had 3 hosting roots (sidebar + detail + inspector) plus 10 more in
MainWindowToolbar's items. Each root re-evaluated its body and negotiated intrinsic sizes throughNSHostingView.SizeConstraints.update(from:)independently. The 1.71s connect-window-open spike captured in the Time Profiler trace had_layoutSubtreeWithOldSizecascading 6 levels deep, withSizeFittingLayoutComputer.Engine.sizeThatFitsandNSAttributedString.MetricsCache.metricsas leaves.Collapsing the 3 layout-region hosting roots into one means:
ViewGraph, oneAttributeGraphfor sidebar+detail+inspector layout_layoutSubtreeWithOldSizeinstead of threeThe 10 toolbar hosting roots stay for now (NSToolbar.delegate-driven items can't directly become SwiftUI
ToolbarContentwhile the window is hosted via NSHostingController — SwiftUI's.toolbarmodifier needs scene context). Their migration is part of the WindowGroup PR.Test plan (caller responsibility)
Critical paths to verify:
dataGrid.autoShowInspectoris on)commandActions.closeTab()(EditorWindow override unchanged)inspectorPresentedand sidebar column visibility across launchesIf any of these break, follow-up fix PRs are expected.
Build & lint