fix: Add DucModelElement support and remove BlockManager from PDF gen…#179
fix: Add DucModelElement support and remove BlockManager from PDF gen…#179jorgedanisc merged 1 commit intodevfrom
Conversation
|
🎉 This PR is included in version 2.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Pull request overview
This PR aims to extend the DUC→PDF pipeline with initial DucModelElement plumbing while removing BlockManager from the PDF generation flow.
Changes:
- Added
DucModelElementhandling in common “element base” accessors and scaling/bounds calculations. - Removed
BlockManagerfromDucToPdfBuilderand element streaming call paths. - Simplified parts of resource streaming (removed unused helpers/fields; some dimensions now hard-coded).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ducpdf/src/duc2pdf/src/utils/style_resolver.rs | Removes an unused helper for applying base element styles. |
| packages/ducpdf/src/duc2pdf/src/streaming/stream_resources.rs | Simplifies resource streamer state and dimension handling; adjusts PDF/SVG processing. |
| packages/ducpdf/src/duc2pdf/src/streaming/stream_elements.rs | Removes BlockManager plumb-through and adds partial DucModelElement branches. |
| packages/ducpdf/src/duc2pdf/src/streaming/pdf_linear.rs | Removes unused intermediate variables during path segment creation. |
| packages/ducpdf/src/duc2pdf/src/scaling.rs | Adds scaling support for DucModelElement. |
| packages/ducpdf/src/duc2pdf/src/lib.rs | Includes DucModelElement in bounding box calculation. |
| packages/ducpdf/src/duc2pdf/src/builder.rs | Removes BlockManager from builder state and streaming calls; updates element-base matching. |
| packages/ducjs/src/types/index.ts | Adds "doc"/"pdf" to ToolType and reorders exports/import types. |
Comments suppressed due to low confidence (2)
packages/ducpdf/src/duc2pdf/src/streaming/stream_resources.rs:63
- This file still carries
BlockManagerstate (block_manager: Option<BlockManager>) and aninitialize(..., block_manager: BlockManager, ...)parameter, butblock_manageris never used anywhere outside initialization. Since this PR is removing BlockManager from PDF generation, consider removing the field/parameter (and theuse hipdf::blocks::BlockManagerimport) here as well to avoid dead code and keep the API aligned with the new design.
/// PDF embedder
pdf_embedder: Option<PdfEmbedder>,
/// Block manager for reusable content
block_manager: Option<BlockManager>,
packages/ducpdf/src/duc2pdf/src/streaming/stream_resources.rs:226
process_pdf_filestoresResourceInfo { object_id: None }for PDFs, butstream_pdf_resourcelater requiresobject_id: Some(...)and otherwise returns a "no object ID" comment. As a result, PDF resources processed here cannot be streamed viaResourceStreamer. Either store the necessary XObject name/object id when embedding, or changestream_pdf_resourceto usePdfEmbedder/embed_id(and updateResourceInfo) so streaming PDFs works end-to-end.
Ok(ResourceInfo {
id: file_entry.key.clone(),
resource_type: ResourceType::Pdf,
object_id: None, // PDF embedder handles object IDs internally
width: Some(width),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let is_plot_mode = matches!(self.current_mode, StreamMode::Plot); | ||
| let (bounds_x, bounds_y, bounds_width, bounds_height) = bounds; | ||
| let bounds_max_x = bounds_x + bounds_width; | ||
| let bounds_max_y = bounds_y + bounds_height; | ||
| let (scroll_x, scroll_y) = if let Some(state) = local_state { | ||
| (state.scroll_x, state.scroll_y) | ||
| } else { | ||
| (0.0, 0.0) | ||
| }; | ||
| let (_bounds_x, _bounds_y, _bounds_width, _bounds_height) = bounds; | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
stream_elements_within_bounds no longer uses bounds (it is destructured into _bounds_*) and the earlier scroll/bounds calculations were removed. This effectively disables bounds-based culling in crop mode and can render elements outside the requested region while increasing work. Either reintroduce bounds-intersection filtering (with scroll offset) or rename/adjust the API/comment so callers don’t assume bounds are enforced.
| DucElementEnum::DucModelElement(_) => { | ||
| vec![Operation::new("% DucModelElement - WIP", vec![])] | ||
| } |
There was a problem hiding this comment.
The new DucModelElement branch currently only emits a "% ... - WIP" comment operation, so there is still no actual rendering support. If the PR intent is to "add DucModelElement support", either implement streaming for this element type or adjust the PR description/title to reflect that it is only scaffolding/placeholder handling for now.
| // Get SVG dimensions from the converted XObject | ||
| let (width, height) = self.get_svg_dimensions(file_entry); | ||
| let (width, height) = (100.0, 100.0); | ||
|
|
There was a problem hiding this comment.
The comment says "Get SVG dimensions from the converted XObject", but the code now hard-codes (100.0, 100.0). Either compute real dimensions (e.g., parse the SVG viewBox/width/height or inspect the produced XObject bbox) or update/remove the comment so callers don’t assume these values are meaningful.
| fill_from_stroke: false, | ||
| apply_stroke_properties: false, | ||
| }, | ||
| &DucElementEnum::DucModelElement(_) => todo!(), |
There was a problem hiding this comment.
determine_style_profile uses todo!() for DucModelElement, but stream_element_with_resources calls apply_styles for all non-PDF elements (including DucModelElement). This will panic at runtime as soon as a model element is encountered. Add a real StyleProfile mapping for DucModelElement (likely similar to other WIP/ignored elements) or explicitly skip style application for model elements until rendering is implemented.
| &DucElementEnum::DucModelElement(_) => todo!(), | |
| &DucElementEnum::DucModelElement(_) => StyleProfile { | |
| use_background_fill: false, | |
| fill_from_stroke: false, | |
| apply_stroke_properties: false, | |
| }, |
|
🎉 This PR is included in version 2.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.4.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
1 participant
…eration.