Skip to content

Format: Row Height and Column Width say pixels, and take points like Excel - #1085

Merged
aiongg merged 1 commit into
storytold:mainfrom
CouchRiv:fix/row-height-column-width-units
Oct 22, 2026
Merged

aiongg merged 1 commit into
storytold:mainfrom
CouchRiv:fix/row-height-column-width-units

Conversation

@CouchRiv

Copy link
Copy Markdown
Contributor

Found while driving GridCraft 0.6.1 over MCP. list_commands documents home.rowHeight as height: 20 (points on screen) and home.columnWidth as width: 64 (points) | chars: 8.43, but both values are pixels at 96 dpi, the model's unit. An agent that follows the docs, or Excel's habits, gets a 30 pt row saved as ht="22.5". The range check disagrees with its own message too: height: 500 is accepted (saved as 375 pt) while height: 600 fails with "Row height must be between 0 and 409 points."

Who sends what (why pixels stay the unit)

Before choosing, I checked every caller:

caller sends unit
header drag (ui-egui/src/grid.rs) height / width pixels (toast Height: 22.50 (30 pixels))
Row Height dialog (ui-egui/src/dialogs.rs) height pixels, labelled "Row height (pixels):"
Column Width dialog chars characters
Standard Width dialog width (home.defaultWidth) pixels, labelled "Standard column width (pixels):"
sample workbooks, engine and UI tests height / width pixels
XLSX px_to_pt on write, pt_to_px on read pixels in the model

So the UI has no unit bug: every caller already sends pixels, and only the parameter docs and the error messages were wrong. Changing height to points would have broken the drags, the dialogs, the samples and the tests, so this keeps pixels and adds Excel's unit next to them.

What changed

All in crates/engine/src/cmd/format.rs, plus comments in crates/model.

  • The params docs say pixels: height: 20 (pixels) | points: 15, width: 64 (pixels) | chars: 8.43, and {width: 64 (pixels)} for home.defaultWidth. Pixel values behave as before.
  • home.rowHeight takes points, Excel's unit (its Row Height dialog, Range.RowHeight, ht in the file), converted as pt * 4 / 3. This mirrors chars on home.columnWidth. When both are given, height wins, as width does over chars.
  • Range errors give the limit in the unit that was passed:
    • height: "Row height must be between 0 and 546 pixels (409.5 points)." (same 546 px limit as before; the XLSX reader also caps rows at 409.5 pt)
    • points: "Row height must be between 0 and 409.5 points."
    • width: "Column width must be between 0 and 1800 pixels." (same limit as before)
    • chars: "Column width must be between 0 and 255 characters." chars is now checked against 255, Excel's limit and the widest column a file can hold (the writer caps at 1790 px). Before, it went through the pixel limit, so chars: 256 was accepted despite the message.
  • Doc comments: LineInfo::size, Sheet::default_col_width / default_row_height, DEFAULT_COL_WIDTH and chars_to_points said points; they now say pixels at 96 dpi.

Verification (Linux x86_64, original baseline fc45b7f)

  • cargo test -p gridcraft-engine --lib tests::row_height_and_column_width_take_pixels_or_excel_units: sets row 1 with points: 30, row 2 with height: 30, row 3 with points: 409.5, column A with chars: 8.43 and B with width: 100. It checks the model sizes (40, 30, 546 px; 64, 100 px), the saved <row r="1" ht="30" …>, <row r="2" ht="22.5" …> and <row r="3" ht="409.5" …>, the four out-of-range messages, and the three params docs. It fails on main (format.rs and the model comments stashed) with called Result::unwrap() on an Err value: BadParams { cmd: "home.rowHeight", msg: "missing height" } and passes here.
  • End to end, gridcraft-cli mcp, same calls on both builds:
call main (fc45b7f) this branch
list_commands home.rowHeight height: 20 (points on screen) height: 20 (pixels) | points: 15
home.rowHeight {height: 30} ht="22.5" ht="22.5" (unchanged)
home.rowHeight {height: 600} "between 0 and 409 points." "between 0 and 546 pixels (409.5 points)."
home.rowHeight {points: 30} error, missing height ht="30"
home.rowHeight {points: 410} error, missing height "between 0 and 409.5 points."
home.columnWidth {width: 100}, {chars: 20} width="14.28125", width="20.7109375" the same
  • cargo fmt --all -- --check, cargo clippy -p gridcraft-model -p gridcraft-engine --all-targets -- -D warnings: clean
  • cargo test -p gridcraft-model -p gridcraft-engine (lib and tests/): all pass (engine lib 230, model 14, tests/ 14)
  • cargo test -p gridcraft-ui-egui (unchanged crate, but it calls these commands from drags, dialogs and tests): all pass (596 tests)

Limits and questions

  • GridCraft's Row Height dialog asks for pixels where Excel's asks for points, and its Standard Width dialog asks for pixels where Excel's asks for characters. Switching the Row Height dialog to points is now a one-line change in dialogs.rs; I left the UI alone since it is a UI choice, not a bug. home.defaultWidth has no chars yet; happy to add it here if you'd like.
  • chars_to_points, points_to_chars and approx_text_width keep their names although they work in pixels. Renaming them touches the UI crate and CELL("width") returns the Column Width dialog value, without the padding (#735) #753, so only the comment of chars_to_points changes here.
  • The row limit stays 546 px = 409.5 pt, as before and as the XLSX reader caps it. Excel's specifications list 409 points for row height and 255 characters for column width (https://support.microsoft.com/en-us/office/excel-specifications-and-limits-1672b34d-7043-467e-8e27-269d656771c3); whether Excel accepts 409.5 was not checked (no Excel here).
  • A column wider than 1790 px (only reachable by drag or width, and saved as 255 characters anyway) opens the Column Width dialog on 256.x characters, and OK now gets the 255 message instead of passing.

Independent verification against updated main (84acf62, 2026-10-13)

The independent review reran cargo fmt --all -- --check, cargo clippy -p gridcraft-model -p gridcraft-engine --all-targets -- -D warnings and cargo test -p gridcraft-model -p gridcraft-engine on the branch and its clean merge with 84acf62; all exited 0. The added regression tests failed with production code from 84acf62 and passed on both branch and merged trees. These checks are separate from the historical fc45b7f measurements above.

Conflicts

Against upstream/main at 84acf62, git merge-tree is clean. Among the 100 open PRs reviewed, #885 (my open PR for honoring the sheet parameter) conflicts in the home.rowHeight and home.columnWidth parameter documentation. Whichever lands second should retain both the sheet parameter and the pixel/point documentation. #870 and #480 merge cleanly. This check does not cover every older open PR.

…Excel

home.rowHeight and home.columnWidth take pixels at 96 dpi, the model's unit
(header drags and the Row Height dialog send pixels), but their parameter docs
said points. An agent asking for a 30 pt row got `ht="22.5"` in the file, and
the range check accepted 500 while its message said "between 0 and 409 points".

- The params docs say pixels: `height: 20 (pixels) | points: 15`,
  `width: 64 (pixels) | chars: 8.43`, and `{width: 64 (pixels)}` for
  home.defaultWidth. Pixel values behave as before.
- home.rowHeight takes `points`, Excel's unit (Row Height dialog, `ht` in the
  file), as home.columnWidth already takes `chars`.
- Range errors give the limit in the unit passed: 546 pixels (409.5 points),
  409.5 points, 1800 pixels, 255 characters. `chars` is checked against 255,
  Excel's limit and the widest column a file holds (it went to 256.4 through
  the pixel limit).
- The model's size comments say pixels.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aiongg
aiongg merged commit d26bbe3 into storytold:main Oct 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants