diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b7af703d..6aa110808 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Table collation on MySQL, previously never read. - PluginKit ABI 20. Every registry plugin needs rebuilding before or with this release. - Cold launch to a usable window, 470ms down to 260ms. +- Half-second grace before any connecting, schema or query progress indicator appears, and a minimum time on screen once one does. +- Window chrome stays put through a connect that finishes inside that grace, instead of collapsing and reopening. - Plugin signature checks run after the first window rather than on the launch thread, at 13ms each. - One gate in front of every path that loads a plugin's executable, enabling one included. - Stale `cloudflared` and `cloud-sql-proxy` cleanup waits for the process to exit before a connection reuses its port. diff --git a/TablePro/Core/Compare/CompareSyncProfileStorage.swift b/TablePro/Core/Compare/CompareSyncProfileStorage.swift index 12ab3219f..01668907a 100644 --- a/TablePro/Core/Compare/CompareSyncProfileStorage.swift +++ b/TablePro/Core/Compare/CompareSyncProfileStorage.swift @@ -46,7 +46,6 @@ internal struct CompareSyncProfile: Codable, Hashable, Identifiable { self.dataOptions = dataOptions self.selectedObjects = selectedObjects } - } extension DatabaseScope: Codable { diff --git a/TablePro/Core/SSH/SSHPublicKeyFile.swift b/TablePro/Core/SSH/SSHPublicKeyFile.swift index 89618df91..9fbbc8e02 100644 --- a/TablePro/Core/SSH/SSHPublicKeyFile.swift +++ b/TablePro/Core/SSH/SSHPublicKeyFile.swift @@ -29,7 +29,7 @@ internal enum SSHPublicKeyFile { /// A `.pub` line is a few hundred bytes. The cap is here because the same path may name a /// private key, a directory entry, or something else entirely. - private static let maximumFileSize = 64 * 1024 + private static let maximumFileSize = 64 * 1_024 /// Every public key an identity file resolves to, in the order OpenSSH looks for them. static func blobs(atIdentityPath path: String) -> [SSHPublicKeyBlob] { diff --git a/TablePro/Core/Services/Infrastructure/ConnectionWindowPaneResolver.swift b/TablePro/Core/Services/Infrastructure/ConnectionWindowPaneResolver.swift index 5199052a1..a00b03c7b 100644 --- a/TablePro/Core/Services/Infrastructure/ConnectionWindowPaneResolver.swift +++ b/TablePro/Core/Services/Infrastructure/ConnectionWindowPaneResolver.swift @@ -6,6 +6,11 @@ import Foundation internal enum ConnectionWindowPane: Equatable { + /// A connect too young to be worth saying anything about. It draws nothing and, unlike every + /// other contentless pane, it leaves the window's chrome alone: a local file opens in about + /// 40ms, and collapsing the sidebar and inspector for that long only to put them back is a + /// layout cycle nobody asked for and a flash the HIG names outright. + case preparing case connecting case unavailable(ConnectionUnavailableReason) case content @@ -23,10 +28,22 @@ internal enum SidebarChromeMode: Equatable { } internal enum ConnectionWindowPaneResolver { + /// `hasOutlastedGrace` is false for the first `LoadingRevealPolicy.grace` of a connect and of + /// the moment before one starts. Neither is a state worth reporting: the first has not lasted + /// long enough to be worth a word, and the second is not "not connected", it is "about to + /// dial", a distinction `.idle` alone cannot draw because it answers for both. Measured on the + /// SQLite sample, reporting them built three pane hierarchies and ran a whole chrome collapse + /// and reveal inside the first 103ms of a window's life, for a 39ms connect. + /// + /// The grace expiring is the exit from `.preparing` in both directions, which is why `.idle` + /// reads it too. A connect that never starts, because the phase disallowed it or the record + /// went missing, would otherwise leave the window silently empty for good. internal static func pane( phase: ConnectionWindowPhase, hasConnection: Bool, - hasRenderableSession: Bool + hasRenderableSession: Bool, + awaitsAutoConnect: Bool = false, + hasOutlastedGrace: Bool = true ) -> ConnectionWindowPane { switch phase { case .closing: @@ -35,19 +52,45 @@ internal enum ConnectionWindowPaneResolver { return hasRenderableSession ? .content : .empty case .idle: if hasRenderableSession { return .content } - return hasConnection ? .unavailable(.notConnected) : .empty + guard hasConnection else { return .empty } + guard awaitsAutoConnect, !hasOutlastedGrace else { return .unavailable(.notConnected) } + return .preparing case .connecting: - return hasConnection ? .connecting : .empty + guard hasConnection else { return .empty } + return hasOutlastedGrace ? .connecting : .preparing case .unavailable(let reason): return hasConnection ? .unavailable(reason) : .empty } } + /// Whether this phase is one the grace timer runs over, so a caller knows when to arm it and + /// when to let it go. It is the exact set of phases `pane` answers differently for depending + /// on `showsProgress`, plus the pre-dial `.idle` that resolves to `.preparing` on its own. + internal static func awaitsProgressGrace( + phase: ConnectionWindowPhase, + awaitsAutoConnect: Bool + ) -> Bool { + switch phase { + case .connecting: + return true + case .idle: + return awaitsAutoConnect + case .connected, .closing, .unavailable: + return false + } + } + /// An object browser and an inspector with nothing to put in them are not chrome, they are two /// empty columns that promise a session the window does not have yet. + /// + /// That argument holds for a wait the user can see and not for one they cannot. `.preparing` + /// is the sub-grace case and keeps the chrome, so the window that opens is the window that + /// stays: on the happy path nothing collapses, nothing is put back, and the panes are built + /// once. Collapsing for 40ms costs `splitView.autosaveName`, both split items and a + /// `recalculateKeyViewLoop()` in each direction, all of it to show an empty column briefly. internal static func hidesChrome(for pane: ConnectionWindowPane) -> Bool { switch pane { - case .content: + case .content, .preparing: return false case .connecting, .unavailable, .empty: return true diff --git a/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift b/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift index 1d9de66b7..32192de90 100644 --- a/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift +++ b/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift @@ -205,10 +205,45 @@ internal final class ConnectionWorkspace { ConnectionWindowPaneResolver.pane( phase: phase, hasConnection: connection != nil, - hasRenderableSession: session != nil && rightPanelState != nil && sessionState != nil + hasRenderableSession: session != nil && rightPanelState != nil && sessionState != nil, + awaitsAutoConnect: autoConnect, + hasOutlastedGrace: hasOutlastedConnectGrace ) } + /// Whether this connection's dialling has lasted long enough to be worth reporting. + /// + /// It belongs to the workspace and not to the window, for the same reason `attemptToken` does: + /// a window hosts several connections and each dials on its own clock, so a window-wide flag + /// would let one connection's slow server put a progress screen over another's finished one. + internal private(set) var hasOutlastedConnectGrace = false + + @ObservationIgnored private var progressGraceTask: Task? + + /// Starts, or leaves running, the wait that decides whether this connect ever says so. + /// + /// `onReveal` is how the timer reaches the renderer, because the workspace owns the state and + /// the controller owns the panes. Re-arming while a wait is already running is a no-op, so the + /// phase churn of a reconnect cannot keep pushing the reveal further out. + internal func armConnectingProgressGrace(onReveal: @escaping @MainActor () -> Void) { + guard !hasOutlastedConnectGrace, progressGraceTask == nil else { return } + progressGraceTask = Task { @MainActor [weak self] in + try? await Task.sleep(for: LoadingRevealPolicy.grace) + guard !Task.isCancelled, let self, !self.isReleased else { return } + self.progressGraceTask = nil + self.hasOutlastedConnectGrace = true + onReveal() + } + } + + /// Ends the wait and takes the reveal with it, so the next connect starts its own grace rather + /// than inheriting a flag the last one set. + internal func cancelConnectingProgressGrace() { + progressGraceTask?.cancel() + progressGraceTask = nil + hasOutlastedConnectGrace = false + } + /// Everything the panes are built from, compared against `panes.renderedKey` to decide whether /// they have to be built at all. internal var paneRenderKey: WorkspacePaneRenderKey { @@ -253,6 +288,7 @@ internal final class ConnectionWorkspace { /// coordinator this tears down, and a coordinator only leaves the app-wide registry on deinit. internal func teardown() { isReleased = true + cancelConnectingProgressGrace() browseCancellable = nil statusCancellable = nil tabsCancellable = nil diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift index 38aeb35e2..fbe2ea931 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift @@ -176,6 +176,12 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi /// even though nothing about the connection has changed. workspace.panes.invalidate() workspaces.insert(workspace) + /// The pending grace belongs to the controller it is leaving for the same reason the + /// panes do: its reveal calls back into that one. Dropping it and arming again is what + /// re-points it here, and re-arming alone would not, because a wait already running is + /// deliberately left alone. + workspace.cancelConnectingProgressGrace() + syncConnectingProgressGrace(of: workspace) } else { adoptWorkspace(payload: payload, autoConnect: autoConnect) } @@ -246,6 +252,11 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi phase: phase ) let adopted = workspaces.insert(workspace) + /// Armed at creation and not only at the first phase change, because a workspace waiting to + /// dial is already resolving to `.preparing` and nothing else would ever time it out. That + /// is the exit `startActivationConnectIfNeeded` cannot promise: it returns without dialling + /// when the phase disallows it or the connection record has gone. + syncConnectingProgressGrace(of: adopted) /// A workspace adopted into a window that is already on screen has to dial for itself. /// `viewWillAppear` is what starts the connect for the window's first workspace, and it @@ -635,6 +646,7 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi guard let workspace = workspaces.workspace(for: connectionId) else { return } let phaseChanged = workspace.phase != next workspace.phase = next + syncConnectingProgressGrace(of: workspace) syncPanes(of: workspace) guard phaseChanged else { return } if workspaces.selectedConnectionId == connectionId { @@ -703,6 +715,29 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi refreshPanes(of: workspace) } + /// Runs the wait that decides whether a connect ever announces itself, for the workspace it + /// names, selected or not. A background connection dials on its own clock. + /// + /// The reveal repaints through `syncPanes` rather than `refreshPanes`, so it costs nothing on + /// the ordinary path where the connect landed first and the flag never flipped: the render key + /// carries the flag, so a repaint is only ever done when the pane it names actually moved. + private func syncConnectingProgressGrace(of workspace: ConnectionWorkspace) { + guard ConnectionWindowPaneResolver.awaitsProgressGrace( + phase: workspace.phase, + awaitsAutoConnect: workspace.autoConnect + ) else { + workspace.cancelConnectingProgressGrace() + return + } + workspace.armConnectingProgressGrace { [weak self, weak workspace] in + guard let self, let workspace, self.isViewLoaded else { return } + self.syncPanes(of: workspace) + guard self.isShowing(workspace) else { return } + self.applyPaneChrome() + self.applyWindowTitle() + } + } + private func syncSelectedPanes() { guard let selected = workspaces.selected else { return } syncPanes(of: selected) diff --git a/TablePro/Models/Sidebar/SidebarObjectListPresentation.swift b/TablePro/Models/Sidebar/SidebarObjectListPresentation.swift index 1ee5366c0..35976023f 100644 --- a/TablePro/Models/Sidebar/SidebarObjectListPresentation.swift +++ b/TablePro/Models/Sidebar/SidebarObjectListPresentation.swift @@ -12,6 +12,10 @@ import Foundation /// database that genuinely has no objects, and telling the two apart is the whole reason /// this lives outside the view. internal enum SidebarObjectListPresentation: Equatable { + /// Loading, and not yet for long enough to say so. An empty column is the placeholder the HIG + /// asks for, and a local database answers in about 110ms, so a spinner there is a flash rather + /// than a report. + case preparing case loading case failed(String) case noMatch @@ -23,11 +27,12 @@ internal enum SidebarObjectListPresentation: Equatable { hasActiveFilter: Bool, hasAnyMatch: Bool, hasRoutines: Bool, - hasTriggers: Bool + hasTriggers: Bool, + hasOutlastedGrace: Bool = true ) -> SidebarObjectListPresentation { switch state { case .idle, .loading: - return .loading + return hasOutlastedGrace ? .loading : .preparing case .failed(let message): return .failed(message) case .loaded(let tables): diff --git a/TablePro/Models/UI/LoadingRevealPolicy.swift b/TablePro/Models/UI/LoadingRevealPolicy.swift new file mode 100644 index 000000000..f2bb9bd21 --- /dev/null +++ b/TablePro/Models/UI/LoadingRevealPolicy.swift @@ -0,0 +1,40 @@ +// +// LoadingRevealPolicy.swift +// TablePro +// + +import Foundation + +/// When progress UI has earned its place on screen. +/// +/// Two rules, and the second is the one that gets left out. Work that finishes inside `grace` +/// shows nothing at all, because an indicator the user cannot read costs a view hierarchy to +/// build and tear down and tells them nothing. Work that outlasts it keeps its indicator for +/// `minimumDwell`, because an indicator revealed at 500ms over work that ends at 510ms is a 10ms +/// flash, which is worse than either of the states it sits between. +/// +/// The HIG carries the same rule from both ends: progress indicators are for "situations where +/// loading takes more than a moment or two", and a first screen that differs from what replaces it +/// gives "an unpleasant flash between the launch screen and your first screen". +internal enum LoadingRevealPolicy { + /// Long enough that nothing local ever reaches it, short enough to stay under the one second + /// at which a wait stops feeling like part of the same gesture. It is the value + /// `DelayedProgressIndicator` already used for schema refreshes. + internal static let grace: Duration = .milliseconds(500) + + internal static let minimumDwell: Duration = .milliseconds(500) + + /// How much longer an indicator revealed at `revealedAt` has to stay before it may go. + /// + /// Measured from the reveal rather than from the moment the work ended, so anything slow + /// enough to have shown an indicator at all has usually already served its dwell and hides + /// the instant it finishes. Only the narrow band just past the grace waits. + internal static func remainingDwell( + revealedAt: ContinuousClock.Instant, + now: ContinuousClock.Instant + ) -> Duration { + let shown = revealedAt.duration(to: now) + guard shown < minimumDwell else { return .zero } + return minimumDwell - shown + } +} diff --git a/TablePro/Views/Components/DelayedProgressIndicator.swift b/TablePro/Views/Components/DelayedProgressIndicator.swift index 583d470b4..1cee86b80 100644 --- a/TablePro/Views/Components/DelayedProgressIndicator.swift +++ b/TablePro/Views/Components/DelayedProgressIndicator.swift @@ -5,31 +5,19 @@ import SwiftUI -/// A spinner that only appears once an operation outlasts `delay`. AppKit and SwiftUI -/// ship no delayed progress indicator, so short refreshes would otherwise flash a -/// spinner that resolves before the user can read it. +/// A small spinner that only appears once an operation outlasts `LoadingRevealPolicy.grace`, and +/// then stays long enough to be read. AppKit and SwiftUI ship no delayed progress indicator. +/// +/// It used to carry the grace and not the dwell, which left it able to flash: work that ended +/// just past the grace showed a spinner for the few milliseconds between the two. struct DelayedProgressIndicator: View { let isActive: Bool - var delay: Duration = .milliseconds(500) - - @State private var isVisible = false var body: some View { - Group { - if isVisible { - ProgressView() - .progressViewStyle(.circular) - .controlSize(.small) - } - } - .task(id: isActive) { - guard isActive else { - isVisible = false - return - } - try? await Task.sleep(for: delay) - guard !Task.isCancelled else { return } - isVisible = true + LoadingReveal(isActive: isActive) { + ProgressView() + .progressViewStyle(.circular) + .controlSize(.small) } } } diff --git a/TablePro/Views/Components/LoadingReveal.swift b/TablePro/Views/Components/LoadingReveal.swift new file mode 100644 index 000000000..49c7c0df4 --- /dev/null +++ b/TablePro/Views/Components/LoadingReveal.swift @@ -0,0 +1,83 @@ +// +// LoadingReveal.swift +// TablePro +// + +import SwiftUI + +/// Drives one Bool from `LoadingRevealPolicy`: false until the work behind it outlasts the grace, +/// then true for at least the minimum dwell. +/// +/// A modifier rather than a wrapper view, because the answer often decides which branch a view +/// renders rather than whether one spinner sits inside it. `ExecutionIndicatorView` is the case +/// that needs it: holding its content back would leave an empty toolbar item, where holding the +/// state back leaves the previous readout standing and the row never changes width. +private struct LoadingRevealGate: ViewModifier { + let isActive: Bool + @Binding var isRevealed: Bool + + @State private var revealedAt: ContinuousClock.Instant? + + func body(content: Content) -> some View { + content.task(id: isActive) { await track() } + } + + private func track() async { + guard isActive else { + await hideAfterDwell() + return + } + guard revealedAt == nil else { return } + try? await Task.sleep(for: LoadingRevealPolicy.grace) + guard !Task.isCancelled else { return } + revealedAt = .now + isRevealed = true + } + + private func hideAfterDwell() async { + guard let revealedAt else { + isRevealed = false + return + } + let remaining = LoadingRevealPolicy.remainingDwell(revealedAt: revealedAt, now: .now) + if remaining > .zero { + try? await Task.sleep(for: remaining) + guard !Task.isCancelled else { return } + } + self.revealedAt = nil + isRevealed = false + } +} + +internal extension View { + /// Reports through `isRevealed` whether progress UI for `isActive` has earned its place yet. + func loadingRevealGate(isActive: Bool, isRevealed: Binding) -> some View { + modifier(LoadingRevealGate(isActive: isActive, isRevealed: isRevealed)) + } +} + +/// Holds progress UI back until the work behind it outlasts `LoadingRevealPolicy.grace`. +/// +/// The content is not built while it is held back, which is the point: a spinner that exists for +/// 40ms still costs a view hierarchy to reconcile in and out, and on the startup path that is work +/// paid for something nobody sees. +/// +/// The dwell only bites where the alternative to the indicator is a view that was already there, +/// as it is in the toolbar, where hiding the executing cluster puts the previous duration back. +/// Where the alternative is the real content, the content wins: holding a loaded table list behind +/// half a second of spinner would be worse than the flash the dwell exists to prevent. +internal struct LoadingReveal: View { + internal let isActive: Bool + @ViewBuilder internal let content: () -> Content + + @State private var isRevealed = false + + internal var body: some View { + Group { + if isRevealed { + content() + } + } + .loadingRevealGate(isActive: isActive, isRevealed: $isRevealed) + } +} diff --git a/TablePro/Views/Results/ResultStatusReadoutView.swift b/TablePro/Views/Results/ResultStatusReadoutView.swift index 3bd9cc9aa..003b604ca 100644 --- a/TablePro/Views/Results/ResultStatusReadoutView.swift +++ b/TablePro/Views/Results/ResultStatusReadoutView.swift @@ -30,9 +30,15 @@ struct ResultStatusReadoutView: View { /// Unlabelled, per the HIG: "Avoid labeling a spinning progress indicator." It stands in /// for the sentence rather than sitting beside one, so the readout has a single owner in /// every state and nothing is added to the row. - ProgressView() - .controlSize(.small) - .accessibilityLabel(String(localized: "Loading…")) + /// + /// Held back for the same reason it is unlabelled. A table on a local database answers + /// in single-digit milliseconds, and a readout that spins for that long says less than + /// the blank it replaces. + LoadingReveal(isActive: true) { + ProgressView() + .controlSize(.small) + .accessibilityLabel(String(localized: "Loading…")) + } case .noRows: Text("No rows") case let .rowCount(count): diff --git a/TablePro/Views/Sidebar/DatabaseTreeView.swift b/TablePro/Views/Sidebar/DatabaseTreeView.swift index d7e108b28..4da8634b0 100644 --- a/TablePro/Views/Sidebar/DatabaseTreeView.swift +++ b/TablePro/Views/Sidebar/DatabaseTreeView.swift @@ -47,6 +47,7 @@ struct DatabaseTreeView: View { let sidebarState: SharedSidebarState @State private var settingsManager = AppSettingsManager.shared + @State private var showsDatabaseProgress = false private var activeDatabase: String? { let name = coordinator?.toolbarState.currentDatabase ?? "" @@ -78,6 +79,15 @@ struct DatabaseTreeView: View { && filteredDatabases.isEmpty } + private var isLoadingDatabases: Bool { + switch treeService.databaseListState(for: connectionId) { + case .idle, .loading: + return true + case .loaded, .failed: + return false + } + } + var body: some View { Group { switch treeService.databaseListState(for: connectionId) { @@ -93,9 +103,14 @@ struct DatabaseTreeView: View { outline } case .idle, .loading: - loadingState + if showsDatabaseProgress { + loadingState + } else { + Color.clear + } } } + .loadingRevealGate(isActive: isLoadingDatabases, isRevealed: $showsDatabaseProgress) .task(id: isConnected) { await treeService.loadDatabases(connectionId: connectionId, databaseType: databaseType) } diff --git a/TablePro/Views/Sidebar/SidebarView.swift b/TablePro/Views/Sidebar/SidebarView.swift index 5c1a29338..5f8aa872f 100644 --- a/TablePro/Views/Sidebar/SidebarView.swift +++ b/TablePro/Views/Sidebar/SidebarView.swift @@ -11,6 +11,7 @@ import TableProPluginKit struct SidebarView: View { @State private var viewModel: SidebarViewModel @State private var settingsManager = AppSettingsManager.shared + @State private var showsSchemaProgress = false private var schemaService: SchemaService { SchemaService.shared } @@ -235,26 +236,43 @@ struct SidebarView: View { } } - @ViewBuilder - private var flatContent: some View { - switch SidebarObjectListPresentation.resolve( + private var objectListPresentation: SidebarObjectListPresentation { + SidebarObjectListPresentation.resolve( state: schemaService.state(for: connectionId), hasActiveFilter: !viewModel.filterQuery.isEmpty, hasAnyMatch: hasAnyMatch, hasRoutines: !routines.isEmpty, - hasTriggers: !triggers.isEmpty - ) { - case .loading: - loadingState - case .failed(let message): - errorState(message: message) - case .noMatch: - noMatchState - case .empty: - emptyState - case .list: - tableList + hasTriggers: !triggers.isEmpty, + hasOutlastedGrace: showsSchemaProgress + ) + } + + /// Asked above the switch rather than inside its loading branch, so which of the two the + /// column renders stays a decision of the pure resolver that already owns every other one, + /// and is tested there rather than buried in a view. + @ViewBuilder + private var flatContent: some View { + let presentation = objectListPresentation + Group { + switch presentation { + case .preparing: + Color.clear + case .loading: + loadingState + case .failed(let message): + errorState(message: message) + case .noMatch: + noMatchState + case .empty: + emptyState + case .list: + tableList + } } + .loadingRevealGate( + isActive: presentation == .preparing || presentation == .loading, + isRevealed: $showsSchemaProgress + ) } private var loadingState: some View { diff --git a/TablePro/Views/Toolbar/ExecutionIndicatorView.swift b/TablePro/Views/Toolbar/ExecutionIndicatorView.swift index 5841b9028..2f0035f38 100644 --- a/TablePro/Views/Toolbar/ExecutionIndicatorView.swift +++ b/TablePro/Views/Toolbar/ExecutionIndicatorView.swift @@ -16,9 +16,19 @@ struct ExecutionIndicatorView: View { let lastClickHouseProgress: ClickHouseQueryProgress? var onCancel: (() -> Void)? + /// Held back rather than the spinner inside it, so a query too fast to report leaves the + /// previous duration standing instead of emptying the item and changing the toolbar's width + /// twice. Clicking a table on a local database runs in single-digit milliseconds, and + /// "Executing…" appearing and going in that time is churn the user reads as a flicker. + /// + /// The Stop button goes with it. Nothing needs cancelling inside the grace, and past it the + /// button is there, which is what the HIG asks: "When it's feasible, let people halt + /// processing." + @State private var showsExecution = false + var body: some View { HStack(spacing: 4) { - if isExecuting { + if showsExecution { ProgressView() .controlSize(.small) .accessibilityLabel(String(localized: "Query executing")) @@ -64,6 +74,7 @@ struct ExecutionIndicatorView: View { .help(String(localized: "Run a query to see execution time")) } } + .loadingRevealGate(isActive: isExecuting, isRevealed: $showsExecution) } // MARK: - Helpers diff --git a/TableProTests/Core/Services/Infrastructure/ConnectionWindowPaneResolverTests.swift b/TableProTests/Core/Services/Infrastructure/ConnectionWindowPaneResolverTests.swift index 759ae9897..60a795333 100644 --- a/TableProTests/Core/Services/Infrastructure/ConnectionWindowPaneResolverTests.swift +++ b/TableProTests/Core/Services/Infrastructure/ConnectionWindowPaneResolverTests.swift @@ -25,7 +25,7 @@ struct ConnectionWindowPaneResolverTests { ] private static let everyPane: [ConnectionWindowPane] = - [.content, .connecting, .empty] + everyUnavailableReason.map { .unavailable($0) } + [.content, .preparing, .connecting, .empty] + everyUnavailableReason.map { .unavailable($0) } @Test("A failed connection shows its own pane, never a spinner and never a blank window") func failureResolvesToUnavailablePane() { @@ -40,9 +40,13 @@ struct ConnectionWindowPaneResolverTests { #expect(pane != .empty) } - @Test("Only a window with content earns a sidebar and an inspector") - func chromeHiddenForEveryNonContentPane() { + @Test("A wait the user can see costs the chrome; one they cannot does not") + func chromeHiddenForEveryNonContentPaneExceptPreparing() { #expect(!ConnectionWindowPaneResolver.hidesChrome(for: .content)) + #expect( + !ConnectionWindowPaneResolver.hidesChrome(for: .preparing), + "collapsing for a 40ms connect only to put it back is the flash the pane exists to stop" + ) #expect(ConnectionWindowPaneResolver.hidesChrome(for: .connecting)) #expect(ConnectionWindowPaneResolver.hidesChrome(for: .empty)) @@ -169,6 +173,142 @@ struct ConnectionWindowPaneResolverTests { #expect(!SidebarChromeMode.hidden.showsObjectBrowser) } + // MARK: - The grace + + @Test("A connect too young to report shows nothing rather than a card") + func connectingInsideTheGraceIsPreparing() { + let pane = ConnectionWindowPaneResolver.pane( + phase: .connecting, + hasConnection: true, + hasRenderableSession: false, + hasOutlastedGrace: false + ) + + #expect(pane == .preparing) + } + + @Test("A connect that outlasts the grace gets its card back, Cancel and all") + func connectingPastTheGraceIsUnchanged() { + let pane = ConnectionWindowPaneResolver.pane( + phase: .connecting, + hasConnection: true, + hasRenderableSession: false, + hasOutlastedGrace: true + ) + + #expect(pane == .connecting) + } + + /// `.idle` answers for a window that has finished dialling and for one that has not begun, and + /// only the caller knows which. Reporting the second as "not connected" is what put a pane on + /// screen for 38ms before the connecting one it was replaced by. + @Test("A window about to dial is not a window that failed to") + func idleAwaitingAutoConnectIsPreparing() { + let pane = ConnectionWindowPaneResolver.pane( + phase: .idle, + hasConnection: true, + hasRenderableSession: false, + awaitsAutoConnect: true, + hasOutlastedGrace: false + ) + + #expect(pane == .preparing) + } + + /// The exit from `.preparing`. `startActivationConnectIfNeeded` returns without dialling when + /// the phase disallows it or the connection record has gone, and without this the window would + /// sit blank with no route out. + @Test("A dial that never starts falls back to the not-connected pane once the grace expires") + func idleThatNeverDialledResolvesOnceTheGraceExpires() { + let pane = ConnectionWindowPaneResolver.pane( + phase: .idle, + hasConnection: true, + hasRenderableSession: false, + awaitsAutoConnect: true, + hasOutlastedGrace: true + ) + + #expect(pane == .unavailable(.notConnected)) + } + + @Test("A window the user has to connect by hand says so at once") + func idleWithoutAutoConnectNeverPrepares() { + for hasOutlastedGrace in [false, true] { + let pane = ConnectionWindowPaneResolver.pane( + phase: .idle, + hasConnection: true, + hasRenderableSession: false, + awaitsAutoConnect: false, + hasOutlastedGrace: hasOutlastedGrace + ) + + #expect(pane == .unavailable(.notConnected)) + } + } + + /// The grace may not delay a failure. A server that refuses in 20ms is an answer, not a wait. + @Test("The grace never holds back a settled outcome") + func settledPhasesIgnoreTheGrace() { + for hasOutlastedGrace in [false, true] { + #expect(ConnectionWindowPaneResolver.pane( + phase: .unavailable(.failed(Self.failure)), + hasConnection: true, + hasRenderableSession: false, + awaitsAutoConnect: true, + hasOutlastedGrace: hasOutlastedGrace + ) == .unavailable(.failed(Self.failure))) + + #expect(ConnectionWindowPaneResolver.pane( + phase: .connected, + hasConnection: true, + hasRenderableSession: true, + awaitsAutoConnect: true, + hasOutlastedGrace: hasOutlastedGrace + ) == .content) + + #expect(ConnectionWindowPaneResolver.pane( + phase: .closing, + hasConnection: true, + hasRenderableSession: true, + awaitsAutoConnect: true, + hasOutlastedGrace: hasOutlastedGrace + ) == .empty) + } + } + + @Test("The timer runs over exactly the phases the grace can answer differently") + func graceIsArmedForDiallingPhasesAlone() { + #expect(ConnectionWindowPaneResolver.awaitsProgressGrace(phase: .connecting, awaitsAutoConnect: false)) + #expect(ConnectionWindowPaneResolver.awaitsProgressGrace(phase: .idle, awaitsAutoConnect: true)) + #expect(!ConnectionWindowPaneResolver.awaitsProgressGrace(phase: .idle, awaitsAutoConnect: false)) + #expect(!ConnectionWindowPaneResolver.awaitsProgressGrace(phase: .connected, awaitsAutoConnect: true)) + #expect(!ConnectionWindowPaneResolver.awaitsProgressGrace(phase: .closing, awaitsAutoConnect: true)) + #expect(!ConnectionWindowPaneResolver.awaitsProgressGrace( + phase: .unavailable(.failed(Self.failure)), + awaitsAutoConnect: true + )) + } + + /// A window with no connection record has nothing to prepare for, so the grace cannot turn an + /// empty window into one that looks like it is working. + @Test("Preparing needs a connection to be preparing for") + func noConnectionStaysEmptyThroughTheGrace() { + #expect(ConnectionWindowPaneResolver.pane( + phase: .connecting, + hasConnection: false, + hasRenderableSession: false, + hasOutlastedGrace: false + ) == .empty) + + #expect(ConnectionWindowPaneResolver.pane( + phase: .idle, + hasConnection: false, + hasRenderableSession: false, + awaitsAutoConnect: true, + hasOutlastedGrace: false + ) == .empty) + } + @Test("Every unavailable reason reaches its pane") func everyUnavailableReasonResolves() { let reasons: [ConnectionUnavailableReason] = [ diff --git a/TableProTests/Core/Services/Infrastructure/ConnectionWorkspaceGraceTests.swift b/TableProTests/Core/Services/Infrastructure/ConnectionWorkspaceGraceTests.swift new file mode 100644 index 000000000..47c8ee755 --- /dev/null +++ b/TableProTests/Core/Services/Infrastructure/ConnectionWorkspaceGraceTests.swift @@ -0,0 +1,115 @@ +// +// ConnectionWorkspaceGraceTests.swift +// TableProTests +// +// The timer that decides whether a connect ever announces itself, and the pane +// the workspace resolves to on each side of it. +// +// Measured on the SQLite sample: the connecting card was on screen for 39ms +// and a "Not connected" pane was built for 38ms before it, so a window opened, +// collapsed its chrome, built three pane hierarchies and put the chrome back, +// all inside 103ms. +// + +import Foundation +@testable import TablePro +import XCTest + +@MainActor +final class ConnectionWorkspaceGraceTests: XCTestCase { + private let connectionId = UUID() + + func testADiallingWorkspaceSaysNothingUntilItsConnectEarnsIt() { + let workspace = makeWorkspace(phase: .connecting) + + XCTAssertEqual(workspace.resolvedPane, .preparing) + XCTAssertFalse( + ConnectionWindowPaneResolver.hidesChrome(for: workspace.resolvedPane), + "the window that opens has to be the window that stays" + ) + } + + func testAConnectThatOutlastsTheGraceGetsItsCard() async { + let workspace = makeWorkspace(phase: .connecting) + var revealCount = 0 + + workspace.armConnectingProgressGrace { revealCount += 1 } + try? await Task.sleep(for: LoadingRevealPolicy.grace + .milliseconds(250)) + + XCTAssertTrue(workspace.hasOutlastedConnectGrace) + XCTAssertEqual(workspace.resolvedPane, .connecting) + XCTAssertEqual(revealCount, 1, "the reveal is what repaints a pane no phase change reaches") + } + + /// A connect that lands first must leave no reveal behind, or the next one on the same + /// workspace would show its card immediately. + func testAConnectThatLandsFirstNeverReveals() async { + let workspace = makeWorkspace(phase: .connecting) + var revealCount = 0 + + workspace.armConnectingProgressGrace { revealCount += 1 } + workspace.cancelConnectingProgressGrace() + try? await Task.sleep(for: LoadingRevealPolicy.grace + .milliseconds(250)) + + XCTAssertFalse(workspace.hasOutlastedConnectGrace) + XCTAssertEqual(revealCount, 0) + } + + /// The phase churn of a reconnect must not keep pushing the reveal further out, or a + /// connection that redials on a loop would never report anything at all. + func testReArmingDoesNotRestartAWaitAlreadyRunning() async { + let workspace = makeWorkspace(phase: .connecting) + var revealCount = 0 + + workspace.armConnectingProgressGrace { revealCount += 1 } + try? await Task.sleep(for: .milliseconds(250)) + workspace.armConnectingProgressGrace { revealCount += 1 } + try? await Task.sleep(for: .milliseconds(400)) + + XCTAssertTrue(workspace.hasOutlastedConnectGrace, "the second arm must not have reset the clock") + XCTAssertEqual(revealCount, 1) + } + + /// `startActivationConnectIfNeeded` returns without dialling when the phase disallows it or the + /// connection record has gone. Without the grace timing `.idle` out too, such a window would sit + /// blank with no message and no Connect button. + func testAWindowThatNeverDialsFallsBackToTheNotConnectedPane() async { + let workspace = makeWorkspace(phase: .idle) + + XCTAssertEqual(workspace.resolvedPane, .preparing) + + workspace.armConnectingProgressGrace {} + try? await Task.sleep(for: LoadingRevealPolicy.grace + .milliseconds(250)) + + XCTAssertEqual(workspace.resolvedPane, .unavailable(.notConnected)) + } + + func testTearingDownAWorkspaceTakesItsPendingRevealWithIt() async { + let workspace = makeWorkspace(phase: .connecting) + var revealCount = 0 + + workspace.armConnectingProgressGrace { revealCount += 1 } + workspace.teardown() + try? await Task.sleep(for: LoadingRevealPolicy.grace + .milliseconds(250)) + + XCTAssertEqual(revealCount, 0) + } + + private func makeWorkspace(phase: ConnectionWindowPhase) -> ConnectionWorkspace { + ConnectionWorkspace( + connectionId: connectionId, + payload: nil, + autoConnect: true, + payloadConnection: DatabaseConnection( + id: connectionId, + name: "Chinook (Sample)", + database: "/tmp/Chinook.sqlite", + type: .sqlite + ), + session: nil, + sessionState: nil, + rightPanelState: nil, + phase: phase + ) + } +} diff --git a/TableProTests/Models/LoadingRevealPolicyTests.swift b/TableProTests/Models/LoadingRevealPolicyTests.swift new file mode 100644 index 000000000..7ae22f809 --- /dev/null +++ b/TableProTests/Models/LoadingRevealPolicyTests.swift @@ -0,0 +1,58 @@ +// +// LoadingRevealPolicyTests.swift +// TableProTests +// +// The dwell half of the policy, which is the half that gets left out. +// +// A grace on its own trades one flicker for another: work that ends just past +// the grace shows a spinner for the handful of milliseconds between the two, +// which is worse than either showing it throughout or never showing it. +// + +@testable import TablePro +import XCTest + +final class LoadingRevealPolicyTests: XCTestCase { + func testAnIndicatorRevealedAMomentAgoHasToStay() { + let revealedAt = ContinuousClock.now + let remaining = LoadingRevealPolicy.remainingDwell( + revealedAt: revealedAt, + now: revealedAt.advanced(by: .milliseconds(10)) + ) + + XCTAssertEqual(remaining, .milliseconds(490), "10ms of spinner is a flash, not a report") + } + + func testAnIndicatorThatHasServedItsDwellMayGoAtOnce() { + let revealedAt = ContinuousClock.now + let remaining = LoadingRevealPolicy.remainingDwell( + revealedAt: revealedAt, + now: revealedAt.advanced(by: .seconds(4)) + ) + + XCTAssertEqual(remaining, .zero, "anything slow enough to show one has already earned it") + } + + func testTheDwellEndsExactlyAtItsBoundary() { + let revealedAt = ContinuousClock.now + let remaining = LoadingRevealPolicy.remainingDwell( + revealedAt: revealedAt, + now: revealedAt.advanced(by: LoadingRevealPolicy.minimumDwell) + ) + + XCTAssertEqual(remaining, .zero) + } + + /// A clock that has not moved is what a same-run-loop-turn reveal and hide look like, and the + /// answer has to be the whole dwell rather than zero. + func testAnIndicatorRevealedAndFinishedInTheSameInstantStaysForTheWholeDwell() { + let revealedAt = ContinuousClock.now + let remaining = LoadingRevealPolicy.remainingDwell(revealedAt: revealedAt, now: revealedAt) + + XCTAssertEqual(remaining, LoadingRevealPolicy.minimumDwell) + } + + func testTheGraceStaysUnderTheSecondAtWhichAWaitStopsFeelingLikeOneGesture() { + XCTAssertLessThan(LoadingRevealPolicy.grace, .seconds(1)) + } +} diff --git a/TableProTests/Models/SidebarObjectListPresentationTests.swift b/TableProTests/Models/SidebarObjectListPresentationTests.swift index 70a4cab49..576fc6705 100644 --- a/TableProTests/Models/SidebarObjectListPresentationTests.swift +++ b/TableProTests/Models/SidebarObjectListPresentationTests.swift @@ -14,14 +14,16 @@ struct SidebarObjectListPresentationTests { hasActiveFilter: Bool = false, hasAnyMatch: Bool = true, hasRoutines: Bool = false, - hasTriggers: Bool = false + hasTriggers: Bool = false, + hasOutlastedGrace: Bool = true ) -> SidebarObjectListPresentation { SidebarObjectListPresentation.resolve( state: state, hasActiveFilter: hasActiveFilter, hasAnyMatch: hasAnyMatch, hasRoutines: hasRoutines, - hasTriggers: hasTriggers + hasTriggers: hasTriggers, + hasOutlastedGrace: hasOutlastedGrace ) } @@ -76,4 +78,30 @@ struct SidebarObjectListPresentationTests { func failureSurfacesMessage() { #expect(resolve(.failed("boom")) == .failed("boom")) } + + // MARK: - The grace + + /// The schema of a local database arrives in about 110ms, so the column stays blank rather + /// than spinning for a tenth of a second. + @Test("A schema read too young to report leaves the column blank") + func loadingInsideTheGracePrepares() { + #expect(resolve(.idle, hasOutlastedGrace: false) == .preparing) + #expect(resolve(.loading, hasOutlastedGrace: false) == .preparing) + } + + /// A read that has taken long enough for the user to wonder is exactly what a spinner is for. + @Test("A schema read that outlasts the grace gets its spinner") + func loadingPastTheGraceShowsTheSpinner() { + #expect(resolve(.idle, hasOutlastedGrace: false) != resolve(.idle)) + #expect(resolve(.loading) == .loading) + } + + /// The grace holds back a wait, never an answer. A refused read has to say so at once, or the + /// user is left looking at a blank column that gives no reason and offers no Retry. + @Test("The grace never delays a failure or a finished list") + func settledStatesIgnoreTheGrace() { + #expect(resolve(.failed("boom"), hasOutlastedGrace: false) == .failed("boom")) + #expect(resolve(.loaded([]), hasOutlastedGrace: false) == .empty) + #expect(resolve(.loaded([table("users")]), hasOutlastedGrace: false) == .list) + } }