Skip to content

feat(document)!: collapse the drawing elements into frame - #821

Merged
andiwand merged 1 commit into
mainfrom
collapse-drawing-elements
Sep 6, 2026
Merged

feat(document)!: collapse the drawing elements into frame#821
andiwand merged 1 commit into
mainfrom
collapse-drawing-elements

Conversation

@andiwand

@andiwand andiwand commented Sep 5, 2026

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

Closes #773. Rebased onto main after the deprecation PR (#818) was dropped:
this is now one commit that goes straight to the breaking change, and the
ODR_DEPRECATED / ODR_INTERNAL_BUILD mechanism #818 introduced is gone with
it.

Breaking, and the API-shape prerequisite for #771: it turns "add a shape kind"
from a vertical slice through four binding layers into one parser-table line.

The model

Gone: Rect, Line, Circle, CustomShape, their four ElementType values,
Element::as_rect/as_line/as_circle/as_custom_shape, four
abstract::*Adapter interfaces with their dispatchers, eighteen near-duplicate
odf readers, three near-identical style writers, and the Java/ObjC/Python
mirrors of all of it.

In their place, Frame:

enum class ShapeType {
  none,     ///< a plain box, drawing no outline of its own
  rect, ellipse, line,
  custom,   ///< an outline of its own, read from Frame::path
};

class Frame … {
  ShapeType shape_type() const;                    // new
  std::optional<DrawingPath> path() const;         // was CustomShape::path
  std::optional<DrawingLine> line() const;         // new: a line's two ends
  // and the placement a frame already had:
  AnchorType anchor_type();  x/y/width/height();  z_index();  transform();
};

ShapeType carries the five kinds the parser can actually tell apart today,
not the nine the issue sketched — draw:polygon, draw:path and friends
already reach the renderer as a custom with a real DrawingPath, so a
separate enumerator for each would carry no information. New kinds are one
enumerator plus one table line when a renderer branch wants them.

A line is the one shape ODF states as two points rather than a box, so it keeps
them, in a DrawingLine alongside the existing DrawingPath/DrawingTransform.

FrameAdapter defaults the three new readers (none, {}, {}): only odf
ever produces a shape, and the other five engines would otherwise carry fifteen
stubs saying so. That default needs the complete types, so
internal/abstract/document.hpp now includes <odr/document_element.hpp>
instead of forward-declaring ten of its types.

odf

create_shape_element records the kind in a SideTable<ShapeType> beside the
element — no extra bytes in ElementRegistry::Element, which is size-critical
— and the parser table maps draw:rect/draw:captionrect,
draw:line/draw:measureline, draw:circle/draw:ellipseellipse
(or custom when draw:kind cuts it), and everything drawn rather than named
custom. draw:frame and draw:g stay plain frames.

The merged frame_x/y/width/height are the old custom_shape_*: read
the svg: attribute, else fall back to the connector's own path box.

Rendering is untouched

translate_frame switches on shape_type() into the five bodies that were
translate_frame/_rect/_line/_circle/_custom_shape, and the three
identical shape-box writers became one translate_shape_properties. Shapes
keep their absolute placement rather than picking up translate_frame_properties,
so the reference output is byte-identical — verified below — and no pin
moves.

That does leave the anchoring/z-index bug §3 of #773 describes open: a
draw:custom-shape with text:anchor-type="paragraph" still lands relative to
the page. The fix is now one line (send shapes through
translate_frame_properties too), but it moves pixels on ~300 reference files,
so it belongs with #771 and its regen rather than here.

Rebased onto #823 and #829

The three red CI legs were the branch's base, not this change: #819 gave
PageLayout a direction that iwork_document.cpp did not name (gcc-14 and
clang-18 -Wmissing-field-initializers), and an odf test's raw string sat
inside a macro argument msvc's traditional preprocessor will not read. main
fixed both in #825. A fourth leg, test, was failing on main too — the
reference-output pins lagged #824 — and #829 has since advanced them.

The rebase lands on #823, which moved the per-type *_adapter(id) dispatchers
into internal::ElementAdapter and the element store into
internal::ElementRegistry. Resolved to keep both: odf's AdapterBase drops
the four shape adapters from its pack, internal::ElementAdapter loses the
four dispatchers it generated for them, and the odf registry's m_shape_types
becomes a SortedSideTable<ShapeType, StoredId> beside the other four — it is
written when the element is created, so the binary search fits.

Verified on this rebase, at 4a80837f

  • -Werror RelWithDebInfo build of the library, tests and CLI: clean.
  • Full suite: 1466 tests, 1460 passed, 0 failures, 6 pre-existing skips.
  • diff -rq of a fresh run against both reference-output trees checked out at
    the revisions test/data.cmake now pins — 29fc194 and d7aa0dc, refactor(html): share one style writer between the sheet and pdf views #829's:
    0 differences, both trees. Nothing this PR touches moves a byte.
  • scripts/format: no changes.
  • clang-tidy on the resolved sources: only the pre-existing
    bugprone-derived-method-shadowing-base-method hits on untouched headers.

Carried over from before the rebase, not re-run here: the JNI junit and pyodr
pytest suites, javap -s against the jar, and the separate -Werror syntax
check of ODRDocumentElement.mm. CI covers all four.

  • New test/src/internal/odf/odf_frame_test.cpp: every draw:* shape is a
    frame naming its kind, a rect keeps the anchor and z-index a frame has, a
    line states its two ends, a custom shape carries its outline.

@andiwand
andiwand force-pushed the deprecate-drawing-elements branch from af7c644 to 1f06b83 Compare September 5, 2026 20:26
@andiwand
andiwand force-pushed the collapse-drawing-elements branch from 07c1351 to 060502b Compare September 5, 2026 20:55
@andiwand
andiwand changed the base branch from deprecate-drawing-elements to main September 5, 2026 20:55
@andiwand
andiwand force-pushed the collapse-drawing-elements branch 2 times, most recently from d3ffac5 to d73cb46 Compare September 6, 2026 11:43
BREAKING CHANGE: `Rect`, `Line`, `Circle` and `CustomShape` are gone, with
their `ElementType` values and `Element::as_rect`/`as_line`/`as_circle`/
`as_custom_shape`. A shape is a `Frame` naming itself through the new
`ShapeType Frame::shape_type()`, and carrying `Frame::path()` and the new
`Frame::line()`. Closes #773.

The four types were an odf-parser artifact — the other five engines already
funnel every shape into `frame` — and cost a public class, an abstract
adapter, a dispatcher, a style writer and three binding mirrors each. Shapes
now carry `anchor_type()` and `z_index()`, which only frames had; the renderer
still places them absolutely, so no emitted html moves.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pd6AtUjaEzVT4FXMQdXLjT
@andiwand
andiwand force-pushed the collapse-drawing-elements branch from d73cb46 to 69f033e Compare September 6, 2026 12:36
@andiwand
andiwand merged commit 2835b21 into main Sep 6, 2026
23 checks passed
@andiwand
andiwand deleted the collapse-drawing-elements branch September 6, 2026 12:37
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.

Element model: the five drawing element types are one concept — collapse rect/line/circle/custom_shape into frame

1 participant