Skip to content

refactor(datagrid): name-based identifiers + native sort indicators#942

Merged
datlechin merged 39 commits intomainfrom
refactor/datagrid-column-architecture
Apr 29, 2026
Merged

refactor(datagrid): name-based identifiers + native sort indicators#942
datlechin merged 39 commits intomainfrom
refactor/datagrid-column-architecture

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

  • Header path now native: native NSTableView sort indicators (highlightedTableColumn + setIndicatorImage(_:in:)) replace Unicode arrows embedded in column titles; FK/chevron button context resets on cell reuse; column reorder no longer scans the settled prefix per move; applyColumnOrder fix.
  • Column identifiers are now the column name (with positional fallback for duplicate names), so saved widths follow the column across schema changes that shift its position. The static DataGridView.columnIdentifier(for:) / dataColumnIndex(from:) helpers are gone, replaced by a ColumnIdentitySchema value type owned by the coordinator.
  • ColumnLayoutStorage singleton replaced by a ColumnLayoutPersisting protocol with an injectable FileColumnLayoutPersister default. The coordinator depends on the protocol, not the concrete class. MainContentCoordinator+ColumnLayout and the hasUserResizedColumns flag are deleted; layout save/restore now lives entirely in the data grid coordinator's lifecycle (load on column build, persist on resize/move/dismantle). The Task-based @Binding mutation inside updateNSView and the isWritingColumnLayout re-entry guard are also gone.

Why not NSTableView.autosaveName: it would force positional column identifiers (the schema-shift bug returns), and depends on an undocumented UserDefaults format Apple can change between macOS releases. The principled native pattern is NSTableColumn.identifier semantics — and we now use the column name as that identifier, which is robust to schema changes. The persister is custom because our domain (per-(connection, table), multi-tab, tab-state-restorable) doesn't fit the autosave abstraction.

Test plan

  • Resize a column on a table tab, switch tabs, switch back: width persists.
  • Reorder columns on a table tab, close + reopen the tab: order restored.
  • Sort by a single column: native arrow appears in the header (no embedded text).
  • Cmd-click a second column header: secondary sort applies, both columns show indicators, primary is highlighted.
  • Open a query result with duplicate column names (SELECT a, b, a): no crash, columns render, identifier scheme falls back to positional.
  • Open an existing table after a schema change that inserts a new column before a saved-width column: the saved width follows the renamed column, not the position.
  • Foreign-key arrow click on a row: navigates to the right referenced row (regression from the C1 cell-reuse fix).
  • Column visibility (Hide / Show All): unchanged behavior.
  • VoiceOver on a sorted header: announces column name and sort direction separately.
  • xcodebuild test — ColumnIdentitySchemaTests (8), FileColumnLayoutPersisterTests (6), ColumnLayoutStateTests, ColumnVisibilityManagerTests, SortStateTests all pass.
  • swiftlint lint --strict — 0 violations.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3922ad5871

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}
saveColumnVisibilityToTab()
saveColumnLayoutForTable()
saveColumnVisibilityForActiveTable()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Persist outgoing table layout before tab context changes

Switching tabs no longer calls saveColumnLayoutForTable(), so layout writes now depend only on the debounced scheduleLayoutPersist() path in the grid coordinator. If the user resizes/reorders a column and switches tabs within the 500ms debounce window, the coordinator is rebound to the new tab before the task fires, so the outgoing tab’s latest layout can be lost (and the delayed write can target the wrong table context). This is a user-visible data-loss regression for column widths/order on quick tab switches.

Useful? React with 👍 / 👎.

…edundant calls, add coordinator + schema-shift tests
…rop SF Symbol fallback and highlightedTableColumn duplication
…icator for pixel-perfect chevron rendering on every sorted column
…3-state, SortableHeaderView only intercepts shift-click for multi-sort
… in offscreen context to avoid ARC crash on sort
…raw duplicate chevron + bold title on primary
@datlechin datlechin merged commit 1456fe0 into main Apr 29, 2026
2 checks passed
@datlechin datlechin deleted the refactor/datagrid-column-architecture branch April 29, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant