From bdd95902f245d410c26855dd3745cc00856a8d11 Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Tue, 17 Mar 2026 22:36:32 -0700 Subject: [PATCH] Restore last-surface close preference toggle (#1679) * test: cover last-surface close preference regression * fix: restore last-surface close preference --- Resources/Localizable.xcstrings | 12 +- Sources/TabManager.swift | 32 ++++- Sources/Workspace.swift | 6 +- Sources/cmuxApp.swift | 45 ++++++- .../AppDelegateShortcutRoutingTests.swift | 59 ++++++++++ cmuxTests/CmuxWebViewKeyEquivalentTests.swift | 110 ++++++++++++++++++ 6 files changed, 248 insertions(+), 16 deletions(-) diff --git a/Resources/Localizable.xcstrings b/Resources/Localizable.xcstrings index a1a73b9d..11f77ec7 100644 --- a/Resources/Localizable.xcstrings +++ b/Resources/Localizable.xcstrings @@ -43256,13 +43256,13 @@ "en": { "stringUnit": { "state": "translated", - "value": "Closing Last Surface Closes Workspace" + "value": "Keep Workspace Open When Closing Last Surface" } }, "ja": { "stringUnit": { "state": "translated", - "value": "最後のサーフェスを閉じるとワークスペースも閉じる" + "value": "最後のサーフェスを閉じてもワークスペースを残す" } } } @@ -43273,13 +43273,13 @@ "en": { "stringUnit": { "state": "translated", - "value": "Closing the last surface keeps the workspace open. Use Cmd+Shift+W to close a workspace explicitly." + "value": "When the focused surface is the last one in its workspace, the close-surface shortcut also closes the workspace." } }, "ja": { "stringUnit": { "state": "translated", - "value": "最後のサーフェスを閉じてもワークスペースは残ります。ワークスペースを明示的に閉じるにはCmd+Shift+Wを使います。" + "value": "フォーカス中のサーフェスがそのワークスペースの最後の1つなら、サーフェスを閉じるショートカットはワークスペースも閉じます。" } } } @@ -43290,13 +43290,13 @@ "en": { "stringUnit": { "state": "translated", - "value": "Closing the last surface also closes its workspace." + "value": "When the focused surface is the last one in its workspace, the close-surface shortcut closes only the surface and keeps the workspace open. Use the close-workspace shortcut to close the workspace explicitly." } }, "ja": { "stringUnit": { "state": "translated", - "value": "最後のサーフェスを閉じると、そのワークスペースも閉じます。" + "value": "フォーカス中のサーフェスがそのワークスペースの最後の1つでも、サーフェスを閉じるショートカットはサーフェスだけを閉じ、ワークスペースは残します。ワークスペースを閉じるショートカットを使うと明示的に閉じられます。" } } } diff --git a/Sources/TabManager.swift b/Sources/TabManager.swift index 97271038..783f0e7c 100644 --- a/Sources/TabManager.swift +++ b/Sources/TabManager.swift @@ -60,6 +60,20 @@ enum WorkspaceAutoReorderSettings { } } +enum LastSurfaceCloseShortcutSettings { + static let key = "closeWorkspaceOnLastSurfaceShortcut" + // Keep the legacy stored meaning so existing values still map to the same + // behavior. The default is flipped to preserve current Cmd+W behavior. + static let defaultValue = true + + 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 @@ -2101,6 +2115,12 @@ class TabManager: ObservableObject { } } + private func shouldCloseWorkspaceOnLastSurfaceShortcut(_ workspace: Workspace, panelId: UUID) -> Bool { + LastSurfaceCloseShortcutSettings.closesWorkspace() && + workspace.panels.count <= 1 && + workspace.panels[panelId] != nil + } + private func closePanelWithConfirmation(tab: Workspace, panelId: UUID) { guard tab.panels[panelId] != nil else { #if DEBUG @@ -2121,18 +2141,20 @@ class TabManager: ObservableObject { if panel is BrowserPanel { return "browser" } return String(describing: type(of: panel)) }() + let closesWorkspaceOnLastSurfaceShortcut = shouldCloseWorkspaceOnLastSurfaceShortcut(tab, panelId: panelId) #if DEBUG dlog( "surface.close.shortcut.begin tab=\(tab.id.uuidString.prefix(5)) " + "panel=\(panelId.uuidString.prefix(5)) kind=\(panelKind) " + - "panelCount=\(tab.panels.count) bonsplitTabs=\(bonsplitTabCount)" + "panelCount=\(tab.panels.count) bonsplitTabs=\(bonsplitTabCount) " + + "closeWorkspaceOnLastSurface=\(closesWorkspaceOnLastSurfaceShortcut ? 1 : 0)" ) #endif - // Route Cmd+W through Bonsplit/Workspace close handling so it matches the tab close - // 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) { + // The last-surface shortcut preference only affects Cmd+W. The tab close button + // continues to use Workspace's explicit-close path when it closes the last surface. + if closesWorkspaceOnLastSurfaceShortcut, + let surfaceId = tab.surfaceIdFromPanelId(panelId) { tab.markExplicitClose(surfaceId: surfaceId) } let closed = tab.closePanel(panelId) diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index d40006d0..444f7170 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -5164,8 +5164,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. + /// Tab IDs whose next close attempt should be treated as an explicit + /// workspace-close gesture from the user (the tab-strip X button, or Cmd+W when + /// the shortcut preference is set to close the workspace on the last surface), + /// rather than an internal close/move flow. private var explicitUserCloseTabIds: Set = [] /// Deterministic tab selection to apply after a tab closes. diff --git a/Sources/cmuxApp.swift b/Sources/cmuxApp.swift index eb17ccd7..ca6f5e78 100644 --- a/Sources/cmuxApp.swift +++ b/Sources/cmuxApp.swift @@ -493,9 +493,10 @@ struct cmuxApp: App { Divider() // Terminal semantics: - // 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. + // Cmd+W closes the focused tab/surface (with confirmation if needed). By + // default, closing the last surface also closes the workspace and the window + // if it was also the last workspace. Users can opt into keeping the workspace + // open instead. Button(String(localized: "menu.file.closeTab", defaultValue: "Close Tab")) { closePanelOrWindow() } @@ -3593,6 +3594,8 @@ 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 @@ -3645,6 +3648,30 @@ struct SettingsView: View { NewWorkspacePlacement(rawValue: newWorkspacePlacement) ?? WorkspacePlacementSettings.defaultPlacement } + private var keepWorkspaceOpenOnLastSurfaceShortcut: Bool { + !closeWorkspaceOnLastSurfaceShortcut + } + + private var keepWorkspaceOpenOnLastSurfaceShortcutBinding: Binding { + Binding( + get: { keepWorkspaceOpenOnLastSurfaceShortcut }, + set: { closeWorkspaceOnLastSurfaceShortcut = !$0 } + ) + } + + private var closeWorkspaceOnLastSurfaceShortcutSubtitle: String { + if keepWorkspaceOpenOnLastSurfaceShortcut { + return String( + localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOn", + defaultValue: "When the focused surface is the last one in its workspace, the close-surface shortcut closes only the surface and keeps the workspace open. Use the close-workspace shortcut to close the workspace explicitly." + ) + } + return String( + localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOff", + defaultValue: "When the focused surface is the last one in its workspace, the close-surface shortcut also closes the workspace." + ) + } + private var selectedSidebarActiveTabIndicatorStyle: SidebarActiveTabIndicatorStyle { SidebarActiveTabIndicatorSettings.resolvedStyle(rawValue: sidebarActiveTabIndicatorStyle) } @@ -4076,6 +4103,17 @@ struct SettingsView: View { SettingsCardDivider() + SettingsCardRow( + String(localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut", defaultValue: "Keep Workspace Open When Closing Last Surface"), + subtitle: closeWorkspaceOnLastSurfaceShortcutSubtitle + ) { + Toggle("", isOn: keepWorkspaceOpenOnLastSurfaceShortcutBinding) + .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.") @@ -5228,6 +5266,7 @@ 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 4c320f12..2dbc01ab 100644 --- a/cmuxTests/AppDelegateShortcutRoutingTests.swift +++ b/cmuxTests/AppDelegateShortcutRoutingTests.swift @@ -6,6 +6,8 @@ import XCTest @testable import cmux #endif +private let lastSurfaceCloseShortcutDefaultsKey = "closeWorkspaceOnLastSurfaceShortcut" + @MainActor final class AppDelegateShortcutRoutingTests: XCTestCase { private var savedShortcutsByAction: [KeyboardShortcutSettings.Action: StoredShortcut] = [:] @@ -714,6 +716,63 @@ final class AppDelegateShortcutRoutingTests: XCTestCase { ) } + func testCmdWKeepsLastSurfaceWorkspaceOpenWhenKeepWorkspaceOpenPreferenceIsEnabled() throws { + guard let appDelegate = AppDelegate.shared else { + XCTFail("Expected AppDelegate.shared") + return + } + + let defaults = UserDefaults.standard + let originalSetting = defaults.object(forKey: lastSurfaceCloseShortcutDefaultsKey) + defaults.set(false, forKey: lastSurfaceCloseShortcutDefaultsKey) + defer { + if let originalSetting { + defaults.set(originalSetting, forKey: lastSurfaceCloseShortcutDefaultsKey) + } else { + defaults.removeObject(forKey: lastSurfaceCloseShortcutDefaultsKey) + } + } + + let windowId = appDelegate.createMainWindow() + defer { closeWindow(withId: windowId) } + + guard let targetWindow = window(withId: windowId), + let manager = appDelegate.tabManagerFor(windowId: windowId), + let workspace = manager.selectedWorkspace, + let initialPanelId = workspace.focusedPanelId else { + XCTFail("Expected test window, manager, workspace, and focused panel") + return + } + + 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)) + + XCTAssertNotNil( + self.window(withId: windowId), + "Cmd+W should keep the window open when the keep-workspace-open preference is enabled" + ) + XCTAssertEqual(manager.tabs.count, 1) + XCTAssertEqual(manager.selectedTabId, workspace.id) + XCTAssertNil(workspace.panels[initialPanelId]) + XCTAssertEqual(workspace.panels.count, 1) + XCTAssertNotEqual(workspace.focusedPanelId, initialPanelId) + } + func testCmdWClosesAuxiliaryWindowInsteadOfMainTerminalPanel() throws { guard let appDelegate = AppDelegate.shared else { XCTFail("Expected AppDelegate.shared") diff --git a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift index 96b4083b..9f0d08f9 100644 --- a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift +++ b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift @@ -14,6 +14,8 @@ import UserNotifications @testable import cmux #endif +private let lastSurfaceCloseShortcutDefaultsKey = "closeWorkspaceOnLastSurfaceShortcut" + private var cmuxUnitTestInspectorAssociationKey: UInt8 = 0 private var cmuxUnitTestInspectorOverrideInstalled = false @@ -5017,6 +5019,43 @@ final class WorkspaceAutoReorderSettingsTests: XCTestCase { } } +final class LastSurfaceCloseShortcutSettingsTests: XCTestCase { + func testDefaultClosesWorkspace() { + 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) } + + XCTAssertTrue(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)" @@ -5823,6 +5862,39 @@ final class TabManagerCloseCurrentPanelTests: XCTestCase { XCTAssertTrue(secondWorkspace.panels.isEmpty) } + func testCloseCurrentPanelKeepsWorkspaceOpenWhenKeepWorkspaceOpenPreferenceIsEnabled() { + let defaults = UserDefaults.standard + let originalSetting = defaults.object(forKey: lastSurfaceCloseShortcutDefaultsKey) + defaults.set(false, forKey: lastSurfaceCloseShortcutDefaultsKey) + defer { + if let originalSetting { + defaults.set(originalSetting, forKey: lastSurfaceCloseShortcutDefaultsKey) + } else { + defaults.removeObject(forKey: lastSurfaceCloseShortcutDefaultsKey) + } + } + + let manager = TabManager() + guard let workspace = manager.selectedWorkspace, + let initialPanelId = workspace.focusedPanelId else { + XCTFail("Expected selected workspace and focused panel") + return + } + + let initialWorkspaceId = workspace.id + + manager.closeCurrentPanelWithConfirmation() + drainMainQueue() + drainMainQueue() + + XCTAssertEqual(manager.tabs.count, 1) + XCTAssertEqual(manager.selectedTabId, initialWorkspaceId) + XCTAssertEqual(manager.tabs.first?.id, initialWorkspaceId) + XCTAssertNil(workspace.panels[initialPanelId]) + XCTAssertEqual(workspace.panels.count, 1) + XCTAssertNotEqual(workspace.focusedPanelId, initialPanelId) + } + func testClosePanelButtonClosesWorkspaceWhenItOwnsTheLastSurface() { let manager = TabManager() let firstWorkspace = manager.tabs[0] @@ -5853,6 +5925,44 @@ final class TabManagerCloseCurrentPanelTests: XCTestCase { XCTAssertTrue(secondWorkspace.panels.isEmpty) } + func testClosePanelButtonStillClosesWorkspaceWhenKeepWorkspaceOpenPreferenceIsEnabled() { + let defaults = UserDefaults.standard + let originalSetting = defaults.object(forKey: lastSurfaceCloseShortcutDefaultsKey) + defaults.set(false, forKey: lastSurfaceCloseShortcutDefaultsKey) + defer { + if let originalSetting { + defaults.set(originalSetting, forKey: lastSurfaceCloseShortcutDefaultsKey) + } else { + defaults.removeObject(forKey: lastSurfaceCloseShortcutDefaultsKey) + } + } + + 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 + } + + 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,