diff --git a/crates/automation/src/dispatch.rs b/crates/automation/src/dispatch.rs index 3e29e7d861..6e97b4d4d7 100644 --- a/crates/automation/src/dispatch.rs +++ b/crates/automation/src/dispatch.rs @@ -755,7 +755,24 @@ impl Automation { self.apply(&a, edit)? } "attachment_edit" => { - let edit = Edit::EditAttachmentDescription { key: a.str("name")?.into(), description: Some(a.str("description")?.into()) }; + let name = a.str("name")?; + let keys: Vec = self + .doc(&a)? + .info + .attachments + .iter() + .filter(|attachment| attachment.name == name) + .filter_map(|attachment| match &attachment.source { + pdfcraft_render::AttachmentSource::Document { key } => Some(key.clone()), + pdfcraft_render::AttachmentSource::Annotation { .. } => None, + }) + .collect(); + let key = match keys.as_slice() { + [key] => key.clone(), + [] => return Err(ToolError::InvalidArgs(format!("there is no document attachment named {name:?}; use a name from doc_info"))), + _ => return Err(ToolError::InvalidArgs(format!("more than one document attachment is named {name:?}; choose an unambiguous name"))), + }; + let edit = Edit::EditAttachmentDescription { key, description: Some(a.str("description")?.into()) }; self.apply(&a, edit)? } "doc_remove_hidden" => self.doc_remove_hidden(&a)?, diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index ab14794448..16284baeab 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -9618,6 +9618,49 @@ fn attachment_descriptions_can_be_edited_cleared_undone_and_saved() { assert_eq!(ok(&mut a, "doc_info", json!({"doc": reopened}))["attachments"][0]["description"], "Résumé 東京"); } +#[test] +fn attachment_edit_resolves_the_listed_name_to_its_name_tree_key() { + let dir = workdir("attachment-name-tree-key"); + std::fs::write(dir.join("payload.txt"), b"payload").unwrap(); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({"path":"a.pdf"}))["doc"].as_u64().unwrap(); + ok(&mut a, "attachment_add", json!({"doc":doc,"path":"payload.txt","name":"storage-key.txt"})); + ok(&mut a, "doc_save", json!({"doc":doc,"path":"seed.pdf","full":true})); + let mut source = pdfcraft_cos::Document::open(std::sync::Arc::new(std::fs::read(dir.join("seed.pdf")).unwrap())).unwrap(); + let key = "storage-key.txt"; + let root = source.root().unwrap(); + let catalog = source.get(root).as_dict().unwrap().clone(); + let names = source.resolve(catalog.get(b"Names").unwrap()).as_dict().unwrap().clone(); + let tree = source.resolve(names.get(b"EmbeddedFiles").unwrap()).as_dict().unwrap().clone(); + let pairs = source.resolve(tree.get(b"Names").unwrap()).as_array().unwrap().clone(); + let spec_ref = pairs.get(1).and_then(pdfcraft_cos::Object::as_ref).unwrap(); + source + .update_dict(spec_ref, |spec| { + let name = pdfcraft_cos::Object::String(pdfcraft_cos::PdfString::text("visible-name.txt")); + spec.set(b"F".to_vec(), name.clone()); + spec.set(b"UF".to_vec(), name); + }) + .unwrap(); + let bytes = pdfcraft_cos::write_full(&source, &pdfcraft_cos::SaveOptions::default()).unwrap(); + std::fs::write(dir.join("named.pdf"), bytes).unwrap(); + + let doc = ok(&mut a, "doc_open", json!({"path":"named.pdf"}))["doc"].as_u64().unwrap(); + assert_eq!(ok(&mut a, "doc_info", json!({"doc":doc}))["attachments"][0]["name"], "visible-name.txt"); + ok(&mut a, "attachment_edit", json!({"doc":doc,"name":"visible-name.txt","description":"Updated"})); + assert_eq!(ok(&mut a, "doc_info", json!({"doc":doc}))["attachments"][0]["description"], "Updated"); + assert!(matches!( + a.call("attachment_edit", &json!({"doc":doc,"name":"missing.txt","description":"x"})), + Err(ToolError::InvalidArgs(message)) if message.contains("no document attachment named") + )); + ok(&mut a, "doc_save", json!({"doc":doc,"path":"edited.pdf","full":true})); + let saved = pdfcraft_cos::Document::open(std::sync::Arc::new(std::fs::read(dir.join("edited.pdf")).unwrap())).unwrap(); + let catalog = saved.get(saved.root().unwrap()).as_dict().unwrap().clone(); + let names = saved.resolve(catalog.get(b"Names").unwrap()).as_dict().unwrap().clone(); + let tree = saved.resolve(names.get(b"EmbeddedFiles").unwrap()).as_dict().unwrap().clone(); + let pairs = saved.resolve(tree.get(b"Names").unwrap()).as_array().unwrap().clone(); + assert_eq!(pairs.first().and_then(pdfcraft_cos::Object::as_string).map(pdfcraft_cos::PdfString::to_text).as_deref(), Some(key)); +} + #[test] fn fdf_nested_comment_values_survive_export_import_and_save_through_tools() { use pdfcraft_cos::{Dict, Document, Object, PdfString, SaveOptions, write_full}; diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 0450fa9d83..92e60dfe89 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -342,15 +342,18 @@ fn dialog_body(ui: &mut egui::Ui, app: &mut PdfCraftApp, dialog: Dialog, req: &m return false; } Dialog::AttachmentDescription => { - let Some((doc, _, _)) = app.attachment_description_draft.as_ref() else { + let Some((doc, generation, _, _)) = app.attachment_description_draft.as_ref() else { req.close = true; return false; }; - if app.active_ids().is_none_or(|(_, active)| active != *doc) { + if app.active_ids().is_none_or(|(_, active)| active != *doc) + || app.session.get(*doc).is_none_or(|current| current.edit_generation() != *generation) + { + app.attachment_description_draft = None; req.close = true; return false; } - let Some((_, key, description)) = app.attachment_description_draft.as_mut() else { + let Some((_, _, key, description)) = app.attachment_description_draft.as_mut() else { req.close = true; return false; }; @@ -1735,9 +1738,22 @@ fn dialog_footer(ui: &mut egui::Ui, app: &mut PdfCraftApp, dialog: Dialog, req: } } else if dialog == Dialog::AttachmentDescription { if widgets::pill_button(ui, tl!("OK"), true).clicked() { - if let Some((doc, key, description)) = app.attachment_description_draft.as_ref() { - req.attachment_description_now = - Some((*doc, Edit::EditAttachmentDescription { key: key.clone(), description: Some(description.clone()) })); + if let Some((doc, generation, key, description)) = app.attachment_description_draft.as_ref() { + let current = app.session.get(*doc).and_then(|doc| { + doc.info + .attachments + .iter() + .find(|attachment| matches!(&attachment.source, pdfcraft_render::AttachmentSource::Document { key: attachment_key } if attachment_key == key)) + .map(|attachment| attachment.description.as_deref().unwrap_or_default().trim().to_owned()) + }); + let normalized = description.trim(); + let still_current = app.session.get(*doc).is_some_and(|doc| doc.edit_generation() == *generation); + if still_current && current.as_deref().is_some_and(|current| current != normalized) { + req.attachment_description_now = Some(( + *doc, + Edit::EditAttachmentDescription { key: key.clone(), description: Some(normalized.to_owned()) }, + )); + } } req.close = true; } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index b936c56a00..745fb0e87b 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -462,7 +462,7 @@ pub struct PdfCraftApp { /// Document Properties ▸ Description fields being edited: (document, Title/Author/Subject/Keywords). pub props_draft: Option<(DocId, [String; 4])>, /// Attachment description being edited: (document, name-tree key, description). - pub attachment_description_draft: Option<(DocId, String, String)>, + pub attachment_description_draft: Option<(DocId, u64, String, String)>, /// Document Properties ▸ Initial View (and reading options) being edited. pub view_draft: Option<(DocId, pdfcraft_engine::InitialView)>, /// Document Properties ▸ Custom being edited. diff --git a/crates/ui-egui/src/panels/right.rs b/crates/ui-egui/src/panels/right.rs index 500398651f..3e5fb18ad1 100644 --- a/crates/ui-egui/src/panels/right.rs +++ b/crates/ui-egui/src/panels/right.rs @@ -364,7 +364,7 @@ pub fn right_panel(app: &mut PdfCraftApp, ui: &mut egui::Ui) { if let Some(key) = deletable && ui.button(tl!("Edit description")).clicked() { - app.attachment_description_draft = Some((id, key.clone(), a.description.clone().unwrap_or_default())); + app.attachment_description_draft = Some((id, doc.edit_generation(), key.clone(), a.description.clone().unwrap_or_default())); app.dialog = Some(crate::Dialog::AttachmentDescription); ui.close(); } diff --git a/crates/ui-egui/tests/it/attachment_row.rs b/crates/ui-egui/tests/it/attachment_row.rs index 144e03ab78..ec21a59cb4 100644 --- a/crates/ui-egui/tests/it/attachment_row.rs +++ b/crates/ui-egui/tests/it/attachment_row.rs @@ -170,6 +170,39 @@ fn the_description_can_be_edited_from_the_row_menu() { assert!(doc.can_undo().is_some_and(|label| label == "Edit attachment description")); } +#[test] +fn keeping_the_attachment_description_does_not_create_an_edit() { + let mut h = harness(); + right_click(&mut h, "notes.txt"); + h.get_by_role(egui::accesskit::Role::Menu).get_by_label("Edit description").click(); + h.run_steps(2); + h.get_by_label("OK").click(); + h.run_steps(3); + let state = h.state(); + let doc = state.session.get(state.views[0].id).unwrap(); + assert_eq!(doc.info.attachments.iter().find(|a| a.name == "notes.txt").unwrap().description.as_deref(), Some("Original")); + assert!(!doc.dirty, "confirming the unchanged description does not dirty the document"); + assert_eq!(doc.can_undo(), None, "confirming the unchanged description does not add undo history"); +} + +#[test] +fn editing_the_document_while_description_dialog_is_open_discards_its_stale_draft() { + let mut h = harness(); + right_click(&mut h, "notes.txt"); + h.get_by_role(egui::accesskit::Role::Menu).get_by_label("Edit description").click(); + h.run_steps(2); + assert!(h.state_mut().apply_edit(pdfcraft_engine::Edit::EditAttachmentDescription { + key: "notes.txt".into(), + description: Some("Changed elsewhere".into()), + })); + h.get_by_label("OK").click(); + h.run_steps(3); + let state = h.state(); + let doc = state.session.get(state.views[0].id).unwrap(); + assert_eq!(doc.info.attachments.iter().find(|a| a.name == "notes.txt").unwrap().description.as_deref(), Some("Changed elsewhere")); + assert_eq!(doc.can_undo(), Some("Edit attachment description")); +} + #[test] fn the_row_buttons_still_take_their_own_clicks() { let dir = Dir::new("buttons");