diff --git a/CHANGELOG.md b/CHANGELOG.md index e13c23046..5f1e162ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Rows saved, added or deleted on iPhone and iPad refused by a MySQL, MariaDB, PostgreSQL or Redshift server that starts sessions read-only. - Copying objects into a connection that already has a transaction open committing it. - Replace-copy into a remote libSQL target failing at `BEGIN`. +- `Cmd+W` closing the whole connection instead of the current tab until something in the window was clicked. ### Security diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController+Focus.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController+Focus.swift index 0d04c771e..845657b02 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController+Focus.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController+Focus.swift @@ -17,6 +17,19 @@ import TableProTextEngine /// a window hosting several connections has several editors registered at once, and a window-wide /// registry cannot tell them apart. internal extension MainSplitViewController { + /// What the window names as its `initialFirstResponder`: the container the selected tab's content + /// is shown in. + /// + /// AppKit picks a first responder once, as the window is first placed on screen, and only from + /// the views that exist at that moment. The editor, the grid and the object list are SwiftUI and + /// do not exist yet, so left to itself AppKit took the first key view it could find, which was + /// the connections strip, and Command W then closed the connection instead of the tab. The + /// container takes no focus itself, so the window keeps it, and the content adopts it once it is + /// built (`SQLEditorCoordinator`, `SidebarOutlineView`), whatever else the window holds by then. + var initialFirstResponderContainer: NSView { + detailPaneHost.view + } + @discardableResult func focusQueryEditor() -> Bool { guard let textView = mountedQueryEditor, let window = view.window else { return false } diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift index 3d50840e5..f2fb0bb14 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift @@ -85,7 +85,7 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan private var navigationSidebar: NavigationSidebarViewController! /// Stable containers, one per split item. The pane they show is the selected workspace's own, /// so switching connection is a view swap and every other connection's tree stays built. - private var detailPaneHost: WorkspacePaneHost! + internal private(set) var detailPaneHost: WorkspacePaneHost! private var inspectorPaneHost: WorkspacePaneHost! /// The editor tab strip's band. It is a titlebar accessory rather than a split item, so it is diff --git a/TablePro/Core/Services/Infrastructure/NavigationSidebarViewController.swift b/TablePro/Core/Services/Infrastructure/NavigationSidebarViewController.swift index cbb507949..1cbd02c24 100644 --- a/TablePro/Core/Services/Infrastructure/NavigationSidebarViewController.swift +++ b/TablePro/Core/Services/Infrastructure/NavigationSidebarViewController.swift @@ -71,6 +71,7 @@ internal final class NavigationSidebarViewController: NSViewController { ]) separator.isHidden = true + rail.isHidden = true } /// The width the sidebar needs on top of the object browser's own minimum. Read from the @@ -81,10 +82,24 @@ internal final class NavigationSidebarViewController: NSViewController { railWidthConstraint.constant + separatorWidthConstraint.constant } + /// A collapsed strip is hidden, not only zero points wide. AppKit counts a zero-width view as + /// visible, so the strip's list stayed a key view: it was what the window focused when first + /// shown, Tab could land on it, and its own Close then took the whole connection on a Command W + /// meant for a tab. Hiding it takes it out of the key view loop, the responder chain and the + /// accessibility tree. + /// + /// It is shown before it grows and hidden once it has shrunk, so the animation stays visible. + /// The keyboard moves on as the collapse starts, because a list that is about to vanish must + /// not keep answering keys for the whole of the animation. internal func setRailVisible(_ visible: Bool, animated: Bool, alongside: (() -> Void)? = nil) { guard isRailVisible != visible else { return } isRailVisible = visible separator.isHidden = !visible + if visible { + railController.view.isHidden = false + } else { + handKeyboardOnFromRail() + } applyRailWidth(animated: animated, alongside: alongside) } @@ -96,12 +111,14 @@ internal final class NavigationSidebarViewController: NSViewController { let separatorWidth: CGFloat = isRailVisible ? 1 : 0 guard railWidthConstraint.constant != width else { alongside?() + hideRailIfCollapsed() return } guard animated, view.window != nil else { railWidthConstraint.constant = width separatorWidthConstraint.constant = separatorWidth alongside?() + hideRailIfCollapsed() return } NSAnimationContext.runAnimationGroup { context in @@ -109,6 +126,29 @@ internal final class NavigationSidebarViewController: NSViewController { railWidthConstraint.animator().constant = width separatorWidthConstraint.animator().constant = separatorWidth alongside?() + } completionHandler: { [weak self] in + self?.hideRailIfCollapsed() } } + + /// Asked again when a collapse finishes, because the strip can have been shown again while it + /// was still shrinking. + private func hideRailIfCollapsed() { + guard !isRailVisible else { return } + railController.view.isHidden = true + } + + /// Moves the keyboard to the next key view, which is what AppKit does itself when a focused + /// view is hidden, only at the start of the collapse rather than the end. Leaving it with the + /// window instead would make every key beep until the next click. The window takes it only + /// when nothing else in the loop can. + private func handKeyboardOnFromRail() { + guard let window = view.window, + let responder = window.firstResponder as? NSView, + responder.isDescendant(of: railController.view) + else { return } + window.selectKeyView(following: responder) + guard let next = window.firstResponder as? NSView, next.isDescendant(of: railController.view) else { return } + window.makeFirstResponder(nil) + } } diff --git a/TablePro/Core/Services/Infrastructure/TabWindowController.swift b/TablePro/Core/Services/Infrastructure/TabWindowController.swift index 72efa8bef..090a8a1c3 100644 --- a/TablePro/Core/Services/Infrastructure/TabWindowController.swift +++ b/TablePro/Core/Services/Infrastructure/TabWindowController.swift @@ -21,6 +21,16 @@ private final class EditorWindow: NSWindow, NSDraggingDestination { super.performClose(sender) } + /// The window's first focus belongs to the tab it shows. AppKit reads `initialFirstResponder` + /// once, as the window is first placed on screen, so it is named as soon as the content that + /// owns the answer is installed. + override var contentViewController: NSViewController? { + didSet { + initialFirstResponder = (contentViewController as? MainSplitViewController)? + .initialFirstResponderContainer + } + } + /// Hiding the toolbar is what drops the content pane's top safe area, so the titlebar has to be /// reconsidered every time the user sends this from View > Show Toolbar. override func toggleToolbarShown(_ sender: Any?) { diff --git a/TableProTests/Core/Services/Infrastructure/ConnectionWindowInitialFocusTests.swift b/TableProTests/Core/Services/Infrastructure/ConnectionWindowInitialFocusTests.swift new file mode 100644 index 000000000..0bebfdfda --- /dev/null +++ b/TableProTests/Core/Services/Infrastructure/ConnectionWindowInitialFocusTests.swift @@ -0,0 +1,151 @@ +import AppKit +import Foundation +@testable import TablePro +import Testing + +/// Command W closed the whole connection instead of the current tab in a window nobody had clicked +/// into yet, whether it was restored at launch or opened fresh. +/// +/// AppKit gives a window its first responder once, as the window is first placed on screen, and +/// only from the views that exist at that moment. The editor, the grid and the object list are +/// SwiftUI and did not exist yet, so the pick fell to the connections strip's list, which was +/// collapsed to zero width but never hidden. The strip answers Close itself, so Command W took the +/// connection. +@Suite("Connection window initial focus", .serialized) +@MainActor +struct ConnectionWindowInitialFocusTests { + @Test("A connections strip that has never been shown is not a key view") + func unshownStripIsNotAKeyView() { + let host = SidebarHost() + defer { host.tearDown() } + + #expect(host.rail.isHidden) + #expect(host.rail.firstKeyViewDescendant == nil) + } + + @Test("A shown connections strip can take the keyboard") + func shownStripIsAKeyView() { + let host = SidebarHost() + defer { host.tearDown() } + + host.sidebar.setRailVisible(true, animated: false) + + #expect(!host.rail.isHidden) + #expect(host.rail.firstKeyViewDescendant != nil) + } + + @Test("A collapsed connections strip leaves the key view loop") + func collapsedStripLeavesTheKeyViewLoop() { + let host = SidebarHost() + defer { host.tearDown() } + + host.sidebar.setRailVisible(true, animated: false) + host.sidebar.setRailVisible(false, animated: false) + + #expect(host.rail.isHidden) + #expect(host.rail.firstKeyViewDescendant == nil) + } + + /// The strip collapses whenever the app-wide entry count drops to one, which closing another + /// connection is enough to do, so the list can be holding the keyboard when it goes. + @Test("Collapsing the connections strip lets go of the keyboard at once") + func collapsingStripLetsGoOfTheKeyboard() throws { + let host = SidebarHost() + defer { host.tearDown() } + + host.sidebar.setRailVisible(true, animated: false) + let list = try #require(host.rail.firstKeyViewDescendant) + #expect(host.window.makeFirstResponder(list)) + + host.sidebar.setRailVisible(false, animated: true) + + let responder = host.window.firstResponder as? NSView + #expect(responder?.isDescendant(of: host.rail) != true) + } + + /// Leaving the keyboard with the window itself would make every key beep until the next click, + /// so it goes where AppKit sends it when a focused view is hidden: the next key view. + @Test("Collapsing the connections strip hands the keyboard to the next key view") + func collapsingStripHandsTheKeyboardOn() throws { + let host = SidebarHost() + defer { host.tearDown() } + let field = NSTextField(frame: NSRect(x: 400, y: 200, width: 120, height: 22)) + host.sidebar.view.addSubview(field) + + host.sidebar.setRailVisible(true, animated: false) + let list = try #require(host.rail.firstKeyViewDescendant) + #expect(host.window.makeFirstResponder(list)) + + host.sidebar.setRailVisible(false, animated: true) + + #expect(host.window.firstResponder !== host.window) + let responder = host.window.firstResponder as? NSView + #expect(responder?.isDescendant(of: host.rail) == false) + } + + /// The strip on screen at first show is the case hiding it cannot reach: two restored + /// connections, or a connection opened while another is already open. + @Test("A connection window leaves its first focus to the tab content, with the strip on screen") + func firstFocusIsLeftForTheContent() throws { + let connection = TestFixtures.makeConnection(name: "Initial focus") + let workspace = ConnectionWorkspace( + connectionId: connection.id, + payload: nil, + autoConnect: false, + payloadConnection: connection, + session: nil, + sessionState: nil, + trailingPaneState: nil, + phase: .connecting + ) + let window = TabWindowController.makeEditorWindow() + window.isReleasedWhenClosed = false + let split = MainSplitViewController(payload: nil, sessionState: nil, adopting: workspace) + window.contentViewController = split + defer { + window.orderOut(nil) + window.contentViewController = nil + workspace.teardown() + } + + let previous = AppSettingsManager.shared.general.showWorkspaceRail + AppSettingsManager.shared.general.showWorkspaceRail = true + defer { AppSettingsManager.shared.general.showWorkspaceRail = previous } + split.applyRailVisibility(workspaceCount: 2) + try #require(split.isWorkspaceRailVisible, "The strip has to be on screen for this to test anything") + + window.orderFront(nil) + + #expect(window.initialFirstResponder === split.initialFirstResponderContainer) + #expect(window.firstResponder === window) + } + + @MainActor + private struct SidebarHost { + let sidebar: NavigationSidebarViewController + let window: NSWindow + + var rail: NSView { + sidebar.railController.view + } + + init() { + sidebar = NavigationSidebarViewController() + window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 600, height: 400), + styleMask: [.titled], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + window.keepsKeyViewLoopCurrent() + window.contentViewController = sidebar + window.orderFront(nil) + } + + func tearDown() { + window.orderOut(nil) + window.contentViewController = nil + } + } +} diff --git a/TableProUITests/CloseTabBeforeFirstClickUITests.swift b/TableProUITests/CloseTabBeforeFirstClickUITests.swift new file mode 100644 index 000000000..eb9e77d4f --- /dev/null +++ b/TableProUITests/CloseTabBeforeFirstClickUITests.swift @@ -0,0 +1,135 @@ +import SQLite3 +import XCTest + +/// Command W closed the whole connection instead of the current tab in a window nobody had clicked +/// into yet: after a relaunch that restored the session, and on a connection opened fresh. +/// +/// AppKit gave the window its first responder as it appeared, and the connections strip's list was +/// the only candidate, on screen or collapsed to zero width. The strip answers Close itself, so the +/// keystroke closed its entry. Clicking the editor or the grid moved the keyboard out of the strip, +/// which is why the bug only showed before the first click. +/// +/// Nothing is clicked in either test, on purpose: the first click is what hid the bug. +final class CloseTabBeforeFirstClickUITests: UITestCase { + func testCommandWBeforeAnyClickClosesTheTabAndKeepsTheConnection() throws { + let app = try launchWithSampleDatabase() + let window = app.windows["main"] + let grid = window.tables.matching(identifier: "data-grid").firstMatch + XCTAssertTrue(grid.waitToExist(timeout: 30), "The sample database opens on a table tab") + + app.typeKey("w", modifierFlags: .command) + + XCTAssertTrue( + waitForPredicate(timeout: 20) { !grid.exists }, + "Command W must close the table tab" + ) + XCTAssertTrue( + window.outlines.firstMatch.waitToExist(timeout: 5), + "The connection must stay open on its empty state, not close with the tab" + ) + } + + /// Two restored connections, so the strip is on screen by the time Command W is pressed, which + /// is where anyone who reopens more than one connection lands. + func testCommandWInARestoredSessionClosesATabAndKeepsBothConnections() throws { + try seedSession(connectionNames: ["Restored A", "Restored B"], tabsEach: 3) + let app = try launchApp() + let window = app.windows["main"] + let strip = window.tables.matching(identifier: "workspace-rail").firstMatch + + XCTAssertTrue( + waitForPredicate(timeout: 60) { strip.exists && strip.tableRows.count == 2 }, + "Both restored connections must be listed in the strip" + ) + XCTAssertTrue( + waitForPredicate(timeout: 30) { self.tabCount(in: window) == 3 }, + "The selected connection must restore its three tabs" + ) + + app.typeKey("w", modifierFlags: .command) + + XCTAssertTrue( + waitForPredicate(timeout: 20) { self.tabCount(in: window) == 2 }, + "Command W must close one tab. Tabs now: \(tabCount(in: window))" + ) + XCTAssertEqual(strip.tableRows.count, 2, "Neither connection may close with the tab") + } + + private func tabCount(in window: XCUIElement) -> Int { + window.descendants(matching: .any).matching(identifier: "editor-tab").count + } + + // MARK: - Fixture + + /// The files the app reads to reopen the last session: the connections, which of them were + /// open, and each one's tabs. + private func seedSession(connectionNames: [String], tabsEach: Int) throws { + let root = try XCTUnwrap(sandboxRoot, "setUpWithError did not prepare a sandbox") + let supportDirectory = root.appendingPathComponent("TablePro", isDirectory: true) + let tabStateDirectory = supportDirectory.appendingPathComponent("TabState", isDirectory: true) + try FileManager.default.createDirectory(at: tabStateDirectory, withIntermediateDirectories: true) + + var connections: [[String: Any]] = [] + var connectionIds: [String] = [] + for (index, name) in connectionNames.enumerated() { + let id = UUID().uuidString + let databaseURL = root.appendingPathComponent("restored-\(index).sqlite") + makeDatabase(at: databaseURL) + connections.append(connectionPayload(id: id, name: name, databasePath: databaseURL.path, sortOrder: index)) + connectionIds.append(id) + try writeJSON( + tabState(tabCount: tabsEach), + to: tabStateDirectory.appendingPathComponent("\(id).json") + ) + } + try writeJSON(connections, to: supportDirectory.appendingPathComponent("connections.json")) + try writeJSON(connectionIds, to: supportDirectory.appendingPathComponent("LastOpenConnections.json")) + } + + private func connectionPayload(id: String, name: String, databasePath: String, sortOrder: Int) -> [String: Any] { + [ + "id": id, + "name": name, + "host": "", + "port": 0, + "database": databasePath, + "username": "", + "type": "SQLite", + "sshEnabled": false, + "sshHost": "", + "sshUsername": "", + "sshAuthMethod": "password", + "sshPrivateKeyPath": "", + "sortOrder": sortOrder, + ] + } + + /// Query tabs, so each one mounts the SQL editor and nothing depends on a table's rows loading. + private func tabState(tabCount: Int) -> [String: Any] { + let tabs: [[String: Any]] = (1 ... tabCount).map { number in + [ + "id": UUID().uuidString, + "title": "Query \(number)", + "query": "SELECT \(number);", + "tabType": ["query": [String: Any]()], + "tableName": NSNull(), + "databaseName": "", + "isView": false, + ] + } + return ["tabs": tabs, "selectedTabId": tabs[0]["id"] ?? ""] + } + + private func writeJSON(_ object: Any, to url: URL) throws { + try JSONSerialization.data(withJSONObject: object, options: [.sortedKeys]) + .write(to: url, options: .atomic) + } + + private func makeDatabase(at url: URL) { + var handle: OpaquePointer? + defer { sqlite3_close(handle) } + XCTAssertEqual(sqlite3_open(url.path, &handle), SQLITE_OK, "Could not create \(url.path)") + let statement = "CREATE TABLE items (id INTEGER PRIMARY KEY, name TEXT); INSERT INTO items (name) VALUES ('one');" + XCTAssertEqual(sqlite3_exec(handle, statement, nil, nil, nil), SQLITE_OK) + } +}