diff --git a/codex-rs/artifact-presentation/src/presentation_artifact/api.rs b/codex-rs/artifact-presentation/src/presentation_artifact/api.rs index 1b9127e998..24ef78bcc7 100644 --- a/codex-rs/artifact-presentation/src/presentation_artifact/api.rs +++ b/codex-rs/artifact-presentation/src/presentation_artifact/api.rs @@ -161,8 +161,16 @@ impl PresentationArtifactRequest { }], ImageInputSource::DataUrl(_) | ImageInputSource::Blob(_) - | ImageInputSource::Uri(_) | ImageInputSource::Placeholder => Vec::new(), + ImageInputSource::Uri(uri) => { + return Err(PresentationArtifactError::UnsupportedFeature { + action: self.action.clone(), + message: format!( + "remote image URIs are not supported for `{}`; download the image locally or provide `data_url`/`blob` instead (`{uri}`)", + self.action + ), + }); + } } } "replace_image" => { @@ -181,8 +189,16 @@ impl PresentationArtifactRequest { }], (None, Some(_), None, None, None) | (None, None, Some(_), None, None) - | (None, None, None, Some(_), None) | (None, None, None, None, Some(_)) => Vec::new(), + (None, None, None, Some(uri), None) => { + return Err(PresentationArtifactError::UnsupportedFeature { + action: self.action.clone(), + message: format!( + "remote image URIs are not supported for `{}`; download the image locally or provide `data_url`/`blob` instead (`{uri}`)", + self.action + ), + }); + } _ => { return Err(PresentationArtifactError::InvalidArgs { action: self.action.clone(), diff --git a/codex-rs/artifact-presentation/src/presentation_artifact/manager.rs b/codex-rs/artifact-presentation/src/presentation_artifact/manager.rs index fa21fb4d30..629f0e332e 100644 --- a/codex-rs/artifact-presentation/src/presentation_artifact/manager.rs +++ b/codex-rs/artifact-presentation/src/presentation_artifact/manager.rs @@ -219,6 +219,9 @@ impl PresentationArtifactManager { } })?; let mut document = PresentationDocument::from_ppt_rs(imported); + if let Some(slide_size) = import_pptx_slide_size(&path)? { + document.slide_size = slide_size; + } import_pptx_images(&path, &mut document, &request.action)?; document }; @@ -1589,10 +1592,6 @@ impl PresentationArtifactManager { ImageInputSource::Uri(uri) => Some(load_image_payload_from_uri(&uri, "replace_image")?), ImageInputSource::Placeholder => None, }; - let fit_mode = args.fit.unwrap_or(ImageFitMode::Stretch); - let lock_aspect_ratio = args - .lock_aspect_ratio - .unwrap_or(fit_mode != ImageFitMode::Stretch); let crop = args .crop .map(|crop| normalize_image_crop(crop, &request.action)) @@ -1606,8 +1605,10 @@ impl PresentationArtifactManager { }); }; image.payload = image_payload; - image.fit_mode = fit_mode; - image.crop = crop; + image.fit_mode = args.fit.unwrap_or(image.fit_mode); + if let Some(crop) = crop { + image.crop = Some(crop); + } if let Some(rotation) = args.rotation { image.rotation_degrees = Some(rotation); } @@ -1617,9 +1618,15 @@ impl PresentationArtifactManager { if let Some(flip_vertical) = args.flip_vertical { image.flip_vertical = flip_vertical; } - image.lock_aspect_ratio = lock_aspect_ratio; - image.alt_text = args.alt; - image.prompt = args.prompt; + if let Some(lock_aspect_ratio) = args.lock_aspect_ratio { + image.lock_aspect_ratio = lock_aspect_ratio; + } + if let Some(alt) = args.alt { + image.alt_text = Some(alt); + } + if let Some(prompt) = args.prompt { + image.prompt = Some(prompt); + } image.is_placeholder = is_placeholder; Ok(PresentationArtifactResponse::new( artifact_id, @@ -1871,12 +1878,43 @@ impl PresentationArtifactManager { message: format!("element `{}` is not a table", args.element_id), }); }; + let row_count = table.rows.len(); + let column_count = table.rows.first().map(Vec::len).unwrap_or(0); + let start_row = args.start_row as usize; + let end_row = args.end_row as usize; + let start_column = args.start_column as usize; + let end_column = args.end_column as usize; + if start_row > end_row || start_column > end_column { + return Err(PresentationArtifactError::InvalidArgs { + action: request.action, + message: "merge bounds must be ordered from top-left to bottom-right".to_string(), + }); + } + if end_row >= row_count || end_column >= column_count { + return Err(PresentationArtifactError::InvalidArgs { + action: request.action, + message: format!( + "merge bounds [{start_row},{start_column}]..=[{end_row},{end_column}] exceed table size {row_count}x{column_count}" + ), + }); + } let region = TableMergeRegion { - start_row: args.start_row as usize, - end_row: args.end_row as usize, - start_column: args.start_column as usize, - end_column: args.end_column as usize, + start_row, + end_row, + start_column, + end_column, }; + if table.merges.iter().any(|merge| { + merge.start_row <= region.end_row + && region.start_row <= merge.end_row + && merge.start_column <= region.end_column + && region.start_column <= merge.end_column + }) { + return Err(PresentationArtifactError::InvalidArgs { + action: request.action, + message: format!("merge region overlaps an existing merge in `{}`", args.element_id), + }); + } table.merges.push(region); Ok(PresentationArtifactResponse::new( artifact_id, @@ -2811,9 +2849,40 @@ impl PresentationArtifactManager { || text_layout.wrap.is_some() || text_layout.auto_fit.is_some() || text_layout.vertical_alignment.is_some(); + let position_rotation = args + .position + .as_ref() + .and_then(|position| position.rotation); + let position_flip_horizontal = args + .position + .as_ref() + .and_then(|position| position.flip_horizontal); + let position_flip_vertical = args + .position + .as_ref() + .and_then(|position| position.flip_vertical); + let has_position_transform = position_rotation.is_some() + || position_flip_horizontal.is_some() + || position_flip_vertical.is_some(); + let has_image_fields = + args.fit.is_some() || args.crop.is_some() || args.lock_aspect_ratio.is_some(); let element = document.find_element_mut(&args.element_id, &request.action)?; match element { PresentationElement::Text(text) => { + if args.stroke.is_some() + || args.rotation.is_some() + || args.flip_horizontal.is_some() + || args.flip_vertical.is_some() + || has_position_transform + || has_image_fields + { + return Err(PresentationArtifactError::UnsupportedFeature { + action: request.action, + message: + "text elements support only `position`, `z_order`, `fill`, and `text_layout` updates" + .to_string(), + }); + } if let Some(position) = args.position { text.frame = apply_partial_position(text.frame, position); } @@ -2823,32 +2892,23 @@ impl PresentationArtifactManager { if has_text_layout { text.rich_text.layout = text_layout; } - if args.stroke.is_some() - || args.rotation.is_some() - || args.flip_horizontal.is_some() - || args.flip_vertical.is_some() - { + } + PresentationElement::Shape(shape) => { + if has_image_fields { return Err(PresentationArtifactError::UnsupportedFeature { action: request.action, message: - "text elements support only `position`, `z_order`, and `fill` updates" + "shape elements support only `position`, `fill`, `stroke`, `rotation`, `flip_horizontal`, `flip_vertical`, `z_order`, and `text_layout` updates" .to_string(), }); } - } - PresentationElement::Shape(shape) => { - let position_rotation = args - .position - .as_ref() - .and_then(|position| position.rotation); - let position_flip_horizontal = args - .position - .as_ref() - .and_then(|position| position.flip_horizontal); - let position_flip_vertical = args - .position - .as_ref() - .and_then(|position| position.flip_vertical); + if has_text_layout && shape.text.is_none() { + return Err(PresentationArtifactError::UnsupportedFeature { + action: request.action, + message: "shape elements without text do not support `text_layout` updates" + .to_string(), + }); + } if let Some(position) = args.position { shape.frame = apply_partial_position(shape.frame, position); } @@ -2879,9 +2939,9 @@ impl PresentationArtifactManager { || args.rotation.is_some() || args.flip_horizontal.is_some() || args.flip_vertical.is_some() - || args.fit.is_some() - || args.crop.is_some() - || args.lock_aspect_ratio.is_some() + || has_position_transform + || has_image_fields + || has_text_layout { return Err(PresentationArtifactError::UnsupportedFeature { action: request.action, @@ -2914,19 +2974,7 @@ impl PresentationArtifactManager { } } PresentationElement::Image(image) => { - let position_rotation = args - .position - .as_ref() - .and_then(|position| position.rotation); - let position_flip_horizontal = args - .position - .as_ref() - .and_then(|position| position.flip_horizontal); - let position_flip_vertical = args - .position - .as_ref() - .and_then(|position| position.flip_vertical); - if args.fill.is_some() || args.stroke.is_some() { + if args.fill.is_some() || args.stroke.is_some() || has_text_layout { return Err(PresentationArtifactError::UnsupportedFeature { action: request.action, message: @@ -2965,6 +3013,9 @@ impl PresentationArtifactManager { || args.rotation.is_some() || args.flip_horizontal.is_some() || args.flip_vertical.is_some() + || has_position_transform + || has_image_fields + || has_text_layout { return Err(PresentationArtifactError::UnsupportedFeature { action: request.action, @@ -2982,6 +3033,9 @@ impl PresentationArtifactManager { || args.rotation.is_some() || args.flip_horizontal.is_some() || args.flip_vertical.is_some() + || has_position_transform + || has_image_fields + || has_text_layout { return Err(PresentationArtifactError::UnsupportedFeature { action: request.action, diff --git a/codex-rs/artifact-presentation/src/presentation_artifact/model.rs b/codex-rs/artifact-presentation/src/presentation_artifact/model.rs index f5cfc41afa..c2fc143fec 100644 --- a/codex-rs/artifact-presentation/src/presentation_artifact/model.rs +++ b/codex-rs/artifact-presentation/src/presentation_artifact/model.rs @@ -132,6 +132,44 @@ impl HyperlinkTarget { fn is_external(&self) -> bool { matches!(self, Self::Url(_) | Self::Email { .. } | Self::File(_)) } + + fn adjust_for_insert(&mut self, inserted_index: usize) { + if let Self::Slide(slide_index) = self + && *slide_index as usize >= inserted_index + { + *slide_index += 1; + } + } + + fn adjust_for_move(&mut self, from_index: usize, to_index: usize) { + let Self::Slide(slide_index_ref) = self else { + return; + }; + let slide_index = *slide_index_ref as usize; + *slide_index_ref = if slide_index == from_index { + to_index as u32 + } else if from_index < to_index && (from_index + 1..=to_index).contains(&slide_index) { + (slide_index - 1) as u32 + } else if to_index < from_index && (to_index..from_index).contains(&slide_index) { + (slide_index + 1) as u32 + } else { + *slide_index_ref + }; + } + + fn adjust_for_delete(&mut self, deleted_index: usize) -> bool { + let Self::Slide(slide_index_ref) = self else { + return false; + }; + let slide_index = *slide_index_ref as usize; + if slide_index == deleted_index { + return true; + } + if slide_index > deleted_index { + *slide_index_ref -= 1; + } + false + } } impl HyperlinkState { @@ -217,6 +255,18 @@ impl HyperlinkState { ppt_rs::escape_xml(&self.target.relationship_target()), ) } + + fn adjust_for_insert(&mut self, inserted_index: usize) { + self.target.adjust_for_insert(inserted_index); + } + + fn adjust_for_move(&mut self, from_index: usize, to_index: usize) { + self.target.adjust_for_move(from_index, to_index); + } + + fn adjust_for_delete(&mut self, deleted_index: usize) -> bool { + self.target.adjust_for_delete(deleted_index) + } } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] @@ -714,6 +764,7 @@ impl PresentationDocument { } Some(_) => {} } + self.adjust_hyperlinks_for_insert(inserted_index); } fn adjust_active_slide_for_move(&mut self, from_index: usize, to_index: usize) { @@ -728,6 +779,7 @@ impl PresentationDocument { active_index }); } + self.adjust_hyperlinks_for_move(from_index, to_index); } fn adjust_active_slide_for_delete(&mut self, deleted_index: usize) { @@ -740,6 +792,36 @@ impl PresentationDocument { Some(active_index) if deleted_index < active_index => Some(active_index - 1), Some(active_index) => Some(active_index), }; + self.adjust_hyperlinks_for_delete(deleted_index); + } + + fn adjust_hyperlinks_for_insert(&mut self, inserted_index: usize) { + self.visit_hyperlinks_mut(|hyperlink| { + hyperlink.adjust_for_insert(inserted_index); + true + }); + } + + fn adjust_hyperlinks_for_move(&mut self, from_index: usize, to_index: usize) { + self.visit_hyperlinks_mut(|hyperlink| { + hyperlink.adjust_for_move(from_index, to_index); + true + }); + } + + fn adjust_hyperlinks_for_delete(&mut self, deleted_index: usize) { + self.visit_hyperlinks_mut(|hyperlink| !hyperlink.adjust_for_delete(deleted_index)); + } + + fn visit_hyperlinks_mut(&mut self, mut visit: F) + where + F: FnMut(&mut HyperlinkState) -> bool, + { + for slide in &mut self.slides { + for element in &mut slide.elements { + element.visit_hyperlinks_mut(&mut visit); + } + } } fn next_layout_id(&mut self) -> String { @@ -1053,6 +1135,38 @@ fn import_pptx_images( Ok(()) } +fn import_pptx_slide_size(path: &Path) -> Result, PresentationArtifactError> { + let file = std::fs::File::open(path).map_err(|error| PresentationArtifactError::ImportFailed { + path: path.to_path_buf(), + message: error.to_string(), + })?; + let mut archive = + ZipArchive::new(file).map_err(|error| PresentationArtifactError::ImportFailed { + path: path.to_path_buf(), + message: error.to_string(), + })?; + let Some(xml) = zip_entry_string_if_exists(&mut archive, "ppt/presentation.xml").map_err( + |message| PresentationArtifactError::ImportFailed { + path: path.to_path_buf(), + message, + }, + )? + else { + return Ok(None); + }; + let Some(width) = + xml_tag_attribute(&xml, "().ok()) + else { + return Ok(None); + }; + let Some(height) = + xml_tag_attribute(&xml, "().ok()) + else { + return Ok(None); + }; + Ok(Some(Rect::from_emu(0, 0, width, height))) +} + fn zip_entry_string_if_exists( archive: &mut ZipArchive, path: &str, @@ -1479,6 +1593,45 @@ impl PresentationElement { Self::Chart(element) => element.z_order = z_order, } } + + fn visit_hyperlinks_mut(&mut self, visit: &mut F) + where + F: FnMut(&mut HyperlinkState) -> bool, + { + match self { + Self::Text(element) => { + if let Some(hyperlink) = element.hyperlink.as_mut() + && !visit(hyperlink) + { + element.hyperlink = None; + } + for range in &mut element.rich_text.ranges { + if let Some(hyperlink) = range.hyperlink.as_mut() + && !visit(hyperlink) + { + range.hyperlink = None; + } + } + } + Self::Shape(element) => { + if let Some(hyperlink) = element.hyperlink.as_mut() + && !visit(hyperlink) + { + element.hyperlink = None; + } + if let Some(rich_text) = element.rich_text.as_mut() { + for range in &mut rich_text.ranges { + if let Some(hyperlink) = range.hyperlink.as_mut() + && !visit(hyperlink) + { + range.hyperlink = None; + } + } + } + } + Self::Connector(_) | Self::Image(_) | Self::Table(_) | Self::Chart(_) => {} + } + } } #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs b/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs index e297e03ac6..6d376e4b8d 100644 --- a/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs +++ b/codex-rs/artifact-presentation/src/presentation_artifact/pptx.rs @@ -1092,8 +1092,7 @@ fn export_text_body_xml( let top_inset = points_to_emu(insets.top); let bottom_inset = points_to_emu(insets.bottom); format!( - // codespell:ignore lIns,rIns,tIns,bIns - r#"{auto_fit}{paragraphs}"# + r#"{auto_fit}{paragraphs}"# // codespell:ignore lIns,rIns,tIns,bIns ) } @@ -1145,8 +1144,7 @@ fn export_table_cell_text_body_xml( let top_inset = points_to_emu(insets.top); let bottom_inset = points_to_emu(insets.bottom); format!( - // codespell:ignore lIns,rIns,tIns,bIns - r#" lIns="{left_inset}" rIns="{right_inset}" tIns="{top_inset}" bIns="{bottom_inset}""#, + r#" lIns="{left_inset}" rIns="{right_inset}" tIns="{top_inset}" bIns="{bottom_inset}""#, // codespell:ignore lIns,rIns,tIns,bIns ) }); 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 d4f1e10101..01fab77dee 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| (preview_render_layer(element), element.z_order())); + ordered.sort_by_key(|element| element.z_order()); for element in ordered { render_element(&mut canvas, document, slide, element, render_scale)?; } @@ -682,38 +682,41 @@ fn render_bar_chart( height: u32, scale: f32, ) { - let baseline = top + height; - draw_chart_axes(image, left, top, width, height); + let (min_value, max_value) = chart_value_domain(chart); + let baseline = chart_axis_y(top, height, min_value, max_value, 0.0); + draw_chart_axes(image, left, top, width, height, baseline); let series_count = chart.series.len().max(1) as u32; let category_count = chart.categories.len().max(1) as u32; - let max_value = chart - .series - .iter() - .flat_map(|series| series.values.iter().copied()) - .fold(0.0f64, f64::max) - .max(1.0); let group_width = width as f32 / category_count as f32; let bar_width = (group_width / series_count as f32 * 0.72).max(2.0); for (series_index, series) in chart.series.iter().enumerate() { let color = chart_series_color(series, series_index); for (value_index, value) in series.values.iter().enumerate() { - let bar_height = ((*value / max_value) as f32 * height as f32).round().max(0.0) as u32; let x = left as f32 + group_width * value_index as f32 + (group_width - bar_width * series_count as f32) / 2.0 + bar_width * series_index as f32; - let y = baseline.saturating_sub(bar_height); + let value_y = chart_axis_y(top, height, min_value, max_value, *value); + let y = value_y.min(baseline); + let bar_height = value_y.abs_diff(baseline).max(1); fill_rect( image, x.round() as u32, y, bar_width.round().max(1.0) as u32, - bar_height.max(1), + bar_height, color, ); } } - render_category_labels(image, &chart.categories, left, baseline + 4, width, scale); + render_category_labels( + image, + &chart.categories, + left, + top + height + 4, + width, + scale, + ); } fn render_line_chart( @@ -725,21 +728,16 @@ fn render_line_chart( height: u32, scale: f32, ) { - draw_chart_axes(image, left, top, width, height); - let baseline = top + height; + let (min_value, max_value) = chart_value_domain(chart); + let baseline = chart_axis_y(top, height, min_value, max_value, 0.0); + draw_chart_axes(image, left, top, width, height, baseline); let point_count = chart.categories.len().max(2); - let max_value = chart - .series - .iter() - .flat_map(|series| series.values.iter().copied()) - .fold(0.0f64, f64::max) - .max(1.0); for (series_index, series) in chart.series.iter().enumerate() { let color = chart_series_color(series, series_index); let mut points = Vec::new(); for (value_index, value) in series.values.iter().enumerate() { let x = left as f32 + width as f32 * value_index as f32 / (point_count - 1) as f32; - let y = baseline as f32 - ((*value / max_value) as f32 * height as f32); + let y = chart_axis_y(top, height, min_value, max_value, *value) as f32; points.push((x, y)); } if points.len() >= 2 @@ -770,7 +768,38 @@ fn render_line_chart( ); } } - render_category_labels(image, &chart.categories, left, baseline + 4, width, scale); + render_category_labels( + image, + &chart.categories, + left, + top + height + 4, + width, + scale, + ); +} + +fn chart_value_domain(chart: &ChartElement) -> (f64, f64) { + let mut min_value = 0.0f64; + let mut max_value = 0.0f64; + for value in chart + .series + .iter() + .flat_map(|series| series.values.iter().copied()) + { + min_value = min_value.min(value); + max_value = max_value.max(value); + } + if (max_value - min_value).abs() < f64::EPSILON { + max_value += 1.0; + } + (min_value, max_value) +} + +fn chart_axis_y(top: u32, height: u32, min_value: f64, max_value: f64, value: f64) -> u32 { + let span = (max_value - min_value).max(f64::EPSILON); + let normalized = ((value - min_value) / span).clamp(0.0, 1.0); + let bottom = top + height; + bottom.saturating_sub((normalized * height as f64).round() as u32) } fn render_pie_chart( @@ -1675,17 +1704,6 @@ 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, @@ -2161,10 +2179,17 @@ fn draw_vertical_line(image: &mut RgbaImage, x: u32, color: Rgba, thickness: fill_rect(image, x.saturating_sub(thickness / 2), 0, thickness.max(1), image.height(), color); } -fn draw_chart_axes(image: &mut RgbaImage, left: u32, top: u32, width: u32, height: u32) { +fn draw_chart_axes( + image: &mut RgbaImage, + left: u32, + top: u32, + width: u32, + plot_height: u32, + baseline: u32, +) { let axis_color = parse_rgba(DEFAULT_TABLE_GRID_HEX, 255); - fill_rect(image, left, top, 1, height, axis_color); - fill_rect(image, left, top + height, width, 1, axis_color); + fill_rect(image, left, top, 1, plot_height, axis_color); + fill_rect(image, left, baseline, width, 1, axis_color); } fn render_category_labels( diff --git a/codex-rs/artifact-presentation/src/presentation_artifact/snapshot.rs b/codex-rs/artifact-presentation/src/presentation_artifact/snapshot.rs index df3eb8af2c..eb85685078 100644 --- a/codex-rs/artifact-presentation/src/presentation_artifact/snapshot.rs +++ b/codex-rs/artifact-presentation/src/presentation_artifact/snapshot.rs @@ -230,54 +230,12 @@ fn load_image_payload_from_uri( uri: &str, action: &str, ) -> Result { - let response = - reqwest::blocking::get(uri).map_err(|error| PresentationArtifactError::InvalidArgs { - action: action.to_string(), - message: format!("failed to fetch image `{uri}`: {error}"), - })?; - let status = response.status(); - if !status.is_success() { - return Err(PresentationArtifactError::InvalidArgs { - action: action.to_string(), - message: format!("failed to fetch image `{uri}`: HTTP {status}"), - }); - } - let content_type = response - .headers() - .get(reqwest::header::CONTENT_TYPE) - .and_then(|value| value.to_str().ok()) - .map(|value| value.split(';').next().unwrap_or(value).trim().to_string()); - let bytes = response - .bytes() - .map_err(|error| PresentationArtifactError::InvalidArgs { - action: action.to_string(), - message: format!("failed to read image `{uri}`: {error}"), - })?; - build_image_payload( - bytes.to_vec(), - infer_remote_image_filename(uri, content_type.as_deref()), - action, - ) -} - -fn infer_remote_image_filename(uri: &str, content_type: Option<&str>) -> String { - let path_name = reqwest::Url::parse(uri) - .ok() - .and_then(|url| { - url.path_segments() - .and_then(Iterator::last) - .map(str::to_owned) - }) - .filter(|segment| !segment.is_empty()); - match (path_name, content_type) { - (Some(path_name), _) if Path::new(&path_name).extension().is_some() => path_name, - (Some(path_name), Some(content_type)) => { - format!("{path_name}.{}", image_extension_from_mime(content_type)) - } - (Some(path_name), None) => path_name, - (None, Some(content_type)) => format!("image.{}", image_extension_from_mime(content_type)), - (None, None) => "image.png".to_string(), - } + Err(PresentationArtifactError::UnsupportedFeature { + action: action.to_string(), + message: format!( + "remote image URIs are not supported for `{action}`; download the image locally or provide `data_url`/`blob` instead (`{uri}`)" + ), + }) } fn build_image_payload( diff --git a/codex-rs/artifact-presentation/src/tests.rs b/codex-rs/artifact-presentation/src/tests.rs index 17ad741e7d..4646ce5c5b 100644 --- a/codex-rs/artifact-presentation/src/tests.rs +++ b/codex-rs/artifact-presentation/src/tests.rs @@ -3,6 +3,7 @@ use base64::Engine; use image::GenericImageView; use pretty_assertions::assert_eq; use std::io::Read; +use std::io::Write; fn zip_entry_text( path: &std::path::Path, @@ -22,6 +23,63 @@ fn zip_entry_names(path: &std::path::Path) -> Result, Box Result<(), Box> { + let input = std::fs::File::open(source)?; + let mut archive = zip::ZipArchive::new(input)?; + let output = std::fs::File::create(target)?; + let mut writer = zip::ZipWriter::new(output); + for index in 0..archive.len() { + let mut entry = archive.by_index(index)?; + let name = entry.name().to_string(); + if name == "ppt/codex-document.json" { + continue; + } + let mut bytes = Vec::new(); + entry.read_to_end(&mut bytes)?; + if name == "ppt/presentation.xml" { + let xml = String::from_utf8(bytes)?; + bytes = replace_slide_size_in_presentation_xml(&xml, slide_width_emu, slide_height_emu) + .into_bytes(); + } + writer.start_file(name, zip::write::SimpleFileOptions::default())?; + writer.write_all(&bytes)?; + } + writer.finish()?; + Ok(()) +} + +fn replace_slide_size_in_presentation_xml(xml: &str, width_emu: u32, height_emu: u32) -> String { + let Some(tag_start) = xml.find("") else { + return xml.to_string(); + }; + let tag_end = tag_start + tag_end_offset; + let tag = &xml[tag_start..tag_end]; + let tag = replace_xml_attribute(tag, "cx", &width_emu.to_string()); + let tag = replace_xml_attribute(&tag, "cy", &height_emu.to_string()); + format!("{}{tag}/>{}", &xml[..tag_start], &xml[tag_end + 2..]) +} + +fn replace_xml_attribute(tag: &str, attribute: &str, value: &str) -> String { + let needle = format!(r#"{attribute}=""#); + let Some(start) = tag.find(&needle) else { + return tag.to_string(); + }; + let value_start = start + needle.len(); + let Some(value_end_offset) = tag[value_start..].find('"') else { + return tag.to_string(); + }; + let value_end = value_start + value_end_offset; + format!("{}{}{}", &tag[..value_start], value, &tag[value_end..]) +} + fn parse_ndjson_lines(ndjson: &str) -> Result, serde_json::Error> { ndjson .lines() @@ -52,6 +110,27 @@ fn rect_contains_pixel( false } +fn rect_contains_pixel_where( + image: &image::DynamicImage, + left: u32, + top: u32, + width: u32, + height: u32, + predicate: F, +) -> bool +where + F: Fn([u8; 4]) -> bool, +{ + for y in top..top.saturating_add(height) { + for x in left..left.saturating_add(width) { + if predicate(image.get_pixel(x, y).0) { + return true; + } + } + } + false +} + #[test] fn manager_can_create_add_text_and_export() -> Result<(), Box> { let temp_dir = tempfile::tempdir()?; @@ -1022,29 +1101,10 @@ fn preview_image_writer_supports_jpeg_scale_and_svg() -> Result<(), Box Result<(), Box> { - let mut image_bytes = std::io::Cursor::new(Vec::new()); - image::DynamicImage::ImageRgba8(image::RgbaImage::from_pixel( - 16, - 8, - image::Rgba([0x11, 0x88, 0xCC, 0xFF]), - )) - .write_to(&mut image_bytes, image::ImageFormat::Png)?; - let png = image_bytes.into_inner(); - - let server = tiny_http::Server::http("127.0.0.1:0").expect("server"); - let port = server.server_addr().to_ip().expect("ip addr").port(); - let server_thread = std::thread::spawn(move || { - for request in server.incoming_requests().take(2) { - let response = tiny_http::Response::from_data(png.clone()).with_header( - tiny_http::Header::from_bytes(&b"Content-Type"[..], &b"image/png"[..]) - .expect("header"), - ); - request.respond(response).expect("respond"); - } - }); - +fn image_uris_are_rejected_for_add_and_replace_images() -> Result<(), Box> { let temp_dir = tempfile::tempdir()?; + let image_path = temp_dir.path().join("local.png"); + image::RgbaImage::from_pixel(16, 8, image::Rgba([0x11, 0x88, 0xCC, 0xFF])).save(&image_path)?; let mut manager = PresentationArtifactManager::default(); let created = manager.execute( PresentationArtifactRequest { @@ -1063,14 +1123,32 @@ fn image_uris_can_add_and_replace_images() -> Result<(), Box Result<(), Box Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let original_path = temp_dir.path().join("original.png"); + let replacement_path = temp_dir.path().join("replacement.png"); + image::RgbaImage::from_pixel(24, 12, image::Rgba([0x11, 0x88, 0xCC, 0xFF])) + .save(&original_path)?; + image::RgbaImage::from_pixel(32, 16, image::Rgba([0xD0, 0x44, 0x55, 0xFF])) + .save(&replacement_path)?; + let replacement_blob = + base64::engine::general_purpose::STANDARD.encode(std::fs::read(&replacement_path)?); + + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Replace image state" }), + }, + 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!({}), + }, + temp_dir.path(), + )?; + let added = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_image".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "path": original_path, + "position": { "left": 24, "top": 24, "width": 200, "height": 120 }, + "fit": "contain", + "alt": "Existing alt text" + }), + }, + temp_dir.path(), + )?; + let image_id = added + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.first()) + .cloned() + .expect("image id"); + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("im/{image_id}"), + "fit": "cover", + "crop": { "left": 0.15, "top": 0.05, "right": 0.10, "bottom": 0.0 }, + "lock_aspect_ratio": true + }), + }, + temp_dir.path(), + )?; manager.execute( PresentationArtifactRequest { artifact_id: Some(artifact_id.clone()), action: "replace_image".to_string(), args: serde_json::json!({ - "element_id": format!("im/{element_id}"), - "uri": format!("http://127.0.0.1:{port}/updated.png"), - "fit": "contain" + "element_id": format!("im/{image_id}"), + "blob": replacement_blob }), }, temp_dir.path(), )?; - let inspect = manager.execute( + let resolved = manager.execute( PresentationArtifactRequest { artifact_id: Some(artifact_id), - action: "inspect".to_string(), - args: serde_json::json!({ "kind": "image" }), + action: "resolve".to_string(), + args: serde_json::json!({ "id": format!("im/{image_id}") }), }, temp_dir.path(), )?; - assert!( - inspect - .inspect_ndjson - .expect("image inspect") - .contains("\"fit\":\"Contain\"") + let record = resolved.resolved_record.expect("resolved image"); + assert_eq!(record["fit"], "Cover"); + assert_eq!(record["alt"], "Existing alt text"); + assert_eq!(record["lockAspectRatio"], true); + assert_eq!(record["isPlaceholder"], false); + assert_eq!( + record + .get("crop") + .and_then(serde_json::Value::as_object) + .and_then(|crop| crop.get("left")) + .and_then(serde_json::Value::as_f64), + Some(0.15) ); - server_thread.join().expect("server thread"); Ok(()) } @@ -1591,6 +1758,141 @@ fn hyperlinks_are_inspectable_and_exported() -> Result<(), Box Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Slide hyperlinks" }), + }, + temp_dir.path(), + )?; + let artifact_id = created.artifact_id; + for slide_index in 0..3 { + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_slide".to_string(), + args: serde_json::json!({ "notes": format!("slide {slide_index}") }), + }, + temp_dir.path(), + )?; + } + + let shape = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_shape".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "geometry": "rectangle", + "position": { "left": 24, "top": 24, "width": 180, "height": 60 }, + "text": "Go" + }), + }, + temp_dir.path(), + )?; + let shape_id = shape + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.first()) + .cloned() + .expect("shape id"); + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "set_hyperlink".to_string(), + args: serde_json::json!({ + "element_id": format!("sh/{shape_id}"), + "link_type": "slide", + "slide_index": 2 + }), + }, + temp_dir.path(), + )?; + + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "insert_slide".to_string(), + args: serde_json::json!({ "index": 0 }), + }, + temp_dir.path(), + )?; + let after_insert = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "resolve".to_string(), + args: serde_json::json!({ "id": format!("sh/{shape_id}") }), + }, + temp_dir.path(), + )?; + assert_eq!( + after_insert + .resolved_record + .as_ref() + .and_then(|record| record.get("hyperlink")) + .and_then(|hyperlink| hyperlink.get("slideIndex")) + .and_then(serde_json::Value::as_u64), + Some(3) + ); + + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "move_slide".to_string(), + args: serde_json::json!({ "from_index": 3, "to_index": 1 }), + }, + temp_dir.path(), + )?; + let after_move = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "resolve".to_string(), + args: serde_json::json!({ "id": format!("sh/{shape_id}") }), + }, + temp_dir.path(), + )?; + assert_eq!( + after_move + .resolved_record + .as_ref() + .and_then(|record| record.get("hyperlink")) + .and_then(|hyperlink| hyperlink.get("slideIndex")) + .and_then(serde_json::Value::as_u64), + Some(1) + ); + + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "delete_slide".to_string(), + args: serde_json::json!({ "slide_index": 1 }), + }, + temp_dir.path(), + )?; + let after_delete = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "resolve".to_string(), + args: serde_json::json!({ "id": format!("sh/{shape_id}") }), + }, + temp_dir.path(), + )?; + assert_eq!( + after_delete + .resolved_record + .as_ref() + .and_then(|record| record.get("hyperlink")), + None + ); + Ok(()) +} + #[test] fn manager_supports_layout_theme_notes_and_inspect() -> Result<(), Box> { let temp_dir = tempfile::tempdir()?; @@ -2666,6 +2968,74 @@ fn notes_visibility_controls_exported_notes() -> Result<(), Box Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ + "name": "Custom size", + "slide_size": { "width": 960, "height": 540 } + }), + }, + 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!({}), + }, + temp_dir.path(), + )?; + + let codex_path = temp_dir.path().join("codex-sized.pptx"); + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "export_pptx".to_string(), + args: serde_json::json!({ "path": codex_path }), + }, + temp_dir.path(), + )?; + + let imported_source = temp_dir.path().join("non-codex-sized.pptx"); + rewrite_pptx_without_codex_metadata( + &temp_dir.path().join("codex-sized.pptx"), + &imported_source, + 12_192_000, + 6_858_000, + )?; + + let imported = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "import_pptx".to_string(), + args: serde_json::json!({ "path": imported_source }), + }, + temp_dir.path(), + )?; + let roundtrip_path = temp_dir.path().join("roundtrip-sized.pptx"); + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(imported.artifact_id), + action: "export_pptx".to_string(), + args: serde_json::json!({ "path": roundtrip_path }), + }, + temp_dir.path(), + )?; + let presentation_xml = zip_entry_text( + &temp_dir.path().join("roundtrip-sized.pptx"), + "ppt/presentation.xml", + )?; + assert!(presentation_xml.contains(r#" Result<(), Box> { let temp_dir = tempfile::tempdir()?; @@ -3008,6 +3378,378 @@ fn connectors_support_arrows_and_inspect() -> Result<(), Box Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Atomic text style" }), + }, + 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!({}), + }, + temp_dir.path(), + )?; + let added = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_text_shape".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "text": "Atomic", + "position": { "left": 40, "top": 40, "width": 180, "height": 60 } + }), + }, + temp_dir.path(), + )?; + let text_id = added + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.first()) + .cloned() + .expect("text id"); + let error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("sh/{text_id}"), + "fill": "#00AA00", + "stroke": { "color": "#222222", "width": 2 } + }), + }, + temp_dir.path(), + ) + .expect_err("unsupported stroke should reject the request"); + assert!(matches!( + error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + let resolved = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "resolve".to_string(), + args: serde_json::json!({ "id": format!("sh/{text_id}") }), + }, + temp_dir.path(), + )?; + assert_eq!( + resolved + .resolved_record + .as_ref() + .and_then(|record| record.get("fill")) + .cloned(), + None + ); + Ok(()) +} + +#[test] +fn update_shape_style_rejects_silently_ignored_text_shape_and_image_fields() +-> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Unsupported style args" }), + }, + 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!({}), + }, + temp_dir.path(), + )?; + let text = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_text_shape".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "text": "Unsupported", + "position": { "left": 24, "top": 24, "width": 160, "height": 60 } + }), + }, + temp_dir.path(), + )?; + let text_id = text + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.first()) + .cloned() + .expect("text id"); + let added_shape = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_shape".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "geometry": "rectangle", + "position": { "left": 24, "top": 120, "width": 160, "height": 80 }, + "fill": "#cccccc" + }), + }, + temp_dir.path(), + )?; + let shape_id = added_shape + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.get(1)) + .cloned() + .expect("shape id"); + let added_image = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_image".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "position": { "left": 220, "top": 24, "width": 120, "height": 80 }, + "prompt": "Placeholder image" + }), + }, + temp_dir.path(), + )?; + let image_id = added_image + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.last()) + .cloned() + .expect("image id"); + + let text_error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("sh/{text_id}"), + "position": { "flip_horizontal": true } + }), + }, + temp_dir.path(), + ) + .expect_err("text nested transforms should be rejected"); + assert!(matches!( + text_error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + + let shape_fit_error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("sh/{shape_id}"), + "fit": "contain" + }), + }, + temp_dir.path(), + ) + .expect_err("shape fit should be rejected"); + assert!(matches!( + shape_fit_error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + + let shape_layout_error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("sh/{shape_id}"), + "text_layout": { "wrap": "none" } + }), + }, + temp_dir.path(), + ) + .expect_err("shape text_layout without text should be rejected"); + assert!(matches!( + shape_layout_error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + + let image_layout_error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("im/{image_id}"), + "text_layout": { "wrap": "none" } + }), + }, + temp_dir.path(), + ) + .expect_err("image text_layout should be rejected"); + assert!(matches!( + image_layout_error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + Ok(()) +} + +#[test] +fn update_shape_style_rejects_silently_ignored_connector_table_and_chart_fields() +-> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Unsupported style args 2" }), + }, + 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!({}), + }, + temp_dir.path(), + )?; + let connector = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_connector".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "connector_type": "straight", + "start": { "left": 20, "top": 20 }, + "end": { "left": 180, "top": 160 } + }), + }, + temp_dir.path(), + )?; + let connector_id = connector + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.first()) + .cloned() + .expect("connector id"); + let table = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_table".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "position": { "left": 220, "top": 20, "width": 180, "height": 100 }, + "rows": [["A", "B"], ["C", "D"]] + }), + }, + temp_dir.path(), + )?; + let table_id = table + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.get(1)) + .cloned() + .expect("table id"); + let chart = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_chart".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "chart_type": "bar", + "position": { "left": 24, "top": 220, "width": 220, "height": 140 }, + "categories": ["A", "B"], + "series": [{ "name": "Actual", "values": [1, 2] }] + }), + }, + temp_dir.path(), + )?; + let chart_id = chart + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.last()) + .cloned() + .expect("chart id"); + + let connector_error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("cn/{connector_id}"), + "position": { "rotation": 10 } + }), + }, + temp_dir.path(), + ) + .expect_err("connector nested transforms should be rejected"); + assert!(matches!( + connector_error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + + let table_error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("tb/{table_id}"), + "position": { "flip_vertical": true } + }), + }, + temp_dir.path(), + ) + .expect_err("table nested transforms should be rejected"); + assert!(matches!( + table_error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + + let chart_error = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("ch/{chart_id}"), + "text_layout": { "wrap": "none" } + }), + }, + temp_dir.path(), + ) + .expect_err("chart text_layout should be rejected"); + assert!(matches!( + chart_error, + PresentationArtifactError::UnsupportedFeature { .. } + )); + Ok(()) +} + #[test] fn shapes_support_stroke_dash_styles() -> Result<(), Box> { let temp_dir = tempfile::tempdir()?; @@ -3404,6 +4146,109 @@ fn manager_supports_table_cell_updates_and_merges() -> Result<(), Box Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Merge validation" }), + }, + 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!({}), + }, + temp_dir.path(), + )?; + let added = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_table".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "position": { "left": 32, "top": 48, "width": 240, "height": 120 }, + "rows": [ + ["A1", "B1"], + ["A2", "B2"] + ] + }), + }, + temp_dir.path(), + )?; + let table_id = added + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.first()) + .map(|id| format!("tb/{id}")) + .expect("table id"); + + let reversed = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "merge_table_cells".to_string(), + args: serde_json::json!({ + "element_id": table_id, + "start_row": 1, + "end_row": 0, + "start_column": 0, + "end_column": 1 + }), + }, + temp_dir.path(), + ) + .expect_err("reversed merge should be rejected"); + assert!(matches!( + reversed, + PresentationArtifactError::InvalidArgs { .. } + )); + + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "merge_table_cells".to_string(), + args: serde_json::json!({ + "element_id": table_id, + "start_row": 0, + "end_row": 0, + "start_column": 0, + "end_column": 1 + }), + }, + temp_dir.path(), + )?; + + let overlapping = manager + .execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "merge_table_cells".to_string(), + args: serde_json::json!({ + "element_id": table_id, + "start_row": 0, + "end_row": 1, + "start_column": 1, + "end_column": 1 + }), + }, + temp_dir.path(), + ) + .expect_err("overlapping merge should be rejected"); + assert!(matches!( + overlapping, + PresentationArtifactError::InvalidArgs { .. } + )); + Ok(()) +} + #[test] fn rich_text_comments_tables_and_charts_roundtrip_through_metadata() -> Result<(), Box> { @@ -4342,6 +5187,111 @@ fn render_preview_renders_chart_primitives() -> Result<(), Box Result<(), Box> +{ + let temp_dir = tempfile::tempdir()?; + let mut manager = PresentationArtifactManager::default(); + let created = manager.execute( + PresentationArtifactRequest { + artifact_id: None, + action: "create".to_string(), + args: serde_json::json!({ "name": "Negative charts" }), + }, + temp_dir.path(), + )?; + let artifact_id = created.artifact_id; + for _ in 0..2 { + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_slide".to_string(), + args: serde_json::json!({}), + }, + temp_dir.path(), + )?; + } + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_chart".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "chart_type": "bar", + "position": { "left": 120, "top": 80, "width": 360, "height": 220 }, + "categories": ["Loss", "Gain"], + "series": [ + { "name": "Actual", "values": [-4, 6], "fill": "#4E79A7" } + ] + }), + }, + temp_dir.path(), + )?; + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_chart".to_string(), + args: serde_json::json!({ + "slide_index": 1, + "chart_type": "line", + "position": { "left": 120, "top": 80, "width": 360, "height": 220 }, + "categories": ["Loss", "Gain"], + "series": [ + { "name": "Actual", "values": [-4, 6], "fill": "#4E79A7" } + ] + }), + }, + temp_dir.path(), + )?; + + let bar_preview = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "render_preview".to_string(), + args: serde_json::json!({ "slide_index": 0 }), + }, + temp_dir.path(), + )?; + let bar_image = load_preview(&bar_preview.rendered_preview.expect("bar preview").png_bytes)?; + assert!( + rect_contains_pixel(&bar_image, 150, 210, 90, 80, [0x4E, 0x79, 0xA7, 0xFF]), + "expected negative bar fill in the lower plot half" + ); + assert!( + rect_contains_pixel(&bar_image, 280, 110, 90, 80, [0x4E, 0x79, 0xA7, 0xFF]), + "expected positive bar fill in the upper plot half" + ); + + let line_preview = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "render_preview".to_string(), + args: serde_json::json!({ "slide_index": 1 }), + }, + temp_dir.path(), + )?; + let line_image = load_preview( + &line_preview + .rendered_preview + .expect("line preview") + .png_bytes, + )?; + let is_series_pixel = |pixel: [u8; 4]| { + pixel[3] == 0xFF + && u16::from(pixel[2]) > u16::from(pixel[0]) + 20 + && u16::from(pixel[2]) > u16::from(pixel[1]) + 20 + }; + assert!( + rect_contains_pixel_where(&line_image, 0, 220, 360, 140, is_series_pixel), + "expected negative line point inside the plot area" + ); + assert!( + rect_contains_pixel_where(&line_image, 240, 0, 480, 220, is_series_pixel), + "expected positive line point inside the plot area" + ); + Ok(()) +} + #[test] fn export_preview_uses_native_renderer_for_single_slide() -> Result<(), Box> { @@ -4479,12 +5429,109 @@ fn export_preview_matches_pptx_stacking_and_rounded_shapes() .expect("exported preview path present"), )?; - assert_eq!(preview.get_pixel(200, 220).0, [0x2A, 0x80, 0xD7, 0xFF]); + assert_eq!(preview.get_pixel(200, 220).0, [0xFF, 0xFF, 0xFF, 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 render_preview_respects_cross_element_z_order() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let image_path = temp_dir.path().join("z-order-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": "Cross element z-order" }), + }, + 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!({}), + }, + temp_dir.path(), + )?; + let added_image = 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": 120, "top": 120, "width": 180, "height": 180 } + }), + }, + temp_dir.path(), + )?; + let image_id = added_image + .artifact_snapshot + .as_ref() + .and_then(|snapshot| snapshot.slides.first()) + .and_then(|slide| slide.element_ids.first()) + .cloned() + .expect("image id"); + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "add_shape".to_string(), + args: serde_json::json!({ + "slide_index": 0, + "geometry": "rectangle", + "position": { "left": 140, "top": 140, "width": 180, "height": 180 }, + "fill": "#FFFFFF" + }), + }, + temp_dir.path(), + )?; + let preview = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "render_preview".to_string(), + args: serde_json::json!({ "slide_index": 0 }), + }, + temp_dir.path(), + )?; + let rendered = load_preview(&preview.rendered_preview.expect("preview").png_bytes)?; + assert_eq!(rendered.get_pixel(180, 180).0, [0xFF, 0xFF, 0xFF, 0xFF]); + + manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id.clone()), + action: "update_shape_style".to_string(), + args: serde_json::json!({ + "element_id": format!("im/{image_id}"), + "z_order": 1 + }), + }, + temp_dir.path(), + )?; + let updated_preview = manager.execute( + PresentationArtifactRequest { + artifact_id: Some(artifact_id), + action: "render_preview".to_string(), + args: serde_json::json!({ "slide_index": 0 }), + }, + temp_dir.path(), + )?; + let updated = load_preview( + &updated_preview + .rendered_preview + .expect("updated preview") + .png_bytes, + )?; + assert_eq!(updated.get_pixel(180, 180).0, [0x2A, 0x80, 0xD7, 0xFF]); + Ok(()) +} + #[test] fn export_preview_writes_all_slides_with_stable_names() -> Result<(), Box> { let temp_dir = tempfile::tempdir()?;