From 5e3e83ff8257e19cd2ebfa36336dc6f74f5c2346 Mon Sep 17 00:00:00 2001 From: William Feht Date: Tue, 15 Sep 2026 18:59:46 -0500 Subject: [PATCH 1/2] fix(browser): honor focused button on permanent-delete Enter Permanent-delete confirmation always activated confirm on Enter, even after focus moved to Cancel. Activate the focused button instead, matching paste and archive conflict dialogs. Confirm stays the initial focus. Defer initial grab_focus on delete and conflict prompts so GTK does not show a keyboard highlight until the user moves focus. Closes #854 --- src/ui/browser/archive.rs | 8 +- src/ui/browser/transfer.rs | 8 +- src/ui/browser/trash.rs | 14 +- src/ui/browser/trash/tests.rs | 333 ++++++++++++++++++++++++---------- 4 files changed, 263 insertions(+), 100 deletions(-) diff --git a/src/ui/browser/archive.rs b/src/ui/browser/archive.rs index ea38cdee8..8d5e60eaf 100644 --- a/src/ui/browser/archive.rs +++ b/src/ui/browser/archive.rs @@ -288,7 +288,13 @@ impl ViewState { } }); layer.add_controller(keys); - replace.grab_focus(); + let initial_focus = replace.clone(); + glib::idle_add_local_once(move || { + initial_focus.grab_focus(); + if let Some(window) = initial_focus.root().and_downcast::() { + window.set_focus_visible(false); + } + }); } /// Opens the compress dialog for the selected `entries`. diff --git a/src/ui/browser/transfer.rs b/src/ui/browser/transfer.rs index c22963461..a6a441978 100644 --- a/src/ui/browser/transfer.rs +++ b/src/ui/browser/transfer.rs @@ -531,7 +531,13 @@ impl ViewState { } }); layer.add_controller(escape); - replace.grab_focus(); + let initial_focus = replace.clone(); + glib::idle_add_local_once(move || { + initial_focus.grab_focus(); + if let Some(window) = initial_focus.root().and_downcast::() { + window.set_focus_visible(false); + } + }); } pub(super) fn show_transfer_dialog( diff --git a/src/ui/browser/trash.rs b/src/ui/browser/trash.rs index 6ac3f18a6..1fc1e7c1c 100644 --- a/src/ui/browser/trash.rs +++ b/src/ui/browser/trash.rs @@ -855,14 +855,19 @@ impl ViewState { let escaped_browser = self.browser.clone(); let focused_cancel = cancel.clone(); let focused_confirm = confirm.clone(); + let enter_buttons = [cancel, confirm.clone(), close]; keys.connect_key_pressed(move |_, key, _, modifiers| { if key == gtk::gdk::Key::Escape { dismiss_modal_layer(&escaped_layer, &escaped_overlay, escaped_root.as_ref()); escaped_browser.focus_active(); glib::Propagation::Stop } else if key == gtk::gdk::Key::Return || key == gtk::gdk::Key::KP_Enter { - focused_confirm.emit_clicked(); - glib::Propagation::Stop + if let Some(button) = enter_buttons.iter().find(|button| button.has_focus()) { + button.emit_clicked(); + glib::Propagation::Stop + } else { + glib::Propagation::Proceed + } } else if !modifiers .intersects(gtk::gdk::ModifierType::CONTROL_MASK | gtk::gdk::ModifierType::ALT_MASK) { @@ -871,6 +876,9 @@ impl ViewState { Some(DeleteConfirmationFocus::Confirm) => focused_confirm.grab_focus(), None => return glib::Propagation::Proceed, }; + if let Some(window) = focused_cancel.root().and_downcast::() { + window.set_focus_visible(true); + } glib::Propagation::Stop } else { glib::Propagation::Proceed @@ -881,7 +889,7 @@ impl ViewState { glib::idle_add_local_once(move || { initial_focus.grab_focus(); if let Some(window) = initial_focus.root().and_downcast::() { - window.set_focus_visible(true); + window.set_focus_visible(false); } }); } diff --git a/src/ui/browser/trash/tests.rs b/src/ui/browser/trash/tests.rs index 04c41be27..d320954ea 100644 --- a/src/ui/browser/trash/tests.rs +++ b/src/ui/browser/trash/tests.rs @@ -2,27 +2,12 @@ use super::*; use crate::model::{FileEntry, Location}; - -#[test] -fn delete_confirmation_direction_keys_choose_an_action() { - assert_eq!( - delete_confirmation_focus_target(gtk::gdk::Key::Left), - Some(DeleteConfirmationFocus::Cancel) - ); - assert_eq!( - delete_confirmation_focus_target(gtk::gdk::Key::h), - Some(DeleteConfirmationFocus::Cancel) - ); - assert_eq!( - delete_confirmation_focus_target(gtk::gdk::Key::Right), - Some(DeleteConfirmationFocus::Confirm) - ); - assert_eq!( - delete_confirmation_focus_target(gtk::gdk::Key::l), - Some(DeleteConfirmationFocus::Confirm) - ); - assert_eq!(delete_confirmation_focus_target(gtk::gdk::Key::Tab), None); -} +use crate::ui::browser::{BrowserView, PeekBehavior}; +use gtk::glib; +use gtk::prelude::{GtkWindowExt, IsA}; +use std::path::{Path, PathBuf}; +use std::rc::Rc; +use std::time::{Duration, Instant}; #[test] fn retryable_delete_entries_keeps_only_the_named_locations() { @@ -49,100 +34,187 @@ fn retryable_delete_entries_keeps_only_the_named_locations() { assert_eq!(kept, vec![retryable]); } -#[test] -fn retryable_delete_entries_is_empty_when_nothing_matches() { - let entry = FileEntry { - location: Location::local("/fixture/photo"), - native_name: "photo".into(), - thumbnail_path: None, - display_name: "photo".into(), - kind: crate::model::EntryKind::File, - size: crate::model::MetadataValue::Unknown, - modified_unix_seconds: crate::model::MetadataValue::Unknown, - is_hidden: false, - mode: crate::model::MetadataValue::Unknown, - image_dimensions: crate::model::MetadataValue::Unknown, - child_count: crate::model::MetadataValue::Unknown, - duration_seconds: crate::model::MetadataValue::Unknown, - }; +struct DeleteConfirmation { + view: BrowserView, + window: gtk::Window, + path: PathBuf, + layer: gtk::Widget, + keys: Vec, + cancel: gtk::Button, + confirm: gtk::Button, + close: gtk::Button, +} - let kept = retryable_delete_entries(vec![entry], &[]); +impl DeleteConfirmation { + fn present() -> (tempfile::TempDir, Self) { + let fixture = tempfile::tempdir().expect("fixture"); + let path = fixture.path().join("keep-me.txt"); + std::fs::write(&path, b"payload").expect("temp file"); + let view = BrowserView::new( + Rc::new(crate::adapters::LocalFileSource), + PeekBehavior::default(), + ); + view.set_operation_provider(Rc::new(crate::adapters::LocalOperationProvider)); + let overlay = gtk::Overlay::new(); + overlay.set_child(Some(&view.widget())); + let window = gtk::Window::builder() + .child(&overlay) + .default_width(1000) + .default_height(650) + .build(); + window.present(); + view.state + .show_delete_confirmation(vec![local_file_entry(&path)]); + let root = window.clone().upcast::(); + wait_until( + || { + button(&root, |button| { + button.label().as_deref() == Some(CONFIRM_LABEL) + }) + .is_some() + }, + "delete confirmation should appear", + ); + let confirm = button(&root, |button| { + button.label().as_deref() == Some(CONFIRM_LABEL) + }) + .expect("confirm"); + let cancel = + button(&root, |button| button.label().as_deref() == Some("Cancel")).expect("cancel"); + let close = button(&root, |button| { + button.tooltip_text().as_deref() == Some("Close dialog") + }) + .expect("close"); + wait_until(|| confirm.has_focus(), "confirm should take initial focus"); + let layer = find_widget(&root, &|widget: >k::Widget| { + widget.has_css_class("app-modal-layer") + }) + .expect("modal layer"); + let keys = key_controllers(&layer); + ( + fixture, + Self { + view, + window, + path, + layer, + keys, + cancel, + confirm, + close, + }, + ) + } - assert!(kept.is_empty()); -} + fn press(&self, key: gtk::gdk::Key) { + assert!(press(&self.keys, key), "modal layer should handle {key:?}"); + } -#[test] -fn restore_confirmation_shows_the_full_destination_path() { - assert_eq!( - restore_destination_text(std::path::Path::new( - "/home/user/Documents/Projects/report.txt" - )), - "/home/user/Documents/Projects/report.txt" - ); -} + fn dismissed(&self) -> bool { + self.layer.parent().is_none() + } -#[test] -fn restore_confirmation_names_the_item_count_and_destination_action() { - assert_eq!(restore_confirmation_title(1), "Restore 1 item?"); - assert_eq!(restore_confirmation_title(3), "Restore 3 items?"); - assert_eq!(restore_confirmation_confirm_label(1), "Restore"); - assert_eq!(restore_confirmation_confirm_label(2), "Restore 2 items"); + fn finish(self) { + self.window.destroy(); + self.view.browser().clear_observer(); + } } -#[test] -fn restore_error_summary_includes_the_failure_reason() { - assert_eq!( - restore_error_summary(&[ - "notes.txt: The original location is outside the trash volume and cannot be restored" - .to_owned() - ]), - "notes.txt: The original location is outside the trash volume and cannot be restored" - ); - let summary = restore_error_summary(&["a: denied".to_owned(), "b: denied".to_owned()]); - assert!(summary.starts_with("2 items could not be restored.")); - assert!(summary.contains("a: denied")); -} +const CONFIRM_LABEL: &str = "Permanently delete 1 item"; #[test] -fn delete_confirmation_renders_every_row_for_a_small_selection() { - let entries = (0..7).map(confirmation_entry).collect::>(); - - let (visible, hidden) = delete_confirmation_rows(&entries); - - assert_eq!(visible.len(), 7); - assert_eq!(hidden, 0); - assert_eq!(delete_confirmation_overflow_label(hidden), None); +fn enter_keeps_file_on_cancel() { + crate::test_support::gtk_test( + "ui::browser::trash::tests::enter_keeps_file_on_cancel", + || { + let (_dir, dialog) = DeleteConfirmation::present(); + dialog.press(gtk::gdk::Key::Left); + wait_until( + || dialog.cancel.has_focus() && !dialog.confirm.has_focus(), + "Left should move focus to Cancel", + ); + dialog.press(gtk::gdk::Key::Return); + wait_until( + || dialog.dismissed(), + "Cancel-focused Enter should dismiss the dialog", + ); + drain_past_dismiss_timeout(); + assert!( + dialog.path.exists(), + "Cancel-focused Enter should not delete the file" + ); + dialog.finish(); + }, + ); } #[test] -fn delete_confirmation_caps_rows_and_summarizes_the_rest() { - let entries = (0..1000).map(confirmation_entry).collect::>(); - - let (visible, hidden) = delete_confirmation_rows(&entries); - - assert_eq!(visible.len(), DELETE_CONFIRMATION_MAX_ROWS); - assert_eq!(hidden, 1000 - DELETE_CONFIRMATION_MAX_ROWS); - assert_eq!( - delete_confirmation_overflow_label(hidden), - Some("… and 950 more items".to_owned()) +fn enter_deletes_on_confirm() { + crate::test_support::gtk_test( + "ui::browser::trash::tests::enter_deletes_on_confirm", + || { + let (dir, dialog) = DeleteConfirmation::present(); + assert!( + dialog.confirm.has_focus(), + "confirm should keep initial focus" + ); + dialog.press(gtk::gdk::Key::Return); + wait_until( + || !dialog.path.exists(), + "confirm-focused Enter should permanently delete the file", + ); + wait_until( + || dialog.dismissed(), + "confirm-focused Enter should dismiss the dialog", + ); + assert!( + !trashed_copy_exists(dir.path(), "keep-me.txt"), + "permanent delete should not leave the file in Trash" + ); + dialog.finish(); + }, ); } #[test] -fn delete_confirmation_overflow_label_uses_the_singular_for_one_item() { - assert_eq!( - delete_confirmation_overflow_label(1), - Some("… and 1 more item".to_owned()) +fn enter_keeps_file_on_close() { + crate::test_support::gtk_test( + "ui::browser::trash::tests::enter_keeps_file_on_close", + || { + let (_dir, dialog) = DeleteConfirmation::present(); + assert!( + dialog.close.grab_focus(), + "Close should take focus before Enter" + ); + assert!( + dialog.close.has_focus() && !dialog.confirm.has_focus(), + "Close should own focus before Enter" + ); + dialog.press(gtk::gdk::Key::Return); + wait_until( + || dialog.dismissed(), + "Close-focused Enter should dismiss the dialog", + ); + drain_past_dismiss_timeout(); + assert!( + dialog.path.exists(), + "Close-focused Enter should not delete the file" + ); + dialog.finish(); + }, ); } -fn confirmation_entry(index: usize) -> FileEntry { - let name = format!("file-{index}.txt"); +fn local_file_entry(path: &Path) -> FileEntry { + let name = path + .file_name() + .expect("should have a file name") + .to_os_string(); FileEntry { - location: Location::local(format!("/fixture/{name}")), - native_name: name.clone().into(), + location: Location::local(path), + native_name: name.clone(), thumbnail_path: None, - display_name: name, + display_name: name.to_string_lossy().into_owned(), kind: crate::model::EntryKind::File, size: crate::model::MetadataValue::Unknown, modified_unix_seconds: crate::model::MetadataValue::Unknown, @@ -153,3 +225,74 @@ fn confirmation_entry(index: usize) -> FileEntry { duration_seconds: crate::model::MetadataValue::Unknown, } } + +fn find_widget + glib::object::IsClass>( + root: >k::Widget, + predicate: &impl Fn(&T) -> bool, +) -> Option { + if let Some(widget) = root.downcast_ref::() + && predicate(widget) + { + return Some(widget.clone()); + } + let mut child = root.first_child(); + while let Some(widget) = child { + child = widget.next_sibling(); + if let Some(found) = find_widget(&widget, predicate) { + return Some(found); + } + } + None +} + +fn button(root: >k::Widget, predicate: impl Fn(>k::Button) -> bool) -> Option { + find_widget(root, &|button: >k::Button| { + button.is_visible() && button.is_sensitive() && predicate(button) + }) +} + +fn key_controllers(widget: &impl IsA) -> Vec { + let controllers = widget.observe_controllers(); + (0..controllers.n_items()) + .filter_map(|index| controllers.item(index)) + .filter_map(|controller| controller.downcast::().ok()) + .collect() +} + +fn press(keys: &[gtk::EventControllerKey], key: gtk::gdk::Key) -> bool { + // GTK prepends controllers, so observe order is last-added first. + keys.iter().any(|controller| { + controller.emit_by_name::( + "key-pressed", + &[&key, &0u32, >k::gdk::ModifierType::empty()], + ) + }) +} + +fn drain_past_dismiss_timeout() { + let deadline = Instant::now() + Duration::from_millis(250); + while Instant::now() < deadline { + glib::MainContext::default().iteration(false); + std::thread::sleep(Duration::from_millis(1)); + } +} + +fn wait_until(condition: impl Fn() -> bool, message: &str) { + let deadline = Instant::now() + Duration::from_secs(5); + while !condition() { + assert!(Instant::now() < deadline, "{message}"); + glib::MainContext::default().iteration(false); + std::thread::sleep(Duration::from_millis(1)); + } +} + +fn trashed_copy_exists(root: &Path, name: &str) -> bool { + let mut stack = vec![root.to_path_buf()]; + if let Ok(data_home) = std::env::var("XDG_DATA_HOME") { + stack.push(PathBuf::from(data_home).join("Trash/files")); + } + if let Ok(home) = std::env::var("HOME") { + stack.push(PathBuf::from(home).join(".local/share/Trash/files")); + } + stack.into_iter().any(|dir| dir.join(name).exists()) +} From e777228e87ad595e620fd5c75c35db50d7f1c2b9 Mon Sep 17 00:00:00 2001 From: William Feht Date: Thu, 17 Sep 2026 19:09:33 -0500 Subject: [PATCH 2/2] fix(model): classify Recent URIs without GVfs File calls is_recent_root used GFile::has_uri_scheme and parent(), which SIGSEGV on gphoto2 backends when tests call Location::parent concurrently. --- src/model/mod.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/model/mod.rs b/src/model/mod.rs index 96a10dbb8..09fcf20ec 100644 --- a/src/model/mod.rs +++ b/src/model/mod.rs @@ -26,6 +26,10 @@ pub(crate) fn uri_contains_credentials(uri: &gio::glib::Uri) -> bool { || uri.user().is_some_and(|user| user.contains([':', ';'])) } +fn uri_scheme_eq(uri: &str, scheme: &str) -> bool { + gio::glib::Uri::parse_scheme(uri).is_some_and(|parsed| parsed.eq_ignore_ascii_case(scheme)) +} + impl Location { pub fn local(path: impl Into) -> Self { Self { @@ -56,13 +60,17 @@ impl Location { /// Directory operations must reject virtual children as well as the root. pub fn is_recent_location(&self) -> bool { self.uri_value() - .is_some_and(|uri| gio::File::for_uri(uri).has_uri_scheme("recent")) + .is_some_and(|uri| uri_scheme_eq(uri, "recent")) } pub fn is_recent_root(&self) -> bool { + // Avoid GFile here: GVfs backends can SIGSEGV when tests call File APIs concurrently. self.uri_value().is_some_and(|uri| { - let file = gio::File::for_uri(uri); - file.has_uri_scheme("recent") && file.parent().is_none() + if !uri_scheme_eq(uri, "recent") { + return false; + } + uri.split_once(':') + .is_some_and(|(_, rest)| rest.trim_start_matches('/').is_empty()) }) }