From 04c0eebfb48cfaf4ab6cbeb55ab47faafff21bdb Mon Sep 17 00:00:00 2001 From: arzafran Date: Fri, 14 Aug 2026 10:00:51 -0300 Subject: [PATCH 1/2] polish: enforce split cap centrally, keyboard access, disabled-state split buttons - Split cap (4 panes) moves from a button-only check to the BonsplitDelegate shouldSplitPane veto, so Cmd+D, drag-to-edge, and socket splits respect it; session restore bypasses the veto so pre-cap layouts round-trip intact - Split capsule disables with a Split limit reached tooltip at the cap instead of vanishing - Anchor frame/bounds/window-join observers are per-object now; the global pair only serves the ancestor-resize path - ensureInstalled caches the terminal host instead of re-walking the window - Tab pills join the key-view loop (Full Keyboard Access gated), activate on Space/Return, draw a capsule focus ring --- Resources/Localizable.xcstrings | 17 ++++ Sources/WindowPaneChromePortal.swift | 131 ++++++++++++++++++++++----- Sources/Workspace+Bonsplit.swift | 14 +++ Sources/Workspace+Persistence.swift | 2 + Sources/Workspace.swift | 3 + 5 files changed, 145 insertions(+), 22 deletions(-) diff --git a/Resources/Localizable.xcstrings b/Resources/Localizable.xcstrings index 3d062889..60cd507e 100644 --- a/Resources/Localizable.xcstrings +++ b/Resources/Localizable.xcstrings @@ -13439,6 +13439,23 @@ } } } + }, + "tabBar.splitLimitReached": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "Split limit reached" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "分割の上限に達しました" + } + } + } } }, "version": "1.0" diff --git a/Sources/WindowPaneChromePortal.swift b/Sources/WindowPaneChromePortal.swift index c9082b25..b401669d 100644 --- a/Sources/WindowPaneChromePortal.swift +++ b/Sources/WindowPaneChromePortal.swift @@ -32,6 +32,8 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr private var bars: [PaneID: NativePaneTabBarView] = [:] private var descriptors: [PaneID: BonsplitPaneChromeDescriptor] = [:] private var observers: [NSObjectProtocol] = [] + private var anchorObservers: [PaneID: (anchor: NSView, tokens: [NSObjectProtocol])] = [:] + private weak var cachedTerminalHost: WindowTerminalHostView? private var isTornDown = false /// Workspace-level split controls, pinned to the terminal area's top-right like /// Maps' map controls; they act on the focused pane rather than per-pane copies. @@ -72,10 +74,16 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr deinit { for observer in observers { NotificationCenter.default.removeObserver(observer) } + for entry in anchorObservers.values { + for token in entry.tokens { NotificationCenter.default.removeObserver(token) } + } } func updatePaneChrome(_ descriptor: BonsplitPaneChromeDescriptor) { descriptors[descriptor.paneID] = descriptor + if let anchor = descriptor.anchorView { + observeAnchor(anchor, paneID: descriptor.paneID) + } ensureInstalled() #if DEBUG dlog( @@ -102,9 +110,8 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr private func updateClusters() { // Anchors can move to another window (workspace drag-out); their stale // descriptors must not steer this window's controls. - let visible = descriptors.values.filter { - $0.isVisible && $0.anchorView?.window === window - } + let inWindow = descriptors.values.filter { $0.anchorView?.window === window } + let visible = inWindow.filter(\.isVisible) guard let active = visible.first(where: { $0.isFocused }) ?? visible.first else { newTabCluster.isHidden = true splitCluster.isHidden = true @@ -126,9 +133,20 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr // Cap workspace splits at a 2x2-equivalent depth; deeper trees degenerate // into slivers. Checked here (live pane count) rather than at publish time, - // where it goes stale when panes collapse without a republish. - if visible.count < 4, active.showsSplitButtons { + // where it goes stale when panes collapse without a republish. At the cap + // the capsule stays visible but disabled so the affordance doesn't vanish. + // Counts registered panes, not visible ones: zoomed-away panes still + // exist, and the Workspace delegate vetoes splits on the same predicate. + if active.showsSplitButtons { + let canSplit = inWindow.count < SplitPolicy.maxPanesPerWorkspace splitCluster.setActions([active.onSplitRight, active.onSplitDown]) + splitCluster.setEnabled( + canSplit, + disabledTooltip: String( + localized: "tabBar.splitLimitReached", + defaultValue: "Split limit reached" + ) + ) splitCluster.isHidden = false hostView.addSubview(splitCluster) splitCluster.frame = NSRect( @@ -153,6 +171,9 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr guard matches else { return } descriptors.removeValue(forKey: paneID) bars.removeValue(forKey: paneID)?.removeFromSuperview() + if let entry = anchorObservers.removeValue(forKey: paneID) { + for token in entry.tokens { NotificationCenter.default.removeObserver(token) } + } updateClusters() // Split-tree churn can register two anchor instances for one pane; the // dying instance publishes last and its dismantle lands here, deleting the @@ -184,26 +205,18 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr self.scheduleSynchronizeAll() } }) - for name in [ - NSView.frameDidChangeNotification, - NSView.boundsDidChangeNotification, - BonsplitPaneChromeAnchorNotifications.anchorDidMoveToWindow, - ] { + // Split collapses and divider animation move the anchor via its + // ancestors — the anchor's own frame never changes, so resync whenever + // an ancestor of any anchor resizes (coalesced). Ancestors are arbitrary + // views, so this pair stays app-global with a cheap window gate; the + // anchors' own geometry is tracked per-object in observeAnchor(_:paneID:). + for name in [NSView.frameDidChangeNotification, NSView.boundsDidChangeNotification] { observers.append(center.addObserver(forName: name, object: nil, queue: .main) { [weak self] note in MainActor.assumeIsolated { guard let self, let view = note.object as? NSView, view.window === self.window else { return } - if let paneID = self.descriptors.first(where: { $0.value.anchorView === view })?.key { - // The anchor can join the window before the terminal host exists - // in the hierarchy; re-check installation before syncing. - self.ensureInstalled() - self.synchronize(paneID) - return - } - // Split collapses and divider animation move the anchor via its - // ancestors — the anchor's own frame never changes, so resync - // whenever an ancestor of any anchor resizes (coalesced). let ancestorOfAnchor = self.descriptors.values.contains { - $0.anchorView?.isDescendant(of: view) == true + guard let anchor = $0.anchorView, anchor !== view else { return false } + return anchor.isDescendant(of: view) } if ancestorOfAnchor { self.scheduleSynchronizeAll() } } @@ -232,9 +245,45 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr }) } + /// Tracks a single anchor's own frame/bounds/window-join notifications, + /// object-scoped so app-wide view churn never reaches these handlers. + private func observeAnchor(_ anchor: NSView, paneID: PaneID) { + if let existing = anchorObservers[paneID] { + if existing.anchor === anchor { return } + for token in existing.tokens { NotificationCenter.default.removeObserver(token) } + } + let center = NotificationCenter.default + var tokens: [NSObjectProtocol] = [] + for name in [ + NSView.frameDidChangeNotification, + NSView.boundsDidChangeNotification, + BonsplitPaneChromeAnchorNotifications.anchorDidMoveToWindow, + ] { + tokens.append(center.addObserver(forName: name, object: anchor, queue: .main) { [weak self] _ in + MainActor.assumeIsolated { + guard let self else { return } + // The anchor can join the window before the terminal host exists + // in the hierarchy; re-check installation before syncing. + self.ensureInstalled() + self.synchronize(paneID) + } + }) + } + anchorObservers[paneID] = (anchor, tokens) + } + private func ensureInstalled() { guard !isTornDown, supportsNativePaneChrome, let window else { return } - guard let terminalHost = findTerminalHost(in: window.contentView) else { + // Called from every descriptor update and geometry pass; a full-window + // recursive search each time is the dominant cost, so cache the host and + // re-find only when it leaves the window. + let terminalHost: WindowTerminalHostView + if let cached = cachedTerminalHost, cached.window === window { + terminalHost = cached + } else if let found = findTerminalHost(in: window.contentView) { + cachedTerminalHost = found + terminalHost = found + } else { #if DEBUG dlog("paneChrome.install.noTerminalHost win=\(window.windowNumber)") #endif @@ -371,6 +420,11 @@ final class WindowPaneChromePortalRegistry: NSObject, BonsplitPaneChromePortalBr private func teardown() { isTornDown = true descriptors.removeAll() + for entry in anchorObservers.values { + for token in entry.tokens { NotificationCenter.default.removeObserver(token) } + } + anchorObservers.removeAll() + cachedTerminalHost = nil bars.values.forEach { $0.removeFromSuperview() } bars.removeAll() splitCluster.isHidden = true @@ -516,6 +570,7 @@ private final class GlassIconClusterView: NSView { private let container = NSView(frame: .zero) private var buttons: [NSButton] = [] private var actions: [() -> Void] = [] + private var defaultTooltips: [String] = [] static let buttonWidth: CGFloat = 34 @@ -541,6 +596,7 @@ private final class GlassIconClusterView: NSView { buttons.append(button) } actions = Array(repeating: {}, count: symbols.count) + defaultTooltips = symbols.map(\.tooltip) } required init?(coder: NSCoder) { fatalError("init(coder:) has not been implemented") } @@ -552,6 +608,16 @@ private final class GlassIconClusterView: NSView { actions = newActions } + /// Disabled buttons keep the capsule visible (the affordance shouldn't + /// vanish at a limit) but explain themselves through the swapped tooltip. + func setEnabled(_ enabled: Bool, disabledTooltip: String? = nil) { + for (index, button) in buttons.enumerated() { + button.isEnabled = enabled + button.contentTintColor = enabled ? .secondaryLabelColor : .tertiaryLabelColor + button.toolTip = enabled ? defaultTooltips[index] : (disabledTooltip ?? defaultTooltips[index]) + } + } + override func layout() { super.layout() glass.frame = bounds @@ -761,6 +827,27 @@ private final class NativeTabPillControl: NSControl, NSMenuDelegate, NSDraggingS override func acceptsFirstMouse(for event: NSEvent?) -> Bool { true } + // Full Keyboard Access: the pill joins the key-view loop, activates on + // Space/Return, and draws the standard ring clipped to its capsule shape. + // Gated like NSButton — unconditional acceptance would let a plain click + // pull first responder off the terminal surface. + override var acceptsFirstResponder: Bool { NSApp.isFullKeyboardAccessEnabled } + + override var focusRingMaskBounds: NSRect { bounds } + + override func drawFocusRingMask() { + NSBezierPath(roundedRect: bounds, xRadius: 14, yRadius: 14).fill() + } + + override func keyDown(with event: NSEvent) { + switch event.keyCode { + case 49, 36, 76: // space, return, keypad enter + selectAction?() + default: + super.keyDown(with: event) + } + } + override func mouseEntered(with event: NSEvent) { onHoverChanged?(true) } diff --git a/Sources/Workspace+Bonsplit.swift b/Sources/Workspace+Bonsplit.swift index 080e1fec..46985c9b 100644 --- a/Sources/Workspace+Bonsplit.swift +++ b/Sources/Workspace+Bonsplit.swift @@ -10,7 +10,21 @@ import Darwin import Network import CoreText +/// One predicate for the workspace split cap: the pane-chrome split buttons +/// disable at this count, and the delegate veto below enforces it for every +/// other entry point (Cmd+D, drag-to-edge, socket commands). +enum SplitPolicy { + static let maxPanesPerWorkspace = 4 +} + extension Workspace: @preconcurrency BonsplitDelegate { + func splitTabBar(_ controller: BonsplitController, shouldSplitPane pane: PaneID, orientation: SplitOrientation) -> Bool { + // Deeper than 2x2 degenerates into slivers; the split-button capsule + // shows a "Split limit reached" tooltip at the same threshold. Session + // restore bypasses the cap — pre-cap layouts must round-trip intact. + isRestoringSessionLayout || controller.allPaneIds.count < SplitPolicy.maxPanesPerWorkspace + } + @MainActor private func shouldCloseWorkspaceOnLastSurface(for tabId: TabID) -> Bool { let manager = owningTabManager ?? AppDelegate.shared?.tabManagerFor(tabId: id) ?? AppDelegate.shared?.tabManager diff --git a/Sources/Workspace+Persistence.swift b/Sources/Workspace+Persistence.swift index 1de8885e..0ed18dcc 100644 --- a/Sources/Workspace+Persistence.swift +++ b/Sources/Workspace+Persistence.swift @@ -120,7 +120,9 @@ extension Workspace { for panelSnapshot in snapshot.panels where panelSnapshotsById[panelSnapshot.id] == nil { panelSnapshotsById[panelSnapshot.id] = panelSnapshot } + isRestoringSessionLayout = true let leafEntries = restoreSessionLayout(snapshot.layout) + isRestoringSessionLayout = false var oldToNewPanelIds: [UUID: UUID] = [:] for entry in leafEntries { diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index 209a70a3..ef5347cd 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -45,6 +45,9 @@ final class Workspace: Identifiable, ObservableObject { /// When true, suppresses auto-creation in didSplitPane (programmatic splits handle their own panels) var isProgrammaticSplit = false + /// When true, the split-cap delegate veto is bypassed: session restore must + /// rebuild pre-cap layouts (5+ panes) without losing panes. + var isRestoringSessionLayout = false var debugStressPreloadSelectionDepth = 0 /// Last terminal panel used as an inheritance source (typically last focused terminal). From 8bc0ce72401533066bfd3bcc7e34d6c113b4a7fb Mon Sep 17 00:00:00 2001 From: arzafran Date: Fri, 14 Aug 2026 10:08:16 -0300 Subject: [PATCH 2/2] test: adapt full-suite tests to the 4-pane split cap, add cap regression test test_g19_alternating_close_reverse and test_new_tab_interactive_after_splits each created 5 panes; the new shouldSplitPane veto refuses the 4th split, so both would die mid-setup. Both now build exactly 4 panes. New test_split_pane_cap asserts the 4th split is refused and the layout survives. --- .../test_new_tab_interactive_after_splits.py | 3 +- tests_v2/test_split_pane_cap.py | 62 +++++++++++++++++++ tests_v2/test_visual_screenshots.py | 9 +-- 3 files changed, 69 insertions(+), 5 deletions(-) create mode 100644 tests_v2/test_split_pane_cap.py diff --git a/tests_v2/test_new_tab_interactive_after_splits.py b/tests_v2/test_new_tab_interactive_after_splits.py index f1cd16ea..c52f35ef 100644 --- a/tests_v2/test_new_tab_interactive_after_splits.py +++ b/tests_v2/test_new_tab_interactive_after_splits.py @@ -191,7 +191,8 @@ def main() -> int: time.sleep(0.35) # Create a multi-pane layout to exercise bonsplit/SwiftUI focus races. - for _ in range(4): + # 3 splits = 4 panes, the workspace split cap; a 4th split is refused. + for _ in range(3): c.new_split("right") time.sleep(0.25) diff --git a/tests_v2/test_split_pane_cap.py b/tests_v2/test_split_pane_cap.py new file mode 100644 index 00000000..a13c53a9 --- /dev/null +++ b/tests_v2/test_split_pane_cap.py @@ -0,0 +1,62 @@ +#!/usr/bin/env python3 +""" +Regression test: the workspace split cap (4 panes). + +Three splits from a fresh workspace succeed (1 baseline pane + 3 = 4 panes, +exactly at the cap). A fourth split must be refused — surface.split returns +no surface_id — and the pane count must stay at 4. The refusal must not +disturb the existing panes. +""" + +import os +import sys +import time +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) +from cmux import cmux, cmuxError + + +SOCKET_PATH = os.environ.get("PROGRAMA_SOCKET", "/tmp/programa-debug.sock") + +MAX_PANES = 4 +SPLIT_WAIT = 0.25 + + +def main() -> int: + with cmux(SOCKET_PATH) as c: + c.activate_app() + time.sleep(0.2) + + c.new_workspace() + time.sleep(0.35) + + # Splits up to the cap succeed. + for i in range(MAX_PANES - 1): + c.new_split("right" if i % 2 == 0 else "down") + time.sleep(SPLIT_WAIT) + + panes = c.list_panes() + if len(panes) != MAX_PANES: + raise cmuxError(f"expected {MAX_PANES} panes at the cap, got {len(panes)}: {panes}") + + # The split past the cap is refused. + try: + c.new_split("right") + except cmuxError: + pass + else: + raise cmuxError(f"split past the {MAX_PANES}-pane cap unexpectedly succeeded") + time.sleep(SPLIT_WAIT) + + # The refusal left the existing layout intact. + panes = c.list_panes() + if len(panes) != MAX_PANES: + raise cmuxError(f"expected {MAX_PANES} panes after refused split, got {len(panes)}: {panes}") + + print("OK: split refused at the 4-pane cap; layout intact") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests_v2/test_visual_screenshots.py b/tests_v2/test_visual_screenshots.py index 2c1514a8..b1e8c70e 100644 --- a/tests_v2/test_visual_screenshots.py +++ b/tests_v2/test_visual_screenshots.py @@ -744,16 +744,17 @@ def test_g19_alternating_close_reverse(client: cmux) -> StateChange: """G19: Alternating splits then close all in reverse.""" change = StateChange( name="Alternating Splits: Close in Reverse", group="G", - description="right, down, right, down → close all in reverse order", - command="split right/down/right/down; close 4,3,2,1", + description="right, down, right → close all in reverse order", + command="split right/down/right; close 3,2,1", ) - directions = ["right", "down", "right", "down"] + # 3 splits = 4 panes, the workspace split cap; a 4th split is refused. + directions = ["right", "down", "right"] for d in directions: client.new_split(d) time.sleep(SPLIT_WAIT) change.before, change.before_state = capture(client, "g19_before") try: - for i in range(4, 0, -1): + for i in range(3, 0, -1): surfaces = client.list_surfaces() n = len(surfaces) if n <= 1: