mirror of
https://github.com/ghostty-org/ghostty.git
synced 2025-09-05 19:08:17 +00:00
macos: fix undo/redo for closing windows with multiple tabs
When closing a window that contains multiple tabs, the undo operation now properly restores all tabs as a single tabbed window rather than just restoring the active tab. The implementation: - Collects undo states from all windows in the tab group before closing - Sorts them by their original tab index to preserve order - Clears tab group references to avoid referencing garbage collected objects - Restores all windows and re-adds them as tabs to the first window - Tracks and restores which tab was focused (or focuses the last tab if none were) AI prompts that generated this commit are below. Each separate prompt is separated by a blank line, so this session was made up with many prompts in a back-and-forth conversation. > We need to update the undo/redo implementation in > @macos/Sources/Features/Terminal/TerminalController.swift `closeWindowImmediately` > to handle the case that multiple windows in a tab group are closed all at once, > and to restore them as a tabbed window. To do this, I think we should collect > all the `undoStates`, sort them by `tabIndex` (null at the end), and then on j > restore, restore them one at a time but add them back to the same tabGroup. We > can't use the tab group in the `undoState` because it will be garbage collected > by then. To be sure, we should just set it to nil. I should note at this point that the feature already worked, but the code quality and organization wasn't up to my standards. If someone using AI were just trying to make something work, they might be done at this point. I do think this is the biggest gap I worry about with AI-assisted development: bridging between the "it works" stage at a junior quality and the "it works and is maintainable" stage at a senior quality. I suspect this will be a balance of LLMs getting better but also senior code reviewers remaining highly involved in the process. > Let's extract all the work you just did into a dedicated private method > called `registerUndoForCloseWindow` Manual: made some tweaks to comments, moved some lines around, didn’t change any logic. > I think we can pull the tabIndex directly from the undoState instead of > storing it in a tuple. > Instead of `var undoStates`, I think we can create a `let undoStates` and > build and filter and sort them all in a chain of functional mappings. > Okay, looking at your logic for restoration, the `var firstController` and > conditionals are littly messy. Can you make your own pass at cleaning those > up and I'll review and provide more specific guidance after. > Excellent. Perfect. The last thing we're missing is restoring the proper > focused window of the tab group. We should store that and make sure the > proper window is made key. If no windows were key, then we should make the > last one key. > Excellent. Any more cleanups or comments you'd recommend in the places you > changed? Notes on the last one: it gave me a bunch of suggestions, I rejected most but did accept some. > Can you write me a commit message summarizing the changes? It wrote me a part of the commit message you're reading now, but I always manually tweak the commit message and add my own flair.
This commit is contained in:
@@ -647,41 +647,125 @@ class TerminalController: BaseTerminalController {
|
||||
private func closeWindowImmediately() {
|
||||
guard let window = window else { return }
|
||||
|
||||
// Regardless of tabs vs no tabs, what we want to do here is keep
|
||||
// track of the window frame to restore, the surface tree, and the
|
||||
// the focused surface. We want to restore that with undo even
|
||||
// if we end up closing.
|
||||
if let undoManager, let undoState {
|
||||
// Register undo action to restore the window
|
||||
undoManager.setActionName("Close Window")
|
||||
undoManager.registerUndo(
|
||||
withTarget: ghostty,
|
||||
expiresAfter: undoExpiration) { ghostty in
|
||||
// Restore the undo state
|
||||
let newController = TerminalController(ghostty, with: undoState)
|
||||
// Register undo for this close operation
|
||||
registerUndoForCloseWindow()
|
||||
|
||||
// Register redo action
|
||||
// Close the window(s)
|
||||
if let tabGroup = window.tabGroup, tabGroup.windows.count > 1 {
|
||||
tabGroup.windows.forEach { $0.close() }
|
||||
} else {
|
||||
window.close()
|
||||
}
|
||||
}
|
||||
|
||||
/// Registers undo for closing window(s), handling both single windows and tab groups.
|
||||
private func registerUndoForCloseWindow() {
|
||||
guard let undoManager else { return }
|
||||
guard let window else { return }
|
||||
|
||||
// If we don't have a tab group or we don't have multiple tabs, then
|
||||
// do a normal single window close.
|
||||
guard let tabGroup = window.tabGroup,
|
||||
tabGroup.windows.count > 1 else {
|
||||
// No tabs, just save this window's state
|
||||
if let undoState {
|
||||
// Register undo action to restore the window
|
||||
undoManager.setActionName("Close Window")
|
||||
undoManager.registerUndo(
|
||||
withTarget: newController,
|
||||
expiresAfter: newController.undoExpiration) { target in
|
||||
target.closeWindowImmediately()
|
||||
withTarget: ghostty,
|
||||
expiresAfter: undoExpiration) { ghostty in
|
||||
// Restore the undo state
|
||||
let newController = TerminalController(ghostty, with: undoState)
|
||||
|
||||
// Register redo action
|
||||
undoManager.registerUndo(
|
||||
withTarget: newController,
|
||||
expiresAfter: newController.undoExpiration) { target in
|
||||
target.closeWindowImmediately()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// Multiple windows in tab group - collect all undo states in sorted order
|
||||
// by tab ordering. Also track which window was key.
|
||||
let undoStates = tabGroup.windows
|
||||
.compactMap { tabWindow -> UndoState? in
|
||||
guard let controller = tabWindow.windowController as? TerminalController,
|
||||
var undoState = controller.undoState else { return nil }
|
||||
// Clear the tab group reference since it is unneeded. It should be
|
||||
// garbage collected but we want to be extra sure we don't try to
|
||||
// restore into it because we're going to recreate it.
|
||||
undoState.tabGroup = nil
|
||||
return undoState
|
||||
}
|
||||
.sorted { (lhs, rhs) in
|
||||
switch (lhs.tabIndex, rhs.tabIndex) {
|
||||
case let (l?, r?): return l < r
|
||||
case (_?, nil): return true
|
||||
case (nil, _?): return false
|
||||
case (nil, nil): return true
|
||||
}
|
||||
}
|
||||
|
||||
// Find the index of the key window in our sorted states. This is a bit verbose
|
||||
// but we only need this for this style of undo so we don't want to add it to
|
||||
// UndoState.
|
||||
let keyWindowIndex: Int?
|
||||
if let keyWindow = tabGroup.windows.first(where: { $0.isKeyWindow }),
|
||||
let keyController = keyWindow.windowController as? TerminalController,
|
||||
let keyUndoState = keyController.undoState {
|
||||
keyWindowIndex = undoStates.firstIndex {
|
||||
$0.tabIndex == keyUndoState.tabIndex }
|
||||
} else {
|
||||
keyWindowIndex = nil
|
||||
}
|
||||
|
||||
guard let tabGroup = window.tabGroup else {
|
||||
// No tabs, no tab group, just perform a normal close.
|
||||
window.close()
|
||||
return
|
||||
}
|
||||
// Register undo action to restore all windows
|
||||
guard !undoStates.isEmpty else { return }
|
||||
|
||||
// If have one window then we just do a normal close
|
||||
if tabGroup.windows.count == 1 {
|
||||
window.close()
|
||||
return
|
||||
}
|
||||
undoManager.setActionName("Close Window")
|
||||
undoManager.registerUndo(
|
||||
withTarget: ghostty,
|
||||
expiresAfter: undoExpiration
|
||||
) { ghostty in
|
||||
// Restore all windows in the tab group
|
||||
let controllers = undoStates.map { undoState in
|
||||
TerminalController(ghostty, with: undoState)
|
||||
}
|
||||
|
||||
// The first controller becomes the parent window for all tabs.
|
||||
// If we don't have a first controller (shouldn't be possible?)
|
||||
// then we can't restore tabs.
|
||||
guard let firstController = controllers.first else { return }
|
||||
|
||||
// Add all subsequent controllers as tabs to the first window
|
||||
for controller in controllers.dropFirst() {
|
||||
controller.showWindow(nil)
|
||||
if let firstWindow = firstController.window,
|
||||
let newWindow = controller.window {
|
||||
firstWindow.addTabbedWindow(newWindow, ordered: .above)
|
||||
}
|
||||
}
|
||||
|
||||
// Make the appropriate window key. If we had a key window, restore it.
|
||||
// Otherwise, make the last window key.
|
||||
if let keyWindowIndex, keyWindowIndex < controllers.count {
|
||||
controllers[keyWindowIndex].window?.makeKeyAndOrderFront(nil)
|
||||
} else {
|
||||
controllers.last?.window?.makeKeyAndOrderFront(nil)
|
||||
}
|
||||
|
||||
tabGroup.windows.forEach { $0.close() }
|
||||
// Register redo action on the first controller
|
||||
undoManager.registerUndo(
|
||||
withTarget: firstController,
|
||||
expiresAfter: firstController.undoExpiration
|
||||
) { target in
|
||||
target.closeWindowImmediately()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Close all windows, asking for confirmation if necessary.
|
||||
@@ -734,7 +818,7 @@ class TerminalController: BaseTerminalController {
|
||||
let surfaceTree: SplitTree<Ghostty.SurfaceView>
|
||||
let focusedSurface: UUID?
|
||||
let tabIndex: Int?
|
||||
private(set) weak var tabGroup: NSWindowTabGroup?
|
||||
weak var tabGroup: NSWindowTabGroup?
|
||||
}
|
||||
|
||||
convenience init(_ ghostty: Ghostty.App,
|
||||
|
Reference in New Issue
Block a user