From 51b9fa751a13fab2884e37292159379372f4da91 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 17 Jun 2025 16:13:23 -0700 Subject: [PATCH] macos: disambiguate close tab vs close window for confirmation This fixes an issue where pressing the red close button in a window or the "x" button on a tab couldn't differentiate and would always close the tab or close the window (depending on tab counts). It seems like in both cases, AppKit triggers the `windowShouldClose` delegate method on the controller, but for the close window case it triggers this on ALL the windows in the group, not just the one that was clicked. I implemented a kind of silly coordinator that debounces `windowShouldClose` calls over 100ms and uses that to differentiate between the two cases. --- macos/Ghostty.xcodeproj/project.pbxproj | 6 +- .../Terminal/TerminalController.swift | 21 +-- .../Extensions/NSWindow+Extension.swift | 6 + .../Helpers/TabGroupCloseCoordinator.swift | 124 ++++++++++++++++++ 4 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 macos/Sources/Helpers/TabGroupCloseCoordinator.swift diff --git a/macos/Ghostty.xcodeproj/project.pbxproj b/macos/Ghostty.xcodeproj/project.pbxproj index a5663202b..5c584709e 100644 --- a/macos/Ghostty.xcodeproj/project.pbxproj +++ b/macos/Ghostty.xcodeproj/project.pbxproj @@ -119,6 +119,7 @@ A5E112932AF73E6E00C6E0C2 /* ClipboardConfirmation.xib in Resources */ = {isa = PBXBuildFile; fileRef = A5E112922AF73E6E00C6E0C2 /* ClipboardConfirmation.xib */; }; A5E112952AF73E8A00C6E0C2 /* ClipboardConfirmationController.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5E112942AF73E8A00C6E0C2 /* ClipboardConfirmationController.swift */; }; A5E112972AF7401B00C6E0C2 /* ClipboardConfirmationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5E112962AF7401B00C6E0C2 /* ClipboardConfirmationView.swift */; }; + A5E4082A2E022E9E0035FEAC /* TabGroupCloseCoordinator.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5E408292E022E9B0035FEAC /* TabGroupCloseCoordinator.swift */; }; A5FEB3002ABB69450068369E /* main.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5FEB2FF2ABB69450068369E /* main.swift */; }; AEE8B3452B9AA39600260C5E /* NSPasteboard+Extension.swift in Sources */ = {isa = PBXBuildFile; fileRef = AEE8B3442B9AA39600260C5E /* NSPasteboard+Extension.swift */; }; C159E81D2B66A06B00FDFE9C /* OSColor+Extension.swift in Sources */ = {isa = PBXBuildFile; fileRef = C159E81C2B66A06B00FDFE9C /* OSColor+Extension.swift */; }; @@ -238,6 +239,7 @@ A5E112922AF73E6E00C6E0C2 /* ClipboardConfirmation.xib */ = {isa = PBXFileReference; lastKnownFileType = file.xib; path = ClipboardConfirmation.xib; sourceTree = ""; }; A5E112942AF73E8A00C6E0C2 /* ClipboardConfirmationController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ClipboardConfirmationController.swift; sourceTree = ""; }; A5E112962AF7401B00C6E0C2 /* ClipboardConfirmationView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ClipboardConfirmationView.swift; sourceTree = ""; }; + A5E408292E022E9B0035FEAC /* TabGroupCloseCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TabGroupCloseCoordinator.swift; sourceTree = ""; }; A5FEB2FF2ABB69450068369E /* main.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = main.swift; sourceTree = ""; }; AEE8B3442B9AA39600260C5E /* NSPasteboard+Extension.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "NSPasteboard+Extension.swift"; sourceTree = ""; }; C159E81C2B66A06B00FDFE9C /* OSColor+Extension.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "OSColor+Extension.swift"; sourceTree = ""; }; @@ -320,12 +322,13 @@ A5333E1B2B5A1CE3008AEFF7 /* CrossKit.swift */, A5CBD0572C9F30860017A1AE /* Cursor.swift */, A5D0AF3C2B37804400D21823 /* CodableBridge.swift */, + A5CBD0552C9E65A50017A1AE /* DraggableWindowView.swift */, A58636702DF298F700E04A10 /* ExpiringUndoManager.swift */, A52FFF582CAA4FF1000C6A5B /* Fullscreen.swift */, A59630962AEE163600D64628 /* HostingWindow.swift */, A5CA378B2D2A4DE800931030 /* KeyboardLayout.swift */, A59FB5D02AE0DEA7009128F3 /* MetalView.swift */, - A5CBD0552C9E65A50017A1AE /* DraggableWindowView.swift */, + A5E408292E022E9B0035FEAC /* TabGroupCloseCoordinator.swift */, A5CA378D2D31D6C100931030 /* Weak.swift */, C1F26EE72B76CBFC00404083 /* VibrantLayer.h */, C1F26EE82B76CBFC00404083 /* VibrantLayer.m */, @@ -792,6 +795,7 @@ A599CDB02CF103F60049FA26 /* NSAppearance+Extension.swift in Sources */, A52FFF572CA90484000C6A5B /* QuickTerminalScreen.swift in Sources */, A5CC36132C9CD72D004D6760 /* SecureInputOverlay.swift in Sources */, + A5E4082A2E022E9E0035FEAC /* TabGroupCloseCoordinator.swift in Sources */, A535B9DA299C569B0017E2E4 /* ErrorView.swift in Sources */, A53A29882DB69D2F00B6E02C /* TerminalCommandPalette.swift in Sources */, A51BFC202B2FB64F00E92F16 /* AboutController.swift in Sources */, diff --git a/macos/Sources/Features/Terminal/TerminalController.swift b/macos/Sources/Features/Terminal/TerminalController.swift index 03a4e548e..01ed25e63 100644 --- a/macos/Sources/Features/Terminal/TerminalController.swift +++ b/macos/Sources/Features/Terminal/TerminalController.swift @@ -5,7 +5,7 @@ import Combine import GhosttyKit /// A classic, tabbed terminal experience. -class TerminalController: BaseTerminalController { +class TerminalController: BaseTerminalController, TabGroupCloseCoordinator.Controller { override var windowNibName: NSNib.Name? { let defaultValue = "Terminal" @@ -882,14 +882,20 @@ class TerminalController: BaseTerminalController { ghostty.newTab(surface: surface) } - //MARK: - NSWindowDelegate + // MARK: NSWindowDelegate + + // TabGroupCloseCoordinator.Controller + lazy private(set) var tabGroupCloseCoordinator = TabGroupCloseCoordinator() override func windowShouldClose(_ sender: NSWindow) -> Bool { - // If we have tabs, then this should only close the tab. - if window?.tabGroup?.windows.count ?? 0 > 1 { - closeTab(sender) - } else { - closeWindow(sender) + tabGroupCloseCoordinator.windowShouldClose(sender) { [weak self] scope in + guard let self else { return } + switch (scope) { + case .tab: closeTab(nil) + case .window: + guard self.window?.isFirstWindowInTabGroup ?? false else { return } + closeWindow(nil) + } } // We will always explicitly close the window using the above @@ -1270,4 +1276,3 @@ extension TerminalController: NSMenuItemValidation { } } } - diff --git a/macos/Sources/Helpers/Extensions/NSWindow+Extension.swift b/macos/Sources/Helpers/Extensions/NSWindow+Extension.swift index 06a9fa4e0..f9ed364aa 100644 --- a/macos/Sources/Helpers/Extensions/NSWindow+Extension.swift +++ b/macos/Sources/Helpers/Extensions/NSWindow+Extension.swift @@ -9,4 +9,10 @@ extension NSWindow { guard windowNumber > 0 else { return nil } return CGWindowID(windowNumber) } + + /// True if this is the first window in the tab group. + var isFirstWindowInTabGroup: Bool { + guard let firstWindow = tabGroup?.windows.first else { return true } + return firstWindow === self + } } diff --git a/macos/Sources/Helpers/TabGroupCloseCoordinator.swift b/macos/Sources/Helpers/TabGroupCloseCoordinator.swift new file mode 100644 index 000000000..ca41bf89c --- /dev/null +++ b/macos/Sources/Helpers/TabGroupCloseCoordinator.swift @@ -0,0 +1,124 @@ +import AppKit + +/// Coordinates close operations for windows that are part of a tab group. +/// +/// This coordinator helps distinguish between closing a single tab versus closing +/// an entire window (with all its tabs). When macOS native tabs are used, close +/// operations can be ambiguous - this coordinator tracks close requests across +/// multiple windows in a tab group to determine the user's intent. +class TabGroupCloseCoordinator { + /// The scope of a close operation. + enum CloseScope { + case tab + case window + } + + /// Protocol that window controllers must implement to use the coordinator. + protocol Controller { + /// The tab group close coordinator instance for this controller. + var tabGroupCloseCoordinator: TabGroupCloseCoordinator { get } + } + + /// Callback type for close operations. + typealias Callback = (CloseScope) -> Void + + // We use weak vars and ObjectIdentifiers below because we don't want to + // create any strong reference cycles during coordination. + + /// The tab group being coordinated. Weak reference to avoid cycles. + private weak var tabGroup: NSWindowTabGroup? + + /// Map of window identifiers to their close callbacks. + private var closeRequests: [ObjectIdentifier: Callback] = [:] + + /// Timer used to debounce close requests and determine intent. + private var debounceTimer: Timer? + + deinit { + trigger(.tab) + } + + /// Call this from the windowShouldClose override in order to track whether + /// a window close event is from a tab or a window. If this window already + /// requested a close then only the latest will be called. + func windowShouldClose( + _ window: NSWindow, + callback: @escaping Callback + ) { + // If this window isn't part of a tab group we assume its a window + // close for the window and let our timer keep running for the rest. + guard let tabGroup = window.tabGroup else { + callback(.window) + return + } + + // Forward to the proper coordinator + if let firstController = tabGroup.windows.first?.windowController as? Controller, + firstController.tabGroupCloseCoordinator !== self { + let coordinator = firstController.tabGroupCloseCoordinator + coordinator.windowShouldClose(window, callback: callback) + return + } + + // If our tab group is nil then we either are seeing this for the first + // time or our weak ref expired and we should fire our callbacks. + if self.tabGroup == nil { + self.tabGroup = tabGroup + debounceTimer?.fire() + debounceTimer = nil + } + + // No matter what, we cancel our debounce and restart this. This opens + // us up to a DoS if close requests are looped but this would only + // happen in hostile scenarios that are self-inflicted. + debounceTimer?.invalidate() + debounceTimer = nil + + // If this tab group doesn't match then I don't really know what to + // do. This shouldn't happen. So we just assume it's a tab close + // and trigger the rest. No right answer here as far as I know. + if self.tabGroup != tabGroup { + callback(.tab) + trigger(.tab) + return + } + + // Add the request + closeRequests[ObjectIdentifier(window)] = callback + + // If close requests matches all our windows then we are done. + if closeRequests.count == tabGroup.windows.count { + let allWindows = Set(tabGroup.windows.map { ObjectIdentifier($0) }) + if Set(closeRequests.keys) == allWindows { + trigger(.window) + return + } + } + + // Setup our new timer + debounceTimer = Timer.scheduledTimer( + withTimeInterval: Duration.milliseconds(100).timeInterval, + repeats: false + ) { [weak self] _ in + self?.trigger(.tab) + } + } + + /// Triggers all pending close callbacks with the given scope. + /// + /// This method is called when the coordinator has determined the user's intent + /// (either closing a tab or the entire window). It executes all pending callbacks + /// and resets the coordinator's state. + /// + /// - Parameter scope: The determined scope of the close operation. + private func trigger(_ scope: CloseScope) { + // Reset our state + tabGroup = nil + debounceTimer?.invalidate() + debounceTimer = nil + + // Trigger all of our callbacks + closeRequests.forEach { $0.value(scope) } + closeRequests = [:] + } +}