Skip to content

refactor(datagrid): consolidate sort click cycle into header view#944

Merged
datlechin merged 44 commits intomainfrom
refactor/datagrid-sort-click-cycle
Apr 29, 2026
Merged

refactor(datagrid): consolidate sort click cycle into header view#944
datlechin merged 44 commits intomainfrom
refactor/datagrid-sort-click-cycle

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Follow-up cleanup to #942. The three-state sort cycle (asc → desc → cleared) was split across two layers:

  • Multi-column (shift-click): SortableHeaderView.mouseDown took over the click and called dataGridSort(..., isMultiSort: true) directly.
  • Single-column (plain click): fell through to AppKit's two-state descriptor toggle, then tableView(_:sortDescriptorsDidChange:) post-hoc intercepted the desc → asc transition and rewrote it as "clear".

Two paths for the same UX, with the single-column path fighting AppKit's descriptor model. Any other code that produced a same-key desc → asc transition would have accidentally triggered clear.

This PR consolidates both paths into SortableHeaderView.mouseDown. The cycle decision is now a pure function (HeaderSortCycle.nextAction) and unit tested. tableView(_:sortDescriptorsDidChange:) and the isSyncingSortDescriptors reentry guard are deleted. tableView.sortDescriptors is still written one-way from the model in syncSortDescriptors for VoiceOver accessibility; nothing in our code reads it anymore.

Test plan

  • Click an unsorted column header: sorts ascending, native indicator appears.
  • Click again: descending.
  • Click again: sort cleared, no indicator.
  • Click again: ascending (cycle restart).
  • Sort by column A, then plain-click column B: column B becomes primary ascending, column A clears.
  • Shift-click a second column while column A is sorted: column A stays primary, column B added as secondary. Both indicators render.
  • Shift-click the secondary column again: cycles its direction, primary unchanged.
  • Drag a column boundary to resize: still works (boundary clicks fall through to super).
  • VoiceOver on a sorted header: announces column name and sort direction.
  • xcodebuild testHeaderSortCycleTests (9 tests) and existing SortStateTests pass.
  • swiftlint lint --strict — 0 violations.

…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
@chatgpt-codex-connector
Copy link
Copy Markdown

💡 Codex Review

switch action {
case .sort(let columnIndex, let ascending, let isMultiSort):
coordinator.delegate?.dataGridSort(
column: columnIndex,
ascending: ascending,

P1 Badge Fall through to AppKit for header drag interactions

SortableHeaderView.mouseDown now consumes every primary-button click on data-column headers by dispatching sort actions directly, but it never calls super.mouseDown in that path. That prevents AppKit from starting native header interactions (column drag-reorder, divider drag-resize, and divider double-click auto-fit), so users lose core table-header behavior as soon as the click lands on a sortable header.



P2 Badge Provide a non-noop default for clear-sort callback

The new single-column cycle emits .clear after descending, but the default dataGridClearSort() implementation is empty. Delegates that still only implement dataGridSort (for example the structure-grid delegate path) receive no state update on the third click, so the header remains stuck in descending and subsequent clicks on that same column keep resolving to clear with no effect.

ℹ️ 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".

@datlechin datlechin merged commit a46dfe6 into main Apr 29, 2026
2 checks passed
@datlechin datlechin deleted the refactor/datagrid-sort-click-cycle branch April 29, 2026 12:45
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