diff --git a/Resources/Localizable.xcstrings b/Resources/Localizable.xcstrings index 10e76340..4a57d594 100644 --- a/Resources/Localizable.xcstrings +++ b/Resources/Localizable.xcstrings @@ -41182,6 +41182,57 @@ } } }, + "settings.app.closeWorkspaceOnLastSurfaceShortcut": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "Cmd+W Closes Workspace on Last Surface" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "最後のサーフェスでCmd+Wがワークスペースも閉じる" + } + } + } + }, + "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOff": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "When the focused surface is the last one in its workspace, Cmd+W closes only the surface and keeps the workspace open." + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "フォーカス中のサーフェスがそのワークスペースの最後の1つでも、Cmd+Wはサーフェスだけを閉じ、ワークスペースは残します。" + } + } + } + }, + "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOn": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "When the focused surface is the last one in its workspace, Cmd+W also closes the workspace." + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "フォーカス中のサーフェスがそのワークスペースの最後の1つなら、Cmd+Wでワークスペースも閉じます。" + } + } + } + }, "settings.app.openSidebarPRLinks": { "extractionState": "manual", "localizations": { diff --git a/Sources/TabManager.swift b/Sources/TabManager.swift index 2c1c4163..c0829e4a 100644 --- a/Sources/TabManager.swift +++ b/Sources/TabManager.swift @@ -51,6 +51,18 @@ 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 @@ -1649,7 +1661,21 @@ class TabManager: ObservableObject { } } + private func shouldCloseWorkspaceOnLastSurfaceShortcut(_ workspace: Workspace) -> Bool { + LastSurfaceCloseShortcutSettings.closesWorkspace() && workspace.panels.count <= 1 + } + private func closePanelWithConfirmation(tab: Workspace, panelId: UUID) { + guard tab.panels[panelId] != nil else { +#if DEBUG + dlog( + "surface.close.shortcut.skip tab=\(tab.id.uuidString.prefix(5)) " + + "panel=\(panelId.uuidString.prefix(5)) reason=missingPanel" + ) +#endif + return + } + let bonsplitTabCount = tab.bonsplitController.allPaneIds.reduce(0) { partial, paneId in partial + tab.bonsplitController.tabs(inPane: paneId).count } @@ -1663,77 +1689,21 @@ 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)" + "panelCount=\(tab.panels.count) bonsplitTabs=\(bonsplitTabCount) " + + "closeWorkspaceOnLastSurface=\(shouldCloseWorkspaceOnLastSurfaceShortcut(tab) ? 1 : 0)" ) #endif - // Cmd+W closes the focused Bonsplit tab (a "tab" in the UI). When the workspace only has - // a single tab left, closing it should close the workspace (and possibly the window), - // rather than creating a replacement terminal. - let effectiveSurfaceCount = max(tab.panels.count, bonsplitTabCount) - let isLastTabInWorkspace = effectiveSurfaceCount <= 1 - if isLastTabInWorkspace { - let willCloseWindow = tabs.count <= 1 - let needsConfirm = workspaceNeedsConfirmClose(tab) - if needsConfirm { - let message = willCloseWindow - ? String(localized: "dialog.closeLastTabWindow.message", defaultValue: "This will close the last tab and close the window.") - : String(localized: "dialog.closeLastTabWorkspace.message", defaultValue: "This will close the last tab and close its workspace.") -#if DEBUG - dlog( - "surface.close.shortcut.confirm tab=\(tab.id.uuidString.prefix(5)) " + - "panel=\(panelId.uuidString.prefix(5)) reason=lastTab" - ) -#endif - guard confirmClose( - title: String(localized: "dialog.closeTab.title", defaultValue: "Close tab?"), - message: message, - acceptCmdD: willCloseWindow - ) else { -#if DEBUG - dlog( - "surface.close.shortcut.cancel tab=\(tab.id.uuidString.prefix(5)) " + - "panel=\(panelId.uuidString.prefix(5)) reason=lastTabConfirmDismissed" - ) -#endif - return - } - } - - AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: tab.id) - if willCloseWindow { - AppDelegate.shared?.closeMainWindowContainingTabId(tab.id) - } else { - closeWorkspace(tab) - } + // Cmd+W should match Bonsplit's tab close button semantics by default. Users can opt + // back into the legacy behavior where closing the last surface also closes its workspace. + if shouldCloseWorkspaceOnLastSurfaceShortcut(tab) { + closeWorkspaceIfRunningProcess(tab) return } - if let terminalPanel = tab.terminalPanel(for: panelId), - terminalPanel.needsConfirmClose() { -#if DEBUG - dlog( - "surface.close.shortcut.confirm tab=\(tab.id.uuidString.prefix(5)) " + - "panel=\(panelId.uuidString.prefix(5)) reason=terminalNeedsConfirm" - ) -#endif - guard confirmClose( - title: String(localized: "dialog.closeTab.title", defaultValue: "Close tab?"), - message: String(localized: "dialog.closeTab.message", defaultValue: "This will close the current tab."), - acceptCmdD: false - ) else { -#if DEBUG - dlog( - "surface.close.shortcut.cancel tab=\(tab.id.uuidString.prefix(5)) " + - "panel=\(panelId.uuidString.prefix(5)) reason=terminalConfirmDismissed" - ) -#endif - return - } - } - - // We already confirmed (if needed); bypass Bonsplit's delegate gating. - let closed = tab.closePanel(panelId, force: true) + // Route the default Cmd+W path through Bonsplit/Workspace close handling so it matches + // the tab close button behavior, including shared confirmation and replacement-panel flow. + let closed = tab.closePanel(panelId) #if DEBUG dlog( "surface.close.shortcut tab=\(tab.id.uuidString.prefix(5)) " + diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index 1bc7e1ed..6564e457 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -4647,6 +4647,7 @@ extension Workspace: BonsplitDelegate { if lastTerminalConfigInheritancePanelId == panelId { lastTerminalConfigInheritancePanelId = nil } + AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: id, surfaceId: panelId) // Keep the workspace invariant for normal close paths. // Detach/move flows intentionally allow a temporary empty workspace so AppDelegate can diff --git a/Sources/cmuxApp.swift b/Sources/cmuxApp.swift index 28f9cc15..46e76f6b 100644 --- a/Sources/cmuxApp.swift +++ b/Sources/cmuxApp.swift @@ -452,8 +452,9 @@ struct cmuxApp: App { Divider() // Terminal semantics: - // Cmd+W closes the focused tab (with confirmation if needed). If this is the last - // tab in the last workspace, it closes the window. + // 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 back into the legacy last-surface Cmd+W behavior. Button(String(localized: "menu.file.closeTab", defaultValue: "Close Tab")) { closePanelOrWindow() } @@ -3075,6 +3076,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 @@ -3120,6 +3123,19 @@ struct SettingsView: View { NewWorkspacePlacement(rawValue: newWorkspacePlacement) ?? WorkspacePlacementSettings.defaultPlacement } + private var closeWorkspaceOnLastSurfaceShortcutSubtitle: String { + if closeWorkspaceOnLastSurfaceShortcut { + return String( + localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOn", + defaultValue: "When the focused surface is the last one in its workspace, Cmd+W also closes the workspace." + ) + } + return String( + localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut.subtitleOff", + defaultValue: "When the focused surface is the last one in its workspace, Cmd+W closes only the surface and keeps the workspace open." + ) + } + private var selectedSidebarActiveTabIndicatorStyle: SidebarActiveTabIndicatorStyle { SidebarActiveTabIndicatorSettings.resolvedStyle(rawValue: sidebarActiveTabIndicatorStyle) } @@ -3487,6 +3503,17 @@ struct SettingsView: View { SettingsCardDivider() + SettingsCardRow( + String(localized: "settings.app.closeWorkspaceOnLastSurfaceShortcut", defaultValue: "Cmd+W Closes Workspace on Last Surface"), + 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.") @@ -4456,6 +4483,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/CmuxWebViewKeyEquivalentTests.swift b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift index 7779523a..2d3c6618 100644 --- a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift +++ b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift @@ -56,6 +56,14 @@ private func installCmuxUnitTestInspectorOverride() { cmuxUnitTestInspectorOverrideInstalled = true } +private func drainMainQueue() { + let expectation = XCTestExpectation(description: "drain main queue") + DispatchQueue.main.async { + expectation.fulfill() + } + XCTWaiter().wait(for: [expectation], timeout: 1.0) +} + final class SplitShortcutTransientFocusGuardTests: XCTestCase { func testSuppressesWhenFirstResponderFallsBackAndHostedViewIsTiny() { XCTAssertTrue( @@ -4636,6 +4644,43 @@ 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)" @@ -5192,6 +5237,121 @@ final class TabManagerCloseWorkspacesWithConfirmationTests: XCTestCase { } } +@MainActor +final class TabManagerCloseCurrentPanelTests: XCTestCase { + func testCloseCurrentPanelKeepsWorkspaceOpenWhenItOwnsTheLastSurface() { + 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 + XCTAssertEqual(manager.tabs.count, 1) + XCTAssertEqual(workspace.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) + } + + 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 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) + } + } + + let manager = TabManager() + let firstWorkspace = manager.tabs[0] + let secondWorkspace = manager.addWorkspace() + + manager.closePanelWithConfirmation(tabId: secondWorkspace.id, surfaceId: UUID()) + + XCTAssertEqual(manager.tabs.map(\.id), [firstWorkspace.id, secondWorkspace.id]) + } + + func testCloseCurrentPanelClearsNotificationsForClosedSurface() { + let appDelegate = AppDelegate.shared ?? AppDelegate() + let manager = TabManager() + let store = TerminalNotificationStore.shared + + let originalTabManager = appDelegate.tabManager + let originalNotificationStore = appDelegate.notificationStore + store.replaceNotificationsForTesting([]) + store.configureNotificationDeliveryHandlerForTesting { _, _ in } + appDelegate.tabManager = manager + appDelegate.notificationStore = store + + defer { + store.replaceNotificationsForTesting([]) + store.resetNotificationDeliveryHandlerForTesting() + appDelegate.tabManager = originalTabManager + appDelegate.notificationStore = originalNotificationStore + } + + guard let workspace = manager.selectedWorkspace, + let initialPanelId = workspace.focusedPanelId else { + XCTFail("Expected selected workspace and focused panel") + return + } + + store.addNotification( + tabId: workspace.id, + surfaceId: initialPanelId, + title: "Unread", + subtitle: "", + body: "" + ) + XCTAssertTrue(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: initialPanelId)) + + manager.closeCurrentPanelWithConfirmation() + drainMainQueue() + drainMainQueue() + + XCTAssertFalse(store.hasUnreadNotification(forTabId: workspace.id, surfaceId: initialPanelId)) + } +} + @MainActor final class TabManagerPendingUnfocusPolicyTests: XCTestCase { func testDoesNotUnfocusWhenPendingTabIsCurrentlySelected() { diff --git a/cmuxUITests/CloseWorkspaceCmdDUITests.swift b/cmuxUITests/CloseWorkspaceCmdDUITests.swift index 54c35d19..b9061916 100644 --- a/cmuxUITests/CloseWorkspaceCmdDUITests.swift +++ b/cmuxUITests/CloseWorkspaceCmdDUITests.swift @@ -27,23 +27,32 @@ final class CloseWorkspaceCmdDUITests: XCTestCase { ) } - func testCmdDConfirmsCloseWhenClosingLastTabClosesWindow() { + func testCmdWClosingLastTabKeepsWorkspaceWindowOpen() { let app = XCUIApplication() - // Closing the last tab should also present a confirmation and accept Cmd+D when it would close the window. - app.launchEnvironment["CMUX_UI_TEST_FORCE_CONFIRM_CLOSE_WORKSPACE"] = "1" + let keyequivPath = "/tmp/cmux-ui-test-keyequiv-\(UUID().uuidString).json" + try? FileManager.default.removeItem(atPath: keyequivPath) + app.launchEnvironment["CMUX_UI_TEST_KEYEQUIV_PATH"] = keyequivPath app.launch() app.activate() - // Close current tab (Cmd+W). With a single workspace and a single tab, this will close the window after confirmation. + let baseline = loadJSON(atPath: keyequivPath)?["closePanelInvocations"].flatMap(Int.init) ?? 0 app.typeKey("w", modifierFlags: [.command]) - XCTAssertTrue(waitForCloseTabAlert(app: app, timeout: 5.0)) + XCTAssertTrue( + waitForKeyequivInt("closePanelInvocations", toBeAtLeast: baseline + 1, atPath: keyequivPath, timeout: 5.0), + "Expected Cmd+W to route through the close-current-tab action" + ) - // Cmd+D should accept the destructive close and close the window. - app.typeKey("d", modifierFlags: [.command]) + if waitForCloseTabAlert(app: app, timeout: 5.0) { + clickCloseOnCloseTabAlert(app: app) + XCTAssertFalse( + isCloseTabAlertPresent(app: app), + "Expected close tab confirmation to dismiss after confirming the close" + ) + } XCTAssertTrue( - waitForNoWindowsOrAppNotRunningForeground(app: app, timeout: 6.0), - "Expected Cmd+D to confirm close and close the last window" + waitForWindowCount(app: app, atLeast: 1, timeout: 6.0), + "Expected Cmd+W on the last tab to keep the workspace window open" ) } @@ -608,12 +617,37 @@ final class CloseWorkspaceCmdDUITests: XCTestCase { private func waitForCloseTabAlert(app: XCUIApplication, timeout: TimeInterval) -> Bool { let deadline = Date().addingTimeInterval(timeout) while Date() < deadline { - if app.dialogs.containing(.staticText, identifier: "Close tab?").firstMatch.exists { return true } - if app.alerts.containing(.staticText, identifier: "Close tab?").firstMatch.exists { return true } - if app.staticTexts["Close tab?"].exists { return true } + if isCloseTabAlertPresent(app: app) { return true } RunLoop.current.run(until: Date().addingTimeInterval(0.05)) } - return false + return isCloseTabAlertPresent(app: app) + } + + // Must match the defaultValue for dialog.closeTab.title in TabManager. + private func isCloseTabAlertPresent(app: XCUIApplication) -> Bool { + if app.dialogs.containing(.staticText, identifier: "Close tab?").firstMatch.exists { return true } + if app.alerts.containing(.staticText, identifier: "Close tab?").firstMatch.exists { return true } + return app.staticTexts["Close tab?"].exists + } + + // Must match the defaultValue for dialog.closeTab.title in TabManager. + private func clickCloseOnCloseTabAlert(app: XCUIApplication) { + let dialog = app.dialogs.containing(.staticText, identifier: "Close tab?").firstMatch + if dialog.exists { + dialog.buttons["Close"].firstMatch.click() + return + } + + let alert = app.alerts.containing(.staticText, identifier: "Close tab?").firstMatch + if alert.exists { + alert.buttons["Close"].firstMatch.click() + return + } + + let anyDialog = app.dialogs.firstMatch + if anyDialog.exists, anyDialog.buttons["Close"].exists { + anyDialog.buttons["Close"].firstMatch.click() + } } private func waitForWindowCount(app: XCUIApplication, toBe count: Int, timeout: TimeInterval) -> Bool { @@ -644,6 +678,17 @@ final class CloseWorkspaceCmdDUITests: XCTestCase { return app.state != .runningForeground || app.windows.count == 0 } + private func waitForKeyequivInt(_ key: String, toBeAtLeast expected: Int, atPath path: String, timeout: TimeInterval) -> Bool { + let deadline = Date().addingTimeInterval(timeout) + while Date() < deadline { + let value = loadJSON(atPath: path)?[key].flatMap(Int.init) ?? 0 + if value >= expected { return true } + RunLoop.current.run(until: Date().addingTimeInterval(0.05)) + } + let value = loadJSON(atPath: path)?[key].flatMap(Int.init) ?? 0 + return value >= expected + } + private func waitForAnyJSON(atPath path: String, timeout: TimeInterval) -> Bool { let deadline = Date().addingTimeInterval(timeout) while Date() < deadline {