From ed901d1b285985a57309542949bdcd94eca2cad7 Mon Sep 17 00:00:00 2001 From: Gustave Date: Tue, 6 Oct 2026 21:53:07 +0200 Subject: [PATCH 1/3] table: Sort a column from its header 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 (#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 Claude-Session: https://claude.ai/code/session_01PRpJSFk3zTwdgAJHehdZFm --- crates/component/src/table/state.rs | 16 +++- crates/kit/tests/collections.rs | 113 +++++++++++++++++++++++++- website/component/data-table.md | 2 + website/zh-CN/component/data-table.md | 2 + 4 files changed, 128 insertions(+), 5 deletions(-) diff --git a/crates/component/src/table/state.rs b/crates/component/src/table/state.rs index 9c590edf95..fc61c4f27f 100644 --- a/crates/component/src/table/state.rs +++ b/crates/component/src/table/state.rs @@ -819,7 +819,18 @@ where } } - fn on_col_head_click(&mut self, col_ix: usize, _: &mut Window, cx: &mut Context) { + /// A click on a sortable column's header, or on its sort icon, cycles the sort; + /// a click on any other header selects the column. + fn on_col_head_click(&mut self, col_ix: usize, window: &mut Window, cx: &mut Context) { + let sortable = self + .col_groups + .get(col_ix) + .is_some_and(|col_group| col_group.column.sort.is_some()); + if self.sortable && sortable { + self.perform_sort(col_ix, window, cx); + return; + } + if !self.col_selectable { return; } @@ -1620,9 +1631,6 @@ where }) .hover(|this| this.bg(cx.theme().tokens.secondary).opacity(7.)) .active(|this| this.bg(cx.theme().tokens.secondary_active).opacity(1.)) - .on_click( - cx.listener(move |table, _, window, cx| table.perform_sort(col_ix, window, cx)), - ) .child( Icon::new(icon) .size_3() diff --git a/crates/kit/tests/collections.rs b/crates/kit/tests/collections.rs index 902cd7f754..762cde5bef 100644 --- a/crates/kit/tests/collections.rs +++ b/crates/kit/tests/collections.rs @@ -1,7 +1,9 @@ mod common; +use std::ops::Range; + use gpui_kit::component::{ list::ListItem, - table::{Column, DataTable, TableDelegate, TableSelection, TableState}, + table::{Column, ColumnSort, DataTable, TableDelegate, TableSelection, TableState}, tree::{Tree, TreeItem, TreeState}, }; use gpui_kit::test::TestWindowExt; @@ -256,3 +258,112 @@ fn table_keyboard_leaves_rows_unselected_when_rows_are_not_selectable(cx: &mut T }) .unwrap(); } + +/// A table whose first column sorts and whose second does not, recording what +/// the table reports back to its delegate. +struct Ledger { + rows: usize, + sorts: Vec<(usize, ColumnSort)>, + visible_rows: Vec>, +} +impl Ledger { + fn new(rows: usize) -> Self { + Self { + rows, + sorts: Vec::new(), + visible_rows: Vec::new(), + } + } +} +impl TableDelegate for Ledger { + fn columns_count(&self, _: &App) -> usize { + 2 + } + fn rows_count(&self, _: &App) -> usize { + self.rows + } + fn column(&self, ix: usize, _: &App) -> Column { + let column = Column::new(format!("column-{ix}"), format!("Column {ix}")).width(px(180.)); + if ix == 0 { column.sortable() } else { column } + } + fn render_td( + &mut self, + row: usize, + col: usize, + _: &mut Window, + _: &mut Context>, + ) -> impl IntoElement { + div().child(format!("{row}:{col}")) + } + fn perform_sort( + &mut self, + col_ix: usize, + sort: ColumnSort, + _: &mut Window, + _: &mut Context>, + ) { + self.sorts.push((col_ix, sort)); + } + fn visible_rows_changed( + &mut self, + visible_range: Range, + _: &mut Window, + _: &mut Context>, + ) { + self.visible_rows.push(visible_range); + } +} +struct LedgerView { + table: Entity>, + stripe: bool, +} +impl Render for LedgerView { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + .size_full() + .child(DataTable::new(&self.table).stripe(self.stripe)) + } +} +fn open_ledger( + cx: &mut TestAppContext, + rows: usize, + stripe: bool, +) -> (gpui_kit::AnyWindowHandle, Entity>) { + cx.update(gpui_kit::init); + let (handle, content) = + common::open_window(cx, Some(size(px(640.), px(320.))), |window, cx| { + cx.new(|cx| LedgerView { + table: cx.new(|cx| TableState::new(Ledger::new(rows), window, cx)), + stripe, + }) + }); + let table = cx + .update_window(handle.into(), |_, _, cx| content.read(cx).table.clone()) + .unwrap(); + (handle.into(), table) +} + +#[gpui_kit::test] +fn table_header_click_sorts_sortable_columns_and_selects_the_rest(cx: &mut TestAppContext) { + let (handle, table) = open_ledger(cx, 3, false); + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + for _ in 0..3 { + window.click(("col-header", 0usize), cx); + } + assert_eq!( + table.read(cx).delegate().sorts, + vec![ + (0, ColumnSort::Descending), + (0, ColumnSort::Ascending), + (0, ColumnSort::Default), + ] + ); + assert_eq!(table.read(cx).selected_col(), None); + + window.click(("col-header", 1usize), cx); + assert_eq!(table.read(cx).selected_col(), Some(1)); + assert_eq!(table.read(cx).delegate().sorts.len(), 3); + }) + .unwrap(); +} diff --git a/website/component/data-table.md b/website/component/data-table.md index bc6c762fab..d49f1620e5 100644 --- a/website/component/data-table.md +++ b/website/component/data-table.md @@ -176,6 +176,8 @@ impl TableDelegate for LargeDataDelegate { ### Sorting Implementation +Clicking the header of a sortable column cycles its sort: descending, ascending, then back to the default order. Clicking the header of a column that is not sortable selects the column. + Implement sorting in your delegate: ```rust diff --git a/website/zh-CN/component/data-table.md b/website/zh-CN/component/data-table.md index bdab756e5f..d451dc3505 100644 --- a/website/zh-CN/component/data-table.md +++ b/website/zh-CN/component/data-table.md @@ -152,6 +152,8 @@ impl TableDelegate for LargeDataDelegate { ## 排序 +点击可排序列的表头会依次切换排序：降序、升序，然后恢复默认顺序。点击不可排序列的表头会选中该列。 + 排序逻辑需要由你的 `TableDelegate` 实现： ```rust From 6a7bb43208fb6ee9ce60965f75ed2cbed5a496bc Mon Sep 17 00:00:00 2001 From: Gustave Date: Tue, 6 Oct 2026 21:53:07 +0200 Subject: [PATCH 2/3] table: Report visible row ranges of one row or none `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 Claude-Session: https://claude.ai/code/session_01PRpJSFk3zTwdgAJHehdZFm --- crates/component/src/table/delegate.rs | 3 +++ crates/component/src/table/state.rs | 17 ++++++++++++-- crates/kit/tests/collections.rs | 32 ++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/crates/component/src/table/delegate.rs b/crates/component/src/table/delegate.rs index 03f112dc3c..26915cb82e 100644 --- a/crates/component/src/table/delegate.rs +++ b/crates/component/src/table/delegate.rs @@ -195,6 +195,9 @@ pub trait TableDelegate: Sized + 'static { /// Called when the visible range of the rows changed. /// + /// The range never ends past `rows_count`; a table whose rows drop to none + /// reports `0..0`. + /// /// NOTE: Make sure this method is fast, because it will be called frequently. /// /// This can used to handle some data update, to only update the visible rows. diff --git a/crates/component/src/table/state.rs b/crates/component/src/table/state.rs index fc61c4f27f..4596eaa059 100644 --- a/crates/component/src/table/state.rs +++ b/crates/component/src/table/state.rs @@ -1344,13 +1344,22 @@ where fn update_visible_range_if_need( &mut self, visible_range: Range, + items_count: usize, axis: Axis, window: &mut Window, cx: &mut Context, ) { - // Skip when visible range is only 1 item. + // Skip when visible range is only 1 item, unless there is at most 1 item. // The visual_list will use first item to measure. - if visible_range.len() <= 1 { + if visible_range.len() <= 1 && items_count > 1 { + return; + } + + // Stripe filler rows are rendered past the last row; never report them. + let end = visible_range.end.min(items_count); + let visible_range = visible_range.start.min(end)..end; + // A range wholly past the last item is stale; the list scrolls back next frame. + if visible_range.is_empty() && items_count > 0 { return; } @@ -2153,6 +2162,7 @@ where move |table, visible_range: Range, window, cx| { table.update_visible_range_if_need( visible_range.clone(), + columns_count.saturating_sub(left_columns_count), Axis::Horizontal, window, cx, @@ -2467,6 +2477,8 @@ where }; let empty_view = if rows_count == 0 { + // The rows list is not rendered, so report the empty range here. + self.update_visible_range_if_need(0..0, 0, Axis::Vertical, window, cx); Some( div() .size_full() @@ -2528,6 +2540,7 @@ where ); table.update_visible_range_if_need( visible_range.clone(), + rows_count, Axis::Vertical, window, cx, diff --git a/crates/kit/tests/collections.rs b/crates/kit/tests/collections.rs index 762cde5bef..8be2221dd1 100644 --- a/crates/kit/tests/collections.rs +++ b/crates/kit/tests/collections.rs @@ -367,3 +367,35 @@ fn table_header_click_sorts_sortable_columns_and_selects_the_rest(cx: &mut TestA }) .unwrap(); } + +#[gpui_kit::test] +fn table_reports_one_row_and_empty_visible_ranges(cx: &mut TestAppContext) { + let (handle, table) = open_ledger(cx, 1, false); + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + assert_eq!(table.read(cx).delegate().visible_rows.last(), Some(&(0..1))); + + table.update(cx, |table, cx| { + table.delegate_mut().rows = 0; + cx.notify(); + }); + window.render_frame(cx); + assert_eq!(table.read(cx).delegate().visible_rows.last(), Some(&(0..0))); + }) + .unwrap(); +} + +#[gpui_kit::test] +fn table_visible_range_stops_at_the_last_row_under_stripe_filler(cx: &mut TestAppContext) { + let (handle, table) = open_ledger(cx, 3, true); + cx.update_window(handle, |_, window, cx| { + window.render_frame(cx); + let reported = table.read(cx).delegate().visible_rows.clone(); + assert!(!reported.is_empty()); + assert!( + reported.iter().all(|range| range.end <= 3), + "reported past the last row: {reported:?}" + ); + }) + .unwrap(); +} From 10b98755735d40814e084dcfd786aff46de868bf Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Wed, 7 Oct 2026 14:57:40 +0800 Subject: [PATCH 3/3] table: Clarify UI test fixture and sorting names Name the recording delegate TableProbe, distinguish counts and callback history, and identify the column sorting capability explicitly. Written with OpenAI Codex assistance. --- crates/component/src/table/state.rs | 4 +- crates/kit/tests/collections.rs | 83 ++++++++++++++++------------- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/crates/component/src/table/state.rs b/crates/component/src/table/state.rs index 4596eaa059..6a0646cb60 100644 --- a/crates/component/src/table/state.rs +++ b/crates/component/src/table/state.rs @@ -822,11 +822,11 @@ where /// A click on a sortable column's header, or on its sort icon, cycles the sort; /// a click on any other header selects the column. fn on_col_head_click(&mut self, col_ix: usize, window: &mut Window, cx: &mut Context) { - let sortable = self + let column_sortable = self .col_groups .get(col_ix) .is_some_and(|col_group| col_group.column.sort.is_some()); - if self.sortable && sortable { + if self.sortable && column_sortable { self.perform_sort(col_ix, window, cx); return; } diff --git a/crates/kit/tests/collections.rs b/crates/kit/tests/collections.rs index 6db6b960d5..5154fc73f2 100644 --- a/crates/kit/tests/collections.rs +++ b/crates/kit/tests/collections.rs @@ -263,39 +263,44 @@ fn table_keyboard_leaves_rows_unselected_when_rows_are_not_selectable(cx: &mut T /// A table whose first column sorts and whose second does not, recording what /// the table reports back to its delegate. -struct Ledger { - rows: usize, - sorts: Vec<(usize, ColumnSort)>, - visible_rows: Vec>, +struct TableProbe { + rows_count: usize, + sort_calls: Vec<(usize, ColumnSort)>, + visible_row_ranges: Vec>, } -impl Ledger { - fn new(rows: usize) -> Self { +impl TableProbe { + fn new(rows_count: usize) -> Self { Self { - rows, - sorts: Vec::new(), - visible_rows: Vec::new(), + rows_count, + sort_calls: Vec::new(), + visible_row_ranges: Vec::new(), } } } -impl TableDelegate for Ledger { +impl TableDelegate for TableProbe { fn columns_count(&self, _: &App) -> usize { 2 } fn rows_count(&self, _: &App) -> usize { - self.rows + self.rows_count } - fn column(&self, ix: usize, _: &App) -> Column { - let column = Column::new(format!("column-{ix}"), format!("Column {ix}")).width(px(180.)); - if ix == 0 { column.sortable() } else { column } + fn column(&self, col_ix: usize, _: &App) -> Column { + let column = + Column::new(format!("column-{col_ix}"), format!("Column {col_ix}")).width(px(180.)); + if col_ix == 0 { + column.sortable() + } else { + column + } } fn render_td( &mut self, - row: usize, - col: usize, + row_ix: usize, + col_ix: usize, _: &mut Window, _: &mut Context>, ) -> impl IntoElement { - div().child(format!("{row}:{col}")) + div().child(format!("{row_ix}:{col_ix}")) } fn perform_sort( &mut self, @@ -304,7 +309,7 @@ impl TableDelegate for Ledger { _: &mut Window, _: &mut Context>, ) { - self.sorts.push((col_ix, sort)); + self.sort_calls.push((col_ix, sort)); } fn visible_rows_changed( &mut self, @@ -312,30 +317,30 @@ impl TableDelegate for Ledger { _: &mut Window, _: &mut Context>, ) { - self.visible_rows.push(visible_range); + self.visible_row_ranges.push(visible_range); } } -struct LedgerView { - table: Entity>, +struct TableProbeView { + table: Entity>, stripe: bool, } -impl Render for LedgerView { +impl Render for TableProbeView { fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { div() .size_full() .child(DataTable::new(&self.table).stripe(self.stripe)) } } -fn open_ledger( +fn open_table_probe( cx: &mut TestAppContext, - rows: usize, + rows_count: usize, stripe: bool, -) -> (gpui_kit::AnyWindowHandle, Entity>) { +) -> (gpui_kit::AnyWindowHandle, Entity>) { cx.update(gpui_kit::init); let (handle, content) = common::open_window(cx, Some(size(px(640.), px(320.))), |window, cx| { - cx.new(|cx| LedgerView { - table: cx.new(|cx| TableState::new(Ledger::new(rows), window, cx)), + cx.new(|cx| TableProbeView { + table: cx.new(|cx| TableState::new(TableProbe::new(rows_count), window, cx)), stripe, }) }); @@ -347,14 +352,14 @@ fn open_ledger( #[gpui_kit::test] fn table_header_click_sorts_sortable_columns_and_selects_the_rest(cx: &mut TestAppContext) { - let (handle, table) = open_ledger(cx, 3, false); + let (handle, table) = open_table_probe(cx, 3, false); cx.update_window(handle, |_, window, cx| { window.render_frame(cx); for _ in 0..3 { window.click(("col-header", 0usize), cx); } assert_eq!( - table.read(cx).delegate().sorts, + table.read(cx).delegate().sort_calls, vec![ (0, ColumnSort::Descending), (0, ColumnSort::Ascending), @@ -365,34 +370,40 @@ fn table_header_click_sorts_sortable_columns_and_selects_the_rest(cx: &mut TestA window.click(("col-header", 1usize), cx); assert_eq!(table.read(cx).selected_col(), Some(1)); - assert_eq!(table.read(cx).delegate().sorts.len(), 3); + assert_eq!(table.read(cx).delegate().sort_calls.len(), 3); }) .unwrap(); } #[gpui_kit::test] fn table_reports_one_row_and_empty_visible_ranges(cx: &mut TestAppContext) { - let (handle, table) = open_ledger(cx, 1, false); + let (handle, table) = open_table_probe(cx, 1, false); cx.update_window(handle, |_, window, cx| { window.render_frame(cx); - assert_eq!(table.read(cx).delegate().visible_rows.last(), Some(&(0..1))); + assert_eq!( + table.read(cx).delegate().visible_row_ranges.last(), + Some(&(0..1)) + ); table.update(cx, |table, cx| { - table.delegate_mut().rows = 0; + table.delegate_mut().rows_count = 0; cx.notify(); }); window.render_frame(cx); - assert_eq!(table.read(cx).delegate().visible_rows.last(), Some(&(0..0))); + assert_eq!( + table.read(cx).delegate().visible_row_ranges.last(), + Some(&(0..0)) + ); }) .unwrap(); } #[gpui_kit::test] fn table_visible_range_stops_at_the_last_row_under_stripe_filler(cx: &mut TestAppContext) { - let (handle, table) = open_ledger(cx, 3, true); + let (handle, table) = open_table_probe(cx, 3, true); cx.update_window(handle, |_, window, cx| { window.render_frame(cx); - let reported = table.read(cx).delegate().visible_rows.clone(); + let reported = table.read(cx).delegate().visible_row_ranges.clone(); assert!(!reported.is_empty()); assert!( reported.iter().all(|range| range.end <= 3),