From b8b6fb2f458f075d68e4996b3046a35d7dd9774d Mon Sep 17 00:00:00 2001 From: Mawfyy Date: Fri, 9 Oct 2026 20:26:23 -0500 Subject: [PATCH] Never crash & persist: Make Current Properties Default and page-display choices survive a restart (#340, #266) (#365) * Never crash & persist: Make Current Properties Default and page display choices survive a restart #340: - Expose per-tool comment styles via CommentPrefs::styles(). - persist() writes a `comment_styles` array (tool, color, opacity, width, fill). - restore() reads each entry through the clamping setters; a new rgb_of helper at the read boundary refuses malformed colors whole (string, wrong length, null, non-finite) and clamps finite out-of-range values into 0..=1, so a hostile settings file can't smuggle a NaN or saturated color. #266: - The page display chosen in a document (view.layout.*, view.fit_one_page, view.fit_width_scrolling) now mirrors into view_defaults, which the existing default-layout/default-zoom keys already persist. New documents and the next launch open in the persisted display; transient zooms do not rewrite the default (matches Acrobat). Tests: - make_current_properties_default: persist + fresh app reuses the style. - the_page_display_you_choose_becomes_the_default: choice mirrors, persists, new document opens in the persisted display; Acrobat fit modes persist too. - comment_style_settings_clamp_and_refuse_hostile_values: hostile JSON is clamped or refused. cargo fmt --check and cargo clippy -p pdfcraft-ui-egui --all-targets are clean. * Fix clippy: chunks_exact(4) -> as_chunks::<4>() in signature_image.rs * 365: keep the comment-style persistence (#340) only; a saved no-fill clears a default fill The #266 half (every View choice rewriting the default page display) is dropped: Preferences already sets that default explicitly. rgb_triple moves out of the module declarations with one doc comment. Co-Authored-By: Claude Opus 5.5 * Fill & Sign dates: Italian names, since Italian is now an interface language (#494) date_languages_are_the_interface_languages requires every interface language to have date names; the unknown-language example in the session test moves from it to xx. --------- Co-authored-by: Mawfyy Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- crates/engine/src/dates.rs | 13 +++++-- crates/ui-egui/src/comments.rs | 5 +++ crates/ui-egui/src/lib.rs | 61 ++++++++++++++++++++++++++++++++ crates/ui-egui/tests/comments.rs | 7 ++++ crates/ui-egui/tests/ui.rs | 46 ++++++++++++++++++++++++ parity/acrobat-features.toml | 2 +- 6 files changed, 131 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/dates.rs b/crates/engine/src/dates.rs index fab1b5db..ebab3237 100644 --- a/crates/engine/src/dates.rs +++ b/crates/engine/src/dates.rs @@ -53,7 +53,7 @@ const ZH_DAYS: [&str; 7] = ["星期日", "星期一", "星期二", "星期三", /// The languages month and weekday names can be written in (Preferences ▸ Date format ▸ Language): /// the interface languages, in the same order and with the same names. -pub const DATE_LANGUAGES: [DateLanguage; 14] = [ +pub const DATE_LANGUAGES: [DateLanguage; 15] = [ DateLanguage { code: "en", name: "English", @@ -219,6 +219,15 @@ pub const DATE_LANGUAGES: [DateLanguage; 14] = [ days: ["неділя", "понеділок", "вівторок", "середа", "четвер", "пʼятниця", "субота"], days_short: ["нд", "пн", "вт", "ср", "чт", "пт", "сб"], }, + DateLanguage { + code: "it", + name: "Italiano", + months: ["gennaio", "febbraio", "marzo", "aprile", "maggio", "giugno", "luglio", "agosto", "settembre", "ottobre", "novembre", "dicembre"], + months_with_day: None, + months_short: ["gen", "feb", "mar", "apr", "mag", "giu", "lug", "ago", "set", "ott", "nov", "dic"], + days: ["domenica", "lunedì", "martedì", "mercoledì", "giovedì", "venerdì", "sabato"], + days_short: ["dom", "lun", "mar", "mer", "gio", "ven", "sab"], + }, ]; /// The date language for `code` (any case), or `None` when there's none. @@ -527,7 +536,7 @@ mod tests { assert_eq!(s.today_text(None, None).as_deref(), Ok("terça-feira 14 novembro")); assert_eq!(s.today_text(Some("d mmmm"), Some("uk")).as_deref(), Ok("14 листопада"), "Ukrainian months after a day"); assert_eq!(s.today_text(Some("dddd d mmmm"), Some("de")).as_deref(), Ok("Dienstag 14 November")); - assert!(s.set_date_language(Some("it")).unwrap_err().contains("zh-hant"), "only the interface languages"); + assert!(s.set_date_language(Some("xx")).unwrap_err().contains("zh-hant"), "only the interface languages"); assert_eq!(s.date_language(), Some("pt-br"), "an unknown language keeps the previous one"); s.set_date_language(None).unwrap(); assert_eq!(s.date_language(), None); diff --git a/crates/ui-egui/src/comments.rs b/crates/ui-egui/src/comments.rs index bfe48b9a..1f195784 100644 --- a/crates/ui-egui/src/comments.rs +++ b/crates/ui-egui/src/comments.rs @@ -263,6 +263,11 @@ impl CommentPrefs { self.styles.iter().find(|(t, _)| *t == tool).map(|(_, s)| s.clone()).unwrap_or_default() } + /// Every tool with its current default style (the persisted settings read this). + pub fn styles(&self) -> impl Iterator { + self.styles.iter().map(|(t, s)| (*t, s)) + } + pub fn set_color(&mut self, tool: CommentTool, c: Rgb) { if let Some((_, s)) = self.styles.iter_mut().find(|(t, _)| *t == tool) { s.color = c; diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1e88c2a4..c75fa73f 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -101,6 +101,7 @@ pub(crate) fn date_text_for_pdf(session: &Session) -> Result { /// The longest author name kept (Preferences ▸ Identity, restored settings). pub(crate) const MAX_AUTHOR_CHARS: usize = 200; + pub mod portable; mod protect; mod recovery; @@ -624,6 +625,25 @@ impl Default for PdfCraftApp { } } +/// A restored colour: an `[r, g, b]` array of finite numbers, each clamped to 0–1 (restored +/// settings are untrusted). Anything else (a string, a wrong length, a null or a non-finite +/// number) is refused whole, so the caller keeps its default. +fn rgb_triple(v: &serde_json::Value) -> Option<[f64; 3]> { + let a = v.as_array()?; + if a.len() != 3 { + return None; + } + let mut out = [0.0f64; 3]; + for (slot, x) in out.iter_mut().zip(a.iter()) { + let n = x.as_f64()?; + if !n.is_finite() { + return None; + } + *slot = n.clamp(0.0, 1.0); + } + Some(out) +} + impl PdfCraftApp { pub fn new() -> Self { Self { @@ -1273,6 +1293,19 @@ impl PdfCraftApp { // Null follows the interface language. "date_language": self.session.date_language(), "author": self.comment_prefs.author, + // Per-tool comment defaults (colours, opacity, widths), so Make Current + // Properties Default survives a restart (#340). + "comment_styles": self + .comment_prefs + .styles() + .map(|(t, s)| serde_json::json!({ + "tool": t.command(), + "color": s.color, + "opacity": s.opacity, + "width": s.width, + "fill": s.fill, + })) + .collect::>(), // Drawn signatures keep their original form (older settings read the same). "signature": match &self.signature { Some(fill_sign::SavedSig::Drawn(s)) => Some(s), _ => None }, "signature_text": match &self.signature { Some(fill_sign::SavedSig::Typed(t)) => Some(t), _ => None }, @@ -1341,6 +1374,34 @@ impl PdfCraftApp { if let Some(author) = v["author"].as_str().map(str::trim).filter(|a| !a.is_empty()) { self.comment_prefs.author = author.chars().take(MAX_AUTHOR_CHARS).collect(); } + // Per-tool comment styles (#340): each field goes through its clamping setter, so a + // malformed or hostile entry is clamped or ignored rather than trusted. + if let Some(styles) = v["comment_styles"].as_array() { + for e in styles { + let Some(tool) = e["tool"].as_str().and_then(crate::comments::CommentTool::from_command) else { continue }; + if let Some(c) = rgb_triple(&e["color"]) { + self.comment_prefs.set_color(tool, c); + } + if let Some(o) = e["opacity"].as_f64() { + self.comment_prefs.set_opacity(tool, o); + } + if let Some(w) = e["width"].as_f64() { + self.comment_prefs.set_width(tool, w); + } + // `"fill": null` is a saved "no fill", which clears a tool's default fill; a + // missing or malformed fill keeps it. + let fill = match e.get("fill") { + Some(serde_json::Value::Null) => Some(None), + Some(v) => rgb_triple(v).map(Some), + None => None, + }; + if let Some(fill) = fill { + let mut s = self.comment_prefs.style(tool); + s.fill = fill; + self.comment_prefs.set_style(tool, s); + } + } + } if let Ok(s) = serde_json::from_value::>>(v["signature"].clone()) && s.iter().all(|st| st.iter().all(|p| p.iter().all(|x| x.is_finite()))) { diff --git a/crates/ui-egui/tests/comments.rs b/crates/ui-egui/tests/comments.rs index cc3b6e82..ff85148a 100644 --- a/crates/ui-egui/tests/comments.rs +++ b/crates/ui-egui/tests/comments.rs @@ -537,6 +537,13 @@ fn make_current_properties_default() { h.run_steps(3); let st = h.state().comment_prefs.style(pdfcraft_ui_egui::comments::CommentTool::Rectangle); assert_eq!((st.color, st.opacity, st.width), ([0.0, 0.47, 0.84], 0.5, 5.0)); + // The default survives a restart (#340): a fresh app reads the persisted settings and its + // next rectangle still takes the style. + let json = h.state().persist(); + let mut fresh = PdfCraftApp::new(); + fresh.restore(&json); + let st = fresh.comment_prefs.style(pdfcraft_ui_egui::comments::CommentTool::Rectangle); + assert_eq!((st.color, st.opacity, st.width), ([0.0, 0.47, 0.84], 0.5, 5.0), "the tool default persisted"); // The next rectangle takes it. h.state_mut().set_option("quick", "square").unwrap(); drag_pt(&mut h, (160.0, 100.0), (260.0, 40.0)); diff --git a/crates/ui-egui/tests/ui.rs b/crates/ui-egui/tests/ui.rs index ba0e3c91..760245bc 100644 --- a/crates/ui-egui/tests/ui.rs +++ b/crates/ui-egui/tests/ui.rs @@ -380,6 +380,52 @@ fn persistence_round_trips_and_tolerates_garbage() { assert_eq!(b.theme, pdfcraft_ui_egui::theme::ThemeKind::Dark); } +/// A tool's saved "no fill" (`"fill": null`) clears a fill it had as its default; a missing or +/// malformed fill keeps it (#340). +#[test] +fn a_saved_no_fill_clears_a_comment_tool_default_fill() { + use pdfcraft_ui_egui::comments::CommentTool; + let filled = |a: &mut PdfCraftApp| { + let mut s = a.comment_prefs.style(CommentTool::Rectangle); + s.fill = Some([0.2, 0.4, 0.6]); + a.comment_prefs.set_style(CommentTool::Rectangle, s); + }; + let mut a = PdfCraftApp::new(); + filled(&mut a); + a.restore(r#"{"comment_styles":[{"tool":"comment.square","fill":null}]}"#); + assert_eq!(a.comment_prefs.style(CommentTool::Rectangle).fill, None, "null clears the fill"); + let mut b = PdfCraftApp::new(); + filled(&mut b); + b.restore(r#"{"comment_styles":[{"tool":"comment.square","color":[0,0,1]},{"tool":"comment.square","fill":"red"}]}"#); + assert_eq!(b.comment_prefs.style(CommentTool::Rectangle).fill, Some([0.2, 0.4, 0.6]), "missing or malformed keeps it"); + // A round trip keeps a cleared fill cleared. + let mut c = PdfCraftApp::new(); + c.restore(&a.persist()); + assert_eq!(c.comment_prefs.style(CommentTool::Rectangle).fill, None); +} + +/// Persisted per-tool comment styles go through their clamping setters: hostile or malformed +/// values are clamped or ignored, never trusted (#340). +#[test] +fn comment_style_settings_clamp_and_refuse_hostile_values() { + use pdfcraft_ui_egui::comments::CommentTool; + let mut a = PdfCraftApp::new(); + a.restore( + r#"{"comment_styles":[ + {"tool":"comment.highlight","color":[2,-1,0.5],"opacity":99,"width":-4}, + {"tool":"comment.note","color":"yellow","opacity":"none"}, + {"tool":"comment.note","color":[0.1,null,0.3]}, + {"tool":"bogus.command","color":[0,0,0]} + ]}"#, + ); + // Out-of-range numbers are clamped into range. + let st = a.comment_prefs.style(CommentTool::Highlight); + assert_eq!((st.color, st.opacity, st.width), ([1.0, 0.0, 0.5], 1.0, 0.5)); + // A malformed colour is refused whole: the tool keeps its default. + let note = a.comment_prefs.style(CommentTool::Note); + assert_eq!(note.color, [1.0, 0.82, 0.0], "the default colour stays"); +} + #[test] fn zoom_keeps_the_point_under_the_cursor_still() { let mut h = harness(|app| { diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index a9eaa8f7..ae6477e7 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -3819,7 +3819,7 @@ status = "shipped" commands = ["comment.list"] tools = [] evidence = ["crates/ui-egui/tests/comments.rs::make_current_properties_default"] -notes = "colour, opacity and line width become the tool's defaults (canvas and Comments panel menus)" +notes = "colour, opacity and line width become the tool's defaults (canvas and Comments panel menus) and survive a restart: the per-tool styles are persisted with the settings and restored through their clamping setters (#340)" [[feature]] id = "comment.author-identity"