Skip to content

Commit d900786

Browse files
datlechinclaude
andcommitted
fix(structure): double title bar + 19 phantom pending changes
Two bugs surfaced from a screenshot of the Edit-mode Structure tab on a freshly-loaded MySQL "users" table: 1. Double title bar — the embedded adw::HeaderBar inside structure_tab inherited GTK4's default show_start_title_buttons / show_end_title _buttons = true and rendered its own min / max / close trio under the AdwApplicationWindow's real chrome. Fix: explicit .show_start_title_buttons(false).show_end_title_buttons(false) on the embedded header. 2. "19 pending changes" before any user input. Cause: GTK fires `changed` and `toggled` synchronously during widget set-up — when rebuild_columns_view tears down + recreates the row widgets, every Entry::set_text / CheckButton::set_active for the loaded values echoes back through the row's connect_* handlers, which treat them as fresh user edits and stamp AlterColumn ops onto the tracker. For 7 columns × ~3 fields each that's exactly the 19 phantom ops the user saw. Fix is two-pronged: - rebuild_columns_view sets the existing suppress_emit flag to true before the rebuild, schedules glib::idle_add_local_once to reset it once the initial signal storm drains. Every signal handler in build_column_row now checks suppress_emit and returns early when it's true, so widget set-up stays silent. - ColumnEdited handler now skips the tracker push entirely when the post-edit DraftColumn matches the prev value (no actual change). When the user reverts an existing column to its original ColumnInfo, retain_ops strips any prior AlterColumn ops on that column so the dirty-count goes back to zero. Defence-in-depth — even if a suppress-flag race ever leaks an event, a no-op edit can no longer pollute the tracker. The third reported anomaly (id PK column showing Nullable=✓) was a downstream consequence of bug #2: a phantom Nullable(true) op fired during rebuild flipped the model's nullable=true for the id row. With #2 fixed, the checkbox now reflects MySQL's actual NOT NULL state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent af5b2d8 commit d900786

1 file changed

Lines changed: 115 additions & 25 deletions

File tree

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

Lines changed: 115 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -266,11 +266,28 @@ impl StructureTab {
266266
// outer columns_box holds (ListBox + Add button) so the rows
267267
// get the standard `.boxed-list` styling: rounded corners,
268268
// separators between rows, hover highlight.
269+
//
270+
// suppress_emit must be true while we tear down + recreate
271+
// the row widgets: Entry::set_text and CheckButton::set_active
272+
// for the initial values fire `changed` / `toggled` signals
273+
// synchronously, and the row's connect_* callbacks (registered
274+
// earlier in the build) would treat those as user edits and
275+
// push phantom AlterColumn ops onto the tracker — every Edit-
276+
// mode reload would land 7 columns × ~3 fields ≈ 19 spurious
277+
// pending changes. We re-enable emit on the next idle tick so
278+
// legitimate user input afterwards flows through.
279+
*self.suppress_emit.borrow_mut() = true;
269280
clear_box(&self.columns_box);
270281
let driver_id = self.driver_id.clone();
271282
let list = boxed_list();
272283
for (i, col) in self.columns.borrow().iter().enumerate() {
273-
let row = wrap_in_list_row(build_column_row(i, col, &driver_id, sender.clone()));
284+
let row = wrap_in_list_row(build_column_row(
285+
i,
286+
col,
287+
&driver_id,
288+
sender.clone(),
289+
self.suppress_emit.clone(),
290+
));
274291
list.append(&row);
275292
}
276293
self.columns_box.append(&list);
@@ -282,6 +299,10 @@ impl StructureTab {
282299
let sender_for_add = sender.clone();
283300
add_button.connect_clicked(move |_| sender_for_add.input(StructureTabInput::AddColumn));
284301
self.columns_box.append(&wrap_button_in_row(add_button));
302+
let suppress = self.suppress_emit.clone();
303+
relm4::gtk::glib::idle_add_local_once(move || {
304+
*suppress.borrow_mut() = false;
305+
});
285306
}
286307

287308
fn rebuild_indexes_view(&self, sender: ComponentSender<Self>) {
@@ -378,11 +399,18 @@ fn clear_box(b: &gtk::Box) {
378399
/// understands why they can't edit. Non-original (newly added) columns
379400
/// always allow full editing — those become AddColumn ops which SQLite
380401
/// accepts at execution time.
402+
///
403+
/// `suppress_emit` lets the caller mark a window during which signal
404+
/// callbacks should NOT push edits onto the change tracker. Used by
405+
/// rebuild_columns_view to silence the spurious `changed` / `toggled`
406+
/// emissions GTK fires while we set initial values on freshly-built
407+
/// widgets.
381408
fn build_column_row(
382409
index: usize,
383410
col: &DraftColumn,
384411
driver_id: &str,
385412
sender: ComponentSender<StructureTab>,
413+
suppress_emit: Rc<RefCell<bool>>,
386414
) -> gtk::Widget {
387415
let row = gtk::Box::builder()
388416
.orientation(gtk::Orientation::Horizontal)
@@ -404,7 +432,11 @@ fn build_column_row(
404432
.build();
405433
name_entry.set_widget_name(&format!("col-name-{index}"));
406434
let sender_for_name = sender.clone();
435+
let suppress_for_name = suppress_emit.clone();
407436
name_entry.connect_changed(move |e| {
437+
if *suppress_for_name.borrow() {
438+
return;
439+
}
408440
sender_for_name.input(StructureTabInput::ColumnEdited {
409441
index,
410442
field: ColumnField::Name(e.text().to_string()),
@@ -422,7 +454,11 @@ fn build_column_row(
422454
if let Some(entry) = combo_entry(&type_combo) {
423455
entry.set_text(&col.data_type);
424456
let sender_for_type = sender.clone();
457+
let suppress_for_type = suppress_emit.clone();
425458
entry.connect_changed(move |e| {
459+
if *suppress_for_type.borrow() {
460+
return;
461+
}
426462
sender_for_type.input(StructureTabInput::ColumnEdited {
427463
index,
428464
field: ColumnField::Type(e.text().to_string()),
@@ -446,7 +482,11 @@ fn build_column_row(
446482
nullable_check.set_tooltip_text(Some(&crate::tr!("Nullability changes aren't supported by SQLite.")));
447483
}
448484
let sender_for_null = sender.clone();
485+
let suppress_for_null = suppress_emit.clone();
449486
nullable_check.connect_toggled(move |c| {
487+
if *suppress_for_null.borrow() {
488+
return;
489+
}
450490
sender_for_null.input(StructureTabInput::ColumnEdited {
451491
index,
452492
field: ColumnField::Nullable(c.is_active()),
@@ -465,7 +505,11 @@ fn build_column_row(
465505
default_entry.set_tooltip_text(Some(&crate::tr!("Default changes aren't supported by SQLite.")));
466506
}
467507
let sender_for_default = sender.clone();
508+
let suppress_for_default = suppress_emit.clone();
468509
default_entry.connect_changed(move |e| {
510+
if *suppress_for_default.borrow() {
511+
return;
512+
}
469513
let text = e.text().to_string();
470514
let value = if text.is_empty() { None } else { Some(text) };
471515
sender_for_default.input(StructureTabInput::ColumnEdited {
@@ -482,7 +526,11 @@ fn build_column_row(
482526
.tooltip_text(crate::tr!("Primary key"))
483527
.build();
484528
let sender_for_pk = sender.clone();
529+
let suppress_for_pk = suppress_emit.clone();
485530
pk_check.connect_toggled(move |c| {
531+
if *suppress_for_pk.borrow() {
532+
return;
533+
}
486534
sender_for_pk.input(StructureTabInput::ColumnEdited {
487535
index,
488536
field: ColumnField::PrimaryKey(c.is_active()),
@@ -497,7 +545,11 @@ fn build_column_row(
497545
.tooltip_text(crate::tr!("Auto-increment / SERIAL"))
498546
.build();
499547
let sender_for_auto = sender.clone();
548+
let suppress_for_auto = suppress_emit;
500549
auto_check.connect_toggled(move |c| {
550+
if *suppress_for_auto.borrow() {
551+
return;
552+
}
501553
sender_for_auto.input(StructureTabInput::ColumnEdited {
502554
index,
503555
field: ColumnField::AutoIncrement(c.is_active()),
@@ -905,7 +957,16 @@ impl SimpleComponent for StructureTab {
905957
structure_tracker::open_tab(init.tab_id);
906958

907959
// Top header: table-name entry + driver label.
908-
let header = adw::HeaderBar::builder().show_title(false).build();
960+
// Embedded headerbar — Structure tab is inside an AdwTabView,
961+
// not a top-level window. Show neither set of window controls
962+
// (start: app-menu / close on macOS-style; end: minimise /
963+
// maximise / close on Linux). Letting them render here was a
964+
// visible double-title-bar bug because the AdwApplicationWindow
965+
// already paints the real ones.
966+
let header = adw::HeaderBar::builder()
967+
.show_start_title_buttons(false)
968+
.show_end_title_buttons(false)
969+
.build();
909970
let title_box = gtk::Box::builder()
910971
.orientation(gtk::Orientation::Horizontal)
911972
.spacing(8)
@@ -1204,32 +1265,61 @@ impl SimpleComponent for StructureTab {
12041265
}
12051266
let new_col = col.clone();
12061267
drop(cols);
1268+
// Skip when nothing actually changed. The Entry /
1269+
// CheckButton handlers that drive ColumnEdited fire
1270+
// not just on user typing — they also echo on
1271+
// programmatic set_text / set_active during widget
1272+
// teardown / focus shifts / IME events. Without this
1273+
// guard each Refresh + interaction can stamp a string
1274+
// of phantom AlterColumn ops onto the tracker.
1275+
if prev == new_col {
1276+
self.regenerate_sql_preview();
1277+
return;
1278+
}
12071279
if matches!(mode, StructureMode::New) {
12081280
self.update_create_op_for_new();
1209-
} else if let Some(_orig) = new_col.original.as_ref() {
1210-
// Existing column: push AlterColumn op (materialize
1211-
// groups all alter ops per column for MySQL).
1212-
let table = self.table_name.borrow().clone();
1213-
let schema = self.schema.clone();
1214-
structure_tracker::with_tab(self.tab_id, |t| {
1215-
t.push(
1216-
StructureOp::AlterColumn {
1217-
schema: schema.clone(),
1218-
table: table.clone(),
1219-
column: new_col.clone(),
1220-
},
1221-
StructureOp::AlterColumn {
1222-
schema: schema.clone(),
1223-
table: table.clone(),
1224-
column: prev.clone(),
1225-
},
1226-
);
1227-
});
1281+
} else if let Some(orig) = new_col.original.as_ref() {
1282+
// Existing column: push AlterColumn op only when
1283+
// the post-edit state actually differs from the
1284+
// baseline ColumnInfo. If the user toggled then
1285+
// toggled back, the model returns to original and
1286+
// the tracker shouldn't carry a no-op op.
1287+
let baseline = DraftColumn::from_info(orig.clone());
1288+
if baseline == new_col {
1289+
// User reverted to original — strip any prior
1290+
// AlterColumn ops on this column from the
1291+
// tracker so dirty-count drops back to zero
1292+
// for this attribute.
1293+
let column_name = new_col.name.clone();
1294+
let table = self.table_name.borrow().clone();
1295+
structure_tracker::with_tab(self.tab_id, |t| {
1296+
let table_for_filter = table.clone();
1297+
t.retain_ops(|op| {
1298+
matches!(op, StructureOp::AlterColumn { table: t, column: c, .. }
1299+
if *t == table_for_filter && c.name == column_name)
1300+
});
1301+
});
1302+
} else {
1303+
let table = self.table_name.borrow().clone();
1304+
let schema = self.schema.clone();
1305+
structure_tracker::with_tab(self.tab_id, |t| {
1306+
t.push(
1307+
StructureOp::AlterColumn {
1308+
schema: schema.clone(),
1309+
table: table.clone(),
1310+
column: new_col.clone(),
1311+
},
1312+
StructureOp::AlterColumn {
1313+
schema: schema.clone(),
1314+
table: table.clone(),
1315+
column: prev.clone(),
1316+
},
1317+
);
1318+
});
1319+
}
12281320
} else {
1229-
// Newly-added column in Edit mode: regenerate the
1230-
// AddColumn op (or rather, update its in-flight
1231-
// payload). We do this by rebuilding the pending
1232-
// ops for added columns each time the user edits.
1321+
// Newly-added column in Edit mode: surgical
1322+
// replace of pending AddColumn ops.
12331323
self.update_added_columns_ops();
12341324
}
12351325
self.regenerate_sql_preview();

0 commit comments

Comments
 (0)