diff --git a/CHANGELOG.md b/CHANGELOG.md index 30baa2dd3..2367f576f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Switch Connection and Open Database now open on a narrow window, and after you remove their toolbar button, instead of doing nothing at all. + ## [0.67.1] - 2026-08-22 ### Fixed diff --git a/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift b/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift index 6c24438b0..a82f38697 100644 --- a/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift +++ b/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift @@ -22,9 +22,6 @@ struct ConnectionToolbarButton: View { /// shows, so drawing it put a second idiom beside the native icon-only items for nothing. .labelStyle(.iconOnly) .help(AppSettingsManager.shared.keyboard.shortcutHint(String(localized: "Switch Connection"), for: .switchConnection)) - .popover(isPresented: $coordinator.isConnectionSwitcherShown, arrowEdge: .bottom) { - ConnectionSwitcherPopover() - } } } @@ -47,9 +44,6 @@ struct DatabaseToolbarButton: View { state.connectionState != .connected || PluginManager.shared.connectionMode(for: state.databaseType) == .fileBased ) - .popover(isPresented: $coordinator.isDatabaseSwitcherShown, arrowEdge: .bottom) { - DatabaseSwitcherPopoverHost(coordinator: coordinator) - } } } } diff --git a/TablePro/Core/Services/Infrastructure/MainWindowToolbar.swift b/TablePro/Core/Services/Infrastructure/MainWindowToolbar.swift index 8b65360cf..4b04aa0d2 100644 --- a/TablePro/Core/Services/Infrastructure/MainWindowToolbar.swift +++ b/TablePro/Core/Services/Infrastructure/MainWindowToolbar.swift @@ -126,6 +126,14 @@ internal final class MainWindowToolbar: NSObject, NSToolbarDelegate { /// without this guard the window would pay for a switch every time it came forward. internal func repoint(to coordinator: MainContentCoordinator?) { guard subject.coordinator !== coordinator else { return } + /// The switcher used to close itself here, because its popover lived inside a view keyed + /// `.id(coordinator.connectionId)` and SwiftUI tore that identity down on a repoint. The + /// presenter owns the surface now, so the dismissal has to be explicit or a workspace + /// switch would leave the chooser open over the connection it no longer belongs to. + subject.coordinator?.switcherPresenter.dismiss() + /// The chip's chooser is SwiftUI-presented and dies with the view a repoint destroys, but + /// its state does not, so it would spring open again on the way back to this connection. + subject.coordinator?.presentedScopeSwitcher = nil pendingChangeObservationGeneration += 1 subject.coordinator = coordinator observePendingChangeState() @@ -135,6 +143,10 @@ internal final class MainWindowToolbar: NSObject, NSToolbarDelegate { } func invalidate() { + /// Window close reaches here rather than through `repoint`, and the panel surface is an + /// independent floating `NSPanel` with no parent-child relationship to the window, so + /// nothing else would take it down with the window that opened it. + subject.coordinator?.switcherPresenter.dismiss() pendingChangeObservationGeneration += 1 sidebarGroup = nil hostingControllers.removeAll() diff --git a/TablePro/Views/Components/PopoverPresenter.swift b/TablePro/Views/Components/PopoverPresenter.swift index 3b4b81db8..007ecca90 100644 --- a/TablePro/Views/Components/PopoverPresenter.swift +++ b/TablePro/Views/Components/PopoverPresenter.swift @@ -26,6 +26,29 @@ enum PopoverPresenter { return popover } + /// Shows a SwiftUI view in an NSPopover anchored to a toolbar item. + /// + /// AppKit resolves the anchor itself: "When the item is in the overflow menu, the popover will + /// be presented from another appropriate affordance in the window." That is the whole reason + /// this overload exists, because a popover declared inside the item's own hosted view cannot + /// present at all once the item is clipped, and a clipped item survives only as its + /// `menuFormRepresentation`. + /// + /// The caller must have resolved `toolbarItem` out of a visible toolbar. AppKit throws + /// `NSInvalidArgumentException` when it cannot locate the item, which Swift cannot catch, so + /// the check belongs at the call site as a precondition rather than here as error handling. + @discardableResult + static func show( + relativeTo toolbarItem: NSToolbarItem, + contentSize: NSSize? = nil, + behavior: NSPopover.Behavior = .semitransient, + @ViewBuilder content: (_ dismiss: @escaping () -> Void) -> Content + ) -> NSPopover { + let popover = make(contentSize: contentSize, behavior: behavior, content: content) + popover.show(relativeTo: toolbarItem) + return popover + } + /// Builds the popover without presenting it, so its sizing can be asserted in tests. /// /// `NSPopover` computes where to put itself from `contentSize` at `show` time, and its header's diff --git a/TablePro/Views/DatabaseSwitcher/DatabaseSwitcherPopover.swift b/TablePro/Views/DatabaseSwitcher/DatabaseSwitcherPopover.swift index 3969d8411..a019c835e 100644 --- a/TablePro/Views/DatabaseSwitcher/DatabaseSwitcherPopover.swift +++ b/TablePro/Views/DatabaseSwitcher/DatabaseSwitcherPopover.swift @@ -7,6 +7,7 @@ struct DatabaseSwitcherPopoverHost: View { /// Which container dimension this presentation switches. An engine can have both, so the caller /// names the one it opened rather than the popover guessing from the engine's primary target. var target: ContainerSwitchTarget? + let dismiss: () -> Void var body: some View { if let coordinator { @@ -38,7 +39,8 @@ struct DatabaseSwitcherPopoverHost: View { }, onRequestExport: { [weak coordinator] containers in coordinator?.openExportDialog(containers: containers) - } + }, + dismiss: dismiss ) } else { EmptyView() @@ -60,12 +62,15 @@ struct DatabaseSwitcherPopover: View { let onRequestDrop: ([DatabaseContainerRef]) -> Void let onRequestExport: ([DatabaseContainerRef]) -> Void - @Environment(\.dismiss) private var dismiss + /// An explicit closure rather than `@Environment(\.dismiss)`: the presenter owns the surface, + /// and this content is hosted in an AppKit popover or panel that SwiftUI cannot dismiss. + let dismiss: () -> Void @State private var viewModel: DatabaseSwitcherViewModel @State private var supportsCreateDatabase = false - private static let popoverWidth: CGFloat = 320 - private static let popoverHeight: CGFloat = 360 + /// One declaration, read by this view's own frame and by whoever presents it, so the + /// surface and its host can never disagree about how big it is. + static let contentSize = NSSize(width: 320, height: 360) private var supportsDropDatabase: Bool { PluginManager.shared.supportsDropDatabase(for: databaseType) @@ -98,7 +103,8 @@ struct DatabaseSwitcherPopover: View { onSelect: @escaping (String) -> Void, onRequestCreate: @escaping () -> Void, onRequestDrop: @escaping ([DatabaseContainerRef]) -> Void, - onRequestExport: @escaping ([DatabaseContainerRef]) -> Void + onRequestExport: @escaping ([DatabaseContainerRef]) -> Void, + dismiss: @escaping () -> Void ) { self.currentDatabase = currentDatabase self.activeDatabase = activeDatabase @@ -110,6 +116,7 @@ struct DatabaseSwitcherPopover: View { self.onRequestCreate = onRequestCreate self.onRequestDrop = onRequestDrop self.onRequestExport = onRequestExport + self.dismiss = dismiss self._viewModel = State( wrappedValue: DatabaseSwitcherViewModel( connectionId: connectionId, @@ -133,7 +140,7 @@ struct DatabaseSwitcherPopover: View { createButton } } - .frame(width: Self.popoverWidth, height: Self.popoverHeight) + .frame(width: Self.contentSize.width, height: Self.contentSize.height) .background(refreshShortcut) .task { await viewModel.fetchDatabases() } .task { await refreshCreateSupport() } diff --git a/TablePro/Views/Main/MainContentCommandActions.swift b/TablePro/Views/Main/MainContentCommandActions.swift index 285b88582..5da9fcc8f 100644 --- a/TablePro/Views/Main/MainContentCommandActions.swift +++ b/TablePro/Views/Main/MainContentCommandActions.swift @@ -1306,7 +1306,7 @@ final class MainContentCommandActions { guard PluginManager.shared.connectionMode(for: type) != .fileBased else { return } coordinator.contentWindow?.makeFirstResponder(nil) coordinator.presentedScopeSwitcher = nil - coordinator.isDatabaseSwitcherShown = true + presentDatabaseSwitcher(on: coordinator, target: nil) } /// The same chooser, opened from the toolbar chip so it appears against the scope it switches. @@ -1317,7 +1317,7 @@ final class MainContentCommandActions { let type = coordinator.connection.type guard PluginManager.shared.switchableContainers(for: type).contains(target) else { return } coordinator.contentWindow?.makeFirstResponder(nil) - coordinator.isDatabaseSwitcherShown = false + coordinator.switcherPresenter.dismiss() coordinator.presentedScopeSwitcher = target } @@ -1326,8 +1326,29 @@ final class MainContentCommandActions { } func openConnectionSwitcher() { - coordinator?.contentWindow?.makeFirstResponder(nil) - coordinator?.isConnectionSwitcherShown = true + guard let coordinator else { return } + coordinator.contentWindow?.makeFirstResponder(nil) + coordinator.presentedScopeSwitcher = nil + coordinator.switcherPresenter.present( + from: coordinator.contentWindow, + anchoredTo: MainWindowToolbar.connectionGroup, + contentSize: ConnectionSwitcherPopover.contentSize + ) { dismiss in + ConnectionSwitcherPopover(dismiss: dismiss) + } + } + + /// Anchored to the connection group rather than to the Database button inside it, because the + /// group is the only item AppKit draws a frame for: its subitems exist to populate the overflow + /// menu and carry no frame of their own. + private func presentDatabaseSwitcher(on coordinator: MainContentCoordinator, target: ContainerSwitchTarget?) { + coordinator.switcherPresenter.present( + from: coordinator.contentWindow, + anchoredTo: MainWindowToolbar.connectionGroup, + contentSize: DatabaseSwitcherPopover.contentSize + ) { dismiss in + DatabaseSwitcherPopoverHost(coordinator: coordinator, target: target, dismiss: dismiss) + } } // MARK: - Undo/Redo (Group A — Called Directly) diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index e3c674d98..76a51298b 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -223,12 +223,14 @@ final class MainContentCoordinator { var cursorPositions: [CursorPosition] = [] var tableMetadata: TableMetadata? var activeSheet: ActiveSheet? - var isDatabaseSwitcherShown = false /// Which scope the toolbar chip is showing a chooser for, so the popover opens against the - /// component the user clicked. Separate from `isDatabaseSwitcherShown`, which belongs to the - /// toolbar button, and the two are cleared together so a window never holds two of them. + /// component the user clicked. Separate from the switchers the presenter owns, and cleared + /// alongside them so a window never holds two of them. var presentedScopeSwitcher: ContainerSwitchTarget? - var isConnectionSwitcherShown = false + /// Owns the connection and database switcher surfaces. The commands present through this + /// rather than flipping a flag a toolbar-hosted view has to observe, because that view is + /// absent whenever its item is clipped into the overflow menu or removed by the user. + @ObservationIgnored lazy var switcherPresenter = ToolbarSwitcherPresenter(panelController: quickSwitcherPanel) var sessionContexts: [PluginSessionContext] = [] var containerDropRequest: DatabaseDropRequest? var importFileURL: URL? diff --git a/TablePro/Views/Toolbar/ConnectionStatusView.swift b/TablePro/Views/Toolbar/ConnectionStatusView.swift index fd87e0233..67f5f422c 100644 --- a/TablePro/Views/Toolbar/ConnectionStatusView.swift +++ b/TablePro/Views/Toolbar/ConnectionStatusView.swift @@ -86,8 +86,17 @@ struct ConnectionStatusView: View { .buttonStyle(.plain) .help(switchableTooltip(component)) .accessibilityLabel(accessibilityLabel(component)) + /// This chip keeps a SwiftUI popover, and correctly so: it lives in the centred status + /// item, has no menu command and no shortcut, so there is no route that can fire it + /// while its own view is off screen. Dismissal clears the state that presents it, + /// which is what `@Environment(\.dismiss)` used to do before the switcher content + /// started taking an explicit closure. .popover(isPresented: presentation(of: component.kind), arrowEdge: .bottom) { - DatabaseSwitcherPopoverHost(coordinator: coordinator, target: component.kind) + DatabaseSwitcherPopoverHost( + coordinator: coordinator, + target: component.kind, + dismiss: { [weak coordinator] in coordinator?.presentedScopeSwitcher = nil } + ) } } else { scopeLabel(component) diff --git a/TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift b/TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift index 8734a9d47..aa669ec8a 100644 --- a/TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift +++ b/TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift @@ -36,14 +36,19 @@ struct ConnectionSwitcherEntry: Identifiable { } struct ConnectionSwitcherPopover: View { - @Environment(\.dismiss) private var dismiss + /// An explicit closure rather than `@Environment(\.dismiss)`, because the presenter owns the + /// surface: `dismiss` reaches a SwiftUI presentation, and this content is hosted in an AppKit + /// popover or panel that SwiftUI knows nothing about. `PopoverPresenter` hands every caller the + /// same shape. + let dismiss: () -> Void @State private var savedConnections: [DatabaseConnection] = [] @State private var selectedConnectionId: UUID? @State private var searchText = "" - private static let popoverWidth: CGFloat = 400 - private static let popoverHeight: CGFloat = 460 + /// One declaration, read by this view's own frame and by whoever presents it, so the + /// surface and its host can never disagree about how big it is. + static let contentSize = NSSize(width: 400, height: 460) private var activeSessions: [UUID: ConnectionSession] { DatabaseManager.shared.activeSessions @@ -85,7 +90,7 @@ struct ConnectionSwitcherPopover: View { manageButton } - .frame(width: Self.popoverWidth, height: Self.popoverHeight) + .frame(width: Self.contentSize.width, height: Self.contentSize.height) .onAppear { savedConnections = ConnectionStorage.shared.loadConnections() if selectedConnectionId == nil { diff --git a/TablePro/Views/Toolbar/ToolbarSwitcherPresenter.swift b/TablePro/Views/Toolbar/ToolbarSwitcherPresenter.swift new file mode 100644 index 000000000..75e531a49 --- /dev/null +++ b/TablePro/Views/Toolbar/ToolbarSwitcherPresenter.swift @@ -0,0 +1,122 @@ +// +// ToolbarSwitcherPresenter.swift +// TablePro +// + +import AppKit +import SwiftUI + +/// Presents the connection and database switchers for one connection, from the command that opens +/// them rather than from a view inside a toolbar item. +/// +/// The switchers used to be `.popover(isPresented:)` on a SwiftUI view mounted as the connection +/// group's hosted content, with the command only flipping a flag. AppKit clips a toolbar item into +/// the overflow menu when the window is narrow, and `NSToolbarItem`'s contract is that a clipped +/// item remains reachable through its `menuFormRepresentation`, never through its view. So the view +/// was not on screen, nothing observed the flag, and Switch Connection did nothing at all: no +/// popover, no error. The same held after Customize Toolbar removed the item, at any window width, +/// which is why the HIG says a toolbar "can't be the only place that presents a command". +/// +/// Two surfaces, chosen by whether an anchor exists: +/// - The item is in a visible toolbar: an `NSPopover` anchored to it, which is the macOS idiom for +/// a toolbar control that reveals a chooser. A clipped item still resolves, and AppKit presents it +/// "from another appropriate affordance in the window" itself. +/// - No anchor: the same content in the floating panel Open Quickly already uses, which belongs to +/// the window rather than to the toolbar. +@MainActor +internal final class ToolbarSwitcherPresenter { + private var popover: NSPopover? + /// The window's one floating panel, passed in rather than built here. `MainContentCoordinator` + /// already owns a `QuickSwitcherPanelController` for Open Quickly, and a second one would give a + /// window two independent panels centred on the same point, neither able to see or dismiss the + /// other. + private let panelController: QuickSwitcherPanelController + private var closeObserver: (any NSObjectProtocol)? + + internal init(panelController: QuickSwitcherPanelController) { + self.panelController = panelController + } + + internal var isPresenting: Bool { + popover?.isShown == true || panelController.isPresented + } + + /// `anchoredTo` is an identifier rather than an item because the item has to be resolved at + /// presentation time: the toolbar rebuilds, and an item the user removed is simply absent. + /// + /// Invoking the command while the switcher is up closes it, matching `showQuickSwitcher()` and + /// the toggle the toolbar button used to give for free. Without it a second press would tear the + /// surface down and rebuild it with empty `@State`, losing whatever the user had typed. + internal func present( + from window: NSWindow?, + anchoredTo identifier: NSToolbarItem.Identifier, + contentSize: NSSize, + @ViewBuilder content: (_ dismiss: @escaping () -> Void) -> some View + ) { + guard !isPresenting else { + dismiss() + return + } + + if let item = Self.anchor(in: window, identifier) { + /// `.transient`, not `PopoverPresenter`'s `.semitransient` default: a semitransient + /// popover ignores interaction outside its own window, so moving to another window or + /// another app would leave the chooser floating over a window it no longer belongs to. + /// The SwiftUI popover this replaces closed on any outside interaction. + let shown = PopoverPresenter.show( + relativeTo: item, + contentSize: contentSize, + behavior: .transient, + content: content + ) + popover = shown + /// AppKit closes a transient popover by itself and nothing else reports it. Without this + /// the presenter holds a closed popover, and through it the hosting controller, the + /// SwiftUI tree and the switcher's loaded container list, until the next presentation. + closeObserver = NotificationCenter.default.addObserver( + forName: NSPopover.didCloseNotification, + object: shown, + queue: .main + ) { [weak self] _ in + MainActor.assumeIsolated { self?.forgetPopover() } + } + return + } + + /// The panel paints no background of its own: it is borderless, clear and corner-masked, and + /// the surface material belongs to its content, the way `QuickSwitcherPanelView` supplies + /// it. Without this the switcher floats as unbacked text with its corners cut off. + let dismissPanel: () -> Void = { [weak self] in self?.panelController.dismiss() } + panelController.present( + content(dismissPanel).quickSwitcherSurface(cornerRadius: QuickSwitcherMetrics.cornerRadius), + over: window + ) + } + + internal func dismiss() { + popover?.performClose(nil) + forgetPopover() + panelController.dismiss() + } + + private func forgetPopover() { + if let closeObserver { + NotificationCenter.default.removeObserver(closeObserver) + } + closeObserver = nil + popover = nil + } + + /// A hidden toolbar is treated as no anchor at all. `toggleToolbarShown` only flips + /// `NSToolbar.isVisible` and leaves the items in place, so the item still resolves and AppKit + /// documents nothing about what anchoring to it then does. Since the failure mode of guessing + /// wrong is an `NSInvalidArgumentException` that Swift cannot catch, this takes the branch it + /// can reason about instead of the one it would have to measure. + internal static func anchor( + in window: NSWindow?, + _ identifier: NSToolbarItem.Identifier + ) -> NSToolbarItem? { + guard let toolbar = window?.toolbar, toolbar.isVisible else { return nil } + return toolbar.items.first { $0.itemIdentifier == identifier } + } +} diff --git a/TableProTests/Services/ToolbarSwitcherAnchorTests.swift b/TableProTests/Services/ToolbarSwitcherAnchorTests.swift new file mode 100644 index 000000000..58ca686b4 --- /dev/null +++ b/TableProTests/Services/ToolbarSwitcherAnchorTests.swift @@ -0,0 +1,115 @@ +// +// ToolbarSwitcherAnchorTests.swift +// TableProTests +// + +import AppKit +import Testing + +@testable import TablePro + +/// The switcher anchors to a toolbar item when one is there and falls back to an unanchored panel +/// when it is not. Getting that decision wrong is not a layout glitch: `NSPopover.show(relativeTo:)` +/// throws `NSInvalidArgumentException` when it cannot locate the item, and Swift cannot catch it, +/// so this is the guard that keeps a missing anchor from being a crash. +@Suite("ToolbarSwitcherPresenter anchor resolution") +@MainActor +struct ToolbarSwitcherAnchorTests { + private static let identifier = NSToolbarItem.Identifier("com.TablePro.tests.anchor") + + private final class Delegate: NSObject, NSToolbarDelegate { + var identifiers: [NSToolbarItem.Identifier] + + init(identifiers: [NSToolbarItem.Identifier]) { + self.identifiers = identifiers + } + + func toolbar( + _ toolbar: NSToolbar, + itemForItemIdentifier itemIdentifier: NSToolbarItem.Identifier, + willBeInsertedIntoToolbar flag: Bool + ) -> NSToolbarItem? { + NSToolbarItem(itemIdentifier: itemIdentifier) + } + + func toolbarDefaultItemIdentifiers(_ toolbar: NSToolbar) -> [NSToolbarItem.Identifier] { + identifiers + } + + func toolbarAllowedItemIdentifiers(_ toolbar: NSToolbar) -> [NSToolbarItem.Identifier] { + identifiers + } + } + + /// Returned so the caller can hold it with `withExtendedLifetime`: `NSToolbar` keeps its + /// delegate weakly, and a deallocated one leaves a toolbar with no items, which would make every + /// case here "pass" for the wrong reason. + private func makeWindow(containing identifiers: [NSToolbarItem.Identifier]) -> (NSWindow, Delegate) { + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 800, height: 400), + styleMask: [.titled], + backing: .buffered, + defer: true + ) + let delegate = Delegate(identifiers: identifiers) + let toolbar = NSToolbar(identifier: "com.TablePro.tests.toolbar") + toolbar.delegate = delegate + window.toolbar = toolbar + /// Set rather than assumed: a window that is never ordered front does not report a visible + /// toolbar, so leaving this to the default made the anchored case look like the unanchored + /// one and the test passed for the wrong reason. + toolbar.isVisible = true + return (window, delegate) + } + + @Test("An item in a visible toolbar is the anchor") + func resolvesItemInVisibleToolbar() { + let (window, delegate) = makeWindow(containing: [Self.identifier]) + withExtendedLifetime(delegate) { + let item = ToolbarSwitcherPresenter.anchor(in: window, Self.identifier) + #expect(item?.itemIdentifier == Self.identifier) + } + } + + /// What Customize Toolbar leaves behind. A clipped item is a different state and keeps its + /// place in `toolbar.items`, so it still resolves and still takes the popover branch; that one + /// needs a real overflowing toolbar and so is not reachable from a unit test. + @Test("An item the toolbar does not carry has no anchor") + func missingItemHasNoAnchor() { + let (window, delegate) = makeWindow(containing: []) + withExtendedLifetime(delegate) { + #expect(ToolbarSwitcherPresenter.anchor(in: window, Self.identifier) == nil) + } + } + + /// View > Hide Toolbar only flips `isVisible` and leaves the items in place, so the item still + /// resolves. Anchoring to an item in a hidden toolbar is undocumented, and the cost of being + /// wrong is an uncatchable exception, so a hidden toolbar counts as no anchor. + @Test("A hidden toolbar has no anchor even though it still carries the item") + func hiddenToolbarHasNoAnchor() { + let (window, delegate) = makeWindow(containing: [Self.identifier]) + withExtendedLifetime(delegate) { + window.toolbar?.isVisible = false + + #expect(window.toolbar?.items.contains { $0.itemIdentifier == Self.identifier } == true) + #expect(ToolbarSwitcherPresenter.anchor(in: window, Self.identifier) == nil) + } + } + + @Test("A window with no toolbar has no anchor") + func windowWithoutToolbarHasNoAnchor() { + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 800, height: 400), + styleMask: [.titled], + backing: .buffered, + defer: true + ) + + #expect(ToolbarSwitcherPresenter.anchor(in: window, Self.identifier) == nil) + } + + @Test("No window has no anchor") + func noWindowHasNoAnchor() { + #expect(ToolbarSwitcherPresenter.anchor(in: nil, Self.identifier) == nil) + } +} diff --git a/TableProUITests/SwitcherWithoutToolbarAnchorUITests.swift b/TableProUITests/SwitcherWithoutToolbarAnchorUITests.swift new file mode 100644 index 000000000..959fcb2db --- /dev/null +++ b/TableProUITests/SwitcherWithoutToolbarAnchorUITests.swift @@ -0,0 +1,82 @@ +// +// SwitcherWithoutToolbarAnchorUITests.swift +// TableProUITests +// +// Covers the defect class where a command that has a menu item and a keyboard shortcut is the +// only thing that can present a chooser, but the chooser is declared inside a toolbar item's own +// view. AppKit clips a toolbar item into the overflow menu on a narrow window and lets the user +// delete it outright in Customize Toolbar, and in both states that view is not on screen, so the +// command set its flag and nothing appeared: no popover, no error, no feedback. +// + +import XCTest + +/// Hiding the toolbar is the deterministic way to reach the no-anchor state from a test. +/// +/// The state CI actually failed in is an overflowed item on a 1024pt screen, which cannot be +/// reproduced on a developer machine: `recomputeWindowMinSize()` puts the window's minimum width +/// near 1364pt with a table tab open, so a pinned frame is clamped back up and the toolbar never +/// runs out of room. Hiding the toolbar removes the anchor the same way, through a route any +/// machine can take, and it exercises the same branch. +final class SwitcherWithoutToolbarAnchorUITests: UITestCase { + func testSwitchConnectionOpensWithTheToolbarHidden() throws { + let app = try launchWithSampleDatabase() + try waitForGrid(in: app) + + let toolbar = app.windows.firstMatch.toolbars.firstMatch + /// Normalised rather than asserted. `MainWindowToolbar` sets `autosavesConfiguration`, and + /// AppKit persists toolbar visibility through its own defaults rather than the sandbox + /// `UITestCase` hands the app, so this suite inherits whatever the last run left behind, + /// including its own. An earlier version of this test hid the toolbar without restoring it + /// and every later run then failed on a precondition instead of on the behaviour. + setToolbar(shown: true, toolbar: toolbar, in: app) + setToolbar(shown: false, toolbar: toolbar, in: app) + /// Restored even when an assertion below fails. `MainWindowToolbar` sets + /// `autosavesConfiguration`, and AppKit persists that visibility through its own defaults + /// rather than the sandbox `UITestCase` hands the app, so leaving the toolbar hidden would + /// hide it for every later launch on the machine and fail suites that have nothing to do + /// with this one. + defer { setToolbar(shown: true, toolbar: toolbar, in: app) } + + app.typeKey("c", modifierFlags: [.command, .control]) + + XCTAssertTrue( + connectionSearchField(in: app).waitToExist(timeout: 15), + "Switch Connection must present with no toolbar item to anchor to" + ) + app.typeKey(.escape, modifierFlags: []) + } + + // MARK: - Helpers + + /// Driven by the key equivalent rather than by walking the View menu, which + /// `UITestCase.launchWithSampleDatabase` documents as the slow and flake-prone route. The menu + /// item is built with a fixed "Show Toolbar" title in both states, so its label is no signal. + private func setToolbar(shown: Bool, toolbar: XCUIElement, in app: XCUIApplication) { + _ = toolbar.waitToExist(timeout: 3) + guard toolbar.exists != shown else { return } + app.typeKey("t", modifierFlags: [.command, .option]) + XCTAssertTrue( + waitForPredicate(timeout: 10) { toolbar.exists == shown }, + "Command Option T must \(shown ? "show" : "hide") the toolbar" + ) + } + + private func waitForGrid(in app: XCUIApplication) throws { + XCTAssertTrue( + app.windows.firstMatch.tables.matching(identifier: "data-grid").firstMatch + .waitToExist(timeout: 30) + ) + app.activate() + } + + /// Keyed on the placeholder because every `NativeSearchField` publishes the same default + /// identifier, so an identifier match cannot tell the switcher surfaces apart. It is also what + /// makes this assertion independent of which surface presented: popover or panel, the field is + /// the same one. + private func connectionSearchField(in app: XCUIApplication) -> XCUIElement { + app.searchFields.matching( + NSPredicate(format: "placeholderValue BEGINSWITH[c] %@", "Search connections") + ).firstMatch + } +} diff --git a/docs/connections/index.mdx b/docs/connections/index.mdx index a803a5d07..0ec82b885 100644 --- a/docs/connections/index.mdx +++ b/docs/connections/index.mdx @@ -78,7 +78,7 @@ A connection's group, tags, and favorite star sync through iCloud unless it is m ## Switch connections and databases -**Switch Connection** (`Ctrl+Cmd+C`) opens a toolbar popover listing active sessions and saved connections: type to filter, arrow keys to move, Return to switch. **Open Database** (`Cmd+K`) moves to another database on the same server. +**Switch Connection** (`Ctrl+Cmd+C`) lists active sessions and saved connections: type to filter, arrow keys to move, Return to switch. **Open Database** (`Cmd+K`) moves to another database on the same server. Database switcher in toolbar