Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ struct MenuValidationContext: Equatable {
var canRestorePreviousValues = false
var isQueryExecuting = false
var hasQueryText = false
var canClearQuery = false
var canClearResults = false
var hasPendingChanges = false
var hasDataPendingChanges = false
var hasRowSelection = false
Expand Down Expand Up @@ -193,6 +195,10 @@ extension MainSplitViewController: NSMenuItemValidation {
return context.isQueryTab && context.isConnected && context.hasQueryText && !context.isQueryExecuting
case #selector(cancelQuery(_:)):
return context.isQueryExecuting
case #selector(clearQuery(_:)):
return context.canClearQuery
case #selector(clearResults(_:)):
return context.canClearResults
case #selector(previewSQL(_:)):
return context.isConnected && context.hasDataPendingChanges
case #selector(saveAsFavorite(_:)):
Expand Down Expand Up @@ -353,6 +359,8 @@ extension MainSplitViewController: NSMenuItemValidation {
canRestorePreviousValues: actions.canRestorePreviousValues,
isQueryExecuting: actions.isQueryExecuting,
hasQueryText: actions.hasQueryText,
canClearQuery: actions.canClearQuery,
canClearResults: actions.canClearResults,
hasPendingChanges: actions.hasPendingChanges,
hasDataPendingChanges: actions.hasDataPendingChanges,
hasRowSelection: actions.hasRowSelection,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,10 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan
super.splitViewDidResizeSubviews(notification)
recomputeWindowMinSize()
toolbarOwner?.syncSidebarSelection()
/// A divider drag can collapse the trailing pane without going through `hideTrailingPane`,
/// and history's activation is keyed on the flag rather than on the pane, so it has to be
/// reconciled here as well or the panel keeps querying behind a collapsed divider.
syncHistoryPanelVisibility()
}

override func viewWillAppear() {
Expand Down Expand Up @@ -935,6 +939,11 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan
isAIEnabled: AppSettingsManager.shared.ai.enabled
)
inspectorPaneHost.show(selected.panes.trailingPane(for: surface))
/// The single reconciliation point. Every route that changes which surface the pane shows,
/// or which workspace it belongs to, ends here: a reveal, a hide, a workspace switch and an
/// availability change all call this, so the flag cannot describe a different connection's
/// pane than the one on screen.
syncHistoryPanelVisibility()
}

// MARK: - Session Bindings
Expand Down Expand Up @@ -1039,7 +1048,9 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan
/// the inspector unconditionally swapped the assistant out from under a half-typed question and
/// persisted the inspector as that connection's surface, on every row the user clicked.
func revealInspectorForSelection() {
guard !isAssistantVisible else { return }
/// History is deliberately opened, the same way the assistant is, so a row click must not
/// take it away and persist the inspector as this connection's surface behind it.
guard !isAssistantVisible, !isHistoryVisible else { return }
showInspector()
}

Expand All @@ -1056,8 +1067,8 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan
/// on screen, and it is what the Find Past Queries tip reads to know it has been answered.
/// Leaving it behind when history moved into the trailing pane would have left the panel
/// mounted and inert, which is the shape of a pane that renders nothing forever.
private func syncHistoryPanelVisibility() {
guard let connectionId = workspaces.selected?.connectionId else { return }
internal func syncHistoryPanelVisibility() {
guard isViewLoaded, let connectionId = workspaces.selected?.connectionId else { return }
let showing = isTrailingPaneOpen && resolvedTrailingSurface == .history
let state = HistoryPanelState.forConnection(connectionId)
guard state.isVisible != showing else { return }
Expand Down
5 changes: 5 additions & 0 deletions TablePro/Models/Query/QueryCommandAvailability.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ struct QueryCommandAvailability {
let canSaveAsFavorite: Bool
let canClearQuery: Bool
let canClearResults: Bool
/// Whether the Run menu has anything live in it. Clear Query leaves the results standing and
/// takes `canRun` away with it, so gating the menu on Run alone hid Clear Results at the one
/// moment it was the command the reader wanted.
let canOpenRunMenu: Bool
let explainVariants: [ExplainVariant]

/// Every hint the bar shows, resolved here so a disabled control can say why rather than just
Expand Down Expand Up @@ -47,6 +51,7 @@ struct QueryCommandAvailability {
canSaveAsFavorite = hasQueryText
canClearQuery = hasQueryText
canClearResults = hasResults
canOpenRunMenu = canRun || hasQueryText || hasResults

runHint = Self.hint(
base: shortcutHint(String(localized: "Run"), .executeQuery),
Expand Down
20 changes: 16 additions & 4 deletions TablePro/Models/Query/QueryResultPresentation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ struct QueryResultInputs: Equatable {
var activeResultRowsAffected = 0
var activeResultExecutionTime: TimeInterval?
var activeResultStatusMessage: String?
/// A failed result carries its own message, which outlives the tab's. Pin a failure, run
/// something that works, and `executionErrorMessage` is cleared while this one is not.
var activeResultErrorMessage: String?
var loadedColumnCount = 0
var loadedRowCount = 0
var executionErrorMessage: String?
Expand Down Expand Up @@ -84,9 +87,18 @@ struct QueryResultPresentation: Equatable {
content = Self.resolveContent(inputs)
showsResultSetSelector = Self.resolvesResultSetSelector(inputs)
showsFilterChrome = Self.resolvesFilterChrome(inputs)
showsFindBar = inputs.isFindBarVisible && inputs.tabType == .table
showsFindBar = inputs.isFindBarVisible
&& inputs.tabType == .table
&& inputs.viewMode.showsFindBar
showsStatusBar = true
showsErrorBanner = inputs.executionErrorMessage != nil
showsErrorBanner = Self.resolvedError(inputs) != nil
}

/// The error the pane is actually showing. The active result's own message wins, because it
/// describes the result on screen; the tab's is the fallback for a failure that produced no
/// result set at all.
static func resolvedError(_ inputs: QueryResultInputs) -> String? {
inputs.activeResultErrorMessage ?? inputs.executionErrorMessage
}

private static func resolveContent(_ inputs: QueryResultInputs) -> QueryResultContent {
Expand Down Expand Up @@ -140,7 +152,7 @@ struct QueryResultPresentation: Equatable {
private static func resolveSettledResult(_ inputs: QueryResultInputs) -> QueryResultContent? {
guard inputs.hasExecuted, !inputs.isExecuting else { return nil }

if inputs.hasActiveResultSet, !inputs.activeResultHasColumns, inputs.executionErrorMessage == nil {
if inputs.hasActiveResultSet, !inputs.activeResultHasColumns, resolvedError(inputs) == nil {
return .statementSucceeded(
rowsAffected: inputs.activeResultRowsAffected,
executionTime: inputs.activeResultExecutionTime,
Expand All @@ -149,7 +161,7 @@ struct QueryResultPresentation: Equatable {
}

guard inputs.loadedColumnCount == 0 else { return resolveEmptyRows(inputs) }
guard inputs.executionErrorMessage == nil else { return nil }
guard resolvedError(inputs) == nil else { return nil }
guard inputs.resultSetCount > 0 else { return .idle }

return .statementSucceeded(
Expand Down
16 changes: 15 additions & 1 deletion TablePro/Models/Query/ResultSetMenuModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,25 @@ struct ResultSetMenuModel: Equatable {

/// The same count in figures, for the tiers where the bar has no room for the sentence. The
/// chooser never leaves the bar entirely, because Pin and Close have no other one-click route.
///
/// A single result is named rather than counted, and a result is named after its table or its
/// leading comment, so the name is as long as the identifier. The chooser is `fixedSize`, so an
/// unbounded name here sets the bar's own floor and pushes the grid out of a narrow pane.
var compactTitle: String {
guard total > 1 else { return entries.first?.label ?? "" }
guard total > 1 else { return Self.truncated(entries.first?.label ?? "") }
return "\(activeOrdinal)/\(total)"
}

/// Long enough to tell two results apart, short enough that no name decides the bar's width.
/// The full name stays on the menu entry and on the control's accessibility label.
private static func truncated(_ label: String) -> String {
let value = label as NSString
guard value.length > compactTitleCharacterLimit else { return label }
return value.substring(to: compactTitleCharacterLimit) + "…"
}

private static let compactTitleCharacterLimit = 16

var activeEntry: ResultSetMenuEntry? {
entries.first { $0.isActive }
}
Expand Down
9 changes: 9 additions & 0 deletions TablePro/Models/UI/TrailingPaneState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ import os
let raw = defaults.string(forKey: Self.surfaceKey(connectionId)),
let stored = TrailingPaneSurface(rawValue: raw) {
self.surface = stored
} else if let connectionId, HistoryPanelPreferencesStorage.load(for: connectionId).isVisible {
/// This connection last had the query history drawer open, and the drawer is now a
/// surface of this pane. Adopting it here is what carries that reader across the move
/// instead of silently closing their history on the first launch after upgrading.
///
/// Deterministic without a migration flag: it is reachable only while no surface has
/// ever been stored for the connection, and storing one is what this initializer's
/// `didSet` does the first time anything changes it.
self.surface = .history
} else {
self.surface = .inspector
}
Expand Down
50 changes: 33 additions & 17 deletions TablePro/Views/Editor/QueryEditorBar.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ struct QueryEditorBar: View {
let commands: QueryCommandAvailability
let isExecuting: Bool
let vimMode: VimMode?
/// Whether the first-run tip pointing at query history is still owed. It anchors to the Run
/// menu, which is the control that produces the history it is telling the reader about.
let showsHistoryTip: Bool

let onRun: () -> Void
let onRunAllStatements: () -> Void
Expand Down Expand Up @@ -121,6 +124,11 @@ struct QueryEditorBar: View {
/// permanently dimmed button costs width the editor wants. TablePlus does the same: its Cancel
/// appears in the query editor for a long query rather than standing there dimmed. The two are
/// never both actionable, so nothing is reachable in one state and not the other.
///
/// The two halves are separately enabled, which is the whole reason this is a `ControlGroup`
/// and not a `Menu(primaryAction:)`. Clear Query leaves the results standing and makes Run
/// unavailable, and disabling one control for both would have taken Clear Results down with it
/// at exactly the moment the reader wanted it.
@ViewBuilder
private var runControl: some View {
if isExecuting {
Expand All @@ -131,26 +139,34 @@ struct QueryEditorBar: View {
.help(commands.stopHint)
.accessibilityIdentifier("query-stop")
} else {
Menu {
Button(String(localized: "Run All Statements"), action: onRunAllStatements)
Button(String(localized: "Run Without Limit"), action: onRunWithoutLimit)
Divider()
Button(String(localized: "Clear Query"), action: onClearQuery)
.disabled(!commands.canClearQuery)
Button(String(localized: "Clear Results"), action: onClearResults)
.disabled(!commands.canClearResults)
} label: {
Label(String(localized: "Run"), systemImage: "play.fill")
} primaryAction: {
onRun()
ControlGroup {
Button(String(localized: "Run"), systemImage: "play.fill", action: onRun)
.labelStyle(.titleAndIcon)
.disabled(!commands.canRun)
.help(commands.runHint)
.accessibilityIdentifier("query-run")

Menu(String(localized: "Run Options"), systemImage: "chevron.down") {
Button(String(localized: "Run All Statements"), action: onRunAllStatements)
.disabled(!commands.canRun)
Button(String(localized: "Run Without Limit"), action: onRunWithoutLimit)
.disabled(!commands.canRun)
Divider()
Button(String(localized: "Clear Query"), action: onClearQuery)
.disabled(!commands.canClearQuery)
Button(String(localized: "Clear Results"), action: onClearResults)
.disabled(!commands.canClearResults)
}
.labelStyle(.iconOnly)
.disabled(!commands.canOpenRunMenu)
.accessibilityIdentifier("query-run-menu")
.modifier(FeatureTipPopoverAnchor(
tip: FindPastQueriesTip(shortcut: FeatureTipShortcut.display(for: .toggleHistory)),
isEnabled: showsHistoryTip
))
}
.menuStyle(.button)
.buttonStyle(.borderedProminent)
.controlSize(.small)
.fixedSize()
.disabled(!commands.canRun)
.help(commands.runHint)
.accessibilityIdentifier("query-run")
}
}
}
2 changes: 2 additions & 0 deletions TablePro/Views/Editor/QueryEditorView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ struct QueryEditorView: View {

let scope: QueryScopeBarModel
let commands: QueryCommandAvailability
var showsHistoryTip: Bool = false
var onRun: () -> Void
var onRunAllStatements: () -> Void
var onRunWithoutLimit: () -> Void
Expand All @@ -56,6 +57,7 @@ struct QueryEditorView: View {
commands: commands,
isExecuting: isExecuting,
vimMode: AppSettingsManager.shared.editor.vimModeEnabled ? vimMode : nil,
showsHistoryTip: showsHistoryTip,
onRun: onRun,
onRunAllStatements: onRunAllStatements,
onRunWithoutLimit: onRunWithoutLimit,
Expand Down
2 changes: 2 additions & 0 deletions TablePro/Views/Main/Child/MainEditorContentView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ struct MainEditorContentView: View {
},
scope: scopeBarModel(for: tab),
commands: commandAvailability(for: tab),
showsHistoryTip: showsHistoryTip,
onRun: { coordinator.runQuery(viewport: .firstRow) },
onRunAllStatements: { coordinator.runAllStatements() },
onRunWithoutLimit: { coordinator.runQuery(viewport: .firstRow, bypassRowLimit: true) },
Expand Down Expand Up @@ -825,6 +826,7 @@ struct MainEditorContentView: View {
inputs.activeResultRowsAffected = activeResultSet?.rowsAffected ?? 0
inputs.activeResultExecutionTime = activeResultSet?.executionTime
inputs.activeResultStatusMessage = activeResultSet?.statusMessage
inputs.activeResultErrorMessage = activeResultSet?.errorMessage
inputs.loadedColumnCount = rows.columns.count
inputs.loadedRowCount = rows.rows.count
inputs.executionErrorMessage = tab.execution.errorMessage
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import AppKit
import Foundation

/// The single answer to "would closing or quitting destroy something the user cannot get back".
Expand Down Expand Up @@ -127,6 +128,19 @@ extension MainContentCoordinator {
tab.showsUnsavedIndicator || hasUnsavedWork(in: tab)
}

/// Pushes the selected tab's answer onto the window's close button.
///
/// The editor's text binding does this on every keystroke, which covers typing. A command that
/// changes the text without going through that binding has to say so itself, or the dot
/// describes the tab as it was before the command ran.
func refreshUnsavedIndicator() {
guard let tab = tabManager.selectedTab, let window = contentWindow else { return }
let showsIndicator = showsUnsavedIndicator(for: tab)
Task { @MainActor in
window.isDocumentEdited = showsIndicator
}
}

func hasAnyUnsavedWork() -> Bool {
changeManager.hasChanges
|| hasPendingDestructiveTableOps
Expand Down
5 changes: 5 additions & 0 deletions TablePro/Views/Main/MainContentCommandActions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,11 @@ final class MainContentCommandActions {
coordinator.tabManager.mutate(at: tabIndex) { $0.content.query = "" }
coordinator.toolbarState.hasQueryText = false
coordinator.scheduleDraftSave()
/// The editor's own text binding recomputes this on every keystroke, and emptying the tab
/// from a command does not go through that binding. Without it a scratch tab keeps the
/// dirty dot it no longer deserves, and a file-backed tab that this command just emptied
/// is not marked modified until some other window event happens to recompute it.
coordinator.refreshUnsavedIndicator()
}

var canClearQuery: Bool {
Expand Down
17 changes: 17 additions & 0 deletions TableProTests/Models/Query/QueryCommandAvailabilityTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,23 @@ struct QueryCommandAvailabilityTests {
#expect(Self.make(hasQueryText: false).formatHint.contains("nothing to format"))
}

/// Clear Query leaves the results standing and takes `canRun` with it. Gating the whole Run
/// menu on `canRun` then hid Clear Results at exactly the moment it was the live command.
@Test("The Run menu stays reachable while a clear command is still valid")
func runMenuOutlivesRun() {
let clearedQueryWithResults = Self.make(hasQueryText: false, hasResults: true)
#expect(clearedQueryWithResults.canRun == false)
#expect(clearedQueryWithResults.canClearResults)
#expect(clearedQueryWithResults.canOpenRunMenu)

let offlineWithText = Self.make(isConnected: false, hasQueryText: true)
#expect(offlineWithText.canRun == false)
#expect(offlineWithText.canOpenRunMenu)

let nothingAtAll = Self.make(hasQueryText: false, hasResults: false)
#expect(nothingAtAll.canOpenRunMenu == false)
}

@Test("Clear Results follows the results, not the query text")
func clearResultsFollowsResults() {
#expect(Self.make(hasResults: true).canClearResults)
Expand Down
Loading
Loading