From 9bfa3b9143f606598bb80d3339a21af69f87fe64 Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Fri, 13 Mar 2026 12:43:24 -0700 Subject: [PATCH] Make Cmd+W close the window when it closes the last terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, Cmd+W on the last surface kept the workspace alive with a replacement shell, unless the user toggled a hidden setting. This was confusing—users expect Cmd+W to close the window when there's nothing left. Now Cmd+W (and the tab-strip X button) always close the workspace when they close its last surface, and close the window when that was the last workspace. Internal/programmatic closes (e.g. process exit, panel moves) still spawn a replacement shell so the workspace stays alive. Key changes: - Track explicit user close gestures via markExplicitClose / onTabCloseRequest - Remove the LastSurfaceCloseShortcutSettings toggle (now always-on) - Use window.performClose for last-workspace window close - Update tests to match the new behavior Co-Authored-By: Claude Opus 4.6 --- Sources/TabManager.swift | 36 ++-- Sources/Workspace.swift | 22 ++- Sources/cmuxApp.swift | 34 +--- .../AppDelegateShortcutRoutingTests.swift | 42 +++++ cmuxTests/CmuxWebViewKeyEquivalentTests.swift | 173 +++++------------- vendor/bonsplit | 2 +- 6 files changed, 129 insertions(+), 180 deletions(-) diff --git a/Sources/TabManager.swift b/Sources/TabManager.swift index c7beb573..fc3b596c 100644 --- a/Sources/TabManager.swift +++ b/Sources/TabManager.swift @@ -51,18 +51,6 @@ enum WorkspaceAutoReorderSettings { } } -enum LastSurfaceCloseShortcutSettings { - static let key = "closeWorkspaceOnLastSurfaceShortcut" - static let defaultValue = false - - static func closesWorkspace(defaults: UserDefaults = .standard) -> Bool { - if defaults.object(forKey: key) == nil { - return defaultValue - } - return defaults.bool(forKey: key) - } -} - enum SidebarBranchLayoutSettings { static let key = "sidebarBranchVerticalLayout" static let defaultVerticalLayout = true @@ -1607,15 +1595,17 @@ class TabManager: ObservableObject { alert.addButton(withTitle: String(localized: "common.close", defaultValue: "Close")) alert.addButton(withTitle: String(localized: "common.cancel", defaultValue: "Cancel")) + if let closeButton = alert.buttons.first { + // Keep Return/Enter bound to the primary destructive action for all close prompts. + alert.window.defaultButtonCell = closeButton.cell as? NSButtonCell + } + // macOS convention: Cmd+D = confirm destructive close (e.g. "Don't Save"). // We only opt into this for the "close last workspace => close window" path to avoid // conflicting with app-level Cmd+D (split right) during normal usage. if acceptCmdD, let closeButton = alert.buttons.first { closeButton.keyEquivalent = "d" closeButton.keyEquivalentModifierMask = [.command] - - // Keep Return/Enter behavior by explicitly setting the default button cell. - alert.window.defaultButtonCell = closeButton.cell as? NSButtonCell } return alert.runModal() == .alertFirstButtonReturn @@ -1741,7 +1731,11 @@ class TabManager: ObservableObject { } if tabs.count <= 1 { // Last workspace in this window: close the window (Cmd+Shift+W behavior). - AppDelegate.shared?.closeMainWindowContainingTabId(workspace.id) + if let window { + window.performClose(nil) + } else { + AppDelegate.shared?.closeMainWindowContainingTabId(workspace.id) + } } else { closeWorkspace(workspace) } @@ -1771,14 +1765,16 @@ class TabManager: ObservableObject { dlog( "surface.close.shortcut.begin tab=\(tab.id.uuidString.prefix(5)) " + "panel=\(panelId.uuidString.prefix(5)) kind=\(panelKind) " + - "panelCount=\(tab.panels.count) bonsplitTabs=\(bonsplitTabCount) " + - "closeWorkspaceOnLastSurfaceSetting=\(LastSurfaceCloseShortcutSettings.closesWorkspace() ? 1 : 0)" + "panelCount=\(tab.panels.count) bonsplitTabs=\(bonsplitTabCount)" ) #endif // Route Cmd+W through Bonsplit/Workspace close handling so it matches the tab close - // button, including shared confirmation, setting-controlled last-surface behavior, and - // replacement-panel flow. + // button, including shared confirmation, last-surface workspace/window-close behavior, + // and the usual replacement-panel flow when the close does not collapse the workspace. + if let surfaceId = tab.surfaceIdFromPanelId(panelId) { + tab.markExplicitClose(surfaceId: surfaceId) + } let closed = tab.closePanel(panelId) #if DEBUG dlog( diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index 8a26b3fd..1ba2fd77 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -1182,6 +1182,9 @@ final class Workspace: Identifiable, ObservableObject { bonsplitController.onExternalTabDrop = { [weak self] request in self?.handleExternalTabDrop(request) ?? false } + bonsplitController.onTabCloseRequest = { [weak self] tabId, _ in + self?.markExplicitClose(surfaceId: tabId) + } // Set ourselves as delegate bonsplitController.delegate = self @@ -1228,6 +1231,10 @@ final class Workspace: Identifiable, ObservableObject { /// Prevents repeated close gestures (e.g., middle-click spam) from stacking dialogs. private var pendingCloseConfirmTabIds: Set = [] + /// Tab IDs whose next close attempt came from an explicit user close gesture + /// (Cmd+W or the tab-strip X button), rather than an internal close/move flow. + private var explicitUserCloseTabIds: Set = [] + /// Deterministic tab selection to apply after a tab closes. /// Keyed by the closing tab ID, value is the tab ID we want to select next. private var postCloseSelectTabId: [TabID: TabID] = [:] @@ -1294,6 +1301,10 @@ final class Workspace: Identifiable, ObservableObject { surfaceIdToPanelId[surfaceId] } + func markExplicitClose(surfaceId: TabID) { + explicitUserCloseTabIds.insert(surfaceId) + } + func surfaceIdFromPanelId(_ panelId: UUID) -> TabID? { surfaceIdToPanelId.first { $0.value == panelId }?.key } @@ -4118,8 +4129,7 @@ extension Workspace: BonsplitDelegate { @MainActor private func shouldCloseWorkspaceOnLastSurface(for tabId: TabID) -> Bool { let manager = owningTabManager ?? AppDelegate.shared?.tabManagerFor(tabId: id) ?? AppDelegate.shared?.tabManager - guard LastSurfaceCloseShortcutSettings.closesWorkspace(), - panels.count <= 1, + guard panels.count <= 1, panelIdFromSurfaceId(tabId) != nil, let manager, manager.tabs.contains(where: { $0.id == id }) else { @@ -4137,6 +4147,10 @@ extension Workspace: BonsplitDelegate { alert.addButton(withTitle: String(localized: "dialog.closeTab.close", defaultValue: "Close")) alert.addButton(withTitle: String(localized: "common.cancel", defaultValue: "Cancel")) + if let closeButton = alert.buttons.first { + alert.window.defaultButtonCell = closeButton.cell as? NSButtonCell + } + // Prefer a sheet if we can find a window, otherwise fall back to modal. if let window = NSApp.keyWindow ?? NSApp.mainWindow { return await withCheckedContinuation { continuation in @@ -4530,6 +4544,8 @@ extension Workspace: BonsplitDelegate { } } + let explicitUserClose = explicitUserCloseTabIds.remove(tab.id) != nil + if forceCloseTabIds.contains(tab.id) { stageClosedBrowserRestoreSnapshotIfNeeded(for: tab, inPane: pane) recordPostCloseSelection() @@ -4543,7 +4559,7 @@ extension Workspace: BonsplitDelegate { return false } - if shouldCloseWorkspaceOnLastSurface(for: tab.id) { + if explicitUserClose && shouldCloseWorkspaceOnLastSurface(for: tab.id) { clearStagedClosedBrowserRestoreSnapshot(for: tab.id) owningTabManager?.closeWorkspaceWithConfirmation(self) return false diff --git a/Sources/cmuxApp.swift b/Sources/cmuxApp.swift index 161c0cfc..0df1321e 100644 --- a/Sources/cmuxApp.swift +++ b/Sources/cmuxApp.swift @@ -452,10 +452,9 @@ struct cmuxApp: App { Divider() // Terminal semantics: - // Cmd+W closes the focused tab/surface (with confirmation if needed) and keeps - // the workspace open by default. Cmd+Shift+W is the explicit workspace-close - // action, unless the user opts into closing the workspace when its last surface - // is closed. + // Cmd+W closes the focused tab/surface (with confirmation if needed). When that + // was the last surface in the workspace, cmux removes the workspace and closes + // the window if it was also the last workspace. Button(String(localized: "menu.file.closeTab", defaultValue: "Close Tab")) { closePanelOrWindow() } @@ -3077,8 +3076,6 @@ struct SettingsView: View { @AppStorage(ShortcutHintDebugSettings.alwaysShowHintsKey) private var alwaysShowShortcutHints = ShortcutHintDebugSettings.defaultAlwaysShowHints @AppStorage(WorkspacePlacementSettings.placementKey) private var newWorkspacePlacement = WorkspacePlacementSettings.defaultPlacement.rawValue - @AppStorage(LastSurfaceCloseShortcutSettings.key) - private var closeWorkspaceOnLastSurfaceShortcut = LastSurfaceCloseShortcutSettings.defaultValue @AppStorage(WorkspaceAutoReorderSettings.key) private var workspaceAutoReorder = WorkspaceAutoReorderSettings.defaultValue @AppStorage(SidebarWorkspaceDetailSettings.hideAllDetailsKey) private var sidebarHideAllDetails = SidebarWorkspaceDetailSettings.defaultHideAllDetails @@ -3124,19 +3121,6 @@ struct SettingsView: View { NewWorkspacePlacement(rawValue: newWorkspacePlacement) ?? WorkspacePlacementSettings.defaultPlacement } - private var closeWorkspaceOnLastSurfaceShortcutSubtitle: String { - if closeWorkspaceOnLastSurfaceShortcut { - return String( - localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOn", - defaultValue: "Closing the last surface also closes its workspace." - ) - } - return String( - localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOff", - defaultValue: "Closing the last surface keeps the workspace open. Use Cmd+Shift+W to close a workspace explicitly." - ) - } - private var selectedSidebarActiveTabIndicatorStyle: SidebarActiveTabIndicatorStyle { SidebarActiveTabIndicatorSettings.resolvedStyle(rawValue: sidebarActiveTabIndicatorStyle) } @@ -3505,17 +3489,6 @@ struct SettingsView: View { SettingsCardDivider() - SettingsCardRow( - String(localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut", defaultValue: "Closing Last Surface Closes Workspace"), - subtitle: closeWorkspaceOnLastSurfaceShortcutSubtitle - ) { - Toggle("", isOn: $closeWorkspaceOnLastSurfaceShortcut) - .labelsHidden() - .controlSize(.small) - } - - SettingsCardDivider() - SettingsCardRow( String(localized: "settings.app.reorderOnNotification", defaultValue: "Reorder on Notification"), subtitle: String(localized: "settings.app.reorderOnNotification.subtitle", defaultValue: "Move workspaces to the top when they receive a notification. Disable for stable shortcut positions.") @@ -4485,7 +4458,6 @@ struct SettingsView: View { ShortcutHintDebugSettings.resetVisibilityDefaults() alwaysShowShortcutHints = ShortcutHintDebugSettings.defaultAlwaysShowHints newWorkspacePlacement = WorkspacePlacementSettings.defaultPlacement.rawValue - closeWorkspaceOnLastSurfaceShortcut = LastSurfaceCloseShortcutSettings.defaultValue workspaceAutoReorder = WorkspaceAutoReorderSettings.defaultValue sidebarHideAllDetails = SidebarWorkspaceDetailSettings.defaultHideAllDetails sidebarShowNotificationMessage = SidebarWorkspaceDetailSettings.defaultShowNotificationMessage diff --git a/cmuxTests/AppDelegateShortcutRoutingTests.swift b/cmuxTests/AppDelegateShortcutRoutingTests.swift index 730a660c..26c25bf2 100644 --- a/cmuxTests/AppDelegateShortcutRoutingTests.swift +++ b/cmuxTests/AppDelegateShortcutRoutingTests.swift @@ -529,6 +529,48 @@ final class AppDelegateShortcutRoutingTests: XCTestCase { XCTAssertNil(self.window(withId: windowId), "Confirming Cmd+Ctrl+W should close the window") } + func testCmdWClosesWindowWhenClosingLastSurfaceInLastWorkspace() { + guard let appDelegate = AppDelegate.shared else { + XCTFail("Expected AppDelegate.shared") + return + } + + let windowId = appDelegate.createMainWindow() + defer { closeWindow(withId: windowId) } + + guard let targetWindow = window(withId: windowId), + let manager = appDelegate.tabManagerFor(windowId: windowId) else { + XCTFail("Expected test window and manager") + return + } + + XCTAssertEqual(manager.tabs.count, 1) + XCTAssertEqual(manager.tabs[0].panels.count, 1) + + guard let event = makeKeyDownEvent( + key: "w", + modifiers: [.command], + keyCode: 13, + windowNumber: targetWindow.windowNumber + ) else { + XCTFail("Failed to construct Cmd+W event") + return + } + +#if DEBUG + XCTAssertTrue(appDelegate.debugHandleCustomShortcut(event: event)) +#else + XCTFail("debugHandleCustomShortcut is only available in DEBUG") +#endif + + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) + + XCTAssertNil( + self.window(withId: windowId), + "Cmd+W on the last surface in the last workspace should close the window" + ) + } + func testCmdPhysicalIWithDvorakCharactersDoesNotTriggerShowNotifications() { guard let appDelegate = AppDelegate.shared else { XCTFail("Expected AppDelegate.shared") diff --git a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift index 7eb7460f..c01ae047 100644 --- a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift +++ b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift @@ -4802,43 +4802,6 @@ final class WorkspaceAutoReorderSettingsTests: XCTestCase { } } -final class LastSurfaceCloseShortcutSettingsTests: XCTestCase { - func testDefaultKeepsWorkspaceOpen() { - let suiteName = "LastSurfaceCloseShortcutSettingsTests.Default.\(UUID().uuidString)" - guard let defaults = UserDefaults(suiteName: suiteName) else { - XCTFail("Failed to create isolated UserDefaults suite") - return - } - defer { defaults.removePersistentDomain(forName: suiteName) } - - XCTAssertFalse(LastSurfaceCloseShortcutSettings.closesWorkspace(defaults: defaults)) - } - - func testStoredTrueClosesWorkspace() { - let suiteName = "LastSurfaceCloseShortcutSettingsTests.Enabled.\(UUID().uuidString)" - guard let defaults = UserDefaults(suiteName: suiteName) else { - XCTFail("Failed to create isolated UserDefaults suite") - return - } - defer { defaults.removePersistentDomain(forName: suiteName) } - - defaults.set(true, forKey: LastSurfaceCloseShortcutSettings.key) - XCTAssertTrue(LastSurfaceCloseShortcutSettings.closesWorkspace(defaults: defaults)) - } - - func testStoredFalseKeepsWorkspaceOpen() { - let suiteName = "LastSurfaceCloseShortcutSettingsTests.Disabled.\(UUID().uuidString)" - guard let defaults = UserDefaults(suiteName: suiteName) else { - XCTFail("Failed to create isolated UserDefaults suite") - return - } - defer { defaults.removePersistentDomain(forName: suiteName) } - - defaults.set(false, forKey: LastSurfaceCloseShortcutSettings.key) - XCTAssertFalse(LastSurfaceCloseShortcutSettings.closesWorkspace(defaults: defaults)) - } -} - final class SidebarBranchLayoutSettingsTests: XCTestCase { func testDefaultUsesVerticalLayout() { let suiteName = "SidebarBranchLayoutSettingsTests.Default.\(UUID().uuidString)" @@ -5397,31 +5360,61 @@ final class TabManagerCloseWorkspacesWithConfirmationTests: XCTestCase { @MainActor final class TabManagerCloseCurrentPanelTests: XCTestCase { - func testCloseCurrentPanelKeepsWorkspaceOpenWhenItOwnsTheLastSurface() { + func testCloseCurrentPanelClosesWorkspaceWhenItOwnsTheLastSurface() { let manager = TabManager() - guard let workspace = manager.selectedWorkspace, - let initialPanelId = workspace.focusedPanelId else { - XCTFail("Expected selected workspace and focused panel") + let firstWorkspace = manager.tabs[0] + let secondWorkspace = manager.addWorkspace() + manager.selectWorkspace(secondWorkspace) + + guard let secondPanelId = secondWorkspace.focusedPanelId else { + XCTFail("Expected focused panel in selected workspace") return } - let initialWorkspaceId = workspace.id - XCTAssertEqual(manager.tabs.count, 1) - XCTAssertEqual(workspace.panels.count, 1) + XCTAssertEqual(manager.selectedTabId, secondWorkspace.id) + XCTAssertEqual(secondWorkspace.panels.count, 1) manager.closeCurrentPanelWithConfirmation() drainMainQueue() drainMainQueue() - XCTAssertEqual(manager.tabs.count, 1, "Closing the last surface should not remove the workspace") - XCTAssertEqual(manager.selectedTabId, initialWorkspaceId) - XCTAssertEqual(manager.tabs.first?.id, initialWorkspaceId) - XCTAssertNil(workspace.panels[initialPanelId], "Expected the original surface to be closed") - XCTAssertEqual(workspace.panels.count, 1, "Expected the workspace to stay alive with a replacement surface") - XCTAssertNotEqual(workspace.focusedPanelId, initialPanelId) + XCTAssertEqual(manager.tabs.map(\.id), [firstWorkspace.id]) + XCTAssertEqual(manager.selectedTabId, firstWorkspace.id) + XCTAssertNil(secondWorkspace.panels[secondPanelId]) + XCTAssertTrue(secondWorkspace.panels.isEmpty) } - func testClosePanelButtonKeepsWorkspaceOpenWhenItOwnsTheLastSurface() { + func testClosePanelButtonClosesWorkspaceWhenItOwnsTheLastSurface() { + let manager = TabManager() + let firstWorkspace = manager.tabs[0] + let secondWorkspace = manager.addWorkspace() + manager.selectWorkspace(secondWorkspace) + + guard let secondPanelId = secondWorkspace.focusedPanelId else { + XCTFail("Expected focused panel in selected workspace") + return + } + + XCTAssertEqual(manager.selectedTabId, secondWorkspace.id) + XCTAssertEqual(secondWorkspace.panels.count, 1) + + guard let secondSurfaceId = secondWorkspace.surfaceIdFromPanelId(secondPanelId) else { + XCTFail("Expected bonsplit surface ID for focused panel") + return + } + + secondWorkspace.markExplicitClose(surfaceId: secondSurfaceId) + XCTAssertFalse(secondWorkspace.closePanel(secondPanelId)) + drainMainQueue() + drainMainQueue() + + XCTAssertEqual(manager.tabs.map(\.id), [firstWorkspace.id]) + XCTAssertEqual(manager.selectedTabId, firstWorkspace.id) + XCTAssertNil(secondWorkspace.panels[secondPanelId]) + XCTAssertTrue(secondWorkspace.panels.isEmpty) + } + + func testGenericClosePanelKeepsWorkspaceOpenWithoutExplicitCloseMarker() { let manager = TabManager() guard let workspace = manager.selectedWorkspace, let initialPanelId = workspace.focusedPanelId else { @@ -5437,85 +5430,15 @@ final class TabManagerCloseCurrentPanelTests: XCTestCase { drainMainQueue() drainMainQueue() - XCTAssertEqual(manager.tabs.count, 1, "Closing the last surface should not remove the workspace") + XCTAssertEqual(manager.tabs.count, 1) XCTAssertEqual(manager.selectedTabId, initialWorkspaceId) XCTAssertEqual(manager.tabs.first?.id, initialWorkspaceId) - XCTAssertNil(workspace.panels[initialPanelId], "Expected the original surface to be closed") - XCTAssertEqual(workspace.panels.count, 1, "Expected the workspace to stay alive with a replacement surface") + XCTAssertNil(workspace.panels[initialPanelId]) + XCTAssertEqual(workspace.panels.count, 1) XCTAssertNotEqual(workspace.focusedPanelId, initialPanelId) } - func testCloseCurrentPanelClosesWorkspaceWhenLastSurfaceShortcutSettingEnabled() { - let defaults = UserDefaults.standard - let originalSetting = defaults.object(forKey: LastSurfaceCloseShortcutSettings.key) - defaults.set(true, forKey: LastSurfaceCloseShortcutSettings.key) - defer { - if let originalSetting { - defaults.set(originalSetting, forKey: LastSurfaceCloseShortcutSettings.key) - } else { - defaults.removeObject(forKey: LastSurfaceCloseShortcutSettings.key) - } - } - - let manager = TabManager() - let firstWorkspace = manager.tabs[0] - let secondWorkspace = manager.addWorkspace() - manager.selectWorkspace(secondWorkspace) - - XCTAssertEqual(manager.selectedTabId, secondWorkspace.id) - XCTAssertEqual(secondWorkspace.panels.count, 1) - - manager.closeCurrentPanelWithConfirmation() - - XCTAssertEqual(manager.tabs.map(\.id), [firstWorkspace.id]) - XCTAssertEqual(manager.selectedTabId, firstWorkspace.id) - } - - func testClosePanelButtonClosesWorkspaceWhenLastSurfaceShortcutSettingEnabled() { - let defaults = UserDefaults.standard - let originalSetting = defaults.object(forKey: LastSurfaceCloseShortcutSettings.key) - defaults.set(true, forKey: LastSurfaceCloseShortcutSettings.key) - defer { - if let originalSetting { - defaults.set(originalSetting, forKey: LastSurfaceCloseShortcutSettings.key) - } else { - defaults.removeObject(forKey: LastSurfaceCloseShortcutSettings.key) - } - } - - let manager = TabManager() - let firstWorkspace = manager.tabs[0] - let secondWorkspace = manager.addWorkspace() - - manager.selectWorkspace(secondWorkspace) - guard let secondPanelId = secondWorkspace.focusedPanelId else { - XCTFail("Expected focused panel in selected workspace") - return - } - - XCTAssertEqual(manager.selectedTabId, secondWorkspace.id) - XCTAssertEqual(secondWorkspace.panels.count, 1) - - XCTAssertFalse(secondWorkspace.closePanel(secondPanelId)) - drainMainQueue() - drainMainQueue() - - XCTAssertEqual(manager.tabs.map(\.id), [firstWorkspace.id]) - XCTAssertEqual(manager.selectedTabId, firstWorkspace.id) - } - - func testCloseCurrentPanelWithLegacySettingIgnoresStaleSurfaceId() { - let defaults = UserDefaults.standard - let originalSetting = defaults.object(forKey: LastSurfaceCloseShortcutSettings.key) - defaults.set(true, forKey: LastSurfaceCloseShortcutSettings.key) - defer { - if let originalSetting { - defaults.set(originalSetting, forKey: LastSurfaceCloseShortcutSettings.key) - } else { - defaults.removeObject(forKey: LastSurfaceCloseShortcutSettings.key) - } - } - + func testCloseCurrentPanelIgnoresStaleSurfaceId() { let manager = TabManager() let firstWorkspace = manager.tabs[0] let secondWorkspace = manager.addWorkspace() diff --git a/vendor/bonsplit b/vendor/bonsplit index fa452db1..73c1ef2d 160000 --- a/vendor/bonsplit +++ b/vendor/bonsplit @@ -1 +1 @@ -Subproject commit fa452db181f361514087558a29204bda7e38218f +Subproject commit 73c1ef2df9a6c8a2837212ecce900794d0f21826