Skip to content

Commit 1f3d0a9

Browse files
committed
fix(grid): column sort beyond 1st click — listen to order-notify too
**Root cause**: `connect_primary_sort_column_notify` in `grid.rs` only fires when the primary sort *column* changes. Clicking the same column a second time keeps the column constant — GTK only flips the direction (Ascending ↔ Descending). The chevron icon updates because GTK manages it internally, but the model never sees a SortChanged dispatch, so `current_sort` stays stale and the next FetchPage emits the same `ORDER BY` clause as the previous one. End result: chevron flips, data doesn't. **Fix**: 1. Wire `connect_primary_sort_order_notify` alongside `connect_primary_sort_column_notify`. Either signal now dispatches SortChanged. 2. Pass the post-click `(col_idx, ascending)` directly in the message instead of toggling at the receiver. Reading off the sorter is idempotent — both notify signals carry the same resolved pair, so receiver short-circuits when it already matches `current_sort`. Without the short-circuit, clicking a different column (which fires BOTH notifies because column changes AND order resets) would have flipped twice. `GridMsg::SortChanged(usize)` → `SortChanged(usize, bool)`. `BrowseTabInput::SortChanged(usize)` → `SortChanged { col_idx, ascending }`. Receiver no longer holds toggle state — sorter is the source of truth.
1 parent bb71907 commit 1f3d0a9

2 files changed

Lines changed: 59 additions & 17 deletions

File tree

linux/crates/app/src/ui/browse_tab.rs

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,10 @@ pub enum BrowseTabInput {
131131
/// User clicked Next page.
132132
NextPage,
133133
/// Sort flipped on column idx (from grid sorter).
134-
SortChanged(usize),
134+
SortChanged {
135+
col_idx: usize,
136+
ascending: bool,
137+
},
135138
/// Page size dropdown changed.
136139
PageSizeChanged(u64),
137140
/// User clicked the Insert button on this tab's paginator bar.
@@ -1348,7 +1351,7 @@ impl SimpleComponent for BrowseTab {
13481351
// outputs that App's forwarder tags with this tab's id.
13491352
let grid_input = sender.input_sender().clone();
13501353
relm4::spawn_local(grid_receiver.forward(grid_input, |msg| match msg {
1351-
GridMsg::SortChanged(idx) => BrowseTabInput::SortChanged(idx),
1354+
GridMsg::SortChanged(col_idx, ascending) => BrowseTabInput::SortChanged { col_idx, ascending },
13521355
GridMsg::CellEdited {
13531356
table,
13541357
row_position,
@@ -1543,12 +1546,18 @@ impl SimpleComponent for BrowseTab {
15431546
let _ = sender.output(BrowseTabOutput::FetchPage);
15441547
let _ = sender.output(BrowseTabOutput::StateChanged);
15451548
}
1546-
BrowseTabInput::SortChanged(col_idx) => {
1547-
let next = match self.current_sort {
1548-
Some((c, asc)) if c == col_idx => Some((c, !asc)),
1549-
_ => Some((col_idx, true)),
1550-
};
1551-
self.current_sort = next;
1549+
BrowseTabInput::SortChanged { col_idx, ascending } => {
1550+
// Idempotent: GtkColumnViewSorter fires both
1551+
// `primary-sort-column` and `primary-sort-order`
1552+
// notifies for one logical click on a different
1553+
// column (column changes; order resets). Each
1554+
// notify dispatches the same post-state pair, so
1555+
// we short-circuit when the pair already matches.
1556+
let next = (col_idx, ascending);
1557+
if self.current_sort == Some(next) {
1558+
return;
1559+
}
1560+
self.current_sort = Some(next);
15521561
self.current_offset = 0;
15531562
self.capture_focus_for_restore();
15541563
let _ = sender.output(BrowseTabOutput::FetchPage);

linux/crates/app/src/ui/grid.rs

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,14 @@ pub type FilterSetter = Rc<dyn Fn(&str)>;
1414

1515
#[derive(Debug)]
1616
pub enum GridMsg {
17-
SortChanged(usize),
17+
/// User clicked a column header. `(col_idx, ascending)` is the
18+
/// resolved post-click state read from the GtkColumnViewSorter
19+
/// — not a "toggle" hint. Reading the sorter directly avoids
20+
/// the receiver having to keep its own toggle state in sync,
21+
/// which broke when both `primary-sort-column` and
22+
/// `primary-sort-order` notify fired for the same logical
23+
/// click (column change resets order; two events, two flips).
24+
SortChanged(usize, bool),
1825
CellEdited {
1926
table: String,
2027
row_position: u32,
@@ -166,17 +173,43 @@ pub fn build_column_view(
166173
.sorter()
167174
.and_then(|s| s.downcast::<gtk::ColumnViewSorter>().ok())
168175
{
169-
view_sorter.connect_primary_sort_column_notify(move |sorter| {
170-
let Some(active) = sorter.primary_sort_column() else {
171-
return;
172-
};
173-
for (idx, col) in columns.iter().enumerate() {
174-
if col == &active {
175-
app_sender.send(GridMsg::SortChanged(idx)).ok();
176-
break;
176+
// Wire BOTH `primary-sort-column` AND `primary-sort-order`
177+
// notify. Listening only to the former misses the case
178+
// where the user clicks the same column a second time:
179+
// GTK keeps `primary-sort-column` constant and just flips
180+
// `primary-sort-order` (Ascending ↔ Descending). The
181+
// chevron updates because GTK manages it internally, but
182+
// without an order-notify listener the model never sees
183+
// SortChanged, `current_sort` stays stale, and the next
184+
// FetchPage issues the same ORDER BY as the previous one.
185+
//
186+
// Reading both column AND order off the sorter (rather
187+
// than dispatching a "toggle" hint) makes the receiver's
188+
// job idempotent: clicking a different column fires both
189+
// notifies in some order, but each carries the post-state
190+
// pair, and the receiver short-circuits when the pair
191+
// already matches `current_sort`.
192+
let dispatch = {
193+
let app_sender = app_sender.clone();
194+
let columns = columns.clone();
195+
move |sorter: &gtk::ColumnViewSorter| {
196+
let Some(active) = sorter.primary_sort_column() else {
197+
return;
198+
};
199+
let ascending = matches!(sorter.primary_sort_order(), gtk::SortType::Ascending);
200+
for (idx, col) in columns.iter().enumerate() {
201+
if col == &active {
202+
app_sender.send(GridMsg::SortChanged(idx, ascending)).ok();
203+
break;
204+
}
177205
}
178206
}
207+
};
208+
view_sorter.connect_primary_sort_column_notify({
209+
let dispatch = dispatch.clone();
210+
move |sorter| dispatch(sorter)
179211
});
212+
view_sorter.connect_primary_sort_order_notify(move |sorter| dispatch(sorter));
180213
}
181214

182215
(column_view, selection, setter)

0 commit comments

Comments
 (0)