Skip to content

Refactor: the per-format element registries, document classes and style registries are copy-paste siblings #770

Description

@andiwand

Every format that builds an element tree ships its own ElementRegistry, its own
Document, its own ElementAdapter and (for the index-based formats) its own
StyleRegistry. The per-format payloads differ, but the machinery around them
is the same code pasted ten times. AGENTS.md documents the pattern and points
at oldms/presentation/ppt_element_registry.* as the example to copy — which is
exactly how it spread.

This is a cleanup issue, not a bug report. Nothing is broken; it is ~1500 lines
that only ever change together, and the copies have already started to drift.

1. Element registries

Ten of them, 3028 lines:

src/odr/internal/odf/odf_element_registry.{hpp,cpp}                       548
src/odr/internal/iwork/iwork_element_registry.{hpp,cpp}                   430
src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.*   417
src/odr/internal/markdown/markdown_element_registry.*                     326
src/odr/internal/oldms/presentation/ppt_element_registry.*                265
src/odr/internal/oldms/spreadsheet/xls_element_registry.*                 267
src/odr/internal/ooxml/text/ooxml_text_element_registry.*                 248
src/odr/internal/ooxml/presentation/ooxml_presentation_element_registry.* 213
src/odr/internal/oldms/text/doc_element_registry.*                        168
src/odr/internal/rtf/rtf_element_registry.*                               146

Each one re-declares the identical node struct:

struct Element final {
  ElementIdentifier parent_id{null_element_id};
  ElementIdentifier first_child_id{null_element_id};
  ElementIdentifier last_child_id{null_element_id};
  ElementIdentifier previous_sibling_id{null_element_id};
  ElementIdentifier next_sibling_id{null_element_id};
  ElementType type{ElementType::none};
};

and re-implements, verbatim, clear, size, create_element, the const and
non-const element_at (m_elements.at(id - 1)), check_element_id, and
append_child. Compare rtf_element_registry.cpp:54-90 with
ppt_element_registry.cpp:100-150 — the bodies are character-identical apart
from the namespace.

On top of that, every payload type costs the same five hand-written members —
create_X_element, X_element_at ×2, check_X_id, and the
std::unordered_map<ElementIdentifier, X> m_Xs — each a two-line body around
one map lookup. That is 42 free functions in the odf registry, 42 in iwork, 35
in markdown, all doing the same four things over different payload structs.

The drift is already visible: the three ooxml registries dropped the
"child already has a parent" guard that append_child has everywhere else
(ooxml_text_element_registry.cpp:108), and the thrown messages say
ElementRegistry:: in some files and DocumentElementRegistry:: in others
(odf_element_registry.cpp:157, ooxml_text_element_registry.cpp:104) —
a name no class has carried for a while.

Sketch of the shared shape, in internal/common/:

  • an ElementRegistry base owning std::vector<Element> m_elements, the
    id↔index convention, check_element_id, create_element, element_at,
    append_child, plus a link_child(parent, child, first, last) for the
    secondary chains (odf already grew exactly this helper —
    odf_element_registry.cpp:135, and iwork/ooxml/markdown each open-code it for
    their column and shape chains);
  • one small class template for a payload side map, giving create, at ×2 and
    the range check, so a format declares PayloadMap<Text> m_texts; and the four
    members come with it, rather than writing them out per type.

What must not be forced by the base: odf's Sheet with its sorted repeat runs,
the several secondary chains (columns, shapes, sheet cells), and the formats
that do not use a registry at all — csv_document.cpp packs the coordinate into
the id, and pdf has its own scheme. Whatever lands has to stay opt-in for those.

2. Document classes and element adapters

Same story one level up, src/odr/internal/*/..._document.cpp (5704 lines
across 12 documents). Ten of them repeat the same four stubs verbatim:

bool Document::is_editable() const noexcept { return false; }
bool Document::is_savable(bool) const noexcept { return false; }
void Document::save(const Path &) const { throw UnsupportedOperation(); }
void Document::save(const Path &, const char *) const { throw UnsupportedOperation(); }

Only odf and the ooxml pair have anything real to say here, so internal::Document
(common/document.hpp) should carry the read-only default and let the two
editable engines override.

Every registry-backed ElementAdapter then opens with the same ~30 lines of
navigation forwarding — six methods each returning one field of
m_registry->element_at(element_id) — followed by the same
element_is_unique/element_is_self_locatable/element_is_editable constants
and the same two util::document::extract_path / navigate_path forwards
(11 copies). See rtf_document.cpp:65-115 next to ppt_document.cpp:78-125:
identical, modulo the (void)element_id vs [[maybe_unused]] spelling.

A RegistryElementAdapter<Registry> CRTP base (or a plain base holding the
registry pointer) removes all of it; the per-format adapter then only declares
which *_adapter(id) hooks it answers. The *_adapter(id) bodies themselves are
also all return element_type(id) == ElementType::x ? this : nullptr; and could
be one small helper.

3. Style registries

Weaker than the element case, but real at the two ends:

  • oldms/text/doc_style.hpp and oldms/presentation/ppt_style.hpp declare the
    same StyleRegistrystd::vector<std::string> m_font_names plus
    std::vector<TextStyle> m_styles, a two-argument constructor and
    text_style(index) — down to the comment explaining that font_name points
    into the owned strings. doc_style.cpp:53-59 and ppt_style.cpp:184-190 are
    the same six lines.
  • markdown_style.hpp and oldms/spreadsheet/xls_style.hpp are the same idea
    with a different payload (ParagraphStyle as well, ResolvedStyle by ixfe).

An index-keyed StyleRegistry<Style...> in internal/common/style.hpp — where
common::ResolvedStyle already lives — covers all four. The odf and ooxml
registries are a genuinely different thing (name-keyed, xml-node-backed, with
parent resolution), and the two ooxml ones plus odf's do share the
generate_indices_ / resolve_*_ shape, but that is a second, smaller question;
this issue only claims the index-keyed ones.

4. Adjacent: the DecodedFile implementations

While mapping the above: every ..._file.cpp writes the same file_meta()

FileMeta result;
result.type = file_type();
result.mimetype = mimetype();
result.document_type = document_type();
return result;

and the same file() / file_type() / mimetype() accessors over one
std::shared_ptr<abstract::File> m_file (rtf_file.cpp, markdown_file.cpp,
svg_file.cpp, csv_file.cpp, xml_file.cpp, …). mimetype and
document_type are already in file_type_table, so most of these could be a
base class parameterised on the FileType alone. Smallest of the four, and
independent of the rest.

Suggested order

  1. internal::Document read-only defaults — mechanical, touches ten files, no
    behaviour change.
  2. The shared element-registry base + payload map; port one small registry (rtf
    or doc) first to settle the shape, then the rest.
  3. The adapter navigation base.
  4. The index-keyed style registry, and the DecodedFile base, either time.

Steps 2 and 3 want AGENTS.md and the per-module AGENTS.md files updated in
the same PR — they currently teach the copy.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions