amadan

oficina / sheets Public

feat(xlsx): imported files keep their column widths, and give them back

An imported file arrived with every column the same width. For the
sheets people actually import — a label column beside four number
columns — that is not a rendering of the document, and it is the same
class of defect as not painting the fills: the information was in the
file and we were not reading it.

Widths come in, and go back out again on export. A round trip that
silently returned every column to the default would be the same
lossless-round-trip failure Format.FillHex exists to prevent.

**The default is asked of the file, not assumed, and the obvious version
of that is wrong in exactly the case that matters.** Taking column A's
width as the sheet's default inverts the layout of any file whose author
widened column A: A then looks like the default and is skipped, while
every ordinary column looks customised and is written. It reads
correctly, it passed my own review, and only a test asking for a
specific number caught it. The default now comes from probing a column
past the used range, where excelize's own fall-through answers with the
sheet's declared default.

Excel's units are not pixels and the conversion cannot be exact — a
column width is measured in characters of a font we are not rendering
in. What it buys is proportion: a column the author made twice as wide
arrives twice as wide, which is what makes an imported sheet
recognisable as the file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0182f7QfojuuqdS4Xd2fYFaL
Paul Campbell pushed by paul@keymail.dev aac7eae2f5fef7b94f84d54e17980e328055af36 parent 8f83c9a
4 files changed, +283 −0
  • internal/sheets/xlsx/export.go +6 −0
  • internal/sheets/xlsx/import.go +11 −0
  • internal/sheets/xlsx/sizes.go +176 −0
  • internal/sheets/xlsx/sizes_test.go +90 −0

CI cancelled — superseded by a newer push — run details

Intent

full ledger: feat/editor-chrome

diff --git a/internal/sheets/xlsx/export.go b/internal/sheets/xlsx/export.go
index 91525d9..3ea177d 100644
--- a/internal/sheets/xlsx/export.go
+++ b/internal/sheets/xlsx/export.go
@@ -53,6 +53,12 @@ func Export(h *store.Handle, w io.Writer) error {
}
}
+ // Layout, so a round trip does not quietly return every column to
+ // the default width — the same obligation Format.FillHex carries.
+ if err := exportSizes(f, sheetName, h); err != nil {
+ return fmt.Errorf("xlsx: %w", err)
+ }
+
if err := f.Write(w); err != nil {
return fmt.Errorf("xlsx: write workbook: %w", err)
}
diff --git a/internal/sheets/xlsx/import.go b/internal/sheets/xlsx/import.go
index f5b5b87..0222075 100644
--- a/internal/sheets/xlsx/import.go
+++ b/internal/sheets/xlsx/import.go
@@ -71,6 +71,17 @@ func Import(r io.Reader, h *store.Handle) error {
}
}
}
+
+ // Layout last, and best effort. A workbook whose widths cannot be
+ // read is still a workbook whose cells can, and refusing the import
+ // over its margins would trade the document for its formatting.
+ widest := 0
+ for _, row := range rows {
+ if len(row) > widest {
+ widest = len(row)
+ }
+ }
+ importSizes(f, sheetName, h, len(rows), widest)
return nil
}
diff --git a/internal/sheets/xlsx/sizes.go b/internal/sheets/xlsx/sizes.go
new file mode 100644
index 0000000..4e7bf62
--- /dev/null
+++ b/internal/sheets/xlsx/sizes.go
@@ -0,0 +1,176 @@
+package xlsx
+
+import (
+ "math"
+
+ "github.com/xuri/excelize/v2"
+
+ "amadan.net/oficina/sheets/internal/sheets/store"
+)
+
+// Column widths and row heights, on import.
+//
+// Without them an imported file arrives with every column the same
+// default width, which for the sheets people actually import — a label
+// column beside four number columns — is not a rendering of the
+// document. It is the same class of defect as not painting the fills:
+// the information is in the file, we simply were not reading it.
+
+// Excel's units are not pixels, and the conversion is the fiddly part.
+//
+// A column width is in "characters" of the workbook's default font, and
+// Excel's own documented formula for the pixel width of a column of
+// width w at 96 DPI with the default 11pt Calibri is
+// round(((256*w + round(128/MDW))/256) * MDW), where MDW is the maximum
+// digit width in pixels — 7 for that font. That rounds to 7w + 5 for
+// every width anybody actually has, and the five pixels are the cell's
+// padding.
+//
+// It is an approximation of a measurement made in a font we are not
+// rendering in, so it is not exact and cannot be: the browser is laying
+// out a different typeface. What it buys is proportion — a column the
+// author made twice as wide arrives twice as wide — and proportion is
+// what makes an imported sheet recognisable.
+const (
+ excelMaxDigitWidth = 7.0
+ excelCellPadding = 5.0
+)
+
+// A row height is in POINTS, which is a real typographic unit and
+// converts to CSS pixels at 96/72.
+const pointsToPixels = 96.0 / 72.0
+
+// importSizes copies a worksheet's column widths and row heights.
+//
+// Best effort, and deliberately not fatal: a workbook whose widths
+// cannot be read is still a workbook whose cells can, and refusing the
+// import over its layout would trade the whole document for its
+// margins. Callers ignore the error for that reason and this function
+// returns none.
+//
+// Only columns and rows the file gives an EXPLICIT size are touched.
+// excelize answers the sheet's default for everything else, and writing
+// that back would turn "the author said nothing" into "the author asked
+// for exactly the default" — a different document, and one whose every
+// column is pinned against a later change to our own default. What
+// counts as the default is asked of the file rather than assumed; see
+// defaultColWidth for the version of this that was wrong.
+func importSizes(f *excelize.File, sheet string, h *store.Handle, rows, cols int) {
+ defW, okW := defaultColWidth(f, sheet, cols)
+ for c := 0; c < cols; c++ {
+ name, err := excelize.ColumnNumberToName(c + 1)
+ if err != nil {
+ continue
+ }
+ w, err := f.GetColWidth(sheet, name)
+ if err != nil || !okW || w == defW {
+ continue
+ }
+ px := int(math.Round(w*excelMaxDigitWidth + excelCellPadding))
+ colID, err := h.EnsureCol(c)
+ if err != nil {
+ continue
+ }
+ // SetColWidth bounds the value itself and errors on one outside
+ // them; a column the file made absurdly wide is skipped rather
+ // than clamped, because clamping would silently claim the
+ // author asked for our maximum.
+ _ = h.SetColWidth(colID, px)
+ }
+
+ defH, okH := defaultRowHeight(f, sheet, rows)
+ for r := 0; r < rows; r++ {
+ ht, err := f.GetRowHeight(sheet, r+1)
+ if err != nil || !okH || ht == defH {
+ continue
+ }
+ rowID, err := h.EnsureRow(r)
+ if err != nil {
+ continue
+ }
+ _ = h.SetRowHeight(rowID, int(math.Round(ht*pointsToPixels)))
+ }
+}
+
+// probeDistance is how far past the used range to look for a sheet's
+// default size.
+//
+// Far enough that no real file has an explicit width out there, near
+// enough to stay inside the format's limits.
+const probeDistance = 64
+
+// defaultColWidth asks the file what an UNTOUCHED column is.
+//
+// **Probing a column past the data, not column A.** GetColWidth answers
+// for any column: an explicit <col> entry if one covers it, then the
+// sheet's declared default, then the format's. So a column outside the
+// used range is answered by exactly the fall-through this needs, using
+// the library's own code path rather than a constant restated here.
+//
+// The obvious version — take column A's width as the default — is
+// wrong in precisely the case that matters, and wrong silently. A file
+// whose author widened column A reports that width as "the default",
+// so column A is then treated as unset and skipped while every ordinary
+// column looks customised and is written. The result is a document with
+// its layout inverted, and it passed review here until a test asked for
+// a specific number.
+func defaultColWidth(f *excelize.File, sheet string, cols int) (float64, bool) {
+ name, err := excelize.ColumnNumberToName(cols + probeDistance + 1)
+ if err != nil {
+ return 0, false
+ }
+ w, err := f.GetColWidth(sheet, name)
+ if err != nil {
+ return 0, false
+ }
+ return w, true
+}
+
+// defaultRowHeight is defaultColWidth's twin — same probe, same reason.
+func defaultRowHeight(f *excelize.File, sheet string, rows int) (float64, bool) {
+ ht, err := f.GetRowHeight(sheet, rows+probeDistance+1)
+ if err != nil {
+ return 0, false
+ }
+ return ht, true
+}
+
+// exportSizes writes a tab's column widths and row heights back out.
+//
+// The inverse of importSizes, and it exists for the same reason
+// Format.FillHex is retained: a round trip that silently drops the
+// author's layout is not lossless, and the file that comes out would be
+// a different document from the one that went in — one where every
+// column had reverted to the default width.
+//
+// Only sized rows and columns are written. A tab where nobody has
+// dragged anything writes nothing, which leaves the workbook's own
+// defaults alone rather than pinning every column against ours.
+func exportSizes(f *excelize.File, sheet string, h *store.Handle) error {
+ cols, rows, err := h.Sizes()
+ if err != nil {
+ return err
+ }
+ for pos, px := range cols {
+ name, err := excelize.ColumnNumberToName(pos + 1)
+ if err != nil {
+ continue
+ }
+ // The inverse of importSizes' conversion. Rounded to two places
+ // because that is the precision the format stores, and a width
+ // carrying fifteen significant figures is noise in a diff.
+ w := (float64(px) - excelCellPadding) / excelMaxDigitWidth
+ if w <= 0 {
+ continue
+ }
+ if err := f.SetColWidth(sheet, name, name, math.Round(w*100)/100); err != nil {
+ return err
+ }
+ }
+ for pos, px := range rows {
+ if err := f.SetRowHeight(sheet, pos+1, math.Round(float64(px)/pointsToPixels*100)/100); err != nil {
+ return err
+ }
+ }
+ return nil
+}
diff --git a/internal/sheets/xlsx/sizes_test.go b/internal/sheets/xlsx/sizes_test.go
new file mode 100644
index 0000000..5f13489
--- /dev/null
+++ b/internal/sheets/xlsx/sizes_test.go
@@ -0,0 +1,90 @@
+package xlsx_test
+
+import (
+ "bytes"
+ "testing"
+
+ "github.com/xuri/excelize/v2"
+
+ "amadan.net/oficina/sheets/internal/sheets/xlsx"
+)
+
+// widthFixture is a one-cell workbook with a deliberately wide column A
+// and a tall row 1, and everything else left at the workbook's default.
+func widthFixture(t *testing.T, width float64, height float64) *bytes.Buffer {
+ t.Helper()
+ f := excelize.NewFile()
+ defer f.Close()
+ sheet := f.GetSheetList()[0]
+ if err := f.SetCellValue(sheet, "A1", "Invoice Quarter"); err != nil {
+ t.Fatalf("set cell: %v", err)
+ }
+ if err := f.SetCellValue(sheet, "B1", "x"); err != nil {
+ t.Fatalf("set cell: %v", err)
+ }
+ if err := f.SetColWidth(sheet, "A", "A", width); err != nil {
+ t.Fatalf("set width: %v", err)
+ }
+ if err := f.SetRowHeight(sheet, 1, height); err != nil {
+ t.Fatalf("set height: %v", err)
+ }
+ var buf bytes.Buffer
+ if err := f.Write(&buf); err != nil {
+ t.Fatalf("write fixture: %v", err)
+ }
+ return &buf
+}
+
+func TestImportKeepsColumnWidths(t *testing.T) {
+ h := importReader(t, widthFixture(t, 30, 40))
+ defer h.Release()
+
+ cols, rows, err := h.Sizes()
+ if err != nil {
+ t.Fatalf("sizes: %v", err)
+ }
+ // 30 characters at Excel's 7px max-digit-width plus 5px padding.
+ if got := cols[0]; got != 215 {
+ t.Errorf("column A = %dpx, want 215 — an imported file whose author widened "+
+ "a column should arrive with it widened", got)
+ }
+ // A column the file said nothing about must stay unsized, not be
+ // pinned at whatever the default happened to be on import day.
+ if _, ok := cols[1]; ok {
+ t.Errorf("column B was given a width the file never set: %d", cols[1])
+ }
+ // 40 points at 96/72.
+ if got := rows[0]; got != 53 {
+ t.Errorf("row 1 = %dpx, want 53", got)
+ }
+}
+
+// TestSizesSurviveARoundTrip is the lossless-round-trip obligation,
+// applied to layout. A file that comes back with every column at the
+// default width is a different document from the one that went in.
+func TestSizesSurviveARoundTrip(t *testing.T) {
+ first := importReader(t, widthFixture(t, 30, 40))
+ var out bytes.Buffer
+ if err := xlsx.Export(first, &out); err != nil {
+ t.Fatalf("export: %v", err)
+ }
+ first.Release()
+
+ second := importReader(t, &out)
+ defer second.Release()
+
+ cols, rows, err := second.Sizes()
+ if err != nil {
+ t.Fatalf("sizes: %v", err)
+ }
+ // Within a pixel: the conversion goes through Excel's character
+ // units and back, and the rounding is not required to be exact —
+ // what must survive is the proportion somebody chose.
+ if got := cols[0]; got < 214 || got > 216 {
+ t.Errorf("after a round trip column A = %dpx, want 215±1 — export is dropping "+
+ "the author's layout", got)
+ }
+ if got := rows[0]; got < 52 || got > 54 {
+ t.Errorf("after a round trip row 1 = %dpx, want 53±1", got)
+ }
+}