Merge integration batch B's review fixes

crates/automation/src/lib.rs

@@ -1316,7 +1316,9 @@ impl Automation {
             })
             .flatten();
         let header_version = bytes.strip_prefix(b"%PDF-").and_then(|b| b.get(..3)).and_then(valid_version);
-        let pdf_version = catalog_version.as_deref().or(header_version);
+        // Viewers read the later of the two (object streams raise the header past a lower catalog
+        // entry). Both are "d.d", so text order is version order.
+        let pdf_version = catalog_version.as_deref().into_iter().chain(header_version).max();
         let o = &r.optimize;
         Ok(json!({
             "path": path.to_string_lossy(),

crates/automation/tests/it/automation.rs

@@ -5942,6 +5942,26 @@ fn optimizing_through_tools() {
     assert!(matches!(a.call("doc_optimize", &json!({ "doc": doc, "path": "x.pdf", "discard": ["everything"] })), Err(ToolError::InvalidArgs(_))));
 }
 
+/// #3227 review: retaining the version, a catalog `/Version` below the 1.5 an object stream
+/// needs is not what viewers read: the header is raised, and the later of the two is reported.
+#[test]
+fn retained_pdf_version_reports_the_header_raised_for_object_streams() {
+    let dir = workdir("optimize-retain-version");
+    let mut a = auto(&dir);
+    let blank = ok(&mut a, "doc_create", json!({ "from": "blank", "width": 300, "height": 400 }))["doc"].as_u64().unwrap();
+    ok(&mut a, "doc_optimize", json!({ "doc": blank, "path": "v14.pdf", "compatibility": "1.4" }));
+    let mut cos = pdfcraft_cos::Document::open(std::sync::Arc::new(std::fs::read(dir.join("v14.pdf")).unwrap())).unwrap();
+    let root = cos.root().unwrap();
+    cos.update_dict(root, |c| c.set(b"Version".to_vec(), pdfcraft_cos::Object::name("1.4"))).unwrap();
+    let bytes = pdfcraft_cos::write_full(&cos, &pdfcraft_cos::SaveOptions { object_streams: false, ..Default::default() }).unwrap();
+    std::fs::write(dir.join("cat14.pdf"), &bytes).unwrap();
+    let doc = ok(&mut a, "doc_open", json!({ "path": "cat14.pdf" }))["doc"].as_u64().unwrap();
+    let r = ok(&mut a, "doc_optimize", json!({ "doc": doc, "path": "out.pdf", "compatibility": "retain", "object_streams": true }));
+    let out = std::fs::read(dir.join("out.pdf")).unwrap();
+    assert!(out.starts_with(b"%PDF-1.5\n") && out.windows(7).any(|w| w == b"/ObjStm"), "{r}");
+    assert_eq!(r["pdf_version"].as_str(), Some("1.5"), "{r}");
+}
+
 /// `data` as LZW with one 9-bit literal code per byte (clear-table first, EOD last): valid for
 /// fewer than 250 bytes, before the code width grows.
 fn lzw_literals(data: &[u8]) -> Vec<u8> {

crates/engine/src/lib.rs

@@ -4505,8 +4505,11 @@ impl Session {
             // An encrypted file needs the password it is protected with now (the owner
             // password when known, so saving never downgrades this session's rights).
             let keys = editor.keys.clone();
+            // The saved file carries no Password-field values (#3943); the open document keeps them.
+            let typed = pdfcraft_forms::password_values(&editor.cos);
             editor.cos =
                 pdfcraft_cos::Document::open_with_password(bytes.clone(), keys.reopen.as_deref()).map_err(|e| EditError::Reopen(e.to_string()))?;
+            pdfcraft_forms::restore_password_values(&mut editor.cos, &typed).map_err(|e| EditError::Reopen(e.to_string()))?;
             editor.saved = bytes;
             editor.saved_keys = keys;
             editor.unsaved_file = false;

crates/engine/src/tests.rs

@@ -2251,6 +2251,22 @@ fn password_field_values_are_kept_in_memory_but_written_to_no_file() {
     assert!(t.get(reopened).unwrap().form.iter().find(|f| f.name == "name").unwrap().value.iter().all(|v| v.is_empty()));
 }
 
+/// #3943 review: recording a save rebases editing on the saved file, which carries no Password
+/// value; the typed value stays in the open document all the same, and a later save still omits it.
+#[test]
+fn a_typed_password_value_survives_save_and_mark_saved() {
+    let fixture = String::from_utf8(form_fixture()).unwrap().replace("/T (name)", "/T (name) /Ff 8192");
+    let mut s = Session::new().with_clock(|| 1_700_000_000);
+    let id = s.open("form.pdf", None, Arc::new(fixture.into_bytes()), None).unwrap();
+    s.apply(id, Edit::SetFieldValue { name: "name".into(), value: FieldValue::Text("hunter2".into()) }).unwrap();
+    let saved = s.save_bytes(id).unwrap();
+    s.mark_saved(id, saved, Some("saved.pdf".into())).unwrap();
+    let value = |s: &Session| s.get(id).unwrap().form.iter().find(|f| f.name == "name").unwrap().value.clone();
+    assert_eq!(value(&s), ["hunter2"], "the open document keeps the value after Save");
+    assert!(!s.get(id).unwrap().dirty, "restoring it is not an unsaved change");
+    assert!(!appears_in_pdf(&s.save_bytes(id).unwrap(), "hunter2"), "the next save still omits it");
+}
+
 /// #3926 / #3943 review: signing writes the signed file from the same copy a save does, so a
 /// typed Password-field value is not signed into the file either.
 #[test]

crates/forms/src/appearance.rs

@@ -130,8 +130,11 @@ impl TextBox {
         // The smallest an automatic size goes ([`auto_text_size_by`]).
         let size = self.size.unwrap_or(4.0);
         if self.multiline {
-            let lines = wrap_with(&shown, size, self.inner_w, width_of).len() as f64;
-            lines * size * 1.15 <= self.inner_h + 1e-6
+            // As the appearance draws it: the first baseline is 0.85 em below the padded top and
+            // a line is drawn while its descent (0.207 em) stays inside the border, so the
+            // padding below the last line is not needed: `inner_h` plus 2 pt.
+            let above = wrap_with(&shown, size, self.inner_w, width_of).len().saturating_sub(1) as f64;
+            above * size * 1.15 + 1.057 * size <= self.inner_h + 2.0 + 0.01
         } else {
             // A line break in a single-line value is drawn as a space.
             let line: String = shown.chars().map(|c| if matches!(c, '\n' | '\r') { ' ' } else { c }).collect();

crates/forms/src/lib.rs

@@ -1345,6 +1345,24 @@ pub fn has_password_values(doc: &Document) -> bool {
     !password_fields(doc).is_empty()
 }
 
+/// The typed values of the text fields with the Password flag, as (name, text) pairs, for
+/// [`restore_password_values`] after the document was rebased on a file written without them.
+pub fn password_values(doc: &Document) -> Vec<(String, String)> {
+    password_fields(doc).into_iter().filter_map(|f| f.value.first().map(|v| (f.name.clone(), v.clone()))).collect()
+}
+
+/// Put values from [`password_values`] back into the open document, in memory only. A field
+/// that is gone or read-only keeps what it has.
+pub fn restore_password_values(doc: &mut Document, values: &[(String, String)]) -> Result<(), FormError> {
+    for (name, v) in values {
+        match set_value(doc, name, &FieldValue::Text(v.clone())) {
+            Ok(()) | Err(FormError::NoSuchField(_) | FormError::ReadOnly(_)) => {}
+            Err(e) => return Err(e),
+        }
+    }
+    Ok(())
+}
+
 /// `r` and its field ancestors (`/Parent`), nearest first, at most 32 deep and without repeats.
 fn field_chain(doc: &Document, r: ObjRef) -> Vec<ObjRef> {
     let mut chain = vec![r];

crates/forms/src/tests.rs

@@ -439,6 +439,38 @@ fn a_text_field_says_what_fits_it_as_its_appearance_draws_it() {
     assert!(tb("comb").fits(&"W".repeat(50)));
 }
 
+/// #4339 review: a multi-line field is full when its appearance stops drawing lines (a line is
+/// drawn while its descent stays inside the border), not when `lines x size x 1.15` outgrows the
+/// padded height: a 16 pt field at 12 pt draws one line.
+#[test]
+fn a_short_multiline_field_takes_the_one_line_it_draws() {
+    let tx = |name: &str, extra: &str, h: i32| {
+        format!(
+            "<< /Type /Annot /Subtype /Widget /FT /Tx /T ({name}) /DA (/Helv 12 Tf 0 g) /Ff {}{extra} /Rect [50 {} 150 700] /P 3 0 R >>",
+            flags::MULTILINE,
+            700 - h
+        )
+    };
+    let objs: Vec<String> = vec![
+        "<< /Type /Catalog /Pages 2 0 R /AcroForm 4 0 R >>".into(),
+        "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 600 800] >>".into(),
+        "<< /Type /Page /Parent 2 0 R /Annots [6 0 R 7 0 R] >>".into(),
+        "<< /Fields [6 0 R 7 0 R] /DA (/Helv 12 Tf 0 g) /DR << /Font << /Helv 5 0 R >> >> >>".into(),
+        "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica /Encoding /WinAnsiEncoding >>".into(),
+        tx("plain", "", 16),
+        tx("boxed", " /MK << /BC [0] >>", 16),
+    ];
+    let doc = document(&objs);
+    let all = fields(&doc);
+    let tb = |name: &str| {
+        let f = field(&all, name);
+        appearance::text_box(&doc, f, &f.widgets[0])
+    };
+    assert!(tb("plain").fits("one line") && !tb("plain").fits("one\ntwo"));
+    // A 1 pt border takes 1 pt from each side: the line's descent then falls outside it.
+    assert!(!tb("boxed").fits("one line"));
+}
+
 /// An edit to a field that doesn't scroll keeps as much of what it inserted as fits, wherever the
 /// caret was, and always keeps a deletion.
 #[test]

crates/js/src/lib.rs

@@ -704,7 +704,7 @@ pub(crate) fn menu_item(name: &str) -> Option<Request> {
 fn exec_menu_item(_: &JsValue, args: &[JsValue], ctx: &mut Context) -> JsResult<JsValue> {
     let item = text(&arg(args, 0), ctx)?;
     if let Some(r) = menu_item(&item) {
-        host(ctx)?.borrow_mut().requests.push(r);
+        host(ctx)?.borrow_mut().request(r);
     }
     Ok(JsValue::undefined())
 }

crates/js/src/tests.rs

@@ -254,6 +254,13 @@ fn kept_bytes(o: &Outcome) -> usize {
     o.console.iter().chain(&o.alerts).map(String::len).sum::<usize>() + o.requests.iter().map(req).sum::<usize>()
 }
 
+#[test]
+fn exec_menu_item_requests_are_capped_like_other_requests() {
+    let o = go("for (var i = 0; i < 12000; i++) app.execMenuItem('Print');", &Event::field("Mouse Up", "agree", ""));
+    assert_eq!(o.error, None);
+    assert_eq!(o.requests.len(), crate::xfa::MAX_EFFECTS);
+}
+
 #[test]
 fn console_alerts_and_requests_are_capped_in_count_and_bytes() {
     let o = go(

crates/ui-egui/src/comments.rs

@@ -1593,10 +1593,23 @@ pub(crate) fn composer(ctx: &egui::Context, view: &mut DocView, info: &DocInfo,
         ComposerKind::Edit(_) => "Edit comment",
     };
     // Its comment scrolled out of the document: keep the draft until it is back, rather than
-    // pushing the card into the window over the mode bar and toolbar (#4021).
+    // pushing the card into the window over the mode bar and toolbar (#4021). An edited comment
+    // that is only partly scrolled away keeps its card, at the nearest point inside the document.
+    let mut anchor = anchor;
     if !viewport.contains(anchor) {
-        c.focus = true;
-        return None;
+        let part_visible = matches!(c.kind, ComposerKind::Edit(index)
+        if info.annotations.iter().any(|a| a.page == page && a.index == index && {
+            let r = a.rect;
+            viewport.intersects(egui::Rect::from_two_pos(
+                cx.to_screen([f64::from(r[0]), f64::from(r[1])]),
+                cx.to_screen([f64::from(r[2]), f64::from(r[3])]),
+            ))
+        }));
+        if !part_visible {
+            c.focus = true;
+            return None;
+        }
+        anchor = anchor.clamp(viewport.min, viewport.max);
     }
     let pos = pos2(anchor.x + 12.0, anchor.y);
     let area = egui::Area::new(egui::Id::new(("comment-composer", view.id.0))).order(egui::Order::Foreground).constrain_to(viewport);

crates/ui-egui/src/js_ui.rs

@@ -77,6 +77,9 @@ impl PdfCraftApp {
                     };
                     // Show/Hide: a panel already showing closes.
                     self.right = (self.right != Some(panel)).then_some(panel);
+                    if self.right.is_some() {
+                        self.pending_panel_reveals.push(crate::docking::Panel::Inspector);
+                    }
                 }
                 Request::Focus(_) | Request::Beep | Request::Reset(_) => {}
             }

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

@@ -725,6 +725,33 @@ fn the_composer_hides_while_its_comment_is_scrolled_away() {
     assert_eq!(h.state().views[0].comments.composer.as_ref().map(|c| c.text.as_str()), Some("Draft"));
 }
 
+/// Editing a large comment whose top-right corner is scrolled away still shows the composer
+/// while the rest of the comment is on screen, so it can be posted or cancelled (#4095).
+#[test]
+fn the_edit_composer_shows_while_part_of_its_comment_is_on_screen() {
+    let mut h = harness(|app| app.set_option("quick", "square").unwrap());
+    drag_pt(&mut h, (20.0, 190.0), (280.0, 10.0));
+    // What "Edit text…" opens: the composer anchored at the comment's top-right corner.
+    h.state_mut().views[0].comments.composer = Some(pdfcraft_ui_egui::comments::Composer {
+        page: 0,
+        at: [280.0, 190.0],
+        kind: pdfcraft_ui_egui::comments::ComposerKind::Edit(0),
+        text: String::new(),
+        focus: true,
+    });
+    h.run_steps(2);
+    let viewport = h.state().views[0].viewport_rect();
+    h.state_mut().views[0].goto = Some((0, 0.4));
+    h.run_steps(4);
+    assert!(at(&h, 280.0, 190.0).y < viewport.top(), "the comment's top-right corner is scrolled away");
+    assert!(at(&h, 280.0, 10.0).y > viewport.top(), "part of the comment is still on screen");
+    let editors: Vec<_> = h.query_all_by(|n| n.role() == egui::accesskit::Role::MultilineTextInput).map(|n| n.rect()).collect();
+    assert!(editors.iter().any(|r| viewport.contains(r.center())), "the editor is drawn: {editors:?} (document {viewport:?})");
+    h.get_by_label("Cancel").click();
+    h.run_steps(2);
+    assert!(h.state().views[0].comments.composer.is_none(), "Cancel closes it");
+}
+
 /// With its whole page scrolled off the screen the composer keeps its draft, and the editor takes
 /// the keyboard again when the page comes back (#4021).
 #[test]

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

@@ -1599,6 +1599,22 @@ fn explicit_panel_requests_reveal_inactive_group_members() {
     h.get_by_label("Header and footer");
 }
 
+/// #3580 review: a panel a script opens (`app.execMenuItem("ShowHideBookmarks")`) is shown even
+/// when the inspector is a background tab of a custom docking layout.
+#[test]
+fn a_script_opened_panel_reveals_an_inactive_inspector() {
+    let (mut h, c) = harness();
+    ok(&mut h, &c, "ui.set", json!({"key":"panel", "value":"pages"}));
+    ok(&mut h, &c, "ui.dock", json!({"operation":"move", "panel":"inspector", "target":"tools", "zone":"center"}));
+    ok(&mut h, &c, "ui.dock", json!({"operation":"activate", "panel":"tools"}));
+    h.run_steps(3);
+    assert!(h.query_by_label("This document has no bookmarks.").is_none());
+    let id = h.state().active_ids().unwrap().1;
+    h.state_mut().run_button_script(id, "", "app.execMenuItem('ShowHideBookmarks')");
+    h.run_steps(4);
+    h.get_by_label("This document has no bookmarks.");
+}
+
 #[test]
 fn home_and_combine_suppress_inspector_without_forgetting_layout_or_choice() {
     use pdfcraft_ui_egui::docking::Panel;