From 2bac43ac458d29d86d2bbc2943c7f48d68e1f4eb Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Fri, 16 Oct 2026 09:40:26 +0700 Subject: [PATCH 1/2] fix(linux-grid): ask about cell edits on close and stop the close hang GTK 4.22 walks up from the old focus after a focus change and loops forever once the inline field has left its cell, so a field that lost the focus stays hidden in the cell until its next edit. A tab close also moves the focus out of the page first: GTK 4.22 leaks a focus that is hidden and then unparented in one go (fixed in 4.24). --- linux/crates/app/src/ui/browse_tab/mod.rs | 8 + linux/crates/app/src/ui/grid/cell_focus.rs | 36 ++++ linux/crates/app/src/ui/grid/grid_cell.rs | 18 ++ linux/crates/app/src/ui/grid/inline_editor.rs | 16 +- linux/crates/app/src/ui/grid/mod.rs | 2 +- linux/crates/app/src/ui/session_window/mod.rs | 6 +- .../crates/app/src/ui/workspace/close_flow.rs | 54 +++++- .../app/src/ui/workspace/close_tab_tests.rs | 175 ++++++++++++++++++ linux/crates/app/src/ui/workspace/mod.rs | 10 + .../app/src/ui/workspace/workspace_tabs.rs | 1 + 10 files changed, 318 insertions(+), 8 deletions(-) create mode 100644 linux/crates/app/src/ui/workspace/close_tab_tests.rs diff --git a/linux/crates/app/src/ui/browse_tab/mod.rs b/linux/crates/app/src/ui/browse_tab/mod.rs index 7315ecc055..06c32ecb88 100644 --- a/linux/crates/app/src/ui/browse_tab/mod.rs +++ b/linux/crates/app/src/ui/browse_tab/mod.rs @@ -270,6 +270,14 @@ impl BrowseTab { } } + /// Commit the cell edit open in the grid, as Return does, so the close + /// prompts count what was typed. + pub fn commit_cell_edit(&self) { + if let Some(view) = self.read(|model| model.current_column_view.clone()).flatten() { + super::grid::commit_open_edit(&view); + } + } + /// `None` once the tab is closed, and while it is handling an input. fn read(&self, read: impl FnOnce(&BrowseModel) -> T) -> Option { self.imp().model.try_borrow().ok()?.as_ref().map(read) diff --git a/linux/crates/app/src/ui/grid/cell_focus.rs b/linux/crates/app/src/ui/grid/cell_focus.rs index 0c7405638b..26938c863d 100644 --- a/linux/crates/app/src/ui/grid/cell_focus.rs +++ b/linux/crates/app/src/ui/grid/cell_focus.rs @@ -5,6 +5,7 @@ use super::GridCell; use super::cell_edit::begin_edit; use super::edit_navigation::{EditDirection, next_edit_target}; use super::grid_columns::ShownColumn; +use super::inline_editor::commit_inline; use super::selection::row_at; fn window_focus(widget: &impl IsA) -> Option { @@ -57,6 +58,18 @@ pub(super) fn return_focus(cell: &GridCell) { } } +/// Commit the one-line edit open in `view`, as Return does. +pub(crate) fn commit_open_edit(view: >k::ColumnView) { + let Some(cell) = focused_cell(view) else { + return; + }; + if cell.inline_text().is_none() || cell.column_view().as_ref() != Some(view) { + return; + } + commit_inline(&cell); + return_focus(&cell); +} + /// Edit the cell Tab or Shift+Tab goes to from `cell`. With none left /// on the page, the focus stays on `cell`. pub(super) fn edit_next(cell: &GridCell, direction: EditDirection) { @@ -247,6 +260,29 @@ mod tests { assert!(grid.take_messages().is_empty(), "Escape sent the typed text"); } + /// GTK ends the focus change by walking up from the field the edit + /// was in. GTK 4.22 logs a critical forever and never returns when the + /// field has left its cell by then. + #[gtk4::test] + fn the_focus_moving_to_another_cell_commits_the_edit() { + let grid = mixed_rows(); + let name = grid.focus(0, 1); + crate::ui::grid::begin_edit(&name); + name.inline_text().expect("an editor").set_text("a2"); + + let previous = glib::log_set_always_fatal(glib::LogLevels::LEVEL_CRITICAL); + let note = grid.focus(1, 3); + glib::log_set_always_fatal(previous); + + assert!(!name.is_editing(), "the edit outlived the focus"); + assert_eq!(edited_rows(&grid.take_messages()), vec![(Some(key(1)), 1, text("a2"))]); + assert_eq!(grid.focused_cell().as_ref(), Some(¬e)); + assert!( + !name.state_flags().contains(gtk::StateFlags::FOCUS_WITHIN), + "the cell the edit left still shows the focus" + ); + } + /// Another window taking the focus is no focus change in this one, /// so the edit stays open, and typing goes on there once it is back. #[gtk4::test] diff --git a/linux/crates/app/src/ui/grid/grid_cell.rs b/linux/crates/app/src/ui/grid/grid_cell.rs index 30e3f371f9..33809fd70e 100644 --- a/linux/crates/app/src/ui/grid/grid_cell.rs +++ b/linux/crates/app/src/ui/grid/grid_cell.rs @@ -30,6 +30,9 @@ mod imp { pub content: OnceCell, /// The one-line editor, laid over the hidden display while open. pub inline: RefCell>, + /// A one-line field that lost the focus, hidden until the cell's + /// next edit. + pub closed_field: RefCell>, /// An open editor popover. Unbind pops it down, or the recycled /// cell is finalized with a GtkPopover child still in it. pub popover: RefCell>, @@ -69,6 +72,9 @@ mod imp { if let Some(editor) = self.inline.take() { editor.widget().unparent(); } + if let Some(field) = self.closed_field.take() { + field.unparent(); + } if let Some(popover) = self.popover.take() { popover.unparent(); } @@ -292,6 +298,18 @@ impl GridCell { self.imp().inline.take() } + pub(super) fn keep_closed_field(&self, field: gtk::Text) { + if let Some(previous) = self.imp().closed_field.replace(Some(field)) { + previous.unparent(); + } + } + + pub(super) fn remove_closed_field(&self) { + if let Some(field) = self.imp().closed_field.take() { + field.unparent(); + } + } + /// Parent `popover` to the cell and keep it there until it closes. /// A popover the user closes ends its edit; one the editor took back /// first has already dealt with it. diff --git a/linux/crates/app/src/ui/grid/inline_editor.rs b/linux/crates/app/src/ui/grid/inline_editor.rs index 11beea5b6f..67d097dc90 100644 --- a/linux/crates/app/src/ui/grid/inline_editor.rs +++ b/linux/crates/app/src/ui/grid/inline_editor.rs @@ -30,6 +30,7 @@ pub struct InlineEditor { impl InlineEditor { pub(super) fn begin(cell: &GridCell, snapshot: EditSnapshot, initial: &str, options: InlineOptions) { cell.cancel_edit(); + cell.remove_closed_field(); let text = gtk::Text::new(); text.set_hexpand(true); text.set_margin_start(8); @@ -55,11 +56,18 @@ impl InlineEditor { &self.text } - /// Take the field out of the cell and show the display again, with - /// the text the field held. + /// Show the display again, with the text the field held. GTK ends a + /// focus change by walking up from the widget that lost the focus, and + /// GTK 4.22 loops forever when that widget has no parent by then, so a + /// field that lost it stays in the cell, hidden, until the next edit. pub(super) fn close(self, cell: &GridCell) -> String { let typed = self.text.text().to_string(); - self.text.unparent(); + if holds_window_focus(self.text.upcast_ref()) { + self.text.unparent(); + } else { + self.text.set_visible(false); + cell.keep_closed_field(self.text); + } if let Some(display) = cell.content() { display.set_visible(true); } @@ -72,7 +80,7 @@ pub(super) fn commit_inline(cell: &GridCell) { finish_inline(cell, true); } -/// End the cell's one-line edit. The field leaves before the commit is +/// End the cell's one-line edit. The field closes before the commit is /// sent, so the redraw the commit causes meets no open editor. fn finish_inline(cell: &GridCell, commit: bool) { let Some(editor) = cell.take_inline() else { diff --git a/linux/crates/app/src/ui/grid/mod.rs b/linux/crates/app/src/ui/grid/mod.rs index 88a5aa631d..cd039973fe 100644 --- a/linux/crates/app/src/ui/grid/mod.rs +++ b/linux/crates/app/src/ui/grid/mod.rs @@ -50,7 +50,7 @@ mod value_order; #[cfg(test)] pub(crate) use self::cell_edit::begin_edit; pub(crate) use self::cell_editability::cell_editability; -pub(crate) use self::cell_focus::edit_row; +pub(crate) use self::cell_focus::{commit_open_edit, edit_row}; pub(crate) use self::cell_lock::CellLock; pub(crate) use self::cell_preset::{CellPreset, cell_presets}; pub(crate) use self::column_factory::{GridSorting, build_column_view}; diff --git a/linux/crates/app/src/ui/session_window/mod.rs b/linux/crates/app/src/ui/session_window/mod.rs index acea2afd5f..30ae2db2c2 100644 --- a/linux/crates/app/src/ui/session_window/mod.rs +++ b/linux/crates/app/src/ui/session_window/mod.rs @@ -290,8 +290,12 @@ impl SessionWindow { } } + /// What the close prompts ask about, with a value typed into a cell + /// counted among the unsaved changes. fn workspace_handle(&self) -> Option { - self.workspace().and_then(|workspace| workspace.handle()) + let workspace = self.workspace()?; + workspace.commit_cell_edits(); + workspace.handle() } fn handle_input(&self, message: SessionWindowInput) { diff --git a/linux/crates/app/src/ui/workspace/close_flow.rs b/linux/crates/app/src/ui/workspace/close_flow.rs index 298b723f56..7ac36e62ad 100644 --- a/linux/crates/app/src/ui/workspace/close_flow.rs +++ b/linux/crates/app/src/ui/workspace/close_flow.rs @@ -1,8 +1,10 @@ use std::collections::HashSet; use std::sync::Arc; +use gtk4 as gtk; use gtk4::glib; -use gtk4::prelude::*; +use libadwaita as adw; +use libadwaita::prelude::*; use tablepro_core::EditorSession; use uuid::Uuid; @@ -10,6 +12,7 @@ use super::unsaved_icon::apply_unsaved_icon; use super::workspace_persistence::WorkspaceTabs; use super::{WorkspaceInput, WorkspaceModel, WorkspaceTab}; use crate::runtime::DeliverInput; +use crate::ui::browse_tab::BrowseTab; use crate::workspace::CloseDecision; /// Every editor tab's connection, taken from its tab. Each tab's run is @@ -61,9 +64,26 @@ impl WorkspaceModel { take_editor_sessions(&self.workspace_tabs) } + /// The table tabs, or only `id` when it is one. + pub(super) fn browse_tabs(&self, id: Option) -> Vec { + self.workspace_tabs + .borrow() + .iter() + .filter(|(tab, _)| id.is_none_or(|id| id == **tab)) + .filter_map(|(_, tab)| match tab { + WorkspaceTab::Table(slot) => Some(slot.browse.clone()), + WorkspaceTab::Editor(_) | WorkspaceTab::Structure(_) => None, + }) + .collect() + } + /// A tab asked to close. One with nothing to lose closes now; any - /// other waits for the prompts. + /// other waits for the prompts. A value typed into a cell is an + /// unsaved change by then, as it is once the focus leaves the cell. pub(super) fn close_workspace_tab_by_id(&mut self, id: Uuid) { + for browse in self.browse_tabs(Some(id)) { + browse.commit_cell_edit(); + } let tab_view = self.tab_view.clone(); if self.close_guards.may_close_tab(&self.workspace_handle, id) { self.finish_close_workspace_tab(id, &tab_view); @@ -82,6 +102,36 @@ impl WorkspaceModel { }); } + /// Take the keyboard focus out of `page` before it closes. The page + /// the view selects next is selected first, so the view hands it the + /// focus while `page` is still whole. GTK 4.22 leaks a focus that is + /// hidden and then unparented in one go, which the close does when + /// that page takes no focus. + pub(super) fn move_focus_off(&self, page: &adw::TabPage) { + let Some(window) = self.window() else { + return; + }; + let child = page.child(); + let holds_focus = || { + gtk::prelude::GtkWindowExt::focus(&window).is_some_and(|focus| focus == child || focus.is_ancestor(&child)) + }; + if !holds_focus() { + return; + } + // The documented choice for a page with no parent, and this + // workspace never gives one. The last page leaves the focus to + // the empty page the stack shows next. + if page.is_selected() && !self.tab_view.select_next_page() && !self.tab_view.select_previous_page() { + return; + } + // Where GTK moves a focus left in a removed widget, from the + // window's first control. + if holds_focus() { + gtk::prelude::GtkWindowExt::set_focus(&window, None::<>k::Widget>); + window.child_focus(gtk::DirectionType::TabForward); + } + } + pub(super) fn on_tab_close_decided(&mut self, id: Uuid, approved: bool) { let tab_view = self.tab_view.clone(); if approved { diff --git a/linux/crates/app/src/ui/workspace/close_tab_tests.rs b/linux/crates/app/src/ui/workspace/close_tab_tests.rs new file mode 100644 index 0000000000..4aca3f6181 --- /dev/null +++ b/linux/crates/app/src/ui/workspace/close_tab_tests.rs @@ -0,0 +1,175 @@ +use gtk4 as gtk; +use gtk4::glib; +use gtk4::prelude::*; +use gtk4::subclass::prelude::*; +use tablepro_core::column::{ColumnDefault, ColumnType}; +use tablepro_core::meta::{RowCountEstimate, RowIdentity, TableEstimate}; +use tablepro_core::{ColumnInfo, QueryResult, Value}; +use tablepro_test_support::{ConnectionScript, FakeReply}; +use uuid::Uuid; + +use super::{OpenMode, WorkspaceInput}; +use crate::runtime::DeliverInput; +use crate::test_support::{after_next_frame, descendants, drain_main_context, wait_until}; +use crate::ui::browse_tab::BrowseTab; +use crate::ui::grid::GridCell; +use crate::ui::session_window::tests::{TestWindow, WAIT}; +use crate::workspace::{answer, visible_alert}; + +fn column(name: &str, primary_key: bool) -> ColumnInfo { + ColumnInfo { + name: name.to_owned(), + column_type: ColumnType::untyped_text("text"), + nullable: !primary_key, + primary_key, + is_auto_increment: false, + is_generated: false, + default: ColumnDefault::None, + comment: None, + } +} + +/// Every table has the same two keyed rows. +fn serving_tables(script: &mut ConnectionScript) { + let columns = [column("id", true), column("name", false)]; + script.fetch_columns.always(FakeReply::ok(columns.to_vec())); + script.row_identity.always(FakeReply::ok(RowIdentity::UniqueKey { + columns: vec!["id".to_owned()], + })); + script.count_rows.always(FakeReply::ok(2)); + script.table_estimate.always(FakeReply::ok(TableEstimate { + rows: RowCountEstimate::Approximate(2), + heap_blocks: None, + })); + script.query_bound.always(FakeReply::ok(QueryResult::new( + columns.iter().map(ColumnInfo::result_column).collect::>(), + vec![ + vec![Value::Int(1), Value::Text("Ada".to_owned())], + vec![Value::Int(2), Value::Text("Grace".to_owned())], + ], + ))); +} + +/// Open `name` in a tab of its own, which comes to the front. +fn open_table(test: &TestWindow, name: &str) -> (Uuid, BrowseTab) { + let workspace = test.workspace(); + let before = workspace.selected_tab(); + workspace + .input() + .expect("the workspace's input") + .deliver(WorkspaceInput::SelectTable { + schema: None, + name: name.to_owned(), + open_mode: OpenMode::NewTab, + }); + let mut id = None; + wait_until(WAIT, || { + id = workspace.selected_tab().filter(|tab| Some(*tab) != before); + id.is_some() + }) + .expect("the table's tab"); + let tab = workspace + .imp() + .tab_view + .selected_page() + .and_then(|page| page.child().downcast::().ok()) + .expect("a table tab in front"); + (id.expect("the tab's id"), tab) +} + +/// Give the keyboard focus to the name of the first row. +fn focus_name(test: &TestWindow, tab: &BrowseTab) -> GridCell { + let mut grid = None; + wait_until(WAIT, || { + grid = descendants(tab) + .into_iter() + .find_map(|widget| widget.downcast::().ok()) + .filter(|view| view.model().is_some_and(|model| model.n_items() == 2)); + grid.is_some() + }) + .expect("the rows"); + let view = grid.expect("the grid"); + let name = crate::ui::grid::data_columns(&view) + .into_iter() + .nth(1) + .expect("the name column"); + view.scroll_to(0, Some(&name), gtk::ListScrollFlags::FOCUS, None); + view.grab_focus(); + let focused = || gtk::prelude::GtkWindowExt::focus(&test.window).and_then(|focus| GridCell::from_focus(&focus)); + wait_until(WAIT, || { + focused().is_some_and(|cell| cell.position() == Some(0) && cell.column() == 1) + }) + .expect("the cell takes the focus"); + focused().expect("the focused cell") +} + +/// What was typed is an unsaved change like any other, so the close asks +/// about it. GTK 4.22 loops forever, logging a critical, when the field +/// leaves its cell while the focus change is under way. +#[gtk4::test] +fn closing_a_tab_while_a_cell_is_edited_asks_about_the_edit() { + let test = TestWindow::connected_with("CloseWhileEditing", serving_tables); + let (id, tab) = open_table(&test, "users"); + let cell = focus_name(&test, &tab); + crate::ui::grid::begin_edit(&cell); + cell.inline_text().expect("a one-line field").set_text("Ada Lovelace"); + + let previous = glib::log_set_always_fatal(glib::LogLevels::LEVEL_CRITICAL); + WidgetExt::activate_action(&test.window, "win.close-current", None).expect("win.close-current"); + drain_main_context(); + glib::log_set_always_fatal(previous); + + wait_until(WAIT, || visible_alert(&test.window).is_some()).expect("the close asks about the edit"); + let tracker = test.workspace().row_tracker(id).expect("the tab's changes"); + assert_eq!(tracker.pending_count(), 1, "what was typed is not an unsaved change"); + assert!(!cell.is_editing()); + + let closed = tab.downgrade(); + drop((tab, cell)); + assert!(answer(&test.window, "discard")); + wait_until(WAIT, || test.workspace().row_tracker(id).is_none()).expect("the tab closes"); + after_next_frame(&test.window); + wait_until(WAIT, || closed.upgrade().is_none()).expect("the closed tab is freed"); +} + +#[gtk4::test] +fn closing_the_window_while_a_cell_is_edited_asks_about_the_edit() { + let test = TestWindow::connected_with("CloseWindowWhileEditing", serving_tables); + let (id, tab) = open_table(&test, "users"); + let cell = focus_name(&test, &tab); + crate::ui::grid::begin_edit(&cell); + cell.inline_text().expect("a one-line field").set_text("Ada Lovelace"); + + test.window.close(); + + wait_until(WAIT, || visible_alert(&test.window).is_some()).expect("the close asks about the edit"); + let tracker = test.workspace().row_tracker(id).expect("the tab's changes"); + assert_eq!(tracker.pending_count(), 1, "what was typed is not an unsaved change"); + assert!(answer(&test.window, "cancel")); + drain_main_context(); + assert_eq!(test.app.windows().len(), 1, "Cancel closed the window"); +} + +/// The focus leaves the tab before it goes. GTK 4.22 leaks a focus that +/// is hidden and then unparented in one go, with the tab around it, which +/// the close does when the tab shown next takes no focus. +#[gtk4::test] +fn a_tab_closed_with_the_focus_in_it_is_freed_when_the_next_takes_none() { + let test = TestWindow::connected_with("CloseFocusedTab", serving_tables); + let (_, next) = open_table(&test, "orders"); + next.set_can_focus(false); + let (_, tab) = open_table(&test, "users"); + drop(focus_name(&test, &tab)); + let closed = tab.downgrade(); + drop(tab); + + WidgetExt::activate_action(&test.window, "win.close-current", None).expect("win.close-current"); + drain_main_context(); + after_next_frame(&test.window); + + wait_until(WAIT, || closed.upgrade().is_none()).expect("the closed tab is freed"); + assert!( + gtk::prelude::GtkWindowExt::focus(&test.window).is_some_and(|focus| focus.root().is_some()), + "the focus was left nowhere" + ); +} diff --git a/linux/crates/app/src/ui/workspace/mod.rs b/linux/crates/app/src/ui/workspace/mod.rs index 23a3249400..f39644dee9 100644 --- a/linux/crates/app/src/ui/workspace/mod.rs +++ b/linux/crates/app/src/ui/workspace/mod.rs @@ -3,6 +3,8 @@ //! disconnect drops it and the window shows the connection picker. mod close_flow; +#[cfg(test)] +mod close_tab_tests; mod editor_label; mod finished_work; mod health_banner; @@ -698,6 +700,14 @@ impl Workspace { self.read(|model| model.workspace_handle.clone()) } + /// Make a value typed into a cell an unsaved change, as the focus + /// leaving the cell does, so the close prompts ask about it. + pub(crate) fn commit_cell_edits(&self) { + for browse in self.read(|model| model.browse_tabs(None)).unwrap_or_default() { + browse.commit_cell_edit(); + } + } + /// The exports running from this workspace. pub(crate) fn exports(&self) -> Option> { self.read(|model| model.exports.clone()) diff --git a/linux/crates/app/src/ui/workspace/workspace_tabs.rs b/linux/crates/app/src/ui/workspace/workspace_tabs.rs index 5d1a44680d..70fc3c1883 100644 --- a/linux/crates/app/src/ui/workspace/workspace_tabs.rs +++ b/linux/crates/app/src/ui/workspace/workspace_tabs.rs @@ -213,6 +213,7 @@ impl WorkspaceModel { } let page = removed.page().clone(); self.index.remove(&page); + self.move_focus_off(&page); tab_view.close_page_finish(&page, true); drop(removed); self.persist_workspace_state(); From fb072a92bf8e91c8136d0ccf5a31dba6a2ae654f Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Fri, 16 Oct 2026 09:57:20 +0700 Subject: [PATCH 2/2] build(linux): list the close tab tests in POTFILES.in --- linux/po/POTFILES.in | 1 + 1 file changed, 1 insertion(+) diff --git a/linux/po/POTFILES.in b/linux/po/POTFILES.in index 921db33cc1..abead6abc7 100644 --- a/linux/po/POTFILES.in +++ b/linux/po/POTFILES.in @@ -428,6 +428,7 @@ crates/app/src/ui/tab_load/stale_banner.rs crates/app/src/ui/welcome_view.rs crates/app/src/ui/window_geometry.rs crates/app/src/ui/workspace/close_flow.rs +crates/app/src/ui/workspace/close_tab_tests.rs crates/app/src/ui/workspace/editor_label.rs crates/app/src/ui/workspace/finished_work.rs crates/app/src/ui/workspace/health_banner.rs