diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 50d895f01..43a66e52b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -73,6 +73,8 @@ mod tests_date_picker; #[cfg(test)] mod tests_dialog_fit; #[cfg(test)] +mod tests_dialog_primary; +#[cfg(test)] mod tests_dialogs; #[cfg(test)] mod tests_empty_library; diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index 4702c507f..d35542053 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -1292,17 +1292,17 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { Dialog::Shortcuts => crate::panels::keymap::body(app, ui, &t), }); ui.add_space(4.0); - ui.horizontal(|ui| { + // the confirm on the right with Cancel to its left (macOS and GNOME's order), filled with + // the accent when it is the dialog's primary action. The row spans the width the window + // gives its content this frame (its default width, or its content's if wider), so it + // reaches the edge without widening the window. + let row_w = ui.available_width().max(ui.min_rect().width()).max(1.0); + let row_h = ui.spacing().interact_size.y; + ui.allocate_ui_with_layout(egui::vec2(row_w, row_h), egui::Layout::left_to_right(egui::Align::Center), |ui| { // a model file LightCraft cannot use has nothing to confirm, nor has a dialog that // only shows something (one Close, no Cancel / OK) let unusable_model = matches!(&dlg, Dialog::DenoiseModel { info, .. } | Dialog::FaceModel { info, .. } if info["kind"] == "unsupported"); - let informational = unusable_model - || matches!(dlg, Dialog::LightroomResult { first: None, .. }) - || matches!( - dlg, - Dialog::About | Dialog::Shortcuts | Dialog::Settings { .. } | Dialog::SystemInfo { .. } | Dialog::AllMetadata { .. } | Dialog::WhatsNew - | Dialog::ManageEffects { .. } - ); + let informational = unusable_model || matches!(dlg, Dialog::LightroomResult { first: None, .. }) || only_shows(&dlg); let sam = &app.session.segmenter; let (sam_installed, sam_running, sam_failed) = (sam.installed(), sam.download_status().running, sam.download_status().error.is_some()); // no download location in this build: nothing to offer but the manual install @@ -1313,13 +1313,6 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { Dialog::LightroomResult { .. } => "Close", _ => "Cancel", }; - if !informational { - let r = ui.button(crate::i18n::tr(cancel)); - crate::widgets::register(ui.ctx(), "button:dialogCancel", r.rect); - if r.clicked() { - close = true; - } - } let add_label; let ok = match &dlg { Dialog::Import { opts } => { @@ -1371,7 +1364,37 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { ) // smart-album rules that can't mean anything wait to be fixed && !matches!(&dlg, Dialog::SmartRules { id, rules, .. } if !rules.check_for(&app.session.catalog, id.map(lightcraft_catalog::AlbumId)).is_empty())); - let r = (!ok.is_empty()).then(|| ui.add_enabled(can_confirm, egui::Button::new(crate::i18n::tr(ok)))); + let primary = !informational && can_confirm && confirm_style(&dlg) == ConfirmStyle::Primary; + // (Cancel first, then the confirm: the keyboard goes through them as the eye does) + let width_of = |label: &str| { + let text = egui::WidgetText::from(crate::i18n::tr(label)) + .into_galley(ui, Some(egui::TextWrapMode::Extend), f32::INFINITY, egui::TextStyle::Button) + .size() + .x; + (text + 2.0 * ui.spacing().button_padding.x).max(ui.spacing().interact_size.x) + }; + let gap = ui.spacing().item_spacing.x; + let mut buttons_w = 0.0; + if !informational { + buttons_w += width_of(cancel) + gap; + } + if !ok.is_empty() { + buttons_w += width_of(ok); + } + ui.add_space((ui.available_width() - buttons_w).max(0.0)); + if !informational { + let r = ui.button(crate::i18n::tr(cancel)); + crate::widgets::register(ui.ctx(), "button:dialogCancel", r.rect); + if r.clicked() { + close = true; + } + } + let button = if primary { + egui::Button::new(egui::RichText::new(crate::i18n::tr(ok)).color(egui::Color32::WHITE)).fill(t.accent) + } else { + egui::Button::new(crate::i18n::tr(ok)) + }; + let r = (!ok.is_empty()).then(|| ui.add_enabled(can_confirm, button)); if let Some(r) = &r { crate::widgets::register(ui.ctx(), "button:dialogOk", r.rect); } @@ -2216,3 +2239,36 @@ mod watermark_position_tests { } } } + +/// How a dialog's confirm button looks: [`ConfirmStyle::Primary`] (the accent's fill) when it +/// makes or brings in something, plain when it deletes or removes (never the eye-catching +/// default) or when the dialog only shows something (its Close). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ConfirmStyle { + Primary, + Plain, +} + +pub fn confirm_style(dlg: &Dialog) -> ConfirmStyle { + match dlg { + Dialog::ConfirmDelete { .. } | Dialog::DeleteKeyword { .. } | Dialog::DeleteEffects { .. } | Dialog::RemoveFolder { .. } => { + ConfirmStyle::Plain + } + _ if only_shows(dlg) => ConfirmStyle::Plain, + _ => ConfirmStyle::Primary, + } +} + +/// A dialog that only shows something: one Close, nothing to confirm or cancel. +fn only_shows(dlg: &Dialog) -> bool { + matches!( + dlg, + Dialog::About + | Dialog::Shortcuts + | Dialog::Settings { .. } + | Dialog::SystemInfo { .. } + | Dialog::AllMetadata { .. } + | Dialog::WhatsNew + | Dialog::ManageEffects { .. } + ) +} diff --git a/crates/ui-egui/src/tests_dialog_primary.rs b/crates/ui-egui/src/tests_dialog_primary.rs new file mode 100644 index 000000000..29078d009 --- /dev/null +++ b/crates/ui-egui/src/tests_dialog_primary.rs @@ -0,0 +1,74 @@ +//! A dialog's confirm button reads as its primary action: on the right, Cancel to its left, and +//! filled with the accent when it makes or brings in something (Import, Merge, Save…). A +//! destructive confirm (Delete, Remove) stays plain: never the eye-catching default. + +use std::time::Duration; + +use serde_json::json; + +use crate::headless::Headless; +use crate::panels::dialogs::{ConfirmStyle, confirm_style}; +use crate::{LightcraftApp, Services}; + +const T: Duration = Duration::from_secs(20); + +fn rect(h: &Headless, id: &str) -> egui::Rect { + h.app.widgets.iter().find(|(w, _)| w == id).map(|(_, r)| *r).unwrap_or_else(|| panic!("{id} not on screen")) +} + +fn png(dir: &std::path::Path, name: &str) -> String { + let img = lightcraft_raster::Rgba8 { width: 8, height: 8, data: vec![[90, 120, 160, 255]; 64] }; + let bytes = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&img), &lightcraft_codecs::EncodeMeta::default()).unwrap(); + let p = dir.join(name); + std::fs::write(&p, bytes).unwrap(); + p.to_string_lossy().to_string() +} + +#[test] +fn the_import_review_puts_its_confirm_on_the_right_as_the_primary_action() { + // Given the import review of two photos + let dir = std::env::temp_dir().join(format!("lc-ui-dialog-primary-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let paths = [png(&dir, "a.png"), png(&dir, "b.png")]; + let app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Services::default()); + let mut h = Headless::new(app, [1400.0, 1000.0], 1.0); + h.step(); + h.app.run("file.addPhotos", json!({"paths": paths})).unwrap(); + h.step_until(T, |h| h.app.widgets.iter().any(|(w, _)| w == "button:dialogOk")); + h.step(); + // Then "Import 2 Photos" is right of Cancel, at the dialog's right edge + let (ok, cancel, window) = (rect(&h, "button:dialogOk"), rect(&h, "button:dialogCancel"), rect(&h, "dialog:window")); + assert!(ok.left() > cancel.right(), "confirm {ok:?} right of cancel {cancel:?}"); + assert!(window.right() - ok.right() < 40.0, "confirm {ok:?} at the right edge of {window:?}"); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn constructive_confirms_are_primary_and_destructive_ones_plain() { + use crate::state::Dialog; + assert_eq!(confirm_style(&Dialog::AutoStack { gap: 60.0 }), ConfirmStyle::Primary); + assert_eq!(confirm_style(&Dialog::ConfirmDelete { count: 3 }), ConfirmStyle::Plain); + // a dialog that only shows something has nothing to confirm: its Close is plain + assert_eq!(confirm_style(&Dialog::About), ConfirmStyle::Plain); +} + +#[test] +fn a_dialog_wider_than_its_content_still_ends_its_footer_at_its_edge() { + // Auto-Stack's title makes it wider than its slider and note: the buttons go to the edge, and + // the window doesn't grow for them + let app = LightcraftApp::new(lightcraft_engine::Session::with_demo(), Services::default()); + let mut h = Headless::new(app, [1400.0, 1000.0], 1.0); + h.step(); + h.app.run("dialog.autoStack", json!({})).unwrap(); + for _ in 0..4 { + h.step(); + } + let (ok, window) = (rect(&h, "button:dialogOk"), rect(&h, "dialog:window")); + assert!(window.right() - ok.right() < 24.0, "confirm {ok:?} at the right edge of {window:?}"); + let width = window.width(); + for _ in 0..4 { + h.step(); + } + assert_eq!(rect(&h, "dialog:window").width(), width, "the footer doesn't widen the window"); +} diff --git a/docs/whats-new.md b/docs/whats-new.md index b3b1c37ef..e52e09f99 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -130,6 +130,11 @@ script, the control channel or MCP is now refused (issue #1506). It used to act on the selected photos instead, or on the part of the list it could read, so a mistyped id could rate, move or delete the wrong photos. +### A dialog's confirm button says it's the next step +- In dialogs, the confirm button is on the right with Cancel to its left, and filled with the accent when it makes or + brings in something (Import N Photos, Merge, Save, Create, Download…). Delete and Remove stay plain, so a destructive + choice is never the eye-catching default. + ### Hard-edged Heal and Clone spots - A spot with Feather at 0 no longer draws black pixels exactly on its radius. This also fixes spots at 0% Opacity changing pixels along their edge. The hard edge now leaves pixels on the radius unchanged.