From aa14b3cd2af2f6c23954a1e55ef794eb126f2f10 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 3 Mar 2026 18:23:38 +0000 Subject: [PATCH] fix more --- .../src/presentation_artifact/pptx.rs | 20 +- .../src/presentation_artifact/render.rs | 207 ++++++++++++++++-- codex-rs/artifact-presentation/src/tests.rs | 84 ++++++- .../core/tests/suite/presentation_artifact.rs | 9 +- 4 files changed, 290 insertions(+), 30 deletions(-) diff --git a/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs b/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs index b9854745c7..ba53ca788b 100644 --- a/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs +++ b/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs @@ -1058,12 +1058,12 @@ fn export_text_body_xml( style.alignment.unwrap_or(TextAlignment::Left), rich_text, ); + let left_inset = points_to_emu(insets.left); + let right_inset = points_to_emu(insets.right); + let top_inset = points_to_emu(insets.top); + let bottom_inset = points_to_emu(insets.bottom); format!( - r#"{auto_fit}{paragraphs}"#, - points_to_emu(insets.left), - points_to_emu(insets.right), - points_to_emu(insets.top), - points_to_emu(insets.bottom), + r#"{auto_fit}{paragraphs}"# ) } @@ -1110,12 +1110,12 @@ fn export_table_cell_text_body_xml( TextAutoFitMode::ResizeShapeToFitText => "".to_string(), }; let insets = cell.rich_text.layout.insets.map(|insets| { + let left_inset = points_to_emu(insets.left); + let right_inset = points_to_emu(insets.right); + let top_inset = points_to_emu(insets.top); + let bottom_inset = points_to_emu(insets.bottom); format!( - r#" lIns="{}" rIns="{}" tIns="{}" bIns="{}""#, - points_to_emu(insets.left), - points_to_emu(insets.right), - points_to_emu(insets.top), - points_to_emu(insets.bottom), + r#" lIns="{left_inset}" rIns="{right_inset}" tIns="{top_inset}" bIns="{bottom_inset}""#, ) }); let paragraphs = export_text_paragraphs_xml( diff --git a/codex-rs/artifact-presentation/src/presentation_artifact/render.rs b/codex-rs/artifact-presentation/src/presentation_artifact/render.rs index 83e8cdfc71..7592136e62 100644 --- a/codex-rs/artifact-presentation/src/presentation_artifact/render.rs +++ b/codex-rs/artifact-presentation/src/presentation_artifact/render.rs @@ -153,7 +153,7 @@ fn render_slide_image( } let mut ordered = slide.elements.iter().collect::>(); - ordered.sort_by_key(|element| element.z_order()); + ordered.sort_by_key(|element| (preview_render_layer(element), element.z_order())); for element in ordered { render_element(&mut canvas, document, slide, element, render_scale)?; } @@ -1123,11 +1123,13 @@ fn layout_text_lines(glyphs: &[StyledGlyph], config: TextRenderConfig) -> Vec Vec config.width { - lines.push(TextLine { - glyphs: std::mem::take(&mut current), - width: current_width, - spaces: current_spaces, - }); + push_text_line( + &mut lines, + &mut current, + &mut current_width, + &mut current_spaces, + config.font_px, + ); current_width = 0; current_spaces = 0; + if matches!(glyph.ch, ' ' | '\t') { + continue; + } + } + if current.is_empty() && matches!(glyph.ch, ' ' | '\t') { + continue; } if glyph.ch == ' ' { current_spaces += 1; @@ -1152,15 +1162,41 @@ fn layout_text_lines(glyphs: &[StyledGlyph], config: TextRenderConfig) -> Vec, + current: &mut Vec, + current_width: &mut u32, + current_spaces: &mut usize, + font_px: u32, +) { + while let Some(glyph) = current.last() { + if !matches!(glyph.ch, ' ' | '\t') { + break; + } + let trimmed = current.pop().expect("last glyph must exist"); + *current_width = current_width.saturating_sub(measure_glyph_width(&trimmed, font_px)); + if trimmed.ch == ' ' { + *current_spaces = current_spaces.saturating_sub(1); + } + } + lines.push(TextLine { + glyphs: std::mem::take(current), + width: *current_width, + spaces: *current_spaces, + }); +} + fn draw_bitmap_glyph( image: &mut RgbaImage, left: u32, @@ -1637,6 +1673,17 @@ fn parse_rgba(hex: &str, alpha: u8) -> Rgba { Rgba([0, 0, 0, alpha]) } +fn preview_render_layer(element: &PresentationElement) -> usize { + match element { + PresentationElement::Text(_) + | PresentationElement::Shape(_) + | PresentationElement::Connector(_) + | PresentationElement::Image(ImageElement { payload: None, .. }) => 0, + PresentationElement::Image(_) | PresentationElement::Chart(_) => 1, + PresentationElement::Table(_) => 2, + } +} + fn shape_path( geometry: ShapeGeometry, width: u32, @@ -1645,9 +1692,10 @@ fn shape_path( let width = width.max(1) as f32; let height = height.max(1) as f32; match geometry { - ShapeGeometry::Rectangle | ShapeGeometry::RoundedRectangle => { + ShapeGeometry::Rectangle => { polygon_path(&[(0.0, 0.0), (width, 0.0), (width, height), (0.0, height)]) } + ShapeGeometry::RoundedRectangle => rounded_rectangle_path(width, height), ShapeGeometry::Ellipse => ellipse_path(width as u32, height as u32), ShapeGeometry::Triangle => polygon_path(&[(width / 2.0, 0.0), (width, height), (0.0, height)]), ShapeGeometry::RightTriangle => polygon_path(&[(0.0, 0.0), (width, height), (0.0, height)]), @@ -1799,6 +1847,55 @@ fn ellipse_path(width: u32, height: u32) -> Result Result { + let radius = (width.min(height) * 0.18).max(2.0); + let radius = radius.min(width / 2.0).min(height / 2.0); + let control = radius * (4.0 * (2.0_f32.sqrt() - 1.0) / 3.0); + let mut builder = PathBuilder::new(); + builder.move_to(radius, 0.0); + builder.line_to(width - radius, 0.0); + builder.cubic_to( + width - radius + control, + 0.0, + width, + radius - control, + width, + radius, + ); + builder.line_to(width, height - radius); + builder.cubic_to( + width, + height - radius + control, + width - radius + control, + height, + width - radius, + height, + ); + builder.line_to(radius, height); + builder.cubic_to( + radius - control, + height, + 0.0, + height - radius + control, + 0.0, + height - radius, + ); + builder.line_to(0.0, radius); + builder.cubic_to( + 0.0, + radius - control, + radius - control, + 0.0, + radius, + 0.0, + ); + builder.close(); + builder.finish().ok_or_else(|| PresentationArtifactError::RenderFailed { + action: "render_preview".to_string(), + message: "failed to build rounded rectangle path".to_string(), + }) +} + fn regular_polygon_path( sides: usize, width: f32, @@ -2171,3 +2268,79 @@ fn resolve_render_slide_index( } Ok(0) } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn wrapped_lines_skip_leading_spaces() { + let text = "One strong visual lands the story."; + let style = TextStyle::default(); + let glyphs = styled_glyphs(text, &style, &RichTextState::default(), None); + let prefix = "One strong visual lands the"; + let width = glyphs + .iter() + .take(prefix.chars().count()) + .map(|glyph| measure_glyph_width(glyph, 14)) + .sum(); + let lines = layout_text_lines( + &glyphs, + TextRenderConfig { + width, + font_px: 14, + line_height: 18, + baseline_offset: 12, + wrap: TextWrapMode::Square, + vertical_alignment: TextVerticalAlignment::Top, + alignment: TextAlignment::Left, + }, + ); + + assert_eq!(lines.len(), 2); + assert_eq!( + lines[0].glyphs.iter().map(|glyph| glyph.ch).collect::(), + prefix + ); + assert_eq!( + lines[1].glyphs.iter().map(|glyph| glyph.ch).collect::(), + "story." + ); + } + + #[test] + fn wrapped_lines_trim_trailing_spaces() { + let text = "One strong visual lands the story."; + let style = TextStyle::default(); + let glyphs = styled_glyphs(text, &style, &RichTextState::default(), None); + let prefix = "One strong visual lands the "; + let width = glyphs + .iter() + .take(prefix.chars().count()) + .map(|glyph| measure_glyph_width(glyph, 14)) + .sum(); + let lines = layout_text_lines( + &glyphs, + TextRenderConfig { + width, + font_px: 14, + line_height: 18, + baseline_offset: 12, + wrap: TextWrapMode::Square, + vertical_alignment: TextVerticalAlignment::Top, + alignment: TextAlignment::Left, + }, + ); + + assert_eq!(lines.len(), 2); + assert_eq!( + lines[0].glyphs.iter().map(|glyph| glyph.ch).collect::(), + "One strong visual lands the" + ); + assert_eq!( + lines[1].glyphs.iter().map(|glyph| glyph.ch).collect::(), + "story." + ); + } +} diff --git a/codex-rs/artifact-presentation/src/tests.rs b/codex-rs/artifact-presentation/src/tests.rs index 793d646976..6f468b586c 100644 --- a/codex-rs/artifact-presentation/src/tests.rs +++ b/codex-rs/artifact-presentation/src/tests.rs @@ -132,7 +132,7 @@ fn manager_can_import_exported_presentation() -> Result<(), Box Result<(), Box Result<(), Box Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let image_path = temp_dir.path().join("stacking-source.png"); + image::RgbaImage::from_pixel(40, 40, image::Rgba([0x2A, 0x80, 0xD7, 0xFF])) + .save(&image_path)?; + + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Preview stacking parity" }), + }, + temp_dir.path(), + )?; + let artifact_id = created.artifact_id; + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_slide".to_string(), + args: serde_json::json!({ + "background_fill": "#D9EAF7" + }), + }, + temp_dir.path(), + )?; + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_image".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "path": image_path, + "position": { "left": 160, "top": 80, "width": 180, "height": 180 }, + "fit": "stretch" + }), + }, + temp_dir.path(), + )?; + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_shape".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "geometry": "rounded_rectangle", + "position": { "left": 120, "top": 200, "width": 220, "height": 120 }, + "fill": "#FFFFFF" + }), + }, + temp_dir.path(), + )?; + + let target_path = temp_dir.path().join("stacking-preview.png"); + let response = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "export_preview".to_string(), + args: serde_json::json!({ + "path": target_path, + "slide_index": 0 + }), + }, + temp_dir.path(), + )?; + let preview = image::open( + response + .exported_paths + .first() + .expect("exported preview path present"), + )?; + + assert_eq!(preview.get_pixel(200, 220).0, [0x2A, 0x80, 0xD7, 0xFF]); + assert_eq!(preview.get_pixel(200, 290).0, [0xFF, 0xFF, 0xFF, 0xFF]); + assert_eq!(preview.get_pixel(122, 202).0, [0xD9, 0xEA, 0xF7, 0xFF]); + Ok(()) +} + #[test] fn export_preview_writes_all_slides_with_stable_names() -> Result<(), Box> { let temp_dir = tempfile::tempdir()?; diff --git a/codex-rs/core/tests/suite/presentation_artifact.rs b/codex-rs/core/tests/suite/presentation_artifact.rs index f3d6e743f9..affd7b2a9b 100644 --- a/codex-rs/core/tests/suite/presentation_artifact.rs +++ b/codex-rs/core/tests/suite/presentation_artifact.rs @@ -3,6 +3,7 @@ use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use codex_core::CodexAuth; +use codex_core::features::Feature; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::InputModality; @@ -46,7 +47,12 @@ async fn presentation_artifact_render_preview_returns_inline_image() -> anyhow:: cwd, session_configured, .. - } = test_codex().build(&server).await?; + } = test_codex() + .with_config(|config| { + config.features.enable(Feature::Artifact); + }) + .build(&server) + .await?; let call_id = "presentation-render-preview"; let arguments = serde_json::json!({ @@ -206,6 +212,7 @@ async fn presentation_artifact_render_preview_fails_for_text_only_model() -> any let TestCodex { codex, cwd, .. } = test_codex() .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) .with_config(|config| { + config.features.enable(Feature::Artifact); config.model = Some(model_slug.to_string()); }) .build(&server)