Merge pull request #5754 from agent-reed/fix/fill-sign-quick-bar-controls

M5.7: the Fill & Sign text controls use a colour dot and a size flyout

ATTRIBUTION.md

@@ -4,7 +4,7 @@
 
 Every asset PdfCraft includes, bundles or uses to build its published material, with its author, source and licence. The policy is in [AGENTS.md](AGENTS.md) §1. The machine-readable list, with the SHA-256 of every asset, is [ATTRIBUTION.toml](ATTRIBUTION.toml). Licence texts are kept beside the assets and summarised in [NOTICE](NOTICE).
 
-## In this repository (256)
+## In this repository (257)
 
 | Asset | Title | Author | Licence | Source | Used for |
 |---|---|---|---|---|---|
@@ -263,6 +263,7 @@ Every asset PdfCraft includes, bundles or uses to build its published material,
 | `assets/icons/superscript.svg` | Lucide icon "superscript" | Lucide Contributors (portions Cole Bemis, Feather) | ISC AND MIT | https://github.com/lucide-icons/lucide (npm lucide-static 1.49.0, icons/superscript.svg) | UI icon, embedded via crates/ui-egui/src/icon_data.rs |
 | `assets/icons/subscript.svg` | Lucide icon "subscript" | Lucide Contributors (portions Cole Bemis, Feather) | ISC AND MIT | https://github.com/lucide-icons/lucide (npm lucide-static 1.49.0, icons/subscript.svg) | UI icon, embedded via crates/ui-egui/src/icon_data.rs |
 | `assets/icons/pilcrow.svg` | Lucide icon "pilcrow" | Lucide Contributors (portions Cole Bemis, Feather) | ISC AND MIT | https://github.com/lucide-icons/lucide (npm lucide-static 1.49.0, icons/pilcrow.svg) | UI icon, embedded via crates/ui-egui/src/icon_data.rs |
+| `assets/icons/a-large-small.svg` | Lucide icon "a-large-small" | Lucide Contributors (portions Cole Bemis, Feather) | ISC AND MIT | https://github.com/lucide-icons/lucide (npm lucide-static 1.49.0, icons/a-large-small.svg) | UI icon, embedded via crates/ui-egui/src/widgets/icon_data.rs |
 | `assets/icons/move-horizontal.svg` | Lucide icon "move-horizontal" | Lucide Contributors (portions Cole Bemis, Feather) | ISC AND MIT | https://github.com/lucide-icons/lucide (npm lucide-static 1.49.0, icons/move-horizontal.svg) | UI icon, embedded via crates/ui-egui/src/icon_data.rs |
 
 ## Compiled in through dependencies (6)

ATTRIBUTION.toml

@@ -3200,6 +3200,17 @@ kind = "icon"
 usage = "UI icon, embedded via crates/ui-egui/src/icon_data.rs"
 sha256 = "d344d9f9222f6c1b698c0c82a753a2212d9bd7785f06c00cd6e4910fe2fee8f8"
 
+[[asset]]
+path = "assets/icons/a-large-small.svg"
+title = "Lucide icon \"a-large-small\""
+author = "Lucide Contributors (portions Cole Bemis, Feather)"
+source = "https://github.com/lucide-icons/lucide (npm lucide-static 1.49.0, icons/a-large-small.svg)"
+licence = "ISC AND MIT"
+licence_file = "assets/icons/LICENSE-lucide.txt"
+kind = "icon"
+usage = "UI icon, embedded via crates/ui-egui/src/widgets/icon_data.rs"
+sha256 = "d4985fc71174d108e322c3b25cb6abe2837a115fe3f239a1918645db436743ec"
+
 [[asset]]
 path = "assets/icons/move-horizontal.svg"
 title = "Lucide icon \"move-horizontal\""

assets/icons/a-large-small.svg

@@ -0,0 +1,18 @@
+<!-- @license lucide-static v1.49.0 - ISC -->
+<svg
+  class="lucide lucide-a-large-small"
+  xmlns="http://www.w3.org/2000/svg"
+  width="24"
+  height="24"
+  viewBox="0 0 24 24"
+  fill="none"
+  stroke="currentColor"
+  stroke-width="2"
+  stroke-linecap="round"
+  stroke-linejoin="round"
+>
+  <path d="m15 16 2.536-7.328a1.02 1.02 1 0 1 1.928 0L22 16" />
+  <path d="M15.697 14h5.606" />
+  <path d="m2 16 4.039-9.69a.5.5 0 0 1 .923 0L11 16" />
+  <path d="M3.304 13h6.392" />
+</svg>

crates/ui-egui/src/canvas/bars.rs

@@ -315,7 +315,7 @@ mod tests {
 
     #[test]
     fn a_wider_quick_bar_centres_by_its_own_width_and_needs_a_wider_margin() {
-        // Fill & Sign's Text tool widens the bar to 70 pt: it centres by that, and 72 pt no longer holds it.
+        // A 70 pt bar centres by that, and 72 pt no longer holds it.
         assert_eq!(quick_bar_offset(120.0, 70.0), 15.0);
         assert_eq!(quick_bar_offset(72.0, 70.0), 14.0);
     }

crates/ui-egui/src/hotbar.rs

@@ -299,6 +299,30 @@ fn core_tool_buttons(ui: &mut egui::Ui, app: &mut PdfCraftApp, cell: f32, popup_
     });
 }
 
+fn controls_divider(ui: &mut egui::Ui, t: &Tokens, vertical: bool) {
+    let (r, _) = if vertical {
+        ui.allocate_exact_size(vec2(crate::canvas::bars::QUICK_BAR_WIDTH, 9.0), Sense::hover())
+    } else {
+        ui.allocate_exact_size(vec2(9.0, crate::canvas::bars::QUICK_BAR_WIDTH), Sense::hover())
+    };
+    if vertical {
+        ui.painter().hline(r.x_range().shrink(6.0), r.center().y, Stroke::new(1.0, t.divider));
+    } else {
+        ui.painter().vline(r.center().x, r.y_range().shrink(6.0), Stroke::new(1.0, t.divider));
+    }
+}
+
+fn selected_fill_text(app: &PdfCraftApp) -> Option<(usize, usize)> {
+    let (index, id) = app.active_ids()?;
+    let (page, annotation) = app.views.get(index)?.comments.selected?;
+    let doc = app.session.get(id).filter(|doc| doc.allows_annotation())?;
+    doc.info
+        .annotations
+        .iter()
+        .any(|a| a.page == page && a.index == annotation && a.fill_sign && a.subtype == "FreeText" && !a.locked)
+        .then_some((page, annotation))
+}
+
 /// Extra controls rendered below/beside the buttons when a comment or fill tool is active.
 fn active_tool_controls(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens, vertical: bool) {
     let current_fill = match app.quick_tool {
@@ -306,47 +330,44 @@ fn active_tool_controls(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens, ve
         _ => None,
     };
     if matches!(current_fill, Some(crate::fill_sign::FillTool::Text | crate::fill_sign::FillTool::Date)) {
-        if vertical {
-            ui.label(tl!("Font size:"));
-        }
-        let size = ui.add(
-            egui::DragValue::new(&mut app.fill_text_size)
-                .range(crate::fill_sign::MIN_TEXT_SIZE..=crate::fill_sign::MAX_TEXT_SIZE)
-                .speed(0.25)
-                .suffix(" pt"),
-        );
-        if ((size.changed() && !size.dragged()) || size.drag_stopped())
-            && let Some((index, id)) = app.active_ids()
-            && let Some((page, annotation)) = app.views.get(index).and_then(|view| view.comments.selected)
-            && app.session.get(id).is_some_and(|doc| {
-                doc.allows_annotation()
-                    && doc
-                        .info
-                        .annotations
-                        .iter()
-                        .any(|a| a.page == page && a.index == annotation && a.fill_sign && a.subtype == "FreeText" && !a.locked)
-            })
-        {
-            app.apply_edit(Edit::FontSizeAnnotation { page, index: annotation, size: app.fill_text_size });
-        }
-        ui.menu_button(tl!("Colour"), |ui| {
-            if let Some(color) = comments::swatch_grid(ui, Some(app.fill_text_color)) {
-                app.fill_text_color = color;
-                ui.close();
+        controls_divider(ui, t, vertical);
+        if let Some(color) = comments::quick_bar_colour(ui, app.fill_text_color) {
+            app.fill_text_color = color;
+            if let Some((page, index)) = selected_fill_text(app) {
+                app.apply_edit(Edit::StyleAnnotation { page, index, color: Some(color), opacity: None, width: None, endings: None });
             }
-        });
+        }
+        let resp = icons::button(ui, "a-large-small", 32.0, false, tl!("Font size"));
+        let resized = egui::Popup::menu(&resp)
+            .align(egui::RectAlign::RIGHT_START)
+            .close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside)
+            .show(|ui| {
+                ui.set_min_width(180.0);
+                ui.label(egui::RichText::new(tl!("Font size")).font(theme::semibold(12.0)));
+                let size = ui.add(
+                    egui::Slider::new(&mut app.fill_text_size, crate::fill_sign::MIN_TEXT_SIZE..=crate::fill_sign::MAX_TEXT_SIZE)
+                        .logarithmic(true)
+                        .suffix(" pt"),
+                );
+                // #1274: the size also applies to the Fill & Sign text that is selected,
+                // once per change (a typed value, or the end of a drag), so a drag adds
+                // one undo step and not one per frame.
+                (size.changed() && !size.dragged()) || size.drag_stopped()
+            })
+            .is_some_and(|popup| popup.inner);
+        let size = app.fill_text_size;
+        if resized
+            && let Some((page, index)) = selected_fill_text(app)
+            && app
+                .active_ids()
+                .and_then(|(_, id)| app.session.get(id)?.comment_props(page, index)?.font_size)
+                .is_none_or(|stored| format!("{stored:.3}") != format!("{size:.3}"))
+        {
+            app.apply_edit(Edit::FontSizeAnnotation { page, index, size });
+        }
     }
     if let QuickTool::Comment(tool) = app.quick_tool {
-        let (r, _) = if vertical {
-            ui.allocate_exact_size(vec2(crate::canvas::bars::QUICK_BAR_WIDTH, 9.0), Sense::hover())
-        } else {
-            ui.allocate_exact_size(vec2(9.0, crate::canvas::bars::QUICK_BAR_WIDTH), Sense::hover())
-        };
-        if vertical {
-            ui.painter().hline(r.x_range().shrink(6.0), r.center().y, Stroke::new(1.0, t.divider));
-        } else {
-            ui.painter().vline(r.center().x, r.y_range().shrink(6.0), Stroke::new(1.0, t.divider));
-        }
+        controls_divider(ui, t, vertical);
         if let Some(color) = comments::quick_bar_controls(ui, tool, &mut app.comment_prefs)
             && let Some((index, id)) = app.active_ids()
             && let Some((page, annotation)) = app.views.get(index).and_then(|view| view.comments.selected)

crates/ui-egui/src/tools/comments.rs

@@ -41,7 +41,7 @@ pub(crate) use input::{anchor, context, context_hit, describe, hit, keys, moving
 pub use menu::{CanvasAction, fill_picker, recent_custom_colours, remember_custom_colour, status_badge, swatch_grid};
 #[cfg(test)]
 use menu::{RECENT_CUSTOM, same_colour};
-pub(crate) use menu::{attachment_items, context_menu, quick_bar_controls};
+pub(crate) use menu::{attachment_items, context_menu, quick_bar_colour, quick_bar_controls};
 pub(crate) use paint::paint_page;
 #[cfg(test)]
 use paint::{changing_points, drawn_shape, edited_in_view};

crates/ui-egui/src/tools/comments/menu.rs

@@ -286,25 +286,32 @@ pub fn fill_picker(ui: &mut egui::Ui, current: Option<Rgb>) -> Option<Option<Rgb
     picked
 }
 
-/// The comment tools' extra quick-bar controls: pin, colour, fill, opacity and thickness.
-pub(crate) fn quick_bar_controls(ui: &mut egui::Ui, tool: CommentTool, prefs: &mut CommentPrefs) -> Option<Rgb> {
-    let style = prefs.style(tool);
-    let mut selected_color = None;
-    if icons::button(ui, "pin", 32.0, prefs.pinned, if prefs.pinned { tl!("Keep tool selected: on") } else { tl!("Keep tool selected") }).clicked() {
-        prefs.pinned = !prefs.pinned;
-    }
+pub(crate) fn quick_bar_colour(ui: &mut egui::Ui, current: Rgb) -> Option<Rgb> {
     let (r, resp) = ui.allocate_exact_size(vec2(32.0, 32.0), Sense::click());
-    ui.painter().circle_filled(r.center(), 9.0, color32(style.color));
+    ui.painter().circle_filled(r.center(), 9.0, color32(current));
     ui.painter().circle_stroke(r.center(), 9.0, Stroke::new(1.0, Color32::from_black_alpha(50)));
     resp.widget_info(|| egui::WidgetInfo::labeled(egui::Role::Button, true, tl!("Colour")));
     let resp = resp.on_hover_text(tl!("Colour"));
+    let mut picked = None;
     egui::Popup::menu(&resp).align(egui::RectAlign::RIGHT_START).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).show(|ui| {
-        if let Some(c) = swatch_grid(ui, Some(style.color)) {
-            prefs.set_color(tool, c);
-            selected_color = Some(c);
+        if let Some(c) = swatch_grid(ui, Some(current)) {
+            picked = Some(c);
             ui.close();
         }
     });
+    picked
+}
+
+/// The comment tools' extra quick-bar controls: pin, colour, fill, opacity and thickness.
+pub(crate) fn quick_bar_controls(ui: &mut egui::Ui, tool: CommentTool, prefs: &mut CommentPrefs) -> Option<Rgb> {
+    let style = prefs.style(tool);
+    if icons::button(ui, "pin", 32.0, prefs.pinned, if prefs.pinned { tl!("Keep tool selected: on") } else { tl!("Keep tool selected") }).clicked() {
+        prefs.pinned = !prefs.pinned;
+    }
+    let selected_color = quick_bar_colour(ui, style.color);
+    if let Some(c) = selected_color {
+        prefs.set_color(tool, c);
+    }
     if tool.has_fill() {
         let resp = fill_button(ui, style.fill);
         egui::Popup::menu(&resp).align(egui::RectAlign::RIGHT_START).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).show(|ui| {

crates/ui-egui/src/widgets/icon_data.rs

@@ -187,6 +187,7 @@ pub static ICONS: &[(&str, &[u8])] = &[
     ("wrench", include_bytes!("../../../../assets/icons/wrench.svg")),
     ("move-horizontal", include_bytes!("../../../../assets/icons/move-horizontal.svg")),
     ("pilcrow", include_bytes!("../../../../assets/icons/pilcrow.svg")),
+    ("a-large-small", include_bytes!("../../../../assets/icons/a-large-small.svg")),
     ("subscript", include_bytes!("../../../../assets/icons/subscript.svg")),
     ("superscript", include_bytes!("../../../../assets/icons/superscript.svg")),
     ("x", include_bytes!("../../../../assets/icons/x.svg")),

crates/ui-egui/src/widgets/icons.rs

@@ -99,7 +99,7 @@ mod tests {
 
     #[test]
     fn shared_catalog_preserves_existing_icons_and_fallback_uri() {
-        assert_eq!(ICONS.len(), 190);
+        assert_eq!(ICONS.len(), 191);
         assert!(exists("minimize-2"));
         for (name, original) in ICONS {
             let expected =

crates/ui-egui/tests/it/fill_sign.rs

@@ -164,7 +164,7 @@ fn fill_text_size_controls_new_text_and_survives_settings_restore() {
     let mut h = harness();
     h.state_mut().execute("sign.fill.text");
     h.run_steps(2);
-    h.get_by_label("Font size:");
+    h.get_by_label("Font size");
     h.state_mut().fill_text_size = 24.0;
     click(&mut h, 50.0, 250.0);
     assert_eq!(h.state().views[0].fill_text.as_ref().map(|t| t.size), Some(24.0));
@@ -201,6 +201,8 @@ fn fill_text_size_resizes_the_selected_text() {
     };
     let before = height(&h);
     let set_size = |h: &mut Harness<'static, PdfCraftApp>, value: &str| {
+        h.get_by_role_and_label(egui::accesskit::Role::Button, "Font size").click();
+        h.run_steps(2);
         h.query_all_by_role(egui::accesskit::Role::SpinButton)
             .find(|n| n.value().is_some_and(|v| v.ends_with(" pt")))
             .expect("the Font size control")
@@ -211,6 +213,8 @@ fn fill_text_size_resizes_the_selected_text() {
         h.run_steps(1);
         h.key_press(egui::Key::Enter);
         h.run_steps(4);
+        h.get_by_role_and_label(egui::accesskit::Role::Button, "Font size").click();
+        h.run_steps(2);
     };
     // Nothing selected: only the size for new text changes.
     h.state_mut().views[0].comments.selected = None;
@@ -1813,6 +1817,158 @@ fn mode_tabs_keep_the_hand_over_a_type_box_scrolled_under_them() {
     assert_eq!(h.state().mode, pdfcraft_ui_egui::Mode::Convert, "the tab takes the click, not the text box");
 }
 
+fn place_selected_text(h: &mut Harness<'static, PdfCraftApp>, text: &str) {
+    h.state_mut().execute("sign.fill.text");
+    click(h, 50.0, 250.0);
+    h.event(egui::Event::Text(text.into()));
+    h.run_steps(1);
+    commit_typed_text(h);
+    let selected = {
+        let s = h.state();
+        s.session
+            .get(s.views[0].id)
+            .unwrap()
+            .info
+            .annotations
+            .iter()
+            .find(|a| a.contents.as_deref() == Some(text))
+            .map(|a| (a.page, a.index))
+            .unwrap()
+    };
+    h.state_mut().views[0].comments.selected = Some(selected);
+    h.run_steps(2);
+}
+
+fn placed_height(h: &Harness<'static, PdfCraftApp>, text: &str) -> f32 {
+    let s = h.state();
+    let a = s.session.get(s.views[0].id).unwrap().info.annotations.iter().find(|a| a.contents.as_deref() == Some(text)).unwrap();
+    a.rect[3] - a.rect[1]
+}
+
+fn placed_color(h: &Harness<'static, PdfCraftApp>, text: &str) -> Option<[f32; 3]> {
+    let s = h.state();
+    s.session.get(s.views[0].id).unwrap().info.annotations.iter().find(|a| a.contents.as_deref() == Some(text)).unwrap().color
+}
+
+#[test]
+fn the_fill_text_size_flyout_opens_from_an_icon_and_leaves_a_stored_size_alone() {
+    let mut h = harness();
+    h.state_mut().fill_text_size = 9.1;
+    place_selected_text(&mut h, "Keep me");
+    let height = placed_height(&h, "Keep me");
+    assert_eq!(h.query_all_by_role(egui::accesskit::Role::SpinButton).filter(|n| n.value().is_some_and(|v| v.ends_with(" pt"))).count(), 0);
+    h.get_by_label("Font size").click();
+    h.run_steps(4);
+    assert!(
+        h.query_all_by_role(egui::accesskit::Role::SpinButton).any(|n| n.value().is_some_and(|v| v.ends_with(" pt"))),
+        "the flyout holds the size"
+    );
+    assert_eq!(h.state().fill_text_size, 9.1, "opening the flyout does not round the size");
+    assert_eq!(placed_height(&h, "Keep me"), height, "opening the flyout does not resize the selected text");
+}
+
+#[test]
+fn dragging_the_fill_text_size_slider_is_one_undo_step() {
+    let mut h = harness();
+    place_selected_text(&mut h, "Drag me");
+    let height = placed_height(&h, "Drag me");
+    h.get_by_label("Font size").click();
+    h.run_steps(2);
+    let slider = h.get_by_role(egui::accesskit::Role::Slider).rect();
+    let y = (slider.min.y + slider.max.y) / 2.0;
+    let from = pos2(slider.min.x + 10.0, y);
+    let to = pos2(slider.max.x - 10.0, y);
+    h.hover_at(from);
+    h.run_steps(1);
+    h.drag_at(from);
+    h.run_steps(1);
+    for k in 1..=4 {
+        h.hover_at(from + (to - from) * (k as f32 / 4.0));
+        h.run_steps(1);
+    }
+    assert_eq!(placed_height(&h, "Drag me"), height, "nothing is applied before the release");
+    h.drop_at(to);
+    h.run_steps(3);
+    assert!(placed_height(&h, "Drag me") > height, "the release resizes the selected text");
+    h.state_mut().execute("edit.undo");
+    h.run_steps(3);
+    assert_eq!(placed_height(&h, "Drag me"), height, "one undo takes the whole drag back");
+}
+
+#[test]
+fn pressing_the_fill_text_size_slider_without_moving_it_adds_no_undo_step() {
+    let mut h = harness();
+    place_selected_text(&mut h, "Hold me");
+    let height = placed_height(&h, "Hold me");
+    h.get_by_label("Font size").click();
+    h.run_steps(2);
+    let slider = h.get_by_role(egui::accesskit::Role::Slider).rect();
+    let at = pos2(slider.min.x + (slider.max.x - slider.min.x) * 0.75, (slider.min.y + slider.max.y) / 2.0);
+    let press = |h: &mut Harness<'static, PdfCraftApp>| {
+        h.hover_at(at);
+        h.run_steps(1);
+        h.drag_at(at);
+        h.run_steps(1);
+        h.drop_at(at);
+        h.run_steps(3);
+    };
+    press(&mut h);
+    let resized = placed_height(&h, "Hold me");
+    assert!(resized > height, "the first press moves the size: {height} -> {resized}");
+    press(&mut h);
+    assert_eq!(placed_height(&h, "Hold me"), resized);
+    h.state_mut().execute("edit.undo");
+    h.run_steps(3);
+    assert_eq!(placed_height(&h, "Hold me"), height, "one undo takes back the only change; the second press added no step");
+}
+
+#[test]
+fn typing_a_fill_text_size_that_rounds_to_the_stored_one_adds_no_undo_step() {
+    let mut h = harness();
+    place_selected_text(&mut h, "Round me");
+    let height = placed_height(&h, "Round me");
+    h.get_by_label("Font size").click();
+    h.run_steps(2);
+    let type_size = |h: &mut Harness<'static, PdfCraftApp>, value: &str| {
+        h.query_all_by_role(egui::accesskit::Role::SpinButton)
+            .find(|n| n.value().is_some_and(|v| v.ends_with(" pt")))
+            .expect("the Font size control")
+            .click();
+        h.run_steps(2);
+        h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::A);
+        h.event(egui::Event::Text(value.into()));
+        h.run_steps(1);
+        h.key_press(egui::Key::Enter);
+        h.run_steps(4);
+    };
+    type_size(&mut h, "36.1234");
+    let resized = placed_height(&h, "Round me");
+    assert!(resized > height, "the first size applies: {height} -> {resized}");
+    type_size(&mut h, "36.1231");
+    assert_eq!(h.state().fill_text_size, 36.1231);
+    h.state_mut().execute("edit.undo");
+    h.run_steps(3);
+    assert_eq!(placed_height(&h, "Round me"), height, "one undo takes back the only change; the rounded retype added no step");
+}
+
+#[test]
+fn the_fill_colour_dot_recolours_the_selected_text() {
+    let mut h = harness();
+    place_selected_text(&mut h, "Paint me");
+    let before = placed_color(&h, "Paint me");
+    h.get_by_label("Colour").click();
+    h.run_steps(2);
+    h.get_by_label("Blue").click();
+    h.run_steps(3);
+    let color = h.state().fill_text_color.map(|v| v as f32);
+    assert!(color[2] > color[0], "blue was selected: {color:?}");
+    assert_ne!(before, Some(color));
+    assert_eq!(placed_color(&h, "Paint me"), Some(color));
+    h.state_mut().execute("edit.undo");
+    h.run_steps(3);
+    assert_eq!(placed_color(&h, "Paint me"), before, "one undo takes the colour back");
+}
+
 #[test]
 fn the_quick_bar_fill_button_shows_the_tool_it_picks() {
     let mut h = harness();