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()) }) } 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 51ae8b75e..2f3dd1836 100644 --- a/src/ui/browser/transfer.rs +++ b/src/ui/browser/transfer.rs @@ -532,7 +532,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 e3fa4fa93..cfd70162c 100644 --- a/src/ui/browser/trash/tests.rs +++ b/src/ui/browser/trash/tests.rs @@ -2,6 +2,12 @@ use super::*; use crate::model::{FileEntry, Location}; +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 delete_confirmation_direction_keys_choose_an_action() { @@ -156,3 +162,267 @@ fn confirmation_entry(index: usize) -> FileEntry { 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, +} + +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, + }, + ) + } + + fn press(&self, key: gtk::gdk::Key) { + assert!(press(&self.keys, key), "modal layer should handle {key:?}"); + } + + fn dismissed(&self) -> bool { + self.layer.parent().is_none() + } + + fn finish(self) { + self.window.destroy(); + self.view.browser().clear_observer(); + } +} + +const CONFIRM_LABEL: &str = "Permanently delete 1 item"; + +#[test] +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 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 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 local_file_entry(path: &Path) -> FileEntry { + let name = path + .file_name() + .expect("should have a file name") + .to_os_string(); + FileEntry { + location: Location::local(path), + native_name: name.clone(), + thumbnail_path: None, + 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, + recent_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, + } +} + +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()) +}