diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d53b6ce..5a3b51ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,12 @@ The workspace `Cargo.toml` `[workspace.package] version` is the single source fo fallback do not warn; referenced extension levels and differences within its seven slots do ([#486](https://github.com/STAIxBWLB/hwp-cli/issues/486)). +- HWP5 picture synthesis reserves instance IDs in preserved raw picture records as well as + object common headers. Merging a writer-made picture document and then inserting an image + no longer reuses the preserved picture-record ID. Existing raw records remain unchanged; + this is a serialized-ID uniqueness fix, with its effect on opening in Hancom unverified + ([#465](https://github.com/STAIxBWLB/hwp-cli/issues/465)). + ## [1.5.0] **Compatibility** diff --git a/crates/hwp-cli/tests/picture_instance_ids.rs b/crates/hwp-cli/tests/picture_instance_ids.rs new file mode 100644 index 00000000..4ad36789 --- /dev/null +++ b/crates/hwp-cli/tests/picture_instance_ids.rs @@ -0,0 +1,202 @@ +//! Writer-authored HWP picture identities after a merge followed by an image insertion. +//! This synthetic contract checks our serialized records, not native Hangul acceptance. + +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use hwp5::container::Hwp5Container; +use hwp5::record::{RecordNode, ScanMode, scan_stream, tag}; +use serde_json::json; + +struct Fixture(PathBuf); + +impl Fixture { + fn new() -> Self { + let dir = + std::env::temp_dir().join(format!("hwp-picture-instance-ids-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + Self(dir) + } + + fn path(&self, name: &str) -> PathBuf { + self.0.join(name) + } +} + +impl Drop for Fixture { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn collect_picture_records(nodes: &[RecordNode], records: &mut Vec>) { + for node in nodes { + if node.tag == tag::SHAPE_COMPONENT_PICTURE { + records.push(node.data.clone()); + } + collect_picture_records(&node.children, records); + } +} + +fn picture_records(file: &Path) -> Vec> { + let mut container = Hwp5Container::open(file).unwrap(); + let sections = container.body_sections(); + assert!( + !sections.is_empty(), + "setup must have serialized body sections" + ); + let mut records = Vec::new(); + for section in sections { + let bytes = container.read_record_stream(§ion).unwrap(); + let scanned = scan_stream(&bytes, ScanMode::Strict).unwrap(); + assert!(scanned.warnings.is_empty()); + collect_picture_records(&scanned.roots, &mut records); + } + records +} + +fn picture_id(record: &[u8]) -> u32 { + // Only writer-authored records are inspected here. build_picture_extras in + // crates/hwp5/src/write.rs emits its picture instance ID at bytes 74..78. + assert_eq!( + record.len(), + 91, + "setup uses the writer's 91-byte picture layout" + ); + u32::from_le_bytes(record[74..78].try_into().unwrap()) +} + +#[test] +fn merged_raw_picture_and_inserted_image_keep_distinct_picture_instance_ids() { + let fixture = Fixture::new(); + let image = fixture.path("pixel.png"); + // Generate an owner-authored pixel rather than importing an image or native document. + image::RgbaImage::from_pixel(1, 1, image::Rgba([200, 30, 50, 255])) + .save(&image) + .unwrap(); + + let first = hwp_convert::from_markdown("Primary anchor.\n\nPrimary tail."); + let mut picture_source = hwp_convert::from_markdown("Secondary anchor."); + hwp_convert::insert_image( + &mut picture_source, + "Secondary anchor.", + &image, + hwp_convert::ImageSize::Mm(10.0, 10.0), + ) + .unwrap(); + let source_path = fixture.path("picture-source.hwp"); + hwp5::write_document(&picture_source, &source_path, &Default::default()).unwrap(); + let source_records = picture_records(&source_path); + assert_eq!( + source_records.len(), + 1, + "source setup must contain one picture" + ); + let original_record = &source_records[0]; + let original_id = picture_id(original_record); + assert_ne!(original_id, 0, "writer-authored source ID must be nonzero"); + hwp5::read_document(&source_path).expect("writer-made source must read successfully"); + let primary_path = fixture.path("primary.hwp"); + hwp5::write_document(&first, &primary_path, &Default::default()).unwrap(); + assert!( + picture_records(&primary_path).is_empty(), + "first input must be picture-free" + ); + let merged_path = fixture.path("merged.hwp"); + let merge = Command::new(env!("CARGO_BIN_EXE_hwp")) + .arg("merge") + .arg(&primary_path) + .arg(&source_path) + .arg("-o") + .arg(&merged_path) + .output() + .unwrap(); + assert!( + merge.status.success(), + "real CLI merge must succeed: {}", + String::from_utf8_lossy(&merge.stderr) + ); + let merged_records = picture_records(&merged_path); + assert_eq!( + merged_records, source_records, + "merge setup must preserve the raw picture record" + ); + let merged_doc = hwp5::read_document(&merged_path).unwrap().document; + assert_eq!( + merged_doc.sections.len(), + 2, + "CLI merge retains both input sections" + ); + let merged_picture = merged_doc.sections[1] + .paragraphs + .iter() + .flat_map(|paragraph| ¶graph.controls) + .find_map(|control| match control { + hwp_model::Control::Picture(picture) => Some(picture), + _ => None, + }) + .expect("the second section must retain the source picture"); + let common_id = u32::from_le_bytes(merged_picture.common_data[32..36].try_into().unwrap()); + assert_ne!( + common_id ^ 0x0010_0000, + original_id, + "merge setup must separate the common id from the preserved picture-record id" + ); + let anchor = merged_doc.sections[0] + .paragraphs + .iter() + .position(|paragraph| paragraph.plain_text() == "Primary anchor.") + .expect("picture-free first input must retain its insertion anchor"); + + let ops = fixture.path("ops.json"); + std::fs::write( + &ops, + serde_json::to_vec(&json!([{ + "op":"insert_blocks", + "address":{"at":{"section":0,"paragraph":anchor}}, + "position":"after", + "missing_style":"create", + "styles":{}, + "blocks":[{"type":"image","path":"pixel.png","width_mm":10}] + }])) + .unwrap(), + ) + .unwrap(); + let output_path = fixture.path("inserted.hwp"); + let output = Command::new(env!("CARGO_BIN_EXE_hwp")) + .arg("edit") + .arg(&merged_path) + .arg("-o") + .arg(&output_path) + .arg("--ops") + .arg(&ops) + .output() + .unwrap(); + assert!( + output.status.success(), + "real CLI image insertion must succeed before checking identities: {}", + String::from_utf8_lossy(&output.stderr) + ); + hwp5::read_document(&output_path).expect("inserted HWP must read successfully"); + let records = picture_records(&output_path); + assert_eq!( + records.len(), + 2, + "both the preserved and inserted pictures must be serialized" + ); + assert!( + records.contains(original_record), + "insertion must preserve the original raw picture record" + ); + let ids: Vec<_> = records.iter().map(|record| picture_id(record)).collect(); + assert!( + ids.contains(&original_id), + "the source picture ID must survive" + ); + assert_eq!( + ids.iter().copied().collect::>().len(), + ids.len(), + "preserved raw and newly inserted pictures must not share a picture instance ID: {ids:?}" + ); +} diff --git a/crates/hwp5/src/write.rs b/crates/hwp5/src/write.rs index 5b31f798..89b9fb99 100644 --- a/crates/hwp5/src/write.rs +++ b/crates/hwp5/src/write.rs @@ -1984,10 +1984,8 @@ fn synthesize_pictures(doc: &mut Document, warnings: &mut WriteReport) { let mut new_items: Vec = Vec::new(); // Pairs of bin_stream index and new name. let mut renames: Vec<(usize, String)> = Vec::new(); - // Unique instance_id per gso object; Hangul may treat equal IDs as one object and ignore - // duplicates. - // A document that already carries synthesized pictures (a re-read output of this writer) - // holds ids from this same range, so start past the highest one instead of reusing it. + // Reserve both common and raw picture-record ids before choosing new id pairs. + // Merge can renumber a common id while preserving its separate raw picture id. let mut inst = InstanceIds::new(doc); // Fill empty HWPX-origin header/footer LIST_HEADER values from section PageDef dimensions: @@ -2032,15 +2030,32 @@ fn synthesize_pictures(doc: &mut Document, warnings: &mut WriteReport) { } } +fn collect_raw_picture_ids(records: &[OpaqueRecord], used: &mut BTreeSet) { + for record in records { + if record.tag == tag::SHAPE_COMPONENT_PICTURE { + // Field source: build_picture_extras writes 12 border bytes + 32 corners + // + 16 clip + 8 margins + 3 adjustments + 2 BinItem id + 1 transparency, + // then this little-endian u32 at 74..78 (also docs/design/03-hwp5-write.md ยง7). + // Read only that writer-authored field; short records have no complete id. + if let Some(bytes) = record.data.get(74..78) { + used.insert(u32::from_le_bytes( + bytes.try_into().expect("slice len checked"), + )); + } + } + collect_raw_picture_ids(&record.children, used); + } +} + /// Allocator for the gso instance ids this writer synthesizes for pictures. /// /// Ids come from `0x3000_0000..0x3010_0000`. An id is handed out only when neither it nor the /// id derived from it for the picture record (`id ^ 0x0010_0000`, which lands in /// `0x3010_0000..0x3020_0000`) is already held by a picture, table or gso container of the -/// document, wherever that id sits. A re-read output of this writer holds ids from this same -/// range, so a plain counter would repeat them. The counter only moves forward and every -/// candidate has bit 20 clear, so an id handed out is never a later candidate and never the -/// derived value of one: only ids the document already holds need avoiding. When the range is +/// document, including separate ids in preserved raw picture records. Merge can change a +/// common id while retaining its raw picture id, so reserving only common ids is insufficient. +/// The counter only moves forward and every candidate has bit 20 clear, so an id handed out +/// is never a later candidate and never the derived value of one: only ids the document already holds need avoiding. When the range is /// used up the allocator returns `None` instead of stepping past the range edge into another /// object's id; the caller leaves that picture unsynthesized and the existing strip step /// reports it as removed. @@ -2066,16 +2081,19 @@ impl InstanceIds { .map(|b| u32::from_le_bytes(b.try_into().expect("slice len checked"))) } fn walk(para: &Paragraph, used: &mut BTreeSet) { + collect_raw_picture_ids(¶.extras, used); for control in ¶.controls { match control { Control::Picture(p) => { used.extend(common_id(&p.common_data)); + collect_raw_picture_ids(&p.extras, used); if let Some(caption) = &p.caption { caption.paragraphs.iter().for_each(|q| walk(q, used)); } } Control::Table(t) => { used.extend(common_id(&t.common_data)); + collect_raw_picture_ids(&t.extras, used); if let Some(caption) = &t.caption { caption.paragraphs.iter().for_each(|q| walk(q, used)); } @@ -2084,6 +2102,8 @@ impl InstanceIds { } } Control::Generic(g) => { + collect_raw_picture_ids(&g.extras, used); + collect_raw_picture_ids(&g.raw_children, used); if g.ctrl_id == *b"gso " { used.extend(common_id(&g.data)); } @@ -2095,12 +2115,15 @@ impl InstanceIds { caption.paragraphs.iter().for_each(|q| walk(q, used)); } } - _ => {} + Control::SectionDef(definition) => { + collect_raw_picture_ids(&definition.extras, used) + } } } } let mut used = BTreeSet::new(); for section in &doc.sections { + collect_raw_picture_ids(§ion.extras, &mut used); section.paragraphs.iter().for_each(|p| walk(p, &mut used)); } Self { @@ -5757,6 +5780,152 @@ mod tests { doc } + fn picture_holding_raw_id(common: u32, raw: u32) -> Control { + let Control::Picture(mut picture) = picture_with_instance_id(common) else { + unreachable!() + }; + picture.extras = build_picture_extras(100, 100, (100, 100), 1, raw); + Control::Picture(picture) + } + + #[test] + fn raw_picture_ids_reserve_both_common_and_derived_candidates() { + for raw in [0x3000_0000, 0x3010_0000] { + let mut doc = doc_with_picture_ids(&[]); + doc.sections[0].paragraphs[0].controls = vec![picture_holding_raw_id(0x5000_0000, raw)]; + let next = InstanceIds::new(&doc).next().unwrap(); + assert_ne!( + next, raw, + "new common id repeats a preserved picture record id" + ); + assert_ne!( + next ^ 0x0010_0000, + raw, + "new picture record id repeats a preserved raw id" + ); + } + } + + #[test] + fn raw_picture_ids_inside_preserved_generic_trees_are_reserved() { + let Control::Generic(mut generic) = shape_caption_holding(Vec::new()) else { + unreachable!() + }; + generic.caption = None; + // build_picture_extras writes the id at payload bytes74..78, beneath SHAPE_COMPONENT. + generic.raw_children = build_picture_extras(100, 100, (100, 100), 1, 0x3010_0000); + let mut doc = doc_with_picture_ids(&[]); + doc.sections[0].paragraphs[0].controls = vec![Control::Generic(generic)]; + let next = InstanceIds::new(&doc).next().unwrap(); + assert_ne!( + next ^ 0x0010_0000, + 0x3010_0000, + "a raw Generic subtree already owns this picture id" + ); + } + + #[test] + fn raw_picture_id_reservation_reaches_caption_paragraphs() { + let mut doc = doc_with_picture_ids(&[]); + doc.sections[0].paragraphs[0].controls = + vec![shape_caption_holding(vec![picture_holding_raw_id( + 0x5000_0000, + 0x3010_0000, + )])]; + let next = InstanceIds::new(&doc).next().unwrap(); + assert_ne!( + next ^ 0x0010_0000, + 0x3010_0000, + "the shape caption owns this raw picture id" + ); + } + + #[test] + fn raw_picture_ids_in_other_preserved_opaque_owners_are_reserved() { + // Opaque preservation can retain a known picture record beneath unknown parents. + // These are allocator fixtures, not a claim that each hierarchy opens in Hangul. + let mut collisions = Vec::new(); + for owner in [ + "section", + "paragraph", + "table", + "section-definition", + "generic-extras", + ] { + let mut doc = hwp_convert::from_markdown("anchor"); + let records = build_picture_extras(100, 100, (100, 100), 1, 0x3010_0000); + match owner { + "section" => doc.sections[0].extras = records, + "paragraph" => doc.sections[0].paragraphs[0].extras = records, + "table" => { + let mut table = synth_table(None); + table.extras = records; + doc.sections[0].paragraphs[0] + .controls + .push(Control::Table(table)); + } + "section-definition" => { + let definition = doc.sections[0].paragraphs[0] + .controls + .iter_mut() + .find_map(|control| match control { + Control::SectionDef(definition) => Some(definition), + _ => None, + }) + .expect("markdown creates a section definition"); + definition.extras = records; + } + "generic-extras" => { + let Control::Generic(mut generic) = shape_caption_holding(Vec::new()) else { + unreachable!() + }; + generic.caption = None; + generic.extras = records; + doc.sections[0].paragraphs[0] + .controls + .push(Control::Generic(generic)); + } + _ => unreachable!(), + } + if InstanceIds::new(&doc).next().unwrap() ^ 0x0010_0000 == 0x3010_0000 { + collisions.push(owner); + } + } + assert!( + collisions.is_empty(), + "raw picture ids were reused from opaque owners: {collisions:?}" + ); + } + + #[test] + fn raw_picture_id_requires_its_complete_field_and_can_exhaust_the_range() { + // The current writer appends the four-byte instance id after 74 prefix bytes. + // A short opaque record must stay unread rather than manufacturing an id from its tail. + for len in [0, 73, 74, 75, 76, 77, 78] { + let mut doc = doc_with_picture_ids(&[]); + let Control::Picture(mut picture) = picture_holding_raw_id(0x5000_0000, 0x3010_0000) + else { + unreachable!() + }; + picture.extras[0].children[0].data.truncate(len); + doc.sections[0].paragraphs[0].controls = vec![Control::Picture(picture)]; + let mut ids = InstanceIds::with_range(&doc, 0x3000_0000, 0x3000_0001); + if len < 78 { + assert_eq!( + ids.next(), + Some(0x3000_0000), + "a {len}-byte record lacks the full id" + ); + } else { + assert_eq!( + ids.next(), + None, + "the one available id derives a preserved raw picture id" + ); + } + } + } + /// An id handed out must be inside the synthesized range and not already used by any /// picture, table or container. `max + 1` of the in-range ids steps over the range edge: /// with `0x300fffff` present it returns `0x30100000`, which another object already holds.