Skip to content

feat!: clean up the public API for 6.0.0 - #627

Merged
andiwand merged 4 commits into
mainfrom
api-cleanup-6.0
Jul 27, 2026
Merged

feat!: clean up the public API for 6.0.0#627
andiwand merged 4 commits into
mainfrom
api-cleanup-6.0

Conversation

@andiwand

@andiwand andiwand commented Jul 27, 2026

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

Breaking API cleanup ahead of the 6.0.0 tag, continuing #622#626. Everything
here needs the major bump, so it is the last cheap moment for it.

Two defects in the public element API

ElementIterator::operator++(int) was const, so it could not advance, and it
returned the successor instead of the pre-increment value:

ElementIterator ElementIterator::operator++(int) const {
  if (!exists_()) return {};
  return {m_adapter, m_adapter->element_next_sibling(m_identifier)};
}

auto old = it++; therefore left it unmoved and handed back the next
element — silently violating the forward_iterator_tag the class advertises.

Element::operator== compared only m_identifier, so element n of one
document compared equal to element n of an unrelated one. Identifiers are
only unique within a document, so the adapter now takes part. Iterators that
don't exist still compare equal to one another, which is what keeps
ElementRange::end() (a null-adapter iterator) usable as a sentinel.

Geometry is typed

Frame, Rect, Line, Circle and CustomShape return Measure instead of
std::string, and Frame::z_index returns std::optional<std::int32_t>
instead of a string. Callers no longer have to re-parse what the engines
already parsed — the OOXML and ppt engines were building a Measure and
calling to_string() purely to satisfy the string interface.

Measure formatting

Typing the geometry routed it through Measure::to_string, which exposed two
problems with the iostream default:

  • It rounded to 4 significant digits. Measured against the 2,564 distinct
    geometry values in the ODF corpus, 27% lost precision — a drawing coordinate
    of -6734.61mm was emitted as -6735mm. This affected every Measure
    (margins, column widths), not just the newly-typed geometry.
  • It fell back to scientific notation. 13421095.26pt renders as
    1.34211e+07pt, which CSS and SVG lengths reject. There were zero
    scientific-notation lengths in the references beforehand.

Naively raising the precision fixes the first and worsens the second, and at 8+
digits it starts exposing binary representation: xlsx stores column widths as
32-bit floats, so 68.55 comes back as 68.550003.

The new util::number::to_string_significant renders a fixed number of
significant digits, always positional. Measure asks for 7 — enough for
document geometry, and the ceiling a float carries (FLT_DIG is 6). It is a
general helper rather than a Quantity private, since the "no scientific
notation" constraint applies to anything we emit into CSS or SVG.

QuantityBase (non-template, protected static) keeps the implementation out of
the public header while adding no public name; empty-base optimisation leaves
sizeof(Measure) unchanged at 16 bytes.

DecoderEngine removed

Since #622 dropped the external backends the enum had exactly one value and
could not select anything. This removes the enum, DecodePreference's
with_engine / engine_priority, the four free functions,
DecodedFile::decoder_engine, both decoder-engine exceptions, the
abstract::DecodedFile hook with its twelve engine overrides, and the Python
and Java equivalents including DecoderEngine.java. The 220-line engine
dispatch in open_strategy.cpp collapses into a TU-local open_file_as.

Also

  • Every exception derives from odr::Exception, so catch (const odr::Exception &) catches all typed errors. Decoders still throw plain
    std::runtime_error for malformed input with no dedicated type, so that
    stays the widest net. Mirrored in Python as pyodr.Error; Java already had
    OdrException. FileNotFound deliberately keeps PyExc_FileNotFoundError.
  • TextStyle::font_namestd::optional<std::string_view>, matching its
    siblings and keeping the zero-copy contract the oldms registries rely on.
  • Color::from_rgb / from_argb replace the Color(std::uint32_t, bool dummy)
    disambiguator.
  • html::edit takes std::string_view; Html::config() gains const.
  • Measure moved to <odr/quantity.hpp>, where AGENTS.md already documented
    it. <odr/style.hpp> still provides it.
  • Duplicate table_position.cpp dropped from ODR_SOURCE_FILES;
    word_perfect / rich_text_format documented as detection-only; README
    corrected (json is supported).

Two crashes fixed during review

DynamicUnit's default constructor left the unit pointer null while name(),
to_string() and to_stream() all dereferenced it. Typing the geometry made
that reachable — an ODF shape missing one of its svg:* attributes fell back
to Measure(0, DynamicUnit()) and segfaulted on render. Caught by Codex.
Fixed at the invariant rather than the 16 call sites: the default constructor
takes the registered empty unit, so the pointer is never null and a fallback
measure compares equal to the parsed equivalent.

MemoryFile::location() returned FileLocation::disk. Nothing branches on it
internally, so this corrects what the API reports rather than changing
behaviour.

Remaining const char * in the public headers

  • DynamicUnit and Quantity take std::string_view. Quantity copies into
    a buffer first, since std::strtod needs a terminator a view does not
    promise, and stops treating a negative char as UB in std::isspace.
  • DocumentPath's const char * and const std::string & constructors
    collapse into one std::string_view.
  • CfbError takes const std::string &, like its forty siblings.
  • File::memory_data() returns std::optional<std::string_view>, mirroring
    the disk_path() beside it — the bytes if in memory, the path if on disk.
    It previously returned a bare pointer that only meant something alongside
    size(), with null doubling as "not in memory".

Reference outputs

Regenerated and already on main in both output repos
(OpenDocument.test.output@6581dcd,
OpenDocument.test-private.output@7ed25fa); the pointers move here.

232 of 1402 files changed. Verified mechanically that the change is formatting
only: 0 numeric-token-count mismatches, insertions exactly equal deletions, and
all 1,011 distinct value changes preserve the unit and the value to within 0.1%
(0.2188in0.21875in, 11.52ch11.52041ch). The ODG drawing outputs
are byte-identical, since ODF geometry already round-trips exactly at 7 digits.

Testing

718 tests pass (7 pre-existing skips: svm, wpd, encrypted doc). Full build
clean including CLI, Python bindings and JNI. A fresh run matches all 1402
reference files byte-for-byte. number_util_test.cpp covers the float-noise
and no-scientific-notation cases directly, and there are regression tests for
both crashes above.

Not verified locally: pytest and clang-tidy are not installed in this
environment, so the Python suite and the tidy job rely on CI.

andiwand and others added 2 commits July 27, 2026 23:33
Breaking changes to `odr::` ahead of the 6.0.0 tag, plus two long-standing
defects in the public element API.

Fixed:

* `ElementIterator::operator++(int)` was `const`, so it could not advance, and
  returned the successor rather than the pre-increment value — `it++` silently
  violated the forward-iterator contract the class advertises.
* `Element` and `ElementIterator` equality compared only the element
  identifier, so element n of one document compared equal to element n of an
  unrelated one. Non-existent iterators still compare equal to each other so
  `ElementRange::end()` keeps working.

Changed:

* Shape geometry is typed: `Frame`, `Rect`, `Line`, `Circle` and `CustomShape`
  return `Measure` instead of `std::string`, and `Frame::z_index` returns
  `std::optional<std::int32_t>`. The OOXML and ppt engines already built a
  `Measure` internally and called `to_string()` only to satisfy the string
  interface, so for them this is a simplification.
* `Measure::to_string` renders 7 significant digits in positional notation via
  the new `util::number::to_string_significant`. The iostream default rounded
  to 4 (a drawing coordinate of `-6734.61mm` came out `-6735mm`) and could fall
  back to scientific notation, which CSS and SVG lengths reject. 7 digits is
  also the ceiling a `float` carries, so an xlsx column width stored as float
  `68.55` does not come back as `68.550003`.
* `TextStyle::font_name` is `std::optional<std::string_view>` rather than
  `const char *`, matching its sibling fields and keeping the zero-copy
  contract the oldms style registries are built around.
* Every exception derives from `odr::Exception`. Decoders still throw plain
  `std::runtime_error` for malformed input with no dedicated type, so that
  remains the widest net. Mirrored in Python as `pyodr.Error`.
* `Color` uses named factories `from_rgb` / `from_argb`, replacing the
  `Color(std::uint32_t, bool dummy)` disambiguator.
* `html::edit` takes `std::string_view`; `Html::config()` is `const`.
* `Measure` moved to `<odr/quantity.hpp>`, where AGENTS.md already documented
  it; `<odr/style.hpp>` still provides it.

Removed:

* `DecoderEngine` and the whole decoder-selection dimension. Since the external
  backends went in #622 the enum had one value and could not select anything.
  Drops `DecodePreference::with_engine` / `engine_priority`,
  `list_decoder_engines`, `decoder_engine_by_name` / `_to_string`,
  `DecodedFile::decoder_engine`, both decoder-engine exceptions, the
  `abstract::DecodedFile` hook and its twelve engine overrides, and the
  bindings' equivalents including `DecoderEngine.java`.

Also drops a duplicate `table_position.cpp` from `ODR_SOURCE_FILES`, documents
`word_perfect` / `rich_text_format` as detection-only, corrects the README
(json is supported), and adds CHANGELOG.md with an API stability policy.

Reference outputs regenerated: 232 files, numeric formatting only, no
structural change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FoqZKnajAkjWg1tdX3EqBq
Every other entry cites its PR; the ones added here had no number until the
PR existed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FoqZKnajAkjWg1tdX3EqBq

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c6c841d68

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/odr/internal/odf/odf_document.cpp
andiwand and others added 2 commits July 27, 2026 23:54
A default-constructed `DynamicUnit` left `m_unit` null while `name()`,
`to_string()` and `to_stream()` all dereferenced it unconditionally. The
geometry work in this branch made that reachable: an ODF shape missing one of
its `svg:*` attributes fell back to `Measure(0, DynamicUnit())`, and rendering
it segfaulted instead of emitting a zero length. Reported by Codex on #627.

Fixed at the invariant rather than the call sites: the default constructor now
takes the registered empty unit, the same one `Measure("5")` parses out of a
bare number. `m_unit` is therefore never null, and a fallback measure compares
equal to the parsed equivalent instead of being a look-alike that isn't.

Also replaces the remaining `const char *` in the public headers:

* `DynamicUnit` and `Quantity` take `std::string_view`. `Quantity` copies into
  a buffer first, since `std::strtod` needs a terminator a view does not
  promise, and stops treating a negative `char` as UB in `std::isspace`.
* `DocumentPath`'s `const char *` and `const std::string &` constructors
  collapse into one taking `std::string_view`.
* `CfbError` takes `const std::string &`, like its forty siblings.

`File::memory_data()` keeps `const char *`: it is a pointer to bytes paired
with `size()`, not a string.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FoqZKnajAkjWg1tdX3EqBq
`File::memory_data()` returned a bare `const char *` that only meant anything
alongside `size()`, and used null to signal "not in memory". It now returns
`std::optional<std::string_view>`, mirroring the `disk_path()` next to it: the
path if the file is on disk, the bytes if it is in memory. The view carries its
own length, so the pointer can no longer be read without one.

Also fixes `MemoryFile::location()`, which reported `FileLocation::disk`.
Nothing branched on it internally — it is only passed through to the bindings —
so this corrects what the API reports rather than changing any behaviour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FoqZKnajAkjWg1tdX3EqBq
@andiwand
andiwand enabled auto-merge (squash) July 27, 2026 22:07
@andiwand
andiwand merged commit 890fa54 into main Jul 27, 2026
18 checks passed
@andiwand
andiwand deleted the api-cleanup-6.0 branch July 27, 2026 22:14
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.

1 participant