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 StyleRegistry — std::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
internal::Document read-only defaults — mechanical, touches ten files, no
behaviour change.
- The shared element-registry base + payload map; port one small registry (rtf
or doc) first to settle the shape, then the rest.
- The adapter navigation base.
- 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.
Every format that builds an element tree ships its own
ElementRegistry, its ownDocument, its ownElementAdapterand (for the index-based formats) its ownStyleRegistry. The per-format payloads differ, but the machinery around themis the same code pasted ten times.
AGENTS.mddocuments the pattern and pointsat
oldms/presentation/ppt_element_registry.*as the example to copy — which isexactly 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:
Each one re-declares the identical node struct:
and re-implements, verbatim,
clear,size,create_element, the const andnon-const
element_at(m_elements.at(id - 1)),check_element_id, andappend_child. Comparertf_element_registry.cpp:54-90withppt_element_registry.cpp:100-150— the bodies are character-identical apartfrom 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 thestd::unordered_map<ElementIdentifier, X> m_Xs— each a two-line body aroundone 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 thatappend_childhas everywhere else(
ooxml_text_element_registry.cpp:108), and the thrown messages sayElementRegistry::in some files andDocumentElementRegistry::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/:ElementRegistrybase owningstd::vector<Element> m_elements, theid↔index convention,
check_element_id,create_element,element_at,append_child, plus alink_child(parent, child, first, last)for thesecondary chains (odf already grew exactly this helper —
odf_element_registry.cpp:135, and iwork/ooxml/markdown each open-code it fortheir column and shape chains);
create,at×2 andthe range check, so a format declares
PayloadMap<Text> m_texts;and the fourmembers come with it, rather than writing them out per type.
What must not be forced by the base: odf's
Sheetwith 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.cpppacks the coordinate intothe 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 linesacross 12 documents). Ten of them repeat the same four stubs verbatim:
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 twoeditable engines override.
Every registry-backed
ElementAdapterthen opens with the same ~30 lines ofnavigation forwarding — six methods each returning one field of
m_registry->element_at(element_id)— followed by the sameelement_is_unique/element_is_self_locatable/element_is_editableconstantsand the same two
util::document::extract_path/navigate_pathforwards(11 copies). See
rtf_document.cpp:65-115next toppt_document.cpp:78-125:identical, modulo the
(void)element_idvs[[maybe_unused]]spelling.A
RegistryElementAdapter<Registry>CRTP base (or a plain base holding theregistry pointer) removes all of it; the per-format adapter then only declares
which
*_adapter(id)hooks it answers. The*_adapter(id)bodies themselves arealso all
return element_type(id) == ElementType::x ? this : nullptr;and couldbe one small helper.
3. Style registries
Weaker than the element case, but real at the two ends:
oldms/text/doc_style.hppandoldms/presentation/ppt_style.hppdeclare thesame
StyleRegistry—std::vector<std::string> m_font_namesplusstd::vector<TextStyle> m_styles, a two-argument constructor andtext_style(index)— down to the comment explaining thatfont_namepointsinto the owned strings.
doc_style.cpp:53-59andppt_style.cpp:184-190arethe same six lines.
markdown_style.hppandoldms/spreadsheet/xls_style.hppare the same ideawith a different payload (
ParagraphStyleas well,ResolvedStyleby ixfe).An index-keyed
StyleRegistry<Style...>ininternal/common/style.hpp— wherecommon::ResolvedStylealready lives — covers all four. The odf and ooxmlregistries 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
DecodedFileimplementationsWhile mapping the above: every
..._file.cppwrites the samefile_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 onestd::shared_ptr<abstract::File> m_file(rtf_file.cpp,markdown_file.cpp,svg_file.cpp,csv_file.cpp,xml_file.cpp, …).mimetypeanddocument_typeare already infile_type_table, so most of these could be abase class parameterised on the
FileTypealone. Smallest of the four, andindependent of the rest.
Suggested order
internal::Documentread-only defaults — mechanical, touches ten files, nobehaviour change.
or doc) first to settle the shape, then the rest.
DecodedFilebase, either time.Steps 2 and 3 want
AGENTS.mdand the per-moduleAGENTS.mdfiles updated inthe same PR — they currently teach the copy.