Skip to content

table: Read and write the selection as one TableSelection value - #3143

Merged
huacnlee merged 8 commits into
longbridge:mainfrom
violetpurpleish:codex/fix-table-selection-getters
Sep 20, 2026
Merged

huacnlee merged 8 commits into
longbridge:mainfrom
violetpurpleish:codex/fix-table-selection-getters

Conversation

@violetpurpleish

@violetpurpleish violetpurpleish commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Description

After selecting a cell and then a row or column, selected_cell() still returned the old cell, contrary to its documentation. The other getters also exposed cached selections from inactive modes, and render_row read the raw row field, so a row left behind by a cell or column selection kept aria_selected, its forced bottom border and hover suppression while looking unselected.

The selection is one fact with four shapes, so this PR exposes it that way:

  • TableSelection mirrors the three select events, and selection() / set_selection() read and write it as one value.
  • selected_row(), selected_col() and selected_cell() are Some only when their own kind of thing is selected. A selected cell does not surface through selected_row(); callers that want its row map it from selected_cell() or match on selection().
  • Every internal "is this selected" check goes through the getters, so the rule lives in one place. Row and column highlighting stay gated on their own mode.
  • The internal cached positions are unchanged and keep serving keyboard navigation across mode changes.

The native DataTable story no longer displays a stale cell 5:3 footer after selecting a row or column.

Public API

gpui-component:

  • pub enum TableSelection { None, Row(usize), Column(usize), Cell(usize, usize) } (new): the current selection as one value, mirroring TableEvent::SelectRow / SelectColumn / SelectCell.
  • TableState<D>::selection(&self) -> TableSelection (new): reads the selection as one value; match on it instead of combining the three positional getters.
  • TableState<D>::set_selection(&mut self, selection: TableSelection, cx: &mut Context<Self>) (new): writes the selection as one value; dispatches to the matching set_selected_* or clear_selection call so scrolling and events are unchanged.
  • TableState<D>::selected_row(&self) -> Option<usize> (corrected): Some only when a row itself is selected.
  • TableState<D>::selected_col(&self) -> Option<usize> (corrected): Some only when a column itself is selected.
  • TableState<D>::selected_cell(&self) -> Option<(usize, usize)> (corrected): Some only when a cell is selected, as documented.

Breaking Changes

Each positional getter now answers only for its own kind of selection, so a selected cell no longer surfaces through selected_row() or selected_col(), and a row or column selection no longer leaves a stale selected_cell(). Callers that used the getters as a history of inactive selections must retain that history themselves. Signatures and internal navigation caches are unchanged.

 table.set_selected_cell(5, 3, cx);
-let row = table.selected_row();
+let row = table.selected_cell().map(|(row_ix, _)| row_ix);
 table.set_selected_row(2, cx);
-assert_eq!(table.selected_cell(), Some((5, 3)));
+assert_eq!(table.selected_cell(), None);
+assert_eq!(table.selection(), TableSelection::Row(2));

How to Test

  • cargo test -p gpui-kit --features test-support --test collections --locked: 5 passed. table_selection_getters_follow_the_active_mode asserts selection() and the three getters after every mode transition, clear_selection, and a set_selection round trip through every variant; table_retains_navigation_positions_when_selection_mode_changes covers resuming row/column keyboard navigation from the retained positions. Both regressions failed on main before the fix.
  • Story, DataTable: Options > Cell Selectable; Go To > Cell 5:3; select row 5; select cell 5:3 again; select the Name column. The footer displays the cell only during cell selection, and a selected cell highlights neither its row nor its column.
  • cargo clippy -- --deny warnings and cargo fmt --check pass.

AI-assisted implementation with Codex and Claude Code; diff reviewed and regression/native checks run.

🤖 Generated with Claude Code

violetpurpleish and others added 3 commits September 20, 2026 12:27
`render_row` still read the raw `selected_row` field, so a row left
behind by cell or column selection kept `aria_selected`, its forced
bottom border and hover suppression while looking unselected. Use the
getters everywhere the "only counts in its mode" rule applies, including
`has_selection`, and collapse the getters to `Option::filter`.

Drop the trailing `render_frame` from the getter test; nothing consumed
that frame.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…umn getters

A selected cell has a row and a column, so `selected_row()` and
`selected_col()` return them in cell mode instead of `None`. Row and
column highlighting stay gated on their own mode so a selected cell
never lights up its whole row or column.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@huacnlee huacnlee added this to the 0.7.0 milestone Sep 20, 2026
huacnlee and others added 4 commits September 20, 2026 20:48
The selection is one fact with four shapes, so expose it as
`TableState::selection() -> TableSelection` mirroring the select
events. The three positional getters go back to being `Some` only when
their own kind of thing is selected, so `selected_row()` never names a
row that merely contains the selected cell; callers that want that row
map it from `selected_cell()`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…om the getters

Restore `is_row` and `is_column`; each positional getter filters its
own field through them, and `selection()` aggregates the three getters
instead of matching on the mode.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Dispatches to the matching `set_selected_*` or `clear_selection` call so
scrolling and events stay identical; persisting and restoring a selection
is now one read and one write.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@huacnlee huacnlee changed the title table: Expose selections only in the active mode table: Read and write the selection as one TableSelection value Sep 20, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@huacnlee
huacnlee enabled auto-merge (squash) September 20, 2026 13:00
@huacnlee

Copy link
Copy Markdown
Member

Thank you.

@huacnlee
huacnlee merged commit f1a6d11 into longbridge:main Sep 20, 2026
12 checks passed
@violetpurpleish
violetpurpleish deleted the codex/fix-table-selection-getters branch September 20, 2026 14:52
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.

2 participants