Skip to content

refactor(datagrid): cell-based sort indicators with native divider behavior#945

Merged
datlechin merged 1 commit intomainfrom
refactor/datagrid-header-divider
Apr 29, 2026
Merged

refactor(datagrid): cell-based sort indicators with native divider behavior#945
datlechin merged 1 commit intomainfrom
refactor/datagrid-header-divider

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Architectural refactor on top of #942 / #944. Replaces the overlay NSImageView indicator approach with native NSTableHeaderCell-based drawing. Side benefits: cleaner code, working hover cursor on dividers, no spurious sort on divider taps.

  • Sort indicators drawn in NSTableHeaderCell: New SortableHeaderCell overrides drawInterior to render the system ascending/descending arrow at the right edge, with a small priority number to its left for secondary sort columns (Numbers-style). SuppressedSortIndicatorCell deleted.
  • Header view simplified: SortableHeaderView no longer manages overlay subviews. updateSortIndicators now pushes state into each column's SortableHeaderCell and triggers a per-column redraw.
  • Divider tap → resize, not sort: mouseDown detects clicks within the column-resize zone and routes them straight to super.mouseDown, so AppKit's column-resize handling runs and no sort cycle fires for taps on the divider.
  • Hover cursor on dividers: mouseMoved sets NSCursor.resizeLeftRight when the pointer is in a resize zone, NSCursor.arrow otherwise. Backed by a .mouseMoved tracking area and a resetCursorRects registration.

Test plan

  • Hover a column divider in a table tab → cursor changes to .
  • Click a divider without dragging → no sort triggered.
  • Click-drag a divider → column resizes.
  • Click a column header (away from divider) → sort cycles asc → desc → cleared.
  • Shift-click a second column → multi-sort, primary shows arrow, secondary shows 2 ↑ style indicator.
  • Shift-click descending secondary column → removed from sort.
  • Drag a column header to reorder → reorder works.
  • Right-click a column header → context menu appears.
  • `xcodebuild test` — `HeaderSortCycleTests` and `SortableHeaderResizeZoneTests` pass.
  • `swiftlint lint --strict` — 0 violations.

@datlechin datlechin merged commit 6f8dc5e into main Apr 29, 2026
2 checks passed
@datlechin datlechin deleted the refactor/datagrid-header-divider branch April 29, 2026 18:33
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: 3437002261

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

repositionIndicators()
override func viewDidMoveToWindow() {
super.viewDidMoveToWindow()
window?.acceptsMouseMovedEvents = true
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Restore window mouse-move policy when header leaves window

viewDidMoveToWindow unconditionally sets window?.acceptsMouseMovedEvents = true, but this commit never restores the previous value when the header view is removed or replaced. Because acceptsMouseMovedEvents is a window-wide switch (default false), any window that ever hosts this header will continue dispatching mouse-moved events globally, which can introduce unnecessary event traffic and side effects in other views after the grid is gone.

Useful? React with 👍 / 👎.

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