Skip to content

Commit 70df59e

Browse files
committed
fix(linux): clear selection after Delete + correct row lookup with drafts
User-reported: Ctrl+A → Delete leaves the "{n} selected" badge stuck because the rows stay in the model (strikethrough until Save), so the MultiSelection bitset is unchanged and connect_selection_changed never fires. Now the bulk-delete commit closure calls selection.unselect_all() at the end of both branches (immediate + post-confirm), which propagates through the existing chrome-update signal: badge hides, Delete tooltip resets to single-row. Deeper bug uncovered while tracing: row_position arrives in filter-model space (the space GtkColumnView activates against) which includes prepended drafts, but DeleteSelectedRow's snapshot loop and row_key_at both indexed result.rows directly. With N drafts visible at the top of the grid, every "delete row at position P" or "edit cell at position P" silently targeted the wrong persisted row's PK (off by N). Both call sites now resolve through selection.model().item(pos), pull cells via row_object.cells_clone(), and skip rows whose draft_id() is Some — drafts have no PK and need a different teardown path anyway. Affects: right-click context-menu Delete, Ctrl+Shift+N Set NULL, and any cell edit on a persisted row that sits below a draft. Latent until the user inserted a draft + acted on a persisted row in the same session.
1 parent aadcae1 commit 70df59e

1 file changed

Lines changed: 57 additions & 16 deletions

File tree

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

Lines changed: 57 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,11 +1031,23 @@ impl BrowseTab {
10311031
}
10321032

10331033
/// Build the (RowKey, current_values) pair for a row at the
1034-
/// given position in the current result. Returns None if the
1035-
/// table has no PK or the position is out of range.
1034+
/// given position. `row_position` is in filter-model space (the
1035+
/// space `GtkColumnView` activates against), which includes
1036+
/// prepended drafts — so we resolve through the selection model
1037+
/// rather than indexing `result.rows` directly. The earlier
1038+
/// direct-indexing version was off by N when N drafts were
1039+
/// prepended, silently targeting the wrong persisted row's PK
1040+
/// from the right-click delete, Set NULL shortcut, and any
1041+
/// cell-edit on a persisted row that sits below a draft.
1042+
///
1043+
/// Returns None for drafts (caller is expected to handle the
1044+
/// draft path before reaching here), for tables with no PK, or
1045+
/// when the position is out of range.
10361046
fn row_key_at(&self, row_position: u32) -> Option<(crate::services::change_tracker::RowKey, Vec<Value>)> {
1037-
let result = self.current_result.as_ref()?;
1038-
let row = result.rows.get(row_position as usize)?;
1047+
let row_obj = self.row_object_at(row_position)?;
1048+
if row_obj.draft_id().is_some() {
1049+
return None;
1050+
}
10391051
let pk_indices: Vec<usize> = self
10401052
.current_columns
10411053
.iter()
@@ -1046,9 +1058,13 @@ impl BrowseTab {
10461058
if pk_indices.is_empty() {
10471059
return None;
10481060
}
1049-
let pk_values: Vec<Value> = pk_indices.iter().map(|&i| row[i].clone()).collect();
1061+
let cells = row_obj.cells_clone();
1062+
let pk_values: Vec<Value> = pk_indices
1063+
.iter()
1064+
.map(|&i| cells.get(i).cloned())
1065+
.collect::<Option<_>>()?;
10501066
let key = crate::services::change_tracker::RowKey::from_pk_values(&pk_values)?;
1051-
Some((key, row.clone()))
1067+
Some((key, cells))
10521068
}
10531069

10541070
/// Returns true when the loaded columns include at least one PK.
@@ -1822,9 +1838,6 @@ impl SimpleComponent for BrowseTab {
18221838
let Some(selection) = self.current_selection.as_ref() else {
18231839
return;
18241840
};
1825-
let Some(result) = self.current_result.as_ref() else {
1826-
return;
1827-
};
18281841
let positions = selected_positions(selection);
18291842
if positions.is_empty() {
18301843
return;
@@ -1843,30 +1856,58 @@ impl SimpleComponent for BrowseTab {
18431856
});
18441857
return;
18451858
}
1846-
// Snapshot (key, row) pairs now so a background
1847-
// RowsLoaded between dialog open and confirmation
1848-
// can't shift positions out from under us. The tracker
1849-
// applies pending state by key, not by position.
1859+
// Snapshot (key, row) pairs by walking the selection's
1860+
// own model. Earlier we indexed `result.rows[pos]`
1861+
// directly, but `pos` is in filter-model space (which
1862+
// includes prepended drafts), not server-result space.
1863+
// With drafts present the off-by-N mismatch silently
1864+
// tracked the wrong PKs for deletion. Going through
1865+
// the model gives us the actual RowObject at that
1866+
// position, so drafts skip cleanly via draft_id and
1867+
// persisted rows resolve to their real cells.
1868+
let Some(model) = selection.model() else {
1869+
return;
1870+
};
18501871
let snapshot: Vec<(crate::services::change_tracker::RowKey, Vec<Value>)> = positions
18511872
.iter()
18521873
.filter_map(|pos| {
1853-
let row = result.rows.get(*pos as usize)?;
1854-
let pk_values: Vec<Value> = pk_indices.iter().map(|&i| row[i].clone()).collect();
1874+
let item = model.item(*pos)?;
1875+
let row_obj = item.downcast::<super::row_object::RowObject>().ok()?;
1876+
// Drafts are in-memory only; track_delete
1877+
// expects a persisted row. Discarding a draft
1878+
// is a separate flow (Ctrl+Z on the Insert).
1879+
if row_obj.draft_id().is_some() {
1880+
return None;
1881+
}
1882+
let cells = row_obj.cells_clone();
1883+
let pk_values: Vec<Value> = pk_indices
1884+
.iter()
1885+
.map(|&i| cells.get(i).cloned())
1886+
.collect::<Option<_>>()?;
18551887
let key = crate::services::change_tracker::RowKey::from_pk_values(&pk_values)?;
1856-
Some((key, row.clone()))
1888+
Some((key, cells))
18571889
})
18581890
.collect();
18591891
if snapshot.is_empty() {
18601892
return;
18611893
}
18621894
let count = snapshot.len();
18631895
let tab_id = self.tab_id;
1896+
let selection_for_commit = selection.clone();
18641897
let commit_delete = move |snapshot: Vec<(crate::services::change_tracker::RowKey, Vec<Value>)>| {
18651898
crate::services::change_tracker::with_tab(tab_id, |t| {
18661899
for (key, row) in snapshot {
18671900
t.track_delete(key, row);
18681901
}
18691902
});
1903+
// Clear the multi-row selection so the
1904+
// "{n} selected" badge disappears and the Delete
1905+
// button tooltip resets. Without this the bitset
1906+
// still contains the now-strikethrough rows
1907+
// (they remain in the model until Save). Spreadsheet
1908+
// convention: a bulk action ends the selection it
1909+
// operated on.
1910+
selection_for_commit.unselect_all();
18701911
};
18711912
if count >= BULK_DELETE_CONFIRM_THRESHOLD {
18721913
// Dialog parent is any descendant of the toplevel

0 commit comments

Comments
 (0)