Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions PHASE-1.2.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# Phase 1.2 — Bug Fixes & Feature Gaps

> Status: 10 of 12 items addressed. See ✅ for completed fixes.

---

## ✅ Fixed in This Phase

### 1. `add image` with `file=` property now works
**File:** `crates/docx-handler/src/mutations.rs:2921`, `crates/pptx-handler/src/add.rs:642`, `crates/docx-handler/src/add.rs:2141`

The code only checked for `src` or `path` property keys, but the CLI passes `file=`.
Added `.or_else(|| properties.get("file"))` as fallback in all three image add paths.

### 2. `--properties 'border=all'` now creates visible borders
**File:** `crates/docx-handler/src/mutations.rs:902`

`"all"` didn't match `"all="` (prefix), `"single"`, or `"thin"`. Added `|| value == "all"` so `border=all` behaves like `border=all=single`.

### 3. `set <xlsx> cell text=` now accepted
**File:** `crates/xlsx-handler/src/mutations.rs:376`

Added `| "text"` alongside `"value"` match arm so `text=Hello` works for xlsx cells.

### 4. Multi-row table `rNcN` text fixed
**File:** `crates/docx-handler/src/add.rs:1871-1874`

Outer loop had `for _ in 0..rows` (no row counter) and format string used `format!("r{}c{}", col_idx + 1, 1)` (hardcoded column). Changed to `for row in 0..rows` and `format!("r{}c{}", row + 1, col_idx + 1)`.

### 5. Image dimensions: bare numbers treated as points
**File:** `crates/docx-handler/src/mutations.rs:3470-3473`

`parse_emu()` treated bare numbers as raw EMU (e.g., `width=200` = 200 EMU ≈ 0.016pt).
Changed else branch to multiply by 12700, treating bare numbers as points.

### 6. Table caption/title support
**File:** `crates/docx-handler/src/add.rs:1840-1846`

`--properties 'title=My Table'` now creates `<w:tblCaption w:val="My Table"/>` in table properties.

### 7. Column widths for tables
**File:** `crates/docx-handler/src/add.rs:1899-1915`

`--properties 'colWidths=100,200,150'` now sets column widths (in points → twips) on `<w:gridCol>` elements.

### 8. Page break support
**File:** `crates/docx-handler/src/mutations.rs:245-279`

`officecli set docx '/body/p[1]' pageBreak=true` inserts `<w:r><w:br w:type="page"/></w:r>` at paragraph start.

### 9. Validate detects empty media files
**File:** `crates/docx-handler/src/handler.rs:771-788`

`officecli validate` now reports an error if any file under `word/media/` has 0 bytes.

### 10. Paragraph background shading
Already existed in the codebase via `shading`/`shd` property on paragraph `set`.

---

## Remaining Issues (Phase 2 candidates)

> Identified by building a real-world laptop shop quotation (.docx) and testing
> all formats (docx, xlsx, pptx, pdf) with the `officecli-v1` binary built from
> Phase 1 changes.

---

## 🟡 Phase 2 Candidates

### 1. Colspan / rowspan (cell merging)

`--properties 'span=2'` on cell exists in the schema but has no observable
effect. Need to verify `<w:gridSpan>` and `<w:vMerge>` work through the DOM
serialization path.

### 2. Tedious cell-by-cell table creation

A 5×6 table requires ~25 CLI calls (1 table + 5 rows + 20 cells). Could add
`--cells 'r1c1=val,r1c2=val,...'` shorthand.

### 3. Column widths in `set` command

`officecli set docx '/body/tbl[1]/col[1]' width=100` is not supported. Column
widths only work at table creation time via `colWidths`.

### 4. `--range` and `--grid` screenshots (verify)

These flags parse correctly but haven't been verified with actual screenshot
capture in this phase.

---

## Summary

| Priority | Count | Key Items |
|----------|-------|-----------|
| ✅ Fixed | 10 | Image 0-byte, Image dims (pt), Xlsx text, Border all, Multi-row text, Caption, Col widths, Page break, Validate media, Paragraph bg |
| 🟡 Phase 2 | 4 | Colspan/rowspan, Cell shorthand, Col set-widths, Screenshot verify |

**Total gaps identified: 14 items — 10 fixed, 4 deferred to Phase 2**
39 changes: 31 additions & 8 deletions crates/docx-handler/src/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1840,6 +1840,12 @@ fn add_table(
.with_attribute("val", style.as_str()),
);
}
if let Some(caption) = properties.get("title").or_else(|| properties.get("caption")) {
children.push(
WordNode::new(WordElementType::Unknown("tblCaption".to_string()))
.with_attribute("val", caption.as_str()),
);
}
if let Some(width) = properties.get("width") {
children.push(
WordNode::new(WordElementType::Unknown("tblW".to_string()))
Expand All @@ -1848,7 +1854,12 @@ fn add_table(
);
}
if let Some(border) = properties.get("border") {
children.push(crate::mutations::build_table_borders(border));
let color = properties.get("borderColor")
.or_else(|| properties.get("tblBorderColor"))
.or_else(|| properties.get("bdrColor"))
.map(|s| s.as_str())
.unwrap_or("000000");
children.push(crate::mutations::build_table_borders(border, color));
}
if let Some(shading) = properties.get("shading").or_else(|| properties.get("shd")) {
children.push(crate::mutations::build_shd_node(shading));
Expand All @@ -1868,10 +1879,10 @@ fn add_table(

// Build table grid
let mut rows_nodes = Vec::new();
for _ in 0..rows {
for row in 0..rows {
let mut cells = Vec::new();
for col_idx in 0..cols {
let text = properties.get(&format!("r{}c{}", col_idx + 1, 1)).cloned();
let text = properties.get(&format!("r{}c{}", row + 1, col_idx + 1)).cloned();
let mut cell = WordNode::new(WordElementType::TableCell);
if let Some(text) = text {
let para = WordNode::new(WordElementType::Paragraph)
Expand All @@ -1893,10 +1904,20 @@ fn add_table(
// Add tblGrid if multiple columns
if cols > 1 {
let mut grid = WordNode::new(WordElementType::Unknown("tblGrid".to_string()));
for _ in 0..cols {
grid.children.push(WordNode::new(WordElementType::Unknown(
"gridCol".to_string(),
)));
let widths: Vec<&str> = properties
.get("colWidths")
.or_else(|| properties.get("colWidth"))
.map(|s| s.split(',').collect())
.unwrap_or_default();
for col_idx in 0..cols {
let mut gc = WordNode::new(WordElementType::Unknown("gridCol".to_string()));
if let Some(w) = widths.get(col_idx) {
if let Ok(pt) = w.trim().parse::<f64>() {
let twips = (pt * 20.0) as i64;
gc = gc.with_attribute("w", &twips.to_string());
}
}
grid.children.push(gc);
}
table.children.push(grid);
}
Expand Down Expand Up @@ -2140,9 +2161,11 @@ fn add_image(
let _src = properties
.get("src")
.or_else(|| properties.get("path"))
.or_else(|| properties.get("file"))
.ok_or_else(|| {
HandlerError::InvalidArgument(
"image requires 'src' or 'path' property pointing to image file".to_string(),
"image requires 'src', 'path', or 'file' property pointing to image file"
.to_string(),
)
})?;

Expand Down
16 changes: 16 additions & 0 deletions crates/docx-handler/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,22 @@ impl DocumentHandler for WordHandler {
});
}

// Check for empty media files under word/media/
for part_path in pkg.list_parts() {
if part_path.starts_with("word/media/") {
if let Ok(bytes) = pkg.read_part_bytes(part_path) {
if bytes.is_empty() {
errors.push(ValidationError {
error_type: "broken-reference".to_string(),
description: format!("media file '{}' is empty (0 bytes)", part_path),
path: None,
part: Some(part_path.clone()),
});
}
}
}
}

Ok(errors)
}

Expand Down
Loading