diff --git a/CHANGELOG.md b/CHANGELOG.md index 689b5e709b..2578b13be3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Copy To and Duplicate Database in the sidebar and the Database menu, carrying structure, data or both to any connection. (#2487) - Tab rows in Settings > General > Tabs, wrapping the strip instead of scrolling it. (#2438) - Autoscrolling while dragging a tab, so a tab can be moved past the run currently on screen. (#2438) +- Move Tab to New Window on a tab's right-click menu, and by dragging a tab out of the strip. (#2438) ### Changed diff --git a/TablePro/Core/Services/Infrastructure/ConnectionCloseAction.swift b/TablePro/Core/Services/Infrastructure/ConnectionCloseAction.swift index 05d6702d2c..c164db8dd8 100644 --- a/TablePro/Core/Services/Infrastructure/ConnectionCloseAction.swift +++ b/TablePro/Core/Services/Infrastructure/ConnectionCloseAction.swift @@ -26,16 +26,35 @@ internal enum ConnectionCloseAction { /// Pure so the case that used to fail silently is pinned by a test: a connection with no session /// has nothing to lose, and asking about it produced an alert nobody could answer. + /// Saves in every window hosting the connection, not only the one that answered first. Each + /// coordinator can save just its own selected tab's live work, so a Save that reached one of + /// them left the other window's grid edits behind and closed over them. + private static func saveEveryWindowsWork( + across coordinators: [MainContentCoordinator], + fallback: MainContentCoordinator? + ) async -> Bool { + let targets = coordinators.isEmpty ? [fallback].compactMap { $0 } : coordinators + for coordinator in targets { + guard await coordinator.commandActions?.saveSelectedTabWork() == true else { return false } + } + return !targets.isEmpty + } + internal static func decision(hasSession: Bool, hasUnsavedWork: Bool) -> Decision { guard hasSession, hasUnsavedWork else { return .closeImmediately } return .confirmUnsavedWork } internal static func close(connectionId: UUID) async { - let coordinator = WindowManager.shared.coordinator(for: connectionId) + /// Every window hosting the connection. A tab torn off into its own window keeps its live + /// grid and structure edits in that window's coordinator, and asking only the first one + /// reported the connection as safe to close over work nobody had been shown. + let coordinators = WindowManager.shared.coordinators(for: connectionId) + let coordinator = coordinators.first ?? WindowManager.shared.coordinator(for: connectionId) let decision = decision( hasSession: coordinator != nil, - hasUnsavedWork: coordinator?.hasAnyUnsavedWork() ?? false + hasUnsavedWork: coordinators.contains { $0.hasAnyUnsavedWork() } + || (coordinators.isEmpty && coordinator?.hasAnyUnsavedWork() == true) ) guard decision == .confirmUnsavedWork else { WindowManager.shared.closeWindow(for: connectionId) @@ -55,7 +74,7 @@ internal enum ConnectionCloseAction { case .save: /// Save closes too, once the save has actually landed. It used to start the save and /// stop there, so the connection the user asked to close stayed open. - guard await coordinator?.commandActions?.saveSelectedTabWork() == true else { + guard await saveEveryWindowsWork(across: coordinators, fallback: coordinator) else { WindowManager.shared.show(wasShowing, inWindowHosting: connectionId) break } diff --git a/TablePro/Core/Services/Infrastructure/EditorTabDetachPolicy.swift b/TablePro/Core/Services/Infrastructure/EditorTabDetachPolicy.swift new file mode 100644 index 0000000000..872a5337c6 --- /dev/null +++ b/TablePro/Core/Services/Infrastructure/EditorTabDetachPolicy.swift @@ -0,0 +1,33 @@ +// +// EditorTabDetachPolicy.swift +// TablePro +// + +import Foundation + +/// Whether a tab may be moved into a window of its own, kept apart from the window machinery so +/// the rule can be tested without one. +/// +/// The rule is narrow on purpose. Detaching moves a `QueryTab`, which carries what is persisted; +/// it does not carry a coordinator's live edit state, so a tab with work that has not been written +/// yet would arrive in the new window with that work silently gone. Refusing is the honest answer, +/// and the same one the strip gives for a reorder it cannot complete: the command dims rather than +/// destroying something quietly. +internal enum EditorTabDetachPolicy { + internal static func canDetach( + tabCount: Int, + hasUnsavedWork: Bool, + isBusy: Bool, + isConnected: Bool + ) -> Bool { + /// The last tab has nowhere to go. Moving it would empty this window and fill an identical + /// one, which is what `Move Connection to New Window` already does and says. + guard tabCount > 1 else { return false } + guard !hasUnsavedWork else { return false } + /// A query or a table load in flight is claimed by the coordinator that started it. Moving + /// the tab out from under one leaves the completion with no tab to write into and the new + /// window with no claim, so it fetches the same page again. + guard !isBusy else { return false } + return isConnected + } +} diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift index 18462475f1..87e7961de9 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift @@ -22,7 +22,7 @@ internal extension MainSplitViewController { /// The strip is built per connection like the other three panes, so a connection the user is /// not looking at keeps its own strip rather than rebuilding it on every switch. func refreshTabStripPane(of workspace: ConnectionWorkspace) { - workspace.panes.tabStrip.rootView = AnyView(buildTabStripView(for: workspace)) + workspace.panes.tabStrip.rootView = buildTabStripView(for: workspace) } func showSelectedTabStrip() { @@ -84,11 +84,11 @@ internal extension MainSplitViewController { /// The command set is handed to the pane's interaction object rather than to the SwiftUI view, /// because the view is no longer what receives a press. AppKit owns the pointer over the /// strip and reaches the app through exactly these closures. - @ViewBuilder - private func buildTabStripView(for workspace: ConnectionWorkspace) -> some View { - if let sessionState = workspace.sessionState { - let interaction = workspace.panes.tabStrip.interaction - let _ = configure(interaction, for: workspace, sessionState: sessionState) + private func buildTabStripView(for workspace: ConnectionWorkspace) -> AnyView { + guard let sessionState = workspace.sessionState else { return AnyView(Color.clear) } + let interaction = workspace.panes.tabStrip.interaction + configure(interaction, for: workspace, sessionState: sessionState) + return AnyView( EditorTabStrip( tabManager: sessionState.tabManager, interaction: interaction, @@ -99,9 +99,7 @@ internal extension MainSplitViewController { workspace?.sessionState?.coordinator.commandActions?.newTab() } ) - } else { - Color.clear - } + ) } private func configure( @@ -127,8 +125,21 @@ internal extension MainSplitViewController { moveTab: { [weak manager] id, destination in manager?.moveTab(id: id, to: destination) }, canMove: { [weak manager] id, offset in manager?.canMoveTab(id: id, by: offset) ?? false }, moveBy: { [weak manager] id, offset in manager?.moveTab(id: id, by: offset) }, - tearOff: { _ in }, - canTearOff: { _ in false }, + tearOff: { [weak workspace] id in + guard let connectionId = workspace?.connectionId else { return } + WindowManager.shared.openTabInNewWindow(connectionId: connectionId, tabId: id) + }, + canTearOff: { [weak workspace] id in + guard let workspace, + let sessionState = workspace.sessionState + else { return false } + return EditorTabDetachPolicy.canDetach( + tabCount: sessionState.tabManager.tabs.count, + hasUnsavedWork: sessionState.coordinator.hasUnsavedWork(forTab: id), + isBusy: sessionState.coordinator.tabExecution.isExecuting(id), + isConnected: DatabaseManager.shared.activeSessions[workspace.connectionId]?.driver != nil + ) + }, /// The resolver's description, not the drawn title: a table tab carries its database /// and schema there even when the short title is unique, and the tooltip is where a /// truncated or duplicated name is told apart. diff --git a/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift b/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift index 7a31993f36..dbba41926c 100644 --- a/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift +++ b/TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift @@ -33,6 +33,23 @@ internal final class TabPersistenceCoordinator { /// instruction deleted the state the disconnect had just saved. private(set) var hasObservedTabs = false + @ObservationIgnored nonisolated(unsafe) private static var shared: [UUID: TabPersistenceCoordinator] = [:] + + /// One per connection, however many windows host it. + /// + /// The saved tab set is the connection's, not the window's, so two windows on one connection + /// must not each keep their own view of whether a restore has run: `hasObservedTabs` is the + /// gate that stops a partial list being written over a full one, and a second instance starts + /// with it closed. A detached window's coordinator would then have every save withheld, and + /// whichever instance the periodic save happened to elect decided whether anything reached + /// disk at all. + internal static func forConnection(_ connectionId: UUID) -> TabPersistenceCoordinator { + if let existing = shared[connectionId] { return existing } + let created = TabPersistenceCoordinator(connectionId: connectionId) + shared[connectionId] = created + return created + } + init(connectionId: UUID) { self.connectionId = connectionId } @@ -110,6 +127,15 @@ internal final class TabPersistenceCoordinator { /// No automatic save path may call this: an empty in-memory tab list is not consent to /// discard what is on disk. internal func clearForUserClosedAllTabs() { + /// The union across every window hosting the connection, not the manager that just + /// emptied. Closing a container can empty a detached window while the original still holds + /// tabs, and discarding the saved set there would take those with it. + guard MainContentCoordinator.aggregatedTabs(for: connectionId).isEmpty else { + Self.logger.debug( + "[persist] clear refused, other windows still hold tabs connId=\(self.connectionId, privacy: .public)" + ) + return + } saveTask?.cancel() saveTask = nil let connId = connectionId diff --git a/TablePro/Core/Services/Infrastructure/WindowManager.swift b/TablePro/Core/Services/Infrastructure/WindowManager.swift index 0cc6b04500..d4dc235e74 100644 --- a/TablePro/Core/Services/Infrastructure/WindowManager.swift +++ b/TablePro/Core/Services/Infrastructure/WindowManager.swift @@ -109,6 +109,85 @@ internal final class WindowManager { } } + /// Moves one tab into a window of its own, the way a window tab is dragged out of its group. + /// + /// The connection is then hosted by two windows. That is a state the app already had before + /// 0.65.0 and kept the machinery for: the session, its driver and its saved tab set are the + /// connection's, not the window's, so both windows share one `ConnectionSession` and the close + /// path already refuses to disconnect while `hasOpenWindow(for:)` still answers true. What each + /// window owns is a `QueryTabManager`, and the tab moves between those. + /// + /// The new window is given the tab through a pre-built session state rather than through the + /// payload, so nothing re-opens or re-restores it, and the intent is `.openContent` because + /// `.restoreOrDefault` would fill the new window with the whole saved set. + @discardableResult + internal func openTabInNewWindow(connectionId: UUID, tabId: UUID) -> Bool { + /// Resolved by which workspace actually holds the tab. After one detach the connection is + /// hosted twice, and the singular lookup names an arbitrary one of them: asking it for a + /// tab that lives in the other window fails a move the user did ask for. + guard let origin = workspaces(for: connectionId).first(where: { workspace in + workspace.sessionState?.tabManager.tabs.contains { $0.id == tabId } ?? false + }), + let sourceState = origin.sessionState, + let tab = sourceState.tabManager.tabs.first(where: { $0.id == tabId }), + let connection = DatabaseManager.shared.activeSessions[connectionId]?.connection + else { return false } + + let payload = EditorTabPayload( + connectionId: connectionId, + tabType: tab.tabType, + tableName: tab.tableContext.tableName, + databaseName: tab.tableContext.databaseName, + schemaName: tab.tableContext.schemaName, + sourceFileURL: tab.content.sourceFileURL, + tabTitle: tab.title, + intent: .openContent + ) + let state = SessionStateFactory.create(connection: connection, payload: nil) + state.tabManager.tabs = [tab] + state.tabManager.selectedTabId = tab.id + + /// The rows the tab already loaded live in its `TabSession`, which belongs to the + /// coordinator's registry rather than to the tab. Without handing it over the new window + /// shows an empty grid it will not refill: the tab's `lastExecutedAt` is already set, so + /// nothing asks for the page again. + if let liveSession = sourceState.coordinator.tabSessionRegistry.session(for: tabId) { + state.coordinator.tabSessionRegistry.register(liveSession) + } + SessionStateFactory.registerPending(state, for: payload.id) + + guard let window = buildWindow(payload: payload, sessionState: state, autoConnect: false) else { + /// The pending entry expires on its own, but leaving it for the timeout would let the + /// next window opened for this connection adopt a session state holding a tab that + /// never left its old window. + SessionStateFactory.removePending(for: payload.id) + return false + } + + /// Removed only once the window exists. `closeTab` is the move-out primitive here: it takes + /// the tab out of the array and settles the selection, and unlike the user's own close it + /// neither prompts nor clears anything from disk. + sourceState.coordinator.tabSessionRegistry.unregister(id: tabId) + sourceState.tabManager.closeTab(id: tabId) + + /// A file's window mapping follows its tab, or reopening the file focuses the window the + /// tab has left and does nothing there. + if let sourceURL = tab.content.sourceFileURL, + let windowId = (window.contentViewController as? MainSplitViewController)? + .workspaces.workspace(for: connectionId)?.sessionState?.coordinator.windowId { + WindowLifecycleMonitor.shared.registerSourceFile(sourceURL, windowId: windowId) + } + + /// Ordered front as a window of its own. Left at `.automatic` the system preference can + /// merge it straight back into the tab group it was asked to leave, which is the trap + /// `openStandaloneWindow` already documents. + window.tabbingMode = .disallowed + window.makeKeyAndOrderFront(nil) + window.tabbingMode = .automatic + AppActivationPolicyController.shared.activate(ignoringOtherApps: true) + return true + } + private func buildWindow( payload: EditorTabPayload, sessionState: SessionStateFactory.SessionState?, @@ -273,10 +352,22 @@ internal final class WindowManager { } internal func workspace(for connectionId: UUID) -> ConnectionWorkspace? { - hosts() - .lazy - .compactMap { $0.workspaces.workspace(for: connectionId) } - .first + workspaces(for: connectionId).first + } + + /// Every window hosting this connection, in window order. + /// + /// A connection is hosted by more than one window as soon as a tab is torn off into its own + /// (`openTabInNewWindow`). The single-workspace lookup above then names an arbitrary one of + /// them, so anything that has to see all of a connection's tabs, or act on all of them, asks + /// this instead. One window still holds at most one workspace per connection, which is why + /// the per-window registry stays keyed by connection id. + internal func workspaces(for connectionId: UUID) -> [ConnectionWorkspace] { + hosts().compactMap { $0.workspaces.workspace(for: connectionId) } + } + + internal func coordinators(for connectionId: UUID) -> [MainContentCoordinator] { + workspaces(for: connectionId).compactMap { $0.sessionState?.coordinator } } /// The window hosting this connection, whatever state it is in. diff --git a/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift b/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift index bf4ce4bf98..76310067a0 100644 --- a/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift +++ b/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift @@ -43,6 +43,16 @@ internal enum WorkspaceCloseAction { return remaining.indices.contains(index) ? remaining[index] : remaining.last } + /// A tab is closed by the window that owns it. Sending every id to one coordinator drops the + /// ones it has never heard of, which is exactly the tabs that were torn off into another window. + private static func closeTabs(_ ids: [UUID], across coordinators: [MainContentCoordinator]) { + for coordinator in coordinators { + let owned = ids.filter { id in coordinator.tabManager.tabs.contains { $0.id == id } } + guard !owned.isEmpty else { continue } + coordinator.closeTabsByUser(ids: owned) + } + } + internal static func close(_ workspace: WorkspaceID) async { let containers = listedContainers(of: workspace.connectionId) Self.logger.info( @@ -56,13 +66,18 @@ internal enum WorkspaceCloseAction { await ConnectionCloseAction.close(connectionId: workspace.connectionId) return } - guard let hosted = WindowManager.shared.workspace(for: workspace.connectionId) else { + /// Every window hosting the connection. A tab torn off into its own window can be the + /// container's only remaining tab, and closing the entry from the window that happens to + /// answer first would leave that tab open under an entry the user just closed. + let hostedWorkspaces = WindowManager.shared.workspaces(for: workspace.connectionId) + guard let hosted = hostedWorkspaces.first else { Self.logger.error("close has no hosted workspace container=\(workspace.container, privacy: .public)") return } - let coordinator = hosted.sessionState?.coordinator - let victims = tabs(in: workspace.container, of: coordinator) + let coordinators = hostedWorkspaces.compactMap { $0.sessionState?.coordinator } + let coordinator = coordinators.first + let victims = coordinators.flatMap { tabs(in: workspace.container, of: $0) } /// Where the user was before the alert. Confirming reveals the work at risk, which switches /// the window to that connection and selects one of the tabs, and an answer that closes /// nothing has to put all of that back: leaving the user on a connection they did not ask @@ -77,7 +92,7 @@ internal enum WorkspaceCloseAction { /// work in it. Closing the entry regardless would destroy exactly what the alert said would /// stay, which is the promise the wording makes. guard closable.isSuperset(of: Set(victims.map(\.id))) else { - coordinator?.closeTabsByUser(ids: victims.map(\.id).filter { closable.contains($0) }) + closeTabs(victims.map(\.id).filter { closable.contains($0) }, across: coordinators) WindowManager.shared.show(wasShowing, inWindowHosting: workspace.connectionId) Self.logger.info( """ @@ -93,10 +108,14 @@ internal enum WorkspaceCloseAction { /// while the window loaded somewhere else. `beginClosing` is what lets the strip drop the /// browse cursor's own row early, and the cursor follows underneath. if !victims.isEmpty { - coordinator?.closeTabsByUser(ids: victims.map(\.id)) + closeTabs(victims.map(\.id), across: coordinators) + } + for hostedWorkspace in hostedWorkspaces { + hostedWorkspace.closeContainer(workspace.container) + } + for hostedWorkspace in hostedWorkspaces { + hostedWorkspace.beginClosing(workspace.container) } - hosted.closeContainer(workspace.container) - hosted.beginClosing(workspace.container) Self.logger.info( """ close done container=\(workspace.container, privacy: .public) \ @@ -106,12 +125,16 @@ internal enum WorkspaceCloseAction { ) let left = await browseAway(from: workspace, among: containers, coordinator: coordinator) - hosted.endClosing() + for hostedWorkspace in hostedWorkspaces { + hostedWorkspace.endClosing() + } guard left else { /// The connection never left, so the container is open again: it is where the next tab /// still opens, and a strip that did not list it would be lying about where the user is. /// The driver's own error is already on screen. - hosted.openContainer(workspace.container) + for hostedWorkspace in hostedWorkspaces { + hostedWorkspace.openContainer(workspace.container) + } WindowManager.shared.show(wasShowing, inWindowHosting: workspace.connectionId) Self.logger.error( "close could not leave container=\(workspace.container, privacy: .public)" @@ -122,10 +145,12 @@ internal enum WorkspaceCloseAction { /// Read again rather than reusing the list from before the switch: a table opened while the /// reconnect ran anchors the container all over again, and the entry would come back with /// one stray tab under it. A tab that new has nothing to lose. - let opened = tabs(in: workspace.container, of: coordinator) + let opened = coordinators.flatMap { tabs(in: workspace.container, of: $0) } if !opened.isEmpty { - coordinator?.closeTabsByUser(ids: opened.map(\.id)) - hosted.closeContainer(workspace.container) + closeTabs(opened.map(\.id), across: coordinators) + for hostedWorkspace in hostedWorkspaces { + hostedWorkspace.closeContainer(workspace.container) + } } landOnRemainingTab(after: workspace, among: containers, coordinator: coordinator) } diff --git a/TablePro/Views/Main/EditorTabContextMenuBuilder.swift b/TablePro/Views/Main/EditorTabContextMenuBuilder.swift index a003141bfe..f06ad43bb6 100644 --- a/TablePro/Views/Main/EditorTabContextMenuBuilder.swift +++ b/TablePro/Views/Main/EditorTabContextMenuBuilder.swift @@ -74,11 +74,11 @@ internal enum EditorTabContextMenuBuilder { private final class ClosureMenuTarget: NSObject { private let action: () -> Void - internal init(action: @escaping () -> Void) { + init(action: @escaping () -> Void) { self.action = action } - @objc internal func fire() { + @objc func fire() { action() } } diff --git a/TablePro/Views/Main/EditorTabStrip.swift b/TablePro/Views/Main/EditorTabStrip.swift index d758f22607..e7de9b9962 100644 --- a/TablePro/Views/Main/EditorTabStrip.swift +++ b/TablePro/Views/Main/EditorTabStrip.swift @@ -247,6 +247,9 @@ private struct EditorTabStripItem: View { .disabled(!canMoveLeft) Button(String(localized: "Move Tab Right")) { commands?.moveBy(tab.id, 1) } .disabled(!canMoveRight) + Divider() + Button(String(localized: "Move Tab to New Window")) { commands?.tearOff(tab.id) } + .disabled(!canTearOff) } .accessibilityElement(children: .combine) .accessibilityAddTraits(.isButton) @@ -264,6 +267,7 @@ private struct EditorTabStripItem: View { } .accessibilityAction(named: Text("Move Tab Left")) { if canMoveLeft { commands?.moveBy(tab.id, -1) } } .accessibilityAction(named: Text("Move Tab Right")) { if canMoveRight { commands?.moveBy(tab.id, 1) } } + .accessibilityAction(named: Text("Move Tab to New Window")) { if canTearOff { commands?.tearOff(tab.id) } } } /// Everything the tab draws lives inside the glass, never over it. A `GlassEffectContainer` @@ -340,6 +344,10 @@ private struct EditorTabStripItem: View { commands?.canKeepOpen(tab.id) ?? false } + private var canTearOff: Bool { + commands?.canTearOff(tab.id) ?? false + } + private var canMoveLeft: Bool { commands?.canMove(tab.id, -1) ?? false } diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift index 46449289c6..c3f0ae8751 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift @@ -41,6 +41,17 @@ extension MainContentCoordinator { return savability(of: tab) != .nothingAtRisk } + /// Whether one tab, named by id, is holding work a move would lose. + /// + /// Detaching a tab into its own window carries the `QueryTab` and nothing the coordinator holds + /// beside it, so this is what the command reads before offering itself. An id that names no tab + /// answers true: refusing a move is recoverable, performing one on a tab this window cannot see + /// is not. + func hasUnsavedWork(forTab id: UUID) -> Bool { + guard let tab = tabManager.tabs.first(where: { $0.id == id }) else { return true } + return savability(of: tab) != .nothingAtRisk + } + /// What a save can actually do for one tab, which is not the same question as whether it holds /// work. A batch close asks both: it saves what it can reach and leaves the rest open, so the /// two answers have to come from one switch. Reading the categories apart, in a second diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift index e9dad3dcd7..b9f7e9bfc0 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift @@ -57,8 +57,13 @@ extension MainContentCoordinator { /// /// The registry is still the fallback, for the window that has not adopted its session yet. static func allTabs(for connectionId: UUID) -> [QueryTab] { - if let hosted = WindowManager.shared.workspace(for: connectionId)?.sessionState?.coordinator { - return hosted.tabManager.tabs + /// Every window hosting the connection, not the first one found. A tab torn off into its + /// own window leaves the connection hosted twice, and answering from one of them numbers a + /// new query after tabs it cannot see and lists a container as closed while the other + /// window still has it open. + let hosted = WindowManager.shared.coordinators(for: connectionId) + if !hosted.isEmpty { + return dedupedById(hosted.flatMap(\.tabManager.tabs)) } let registered = activeCoordinators.values.filter { $0.connectionId == connectionId } if registered.count > 1 { @@ -69,7 +74,14 @@ extension MainContentCoordinator { """ ) } - return registered.flatMap { $0.tabManager.tabs } + return dedupedById(registered.flatMap { $0.tabManager.tabs }) + } + + /// One tab belongs to one window, but the registry fallback can list a connection's throwaway + /// coordinators alongside its real ones, and both would report the same tab. + private static func dedupedById(_ tabs: [QueryTab]) -> [QueryTab] { + var seen = Set() + return tabs.filter { seen.insert($0.id).inserted } } static func coordinator( diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index fcc0af2675..b5659dbbae 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -415,15 +415,22 @@ final class MainContentCoordinator { _didActivate.withLock { $0 } } - /// One window hosts every connection and a connection has one coordinator, so a - /// connection's tabs are simply that coordinator's list. Tabs used to be scattered across - /// a connection's windows and had to be gathered and renumbered. + /// Every tab the connection has open, across every window hosting it. Tearing a tab off into + /// its own window splits one connection's tabs over two coordinators, and the saved set is the + /// union: saving from one of them alone writes a partial list over the full one, which is how + /// tabs that were never closed get erased. + /// + /// Deduped by tab id, because `activeCoordinators` also holds the throwaway coordinators + /// SwiftUI builds and discards while re-evaluating a body, and those report the same tabs as + /// the real one until they deallocate. static func aggregatedTabs(for connectionId: UUID) -> [QueryTab] { - activeCoordinators.values + var seen = Set() + return activeCoordinators.values .filter { $0.connectionId == connectionId } .flatMap { coordinator in coordinator.tabManager.tabs.map(coordinator.enrichedForPersistence) } + .filter { seen.insert($0.id).inserted } } /// Resolve transient view state that only the live coordinator knows about @@ -628,7 +635,7 @@ final class MainContentCoordinator { dialect: dialect, dialectQuote: dialect.map { quoteIdentifierFromDialect($0) } ) - self.persistence = TabPersistenceCoordinator(connectionId: connection.id) + self.persistence = TabPersistenceCoordinator.forConnection(connection.id) ConnectionDataCache.shared(for: connection.id).ensureLoaded() changeManager.undoManagerProvider = { [weak self] in self?.contentWindow?.undoManager } diff --git a/TableProTests/Core/Services/EditorTabDetachPolicyTests.swift b/TableProTests/Core/Services/EditorTabDetachPolicyTests.swift new file mode 100644 index 0000000000..d37a7d039d --- /dev/null +++ b/TableProTests/Core/Services/EditorTabDetachPolicyTests.swift @@ -0,0 +1,51 @@ +// +// EditorTabDetachPolicyTests.swift +// TableProTests +// + +import Foundation +import Testing + +@testable import TablePro + +@Suite("Editor tab detach policy") +struct EditorTabDetachPolicyTests { + @Test("A tab among others, with nothing pending, on a live connection, can be detached") + func ordinaryTabDetaches() { + #expect(EditorTabDetachPolicy.canDetach(tabCount: 3, hasUnsavedWork: false, isBusy: false, isConnected: true)) + } + + /// Moving the only tab would empty this window and fill an identical one, which is what + /// `Move Connection to New Window` already does and says. + @Test("The last tab has nowhere to go") + func lastTabDoesNotDetach() { + #expect(!EditorTabDetachPolicy.canDetach(tabCount: 1, hasUnsavedWork: false, isBusy: false, isConnected: true)) + #expect(!EditorTabDetachPolicy.canDetach(tabCount: 0, hasUnsavedWork: false, isBusy: false, isConnected: true)) + } + + /// Detaching carries the `QueryTab` and nothing the coordinator holds beside it, so a tab with + /// work that has not been written yet would arrive in the new window with that work gone. + @Test("A tab holding unsaved work is refused") + func unsavedWorkBlocksDetach() { + #expect(!EditorTabDetachPolicy.canDetach(tabCount: 3, hasUnsavedWork: true, isBusy: false, isConnected: true)) + } + + /// A running query or table load is claimed by the coordinator that started it. Moving the tab + /// leaves the completion with no tab to write into, and the new window fetches the page again. + @Test("A tab with work in flight is refused") + func busyTabDoesNotDetach() { + #expect(!EditorTabDetachPolicy.canDetach( + tabCount: 3, + hasUnsavedWork: false, + isBusy: true, + isConnected: true + )) + } + + /// The new window adopts the live session. There is nothing for it to adopt while the + /// connection is down, and it would open onto the not-connected pane. + @Test("A disconnected connection has no session for the new window to adopt") + func disconnectedBlocksDetach() { + #expect(!EditorTabDetachPolicy.canDetach(tabCount: 3, hasUnsavedWork: false, isBusy: false, isConnected: false)) + } +} diff --git a/TableProUITests/EditorTabDetachUITests.swift b/TableProUITests/EditorTabDetachUITests.swift new file mode 100644 index 0000000000..db6c4e32ae --- /dev/null +++ b/TableProUITests/EditorTabDetachUITests.swift @@ -0,0 +1,137 @@ +import AppKit +import XCTest + +/// Moving a tab into a window of its own, and the two things that go wrong once one connection is +/// hosted by two windows: the tab is lost from both, or the shared session goes down with whichever +/// window closes first. +/// +/// Three tables, not two. `ConnectionWindowPaneResolver.showsTabStrip` hides the strip at one tab, +/// the way Safari hides its tab bar, so a source window left holding a single tab would have no +/// strip for these to read. The detached window has none either, which is why it is identified by +/// its title rather than by a tab element. +final class EditorTabDetachUITests: UITestCase { + private static let tables = ["Album", "Artist", "Customer"] + + func testMoveTabToNewWindowTakesTheTabOutOfTheStrip() throws { + let app = try launchWithSampleDatabase() + let window = try readyWindow(of: app) + openTables(Self.tables, in: window) + XCTAssertTrue( + waitForPredicate(timeout: 25) { self.tabLabels(in: window).count >= 3 }, + "The strip must show a tab per open table, got \(tabLabels(in: window))" + ) + + let before = tabLabels(in: window) + let moved = try XCTUnwrap(before.last, "The strip must hold a tab to move") + + detach(tabNamed: moved, in: window, of: app) + + XCTAssertTrue( + waitForPredicate(timeout: 20) { app.windows.count >= 2 }, + "Move Tab to New Window must open a second window" + ) + XCTAssertTrue( + waitForPredicate(timeout: 20) { !self.tabLabels(in: window).contains(moved) }, + "The tab must leave the window it was moved out of, still shows \(tabLabels(in: window))" + ) + XCTAssertTrue( + waitForPredicate(timeout: 20) { + app.windows.allElementsBoundByIndex.contains { $0.title.contains(moved) } + }, + "A window must be titled for the moved tab, titles are \(windowTitles(in: app))" + ) + } + + /// The session belongs to the connection, not to either window. + func testClosingTheDetachedWindowLeavesTheOriginalWorking() throws { + let app = try launchWithSampleDatabase() + let window = try readyWindow(of: app) + openTables(Self.tables, in: window) + XCTAssertTrue(waitForPredicate(timeout: 25) { self.tabLabels(in: window).count >= 3 }) + + let moved = try XCTUnwrap(tabLabels(in: window).last) + detach(tabNamed: moved, in: window, of: app) + XCTAssertTrue(waitForPredicate(timeout: 20) { app.windows.count >= 2 }) + + let detached = try XCTUnwrap( + app.windows.allElementsBoundByIndex.first { $0.title.contains(moved) }, + "The detached window must be findable by title" + ) + detached.buttons[XCUIIdentifierCloseWindow].click() + + XCTAssertTrue( + waitForPredicate(timeout: 20) { app.windows.count == 1 }, + "Closing the detached window must close it rather than empty it" + ) + XCTAssertTrue( + waitForPredicate(timeout: 20) { self.tabLabels(in: window).count == 2 }, + "The original window keeps its remaining tabs, shows \(tabLabels(in: window))" + ) + } + + /// A tab with a query still running is claimed by the coordinator that started it, so the + /// command stands down rather than orphaning the result. + func testTheCommandIsOfferedOnAnIdleTab() throws { + let app = try launchWithSampleDatabase() + let window = try readyWindow(of: app) + openTables(Self.tables, in: window) + XCTAssertTrue(waitForPredicate(timeout: 25) { self.tabLabels(in: window).count >= 3 }) + + let target = try XCTUnwrap(tabLabels(in: window).last) + tab(named: target, in: window).rightClick() + + let item = app.menuItems["Move Tab to New Window"] + XCTAssertTrue(item.waitToExist(timeout: 5), "The command must be listed") + XCTAssertTrue(item.isEnabled, "It must be offered on an idle tab among others") + app.typeKey(XCUIKeyboardKey.escape, modifierFlags: []) + } + + // MARK: - Helpers + + private func detach(tabNamed name: String, in window: XCUIElement, of app: XCUIApplication) { + let target = tab(named: name, in: window) + XCTAssertTrue(waitUntilHittable(target, timeout: 20), "The tab must be hittable") + target.rightClick() + let item = app.menuItems["Move Tab to New Window"] + XCTAssertTrue(item.waitToExist(timeout: 5), "The command must be listed") + item.click() + } + + private func readyWindow(of app: XCUIApplication) throws -> XCUIElement { + let window = app.windows.firstMatch + XCTAssertTrue(window.waitToExist(timeout: 30)) + XCTAssertTrue( + waitForPredicate(timeout: 30) { window.outlines.firstMatch.outlineRows.count > 1 }, + "The object browser must list the sample database's tables" + ) + return window + } + + private func openTables(_ names: [String], in window: XCUIElement) { + for name in names { + let row = objectBrowserRow(name, in: window) + XCTAssertTrue(row.waitToExist(timeout: 20), "The object browser must list \(name)") + row.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).doubleClick() + Thread.sleep(forTimeInterval: NSEvent.doubleClickInterval) + } + } + + private func tabLabels(in window: XCUIElement) -> [String] { + window.descendants(matching: .any) + .matching(identifier: "editor-tab") + .allElementsBoundByIndex + .sorted { $0.frame.minX < $1.frame.minX } + .map { $0.label } + } + + private func windowTitles(in app: XCUIApplication) -> [String] { + app.windows.allElementsBoundByIndex.map { $0.title } + } + + private func tab(named name: String, in window: XCUIElement) -> XCUIElement { + window.descendants(matching: .any) + .matching(identifier: "editor-tab") + .matching(NSPredicate(format: "label == %@", name)) + .firstMatch + } +} diff --git a/docs/features/tabs.mdx b/docs/features/tabs.mdx index 4b84257692..959fe0c598 100644 --- a/docs/features/tabs.mdx +++ b/docs/features/tabs.mdx @@ -85,6 +85,14 @@ Right-click a tab for **Move Tab Left** and **Move Tab Right**, which move it on Tabs cannot be pinned. Pinning exists for result tabs inside a query tab (`Cmd+Option+P`). +## Moving a tab to its own window + +Drag a tab down out of the strip, or right-click it and choose **Move Tab to New Window**, to open it in a window of its own. Both windows work on the same connection and the same session, so closing one leaves the other connected. + +The command dims for a window's only tab, for a tab with a query or a table load still running, and while the connection is down. **Move Connection to New Window**, on the connections strip, moves a whole connection instead. + +Tabs with unsaved work stay put. A move carries the tab, not the edits pending in the grid or the ALTERs staged in the structure editor, so save those first and the command comes back. + ## Tabs that stop fitting The strip keeps one row and scrolls it, the way every macOS tab bar does. Scroll over it with two fingers to reach the rest.