Skip to content

table: Sort from the header cell and report one-row visible ranges - #3385

Open
AncientPixel wants to merge 2 commits into
longbridge:mainfrom
AncientPixel:table-header-sort-visible-ranges
Open

AncientPixel wants to merge 2 commits into
longbridge:mainfrom
AncientPixel:table-header-sort-visible-ranges

Conversation

@AncientPixel

Copy link
Copy Markdown
Contributor

Closes #3362

Description

Two small DataTable fixes in TableState, one commit each. They are separate, so I can split them into two PRs if you prefer.

1. Sort a column from its header

On main, a click on a column header only selects the column: the header's on_click (crates/component/src/table/state.rs:1650-1654) calls on_col_head_click (state.rs:822-836), which calls set_selected_col. Sorting happens only in the click handler of the small sort icon (state.rs:1614-1625). As #3362 says, users read that icon as a direction indicator, not a button, and expect a header click to sort.

Now on_col_head_click (state.rs:824) sorts when the table is sortable and the column has a sort (Column::sortable(), ascending(), descending() or sort(..)). It calls the same perform_sort, so the cycle is unchanged: default, descending, ascending, default. The sort icon's own click handler is removed. A click on the icon now bubbles to the header and sorts once. The icon keeps its hover and active styles. A click on a header without a sort still selects the column, as before.

Behaviour change: a click on a sortable header now sorts instead of selecting the column. Column selection stays reachable for those columns from the keyboard (left/right/tab, which already select columns) and from code (set_selected_col, set_selection). A mouse-only user can no longer select a sortable column. I did not add an option to restore the old click behaviour, because the issue asks for the new behaviour as the default and nothing in the table depends on selecting a sortable column by mouse. If you prefer an option, I can add one.

The sort icon's header background (#2379) is not touched.

2. Report visible row ranges of one row or none

On main, update_visible_range_if_need returned early for any range of one row or fewer (state.rs:1340-1344), because uniform_list also calls the render callback with ix..ix+1 to measure a row. As a result:

  • a table with one row never reported 0..1;
  • a table whose rows dropped to zero never reported anything, because the empty branch (state.rs:2461-2490) does not render the list, so visible_rows_changed kept the stale range;
  • with stripe, the filler rows (state.rs:2443-2447) are part of the list, so the reported end could pass rows_count (a 3-row striped table reported 0..8).

An app that shows "Showing a-b of n" under a table had to clamp the range itself and still showed a wrong range on first load.

The fix (state.rs:1344-1364):

  • update_visible_range_if_need takes the item count. The one-item guard applies only when there is more than one item.
  • The reported end is clamped to the item count. If that leaves an empty range while there are items (a stale range scrolled past a shrunken list), nothing is reported; the list scrolls back on the next frame and reports the real range.
  • The empty branch reports 0..0 (state.rs:2480). This is the only report made from render itself; the others run inside the list callbacks. The existing equality check means this fires once, and not at all for a table that starts empty.

The column axis had the same guard. Its virtual list (crates/base/src/virtual_list.rs:314-335) measures with ix..ix+1 too, so a table with one scrollable column never reported 0..1. The column call now passes the number of scrollable columns (columns_count - left fixed columns) and gets the same treatment. The reported column range is still relative to the scrollable columns, as before. No test covers the column path.

The visible_rows_changed doc comment now says the range never ends past rows_count and that an empty table reports 0..0.

Remaining edge: when there is more than one row but the viewport is shorter than one row, the real range is one row long and is still skipped, because it cannot be told apart from the measuring call.

This change was written with AI assistance (Claude Code).

Breaking Changes

No public signature changes. There are two behaviour changes:

  • A click on the header of a sortable column now cycles its sort and no longer selects the column or emits TableEvent::SelectColumn. Headers without a sort behave as before.
  • TableDelegate::visible_rows_changed now also fires with 0..1 for a one-row table and 0..0 when the rows drop to none, and its end never passes rows_count. visible_columns_changed likewise fires for a single scrollable column.

How to Test

New UI tests in crates/kit/tests/collections.rs, using a delegate that records perform_sort and visible_rows_changed calls:

  • table_header_click_sorts_sortable_columns_and_selects_the_rest: clicks ("col-header", 0) three times and checks perform_sort saw descending, ascending, default, with no column selected; then clicks ("col-header", 1) (no sort) and checks column 1 is selected and no sort ran.
  • table_reports_one_row_and_empty_visible_ranges: a 1-row table reports 0..1; after the row count drops to 0 and a frame renders, it reports 0..0.
  • table_visible_range_stops_at_the_last_row_under_stripe_filler: a 3-row striped table never reports a range ending past 3.

All three failed on main ([] sorts, no 0..1 report, [0..3, 0..8]) and pass with the change.

cargo test -p gpui-kit --features "test-support component" --test collections   # 8 passed
cargo test -p gpui-component --lib table                                        # 9 passed
cargo fmt --all --check
cargo clippy -p gpui-component -p gpui-kit --features "gpui-kit/test-support gpui-kit/component" --all-targets -- -D warnings -A clippy::nonminimal_bool

Clippy is clean for the touched crates. The -A clippy::nonminimal_bool is only there because clippy on Rust 1.95 flags crates/base/src/calendar.rs:132, which this PR does not touch.

Checklist

  • I have read the CONTRIBUTING document and followed the guidelines.
  • Reviewed the changes in this PR and confirmed AI generated code (If any) is accurate.
  • Passed cargo run for story tests related to the changes.
  • Tested macOS, Windows and Linux platforms performance (if the change is platform-specific) Not applicable: the change is not platform-specific.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PRpJSFk3zTwdgAJHehdZFm

AncientPixel and others added 2 commits October 6, 2026 21:53
A click on a column header only selected the column. Sorting needed a
click on the small sort icon, which users read as a direction
indicator rather than a button (longbridge#3362).

A click anywhere on a sortable column's header now cycles its sort.
The sort icon no longer has its own click handler, so a click on it
bubbles to the header and sorts once. A click on a header without a
sort still selects the column, and the arrow keys still select any
column.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRpJSFk3zTwdgAJHehdZFm
`visible_rows_changed` skipped every range of one row or fewer,
because uniform_list also calls its render callback with `ix..ix+1`
to measure a row. So a one-row table never reported `0..1`, a table
that dropped to no rows kept its stale range, and with `stripe` the
filler rows pushed the reported end past `rows_count`. A "showing a-b
of n" footer had to clamp locally and showed a wrong range on load.

The one-row guard now applies only when there is more than one item,
the reported end is clamped to the item count, and the empty-table
branch reports `0..0`. A clamped range left empty while items exist is
stale (the list scrolls back next frame) and is not reported. The
column axis gets the same item-count guard, since its virtual list
measures the same way; that path has no dedicated test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRpJSFk3zTwdgAJHehdZFm
@AncientPixel

Copy link
Copy Markdown
Contributor Author

The failing GPUI Fast (windows-latest) check isn't from this change. It fails at link time with LNK1123: failure during conversion to COFF, the same way it fails on main at c0bebdc, the commit this branch is based on. #3380 looks like the fix. Once it lands I'll rebase onto main so CI runs again.

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.

Datatable: Option to allow for sorting entire header column

1 participant