Conversation
153ae71 to
7df11f3
Compare
…nc, Add Row routing, and window NSUndoManager
7df11f3 to
3ad6a04
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive Structure-tab fix bundle. Eight user-visible bugs and the architectural seams they exposed.
User-reported bugs (root cause + fix)
Double-click and Return on dropdown / type-picker columns did nothing.
editEligibilityblocked dropdown / typePicker columns from inline edit, so the gestures fell through to a no-op. Removed the block; the cell body now opens the inline text editor (so the user can type a value the picker doesn't list, e.g. a custom column type), and the chevron button continues to open the picker. Mirrors NSComboBox semantics: text-editable field plus chevron with predefined options.Cmd+Z left the yellow modified tint in place after the change reverted.
TableStructureViewonly observedhasChanges; mutations that did not toggle that flag never refreshed the grid. View now also observesreloadVersionanddataGridUndo/dataGridRedoaskNSTableViewto redraw visible rows.Pressing Delete on a row only marked the focused cell red, not the full row.
StructureRowViewWithMenuextendedNSTableRowViewdirectly and had noapplyVisualState/drawBackground. Now subclassesDataGridRowViewso it inherits the deleted-row tint, layer-backing optimization, and cell-emphasis invalidation.tableView(_:rowViewForRow:)also applies the visual state to delegate-provided row views so recycled rows pick up state changes.Cmd+Shift+N added a row to the change manager + SQL Preview but the grid kept the old row count.
MainContentCommandActions.addNewRowalways routed to the data-tabRowEditingCoordinator. Now branches onresultsViewMode == .structureand dispatches to the structure grid delegate.Editing a cell did not update the displayed value.
tableRowsProvider: { tableRows }captured a snapshot at body-evaluation time. After an edit,tableView.reloadData(forRowIndexes:)re-rendered the cell from the stale source. The provider is now a closure that rebuilds viamakeCurrentProvider().asTableRows()on every call (mirrors the data tab'scoordinator.tabSessionRegistry.existingTableRows(for:)pattern).Save and discard left the yellow modified tint in place.
loadSchemaresets working state and bumpsreloadVersion, butDataGridView.updateNSViewonly callsreloadDatawhen row count or column schema changes. A column rename leaves the row count identical, so cells kept their pre-save visual state. Save and discard paths now callgridDelegate.reloadAllVisibleRows()after the manager resets.Editing one cell tinted the entire row yellow.
StructureChangeManager.getVisualStatepopulatedmodifiedColumnswith a hardcodedSet(0..<6)(over-counting in MySQL with charset/collation, under-counting elsewhere). Replaced with a real per-field diff:StructureEditingSupportgainscolumnModifiedIndices/indexModifiedIndices/foreignKeyModifiedIndiceshelpers; the delegate'sdataGridVisualState(forRow:)calls them withorderedFieldsso only the changed cells get the yellow tint.Right-click "Undo Delete" undid the wrong action. The closure called
dataGridUndo()(global Cmd+Z), so if the user deleted column A, then renamed column B, "Undo Delete" on A undid the rename of B. AddedStructureChangeManager.undoDelete(for:at:)that clears the per-row deletion mark without touching the global NSUndoManager stack. MirrorsDataChangeManager.undoRowDeletion(rowIndex:). Right-click and Cmd+Z are now independent affordances.CreateTable pickers rendered as plain text.
CreateTableViewconstructedDataGridConfigurationwithoutcustomDropdownOptions, so on-delete / on-update / index-type cells showed plain text without the picker. Now passed correctly.Architectural changes (forced into view by the bugs)
Shared NSTableView refresh primitive.
TableViewCoordinatornow exposesreloadVisibleRowsAndStates()andreloadRowAndState(at:)that pairreloadData(forRowIndexes:)withenumerateAvailableRowViews+applyVisualState. This is Apple's documented two-layer pattern:reloadData(forRowIndexes:)re-fetches cells but does not touch row views, so per-row decoration (deleted/inserted tint, deleted-row context menu state) needs the second pass. Both delegates use the same primitive instead of each duplicating only-half of the contract.RowVisualStateas the row-view source of truth.DataGridRowViewnow storesvisualState: RowVisualStateand derivesrowTintfrom it.StructureRowViewWithMenureadsvisualState.isDeleteddirectly inmenu(for:)instead of caching a shadowisRowDeletedflag that was only assigned on row-view creation.RowVisualStatealso gainsEquatableconformance andapplyVisualStateshort-circuits on no change.Private
UndoManagerperStructureChangeManagerinstance. The window has no NSDocument-backed undoManager and no view in the responder chain provides one, so wiring through the responder chain silently no-ops. Cmd+Z is dispatched by the app's own.commandsblock inTableProApptoMainContentCommandActions.undoChange(), which calls into the manager directly. The private UndoManager dies with the manager, so the prior_NSUndoStack popAndInvokecrash on stale shared state is structurally impossible.Dead code removed
StructureChangeManager.updatePrimaryKey(_:)(zero callers).StructureRowProvider.updateValue(_:at:columnIndex:),appendRow(_:),removeRow(at:)(three stub no-ops with zero callers).StructureChangeManager.getVisualState(for:tab:),visualStateCache,rebuildVisualStateCache,VisualStateCacheKeyand 15 cache-rebuild call sites. The cache was load-bearing only for the broken whole-row-tint logic; per-call recomputation is sub-millisecond at expected table sizes.RowVisualState.isFullRowModified(a workaround for the hardcodedSet(0..<6)that the per-field diff replaces).Tests
StructureEditingSupportFieldDiffTestscovers the three diff helpers andundoDelete(for:at:):columnModifiedIndices: identical inputs produce empty set; rename flags only the name index; two-field edit flags exactly those indices; fields missing fromorderedFields(e.g., collation in PostgreSQL) are correctly skipped; all 9 cases ofStructureColumnFieldare diffable.indexModifiedIndices: identical inputs empty; column-prefix-only change flags index 1; unique toggle flags index 3; intentionally-excludedisPrimaryandcommentproduce no indices.foreignKeyModifiedIndices: identical inputs empty; on-delete / on-update flag independently; all 7 grid columns are covered.undoDelete(for:at:): clears delete marks; ignores non-delete pending changes; bounds-checks; no-ops on.ddl/.parts.Out of scope (future PRs)
applyColumnDeleteUndoclearspendingChangesentirely, losing a prior.modifyColumnif the user edited a column then deleted it then Cmd+Z'd. The new visual-state logic correctly surfaces the inconsistency (yellow tint on the still-modified cell), but the manager-level undo behavior is the actual bug.ChangeManagingprotocol over-specifies forStructureChangeManager(data-specific operations stubbed as no-ops). Should split into base +DataChangeManagingsub-protocol.+/-toolbar buttons,ContentUnavailableViewfor empty Indexes / FK tabs, 7 unlocalized strings,DDLTextViewblank-rectangle empty handling.ClickHousePartsViewbuilds rawALTER TABLESQL inline, bypassing the plugin driver DDL layer.StructureMenuTargetlives inViews/Structure/but is referenced fromViews/Main/Child/DataTabGridDelegate.swift. Type ownership is in the wrong folder.Files
Core/SchemaTracking/StructureChangeManager.swiftupdatePrimaryKey,getVisualState,visualStateCache,rebuildVisualStateCache. AdddeleteInsertState(for:tab:)andundoDelete(for:at:).Models/Schema/...(no diff)EditableColumnDefinitionetc. unchanged; relied on for diff helpers.Views/Results/DataGridCoordinator.swiftreloadVisibleRowsAndStates()andreloadRowAndState(at:).invalidateCachesForUndoRedocollapses to one call.Views/Results/DataGridRowView.swiftfinal. StorevisualState. MakeapplyVisualStateidempotent.Views/Results/DataGridView.swiftisFullRowModified.RowVisualState: Equatable.Views/Results/Cells/DataGridCellView.swiftstate.visualState.isModified(columnIndex:).Views/Results/Extensions/DataGridView+Columns.swifttableView(_:rowViewForRow:)applies visual state to delegate row views.Views/Results/Extensions/DataGridView+Editing.swifteditEligibility.Views/Results/KeyHandlingTableView.swiftViews/Structure/StructureChangeManager.swiftViews/Structure/StructureEditingSupport.swiftcolumnModifiedIndices/indexModifiedIndices/foreignKeyModifiedIndicesfield-diff helpers.Views/Structure/StructureGridDelegate.swiftattachedCoordinator.dataGridDidEditCellanddataGridDeleteRowsreload via shared primitive.dataGridVisualState(forRow:)computes per-cell modified columns.onUndoDeletecallsundoDelete(for:at:)thenreloadRowAndState(at:).Views/Structure/CreateTableGridDelegate.swiftdataGridVisualState(forRow:)for inserted-row tint.Views/Structure/CreateTableView.swiftcustomDropdownOptions. Rebuild row snapshot fresh per call.Views/Structure/StructureRowViewWithMenu.swiftDataGridRowView. DropisRowDeletedshadow flag. ReadvisualState.isDeletedin menu.onUndoDelete: ((Int) -> Void)?takes the row index.Views/Structure/StructureRowProvider.swiftViews/Structure/StructureViewActionHandler.swiftaddRowclosure.Views/Structure/TableStructureView.swiftreloadVersion. WireactionHandler.addRow. Rebuild row snapshot fresh per call.Views/Structure/TableStructureView+Schema.swiftreloadAllVisibleRows(). Generate-preview clears stale SQL on empty change set.Views/Main/MainContentCommandActions.swiftaddNewRowbranches onresultsViewMode == .structure.TableProTests/Views/Structure/StructureEditingSupportFieldDiffTests.swiftundoDelete(for:at:).Test plan
Manual:
user_idtouser_idd. Only the Name cell tints yellow.+ New Table. Set type toINT. Set On Delete toCASCADE. Both pickers open as dropdowns.Automated:
xcodebuild test -only-testing:TableProTests/StructureEditingSupportFieldDiffTestsandxcodebuild test -only-testing:TableProTests/StructureChangeManagerUndoDeleteTests.swiftlint lint --strictclean on changed files.