From 566d26da264d06e3cc48e326892a6bfb159afa47 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 09:23:19 +0200 Subject: [PATCH 1/4] feat(svm): write the svg through a writer that escapes, and log what we drop The svm to svg translator wrote markup straight into the stream, which means text went in unescaped: an `&`, `<` or `>` in a chart label makes the svg malformed, and a malformed svg renders as nothing at all - the whole image, not the one label. It also had no logger, so an action we do not implement, and the failure that falls back to emitting the raw `.svm` bytes as a data url no browser renders, were both silent. `svg::SvgWriter` is the counterpart to `html::HtmlWriter`: elements, attributes, style declarations and text, escaped, plus number formatting that does not depend on the stream's locale or precision. `svm_to_svg.cpp` writes through it, `Translator::svg` becomes `svm::translate_to_svg` and takes a `Logger`. Threading that logger down needed one: `abstract::HtmlService` gained `logger()`, so views reach the sink the service was built with, and `WritingState` carries it to `translate_image_src`. `write_image_src` now asks the file type before trying an svm translation rather than attempting one on every image and catching, which is what makes a failure worth logging. Tests build their metafile as bytes inline, so an action is testable without a fixture. Output on the four svm fixtures is unchanged except for coordinate precision, which improves: `std::to_chars` with three decimals where the stream's six significant digits used to round 10519.375 to 10519.4. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CmCr22NW6wPQKiQidk96bq --- AGENTS.md | 3 +- CHANGELOG.md | 9 + CMakeLists.txt | 1 + src/odr/internal/abstract/html_service.hpp | 5 +- src/odr/internal/html/common.hpp | 8 +- src/odr/internal/html/document.cpp | 4 +- src/odr/internal/html/document_element.cpp | 3 +- src/odr/internal/html/filesystem.cpp | 2 +- src/odr/internal/html/html_service.cpp | 4 + src/odr/internal/html/html_service.hpp | 3 + src/odr/internal/html/image_file.cpp | 56 +++--- src/odr/internal/html/image_file.hpp | 4 +- src/odr/internal/html/media_file.cpp | 2 +- src/odr/internal/html/pdf_file.cpp | 4 +- src/odr/internal/html/text_file.cpp | 2 +- src/odr/internal/html/xml_file.cpp | 2 +- src/odr/internal/svg/AGENTS.md | 17 ++ src/odr/internal/svg/svg_writer.cpp | 154 +++++++++++++++ src/odr/internal/svg/svg_writer.hpp | 61 ++++++ src/odr/internal/svm/AGENTS.md | 76 ++++++++ src/odr/internal/svm/PLAN.md | 65 +++++++ src/odr/internal/svm/README.md | 21 -- src/odr/internal/svm/svm_format.cpp | 115 +++++++++++ src/odr/internal/svm/svm_format.hpp | 5 + src/odr/internal/svm/svm_to_svg.cpp | 208 ++++++++++---------- src/odr/internal/svm/svm_to_svg.hpp | 15 +- test/CMakeLists.txt | 1 + test/src/internal/html/image_file_test.cpp | 4 +- test/src/internal/svg/svg_writer_test.cpp | 84 ++++++++ test/src/internal/svm/svm_test.cpp | 214 ++++++++++++++++++++- 30 files changed, 986 insertions(+), 166 deletions(-) create mode 100644 src/odr/internal/svg/svg_writer.cpp create mode 100644 src/odr/internal/svg/svg_writer.hpp create mode 100644 src/odr/internal/svm/AGENTS.md create mode 100644 src/odr/internal/svm/PLAN.md delete mode 100644 src/odr/internal/svm/README.md create mode 100644 test/src/internal/svg/svg_writer_test.cpp diff --git a/AGENTS.md b/AGENTS.md index 346a58c3a..3bf16506a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,7 +71,8 @@ bytes ─▶ magic/open_strategy ─▶ DecodedFile ─▶ Document ─▶ Eleme | `src/odr/internal/markdown/` | Markdown (CommonMark + GFM via md4c), decoded to a text document; see [`markdown/AGENTS.md`](src/odr/internal/markdown/AGENTS.md) + [`markdown/PLAN.md`](src/odr/internal/markdown/PLAN.md). | | `src/odr/internal/xml/` | XML, rendered as a source view; see [`xml/AGENTS.md`](src/odr/internal/xml/AGENTS.md). | | `src/odr/internal/svg/` | SVG, detected by reading it as xml; see [`svg/AGENTS.md`](src/odr/internal/svg/AGENTS.md). | -| `src/odr/internal/{csv,json,text,svm}/` | Smaller formats. | +| `src/odr/internal/svm/` | StarView metafile, the vector image odf/ooxml packages carry for charts and OLE objects; translated to svg. See [`svm/AGENTS.md`](src/odr/internal/svm/AGENTS.md) + [`svm/PLAN.md`](src/odr/internal/svm/PLAN.md). | +| `src/odr/internal/{csv,json,text}/` | Smaller formats. | | `cli/src/` | CLI tools: `translate`, `back_translate`, `meta`, `server`. | | `python/` | Python bindings (`pyodr`, pybind11); see [`python/AGENTS.md`](python/AGENTS.md). | | `jni/` | JNI bindings (Java package `app.opendocument.core`); see [`jni/AGENTS.md`](jni/AGENTS.md). | diff --git a/CHANGELOG.md b/CHANGELOG.md index 79a80d5d3..25e27c0ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,15 @@ The release run heads these entries with the version and opens a fresh - A spreadsheet decodes in less memory: 626 MB peak instead of 914 MB on a 297 MB `content.xml`. Rendered output is unchanged. +- Text in a StarView metafile (the picture odf and ooxml packages carry for a + chart or an OLE object) is escaped on its way into the svg we generate from + it. An `&`, `<` or `>` in a label used to make the svg malformed, which costs + the whole image rather than the one label. + +- What a StarView metafile translation drops is logged: every action we do not + implement, and the failure that used to fall back to emitting the raw `.svm` + bytes as a data url no browser renders. + ## v6.12.0 - 2026-08-30 - New `Document::save(std::ostream &)` and `Document::save_to_memory()`, which diff --git a/CMakeLists.txt b/CMakeLists.txt index c822cfe34..81d879491 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -272,6 +272,7 @@ set(ODR_SOURCE_FILES "src/odr/internal/rtf/rtf_tokenizer.cpp" "src/odr/internal/svg/svg_file.cpp" + "src/odr/internal/svg/svg_writer.cpp" "src/odr/internal/svm/svm_file.cpp" "src/odr/internal/svm/svm_format.cpp" diff --git a/src/odr/internal/abstract/html_service.hpp b/src/odr/internal/abstract/html_service.hpp index 7b061c532..111c864ea 100644 --- a/src/odr/internal/abstract/html_service.hpp +++ b/src/odr/internal/abstract/html_service.hpp @@ -7,7 +7,8 @@ namespace odr { class File; -} +class Logger; +} // namespace odr namespace odr::internal::html { class HtmlWriter; @@ -20,6 +21,8 @@ class HtmlService { virtual ~HtmlService() = default; [[nodiscard]] virtual const HtmlConfig &config() const = 0; + /// The sink everything written through this service logs to. + [[nodiscard]] virtual const Logger &logger() const = 0; [[nodiscard]] virtual const HtmlViews &list_views() const = 0; virtual void warmup() const = 0; diff --git a/src/odr/internal/html/common.hpp b/src/odr/internal/html/common.hpp index 8b72be1a6..a2e4c2ca1 100644 --- a/src/odr/internal/html/common.hpp +++ b/src/odr/internal/html/common.hpp @@ -14,6 +14,7 @@ namespace odr { struct Color; struct HtmlConfig; class Html; +class Logger; } // namespace odr namespace odr::internal::abstract { @@ -24,17 +25,20 @@ namespace odr::internal::html { struct WritingState { WritingState(HtmlWriter &out, const HtmlConfig &config, - HtmlResources &resources) - : m_out{&out}, m_config{&config}, m_resources(&resources) {} + HtmlResources &resources, const Logger &logger) + : m_out{&out}, m_config{&config}, m_resources(&resources), + m_logger{&logger} {} [[nodiscard]] HtmlWriter &out() const { return *m_out; } [[nodiscard]] const HtmlConfig &config() const { return *m_config; } [[nodiscard]] HtmlResources &resources() const { return *m_resources; } + [[nodiscard]] const Logger &logger() const { return *m_logger; } private: HtmlWriter *m_out; const HtmlConfig *m_config; HtmlResources *m_resources; + const Logger *m_logger; }; /// Writes the viewport meta tag. Precedence: `config.viewport_content` (raw, diff --git a/src/odr/internal/html/document.cpp b/src/odr/internal/html/document.cpp index 88a34ec66..17f588253 100644 --- a/src/odr/internal/html/document.cpp +++ b/src/odr/internal/html/document.cpp @@ -279,7 +279,7 @@ class HtmlFragmentView final : public abstract::HtmlView { HtmlResources write_html(HtmlWriter &out) const override { HtmlResources resources; - WritingState state(out, service().config(), resources); + WritingState state(out, service().config(), resources, service().logger()); m_fragment->write_document(out, state); return resources; } @@ -415,7 +415,7 @@ class HtmlServiceImpl final : public HtmlService { HtmlResources write_document(HtmlWriter &out) const { HtmlResources resources; - WritingState state(out, config(), resources); + WritingState state(out, config(), resources, logger()); // every page in one file, so the column is as wide as the widest of them const std::optional content = diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index a7614980a..dea4c96cb 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -570,7 +570,8 @@ void html::translate_image(const Element &element, const WritingState &state) { clb("src", [&](std::ostream &o) { // reached only for internal images, which have a file // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - translate_image_src(image.file().value(), o, state.config()); + translate_image_src(image.file().value(), o, state.config(), + state.logger()); }); } }) diff --git a/src/odr/internal/html/filesystem.cpp b/src/odr/internal/html/filesystem.cpp index bc0bb172f..e267df552 100644 --- a/src/odr/internal/html/filesystem.cpp +++ b/src/odr/internal/html/filesystem.cpp @@ -177,7 +177,7 @@ class HtmlServiceImpl final : public HtmlService { HtmlResources write_filesystem(HtmlWriter &out) const { HtmlResources resources; - const WritingState state(out, config(), resources); + const WritingState state(out, config(), resources, logger()); const FileWalker file_walker = m_filesystem.file_walker("/"); diff --git a/src/odr/internal/html/html_service.cpp b/src/odr/internal/html/html_service.cpp index 2f79e3510..f0152ca03 100644 --- a/src/odr/internal/html/html_service.cpp +++ b/src/odr/internal/html/html_service.cpp @@ -11,6 +11,8 @@ HtmlService::HtmlService(HtmlConfig config, const Logger &logger) const HtmlConfig &HtmlService::config() const { return m_config; } +const Logger &HtmlService::logger() const { return m_logger; } + HtmlView::HtmlView(const abstract::HtmlService &service, std::string name, std::size_t index, std::string path) : m_service{&service}, m_name{std::move(name)}, m_index{index}, @@ -32,6 +34,8 @@ const std::optional &HtmlView::sheet_cut() const { const abstract::HtmlService &HtmlView::service() const { return *m_service; } +const Logger &HtmlView::logger() const { return m_service->logger(); } + HtmlResources HtmlView::write_html(HtmlWriter &out) const { return m_service->write_html(path(), out); } diff --git a/src/odr/internal/html/html_service.hpp b/src/odr/internal/html/html_service.hpp index c373fb7a7..20029a4e4 100644 --- a/src/odr/internal/html/html_service.hpp +++ b/src/odr/internal/html/html_service.hpp @@ -12,6 +12,7 @@ class HtmlService : public abstract::HtmlService { HtmlService(HtmlConfig config, const Logger &logger); [[nodiscard]] const HtmlConfig &config() const override; + [[nodiscard]] const Logger &logger() const override; private: HtmlConfig m_config; @@ -31,6 +32,8 @@ class HtmlView : public abstract::HtmlView { [[nodiscard]] const HtmlConfig &config() const override; [[nodiscard]] const std::optional &sheet_cut() const override; [[nodiscard]] const abstract::HtmlService &service() const; + /// The service's sink. + [[nodiscard]] const Logger &logger() const; HtmlResources write_html(HtmlWriter &out) const override; diff --git a/src/odr/internal/html/image_file.cpp b/src/odr/internal/html/image_file.cpp index 94df1dab6..17665daaa 100644 --- a/src/odr/internal/html/image_file.cpp +++ b/src/odr/internal/html/image_file.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -23,25 +24,30 @@ namespace { /// A starview metafile is converted to svg; anything else goes out as its own /// bytes labelled @p mime_type. void write_image_src(const ImageFile &image_file, std::ostream &out, - const std::string &mime_type) { - // try svm - try { - // TODO `image_file` is already an `SvmFile` - // TODO `impl()` might be a bit dirty - const std::shared_ptr image_file_impl = - image_file.file().impl(); - // TODO memory file might not be necessary; other istreams didn't support - // `tellg` - const svm::SvmFile svm_file(std::make_shared(*image_file_impl)); - std::ostringstream svg_out; - svm::Translator::svg(svm_file, svg_out); - // TODO use stream - out << file_to_url(svg_out.str(), "image/svg+xml"); - } catch (...) { - // else it is a usual image and goes out as it came in - // TODO use stream - out << file_to_url(*image_file.stream(), mime_type); + const std::string &mime_type, const Logger &logger) { + if (image_file.file_type() == FileType::starview_metafile) { + try { + // TODO `image_file` is already an `SvmFile` + // TODO `impl()` might be a bit dirty + const std::shared_ptr image_file_impl = + image_file.file().impl(); + // TODO memory file might not be necessary; other istreams didn't support + // `tellg` + const svm::SvmFile svm_file( + std::make_shared(*image_file_impl)); + std::ostringstream svg_out; + svm::translate_to_svg(svm_file, svg_out, logger); + // TODO use stream + out << file_to_url(svg_out.str(), "image/svg+xml"); + return; + } catch (const std::exception &e) { + // the raw bytes below render as nothing, so say why + ODR_WARNING(logger, "svm translation failed: " << e.what()); + } } + + // TODO use stream + out << file_to_url(*image_file.stream(), mime_type); } /// An image file knows exactly which format it is holding, so the data url @@ -110,7 +116,7 @@ class HtmlServiceImpl final : public HtmlService { HtmlResources write_image(HtmlWriter &out) const { HtmlResources resources; - const WritingState state(out, config(), resources); + const WritingState state(out, config(), resources, logger()); out.write_begin(); out.write_header_begin(); @@ -149,7 +155,8 @@ class HtmlServiceImpl final : public HtmlService { out.out() << " alt=\"Error: image not found or unsupported\""; out.out() << " src=\""; - write_image_src(m_image_file, out.out(), image_mime_type(m_image_file)); + write_image_src(m_image_file, out.out(), image_mime_type(m_image_file), + logger()); out.out() << "\">"; } @@ -176,9 +183,9 @@ class HtmlServiceImpl final : public HtmlService { namespace odr::internal { void html::translate_image_src(const File &file, std::ostream &out, - const HtmlConfig &config) { + const HtmlConfig &config, const Logger &logger) { try { - translate_image_src(DecodedFile(file).as_image_file(), out, config); + translate_image_src(DecodedFile(file).as_image_file(), out, config, logger); } catch (...) { // nothing named it, so the label is a guess - browsers sniff `` and // `image/jpg` is what they have been handed here for years @@ -188,8 +195,9 @@ void html::translate_image_src(const File &file, std::ostream &out, } void html::translate_image_src(const ImageFile &image_file, std::ostream &out, - const HtmlConfig & /*config*/) { - write_image_src(image_file, out, image_mime_type(image_file)); + const HtmlConfig & /*config*/, + const Logger &logger) { + write_image_src(image_file, out, image_mime_type(image_file), logger); } HtmlService html::create_image_service(const ImageFile &image_file, diff --git a/src/odr/internal/html/image_file.hpp b/src/odr/internal/html/image_file.hpp index 45225d794..8f6f139c7 100644 --- a/src/odr/internal/html/image_file.hpp +++ b/src/odr/internal/html/image_file.hpp @@ -14,9 +14,9 @@ class Logger; namespace odr::internal::html { void translate_image_src(const File &file, std::ostream &out, - const HtmlConfig &config); + const HtmlConfig &config, const Logger &logger); void translate_image_src(const ImageFile &image_file, std::ostream &out, - const HtmlConfig &config); + const HtmlConfig &config, const Logger &logger); HtmlService create_image_service(const ImageFile &image_file, HtmlConfig config, const Logger &logger); diff --git a/src/odr/internal/html/media_file.cpp b/src/odr/internal/html/media_file.cpp index 08a56b943..ece12d551 100644 --- a/src/odr/internal/html/media_file.cpp +++ b/src/odr/internal/html/media_file.cpp @@ -151,7 +151,7 @@ class HtmlServiceImpl final : public HtmlService { HtmlResources write_media(HtmlWriter &out) const { HtmlResources resources; - const WritingState state(out, config(), resources); + const WritingState state(out, config(), resources, logger()); // The media stays a resource rather than a data URI: a video is regularly // larger than everything else we emit put together, and base64 in the diff --git a/src/odr/internal/html/pdf_file.cpp b/src/odr/internal/html/pdf_file.cpp index 1c275fb0d..794ea8114 100644 --- a/src/odr/internal/html/pdf_file.cpp +++ b/src/odr/internal/html/pdf_file.cpp @@ -1377,7 +1377,7 @@ class HtmlServiceImpl final : public HtmlService { const PageHref &page_href) const { HtmlResources resources; ImageRegistry images(config(), resources); - const WritingState state(out, config(), resources); + const WritingState state(out, config(), resources, logger()); pdf::DocumentParser &parser = *m_parser; LinkResolver &link_resolver = *m_link_resolver; @@ -1956,7 +1956,7 @@ class HtmlServiceImpl final : public HtmlService { const std::size_t first_page_number, const PageHref &page_href) const { HtmlResources resources; ImageRegistry images(config(), resources); - const WritingState state(out, config(), resources); + const WritingState state(out, config(), resources, logger()); pdf::DocumentParser &parser = *m_parser; LinkResolver &link_resolver = *m_link_resolver; diff --git a/src/odr/internal/html/text_file.cpp b/src/odr/internal/html/text_file.cpp index 4b82390d9..2e2b43a5b 100644 --- a/src/odr/internal/html/text_file.cpp +++ b/src/odr/internal/html/text_file.cpp @@ -87,7 +87,7 @@ class HtmlServiceImpl final : public HtmlService { HtmlResources write_text(HtmlWriter &out) const { HtmlResources resources; - const WritingState state(out, config(), resources); + const WritingState state(out, config(), resources, logger()); const auto [text, charset] = body_and_charset(); diff --git a/src/odr/internal/html/xml_file.cpp b/src/odr/internal/html/xml_file.cpp index bb89ed8e0..583abefa2 100644 --- a/src/odr/internal/html/xml_file.cpp +++ b/src/odr/internal/html/xml_file.cpp @@ -247,7 +247,7 @@ class HtmlServiceImpl final : public HtmlService { HtmlResources write_xml(HtmlWriter &out) const { HtmlResources resources; - const WritingState state(out, config(), resources); + const WritingState state(out, config(), resources, logger()); const pugi::xml_document &document = m_xml_file->document(); diff --git a/src/odr/internal/svg/AGENTS.md b/src/odr/internal/svg/AGENTS.md index 85319fd4c..11283c8bf 100644 --- a/src/odr/internal/svg/AGENTS.md +++ b/src/odr/internal/svg/AGENTS.md @@ -68,6 +68,23 @@ If scalable, selectable svg is wanted later, isolation beats modification: serve the file as its own resource in a sandboxed iframe, where the browser contains it and the file stays intact. +## Writing svg is this module's other half + +`svg_writer.*` is the counterpart to `html/html_writer.*`: elements, +attributes, style declarations and text, escaped, for code that *generates* +svg. Today that is `svm/svm_to_svg.cpp`, translating a StarView metafile. + +Two things it does that a raw `operator<<` does not: + +- **It escapes.** svg is xml, so an unescaped `&` in a chart label does not + spoil one label, it costs the whole image — an xml parse error renders + nothing at all. `html::escape_text` is the wrong tool for it: that one emits + ` `, which is an html entity and undefined in xml. +- **It formats numbers itself**, in fixed notation via `std::to_chars`, + independent of whatever locale or precision the stream carries. A stream + imbued with a german locale would otherwise write `1,5` into a coordinate, + and a `style` declaration is css, where `1.2e+3` is not a length. + ## The xml layer is not free `XmlFile` holds the parsed tree for as long as the file is open, and pugixml's diff --git a/src/odr/internal/svg/svg_writer.cpp b/src/odr/internal/svg/svg_writer.cpp new file mode 100644 index 000000000..86d2756b8 --- /dev/null +++ b/src/odr/internal/svg/svg_writer.cpp @@ -0,0 +1,154 @@ +#include + +#include +#include +#include +#include +#include +#include + +namespace odr::internal { + +namespace { + +/// Tab, line feed and carriage return are the only characters below `0x20` an +/// xml 1.0 document may contain. +bool is_xml_control(const char c) { + const auto value = static_cast(c); + return value < 0x20 && c != '\t' && c != '\n' && c != '\r'; +} + +std::string escape(const std::string_view text, const bool attribute) { + std::string result; + result.reserve(text.size()); + + for (const char c : text) { + switch (c) { + case '&': + result += "&"; + break; + case '<': + result += "<"; + break; + case '>': + result += ">"; + break; + case '"': + result += attribute ? """ : "\""; + break; + default: + if (!is_xml_control(c)) { + result += c; + } + break; + } + } + + return result; +} + +} // namespace + +std::string svg::format_number(const double value) { + if (!std::isfinite(value)) { + return "0"; + } + + // the classic locale, because a stream imbued with a german one writes + // `1,5`, and that is two coordinates rather than one + std::ostringstream stream; + stream.imbue(std::locale::classic()); + stream << value; + std::string result = stream.str(); + + if (result.find('e') != std::string::npos) { + // a `style` declaration is css, and css reads no exponent, so a number + // that big is written out in full - at which point its fraction is noise + stream.str({}); + stream << std::fixed << std::setprecision(0) << value; + result = stream.str(); + } + + return result; +} + +std::string svg::escape_text(const std::string_view text) { + return escape(text, false); +} + +std::string svg::escape_attribute(const std::string_view value) { + return escape(value, true); +} + +svg::SvgWriter::SvgWriter(std::ostream &out) : m_out{&out} {} + +void svg::SvgWriter::close_tag(const bool with_content) { + if (!m_tag_open) { + return; + } + + if (!m_style.empty()) { + *m_out << " style=\"" << m_style << "\""; + m_style.clear(); + } + + *m_out << (with_content ? ">" : " />"); + m_tag_open = false; +} + +void svg::SvgWriter::write_element_begin(const std::string_view name) { + close_tag(true); + + *m_out << "<" << name; + m_stack.emplace_back(name); + m_tag_open = true; +} + +void svg::SvgWriter::write_element_end() { + if (m_stack.empty()) { + throw std::runtime_error("no element to end"); + } + + if (m_tag_open) { + close_tag(false); + } else { + *m_out << ""; + } + m_stack.pop_back(); +} + +void svg::SvgWriter::write_attribute(const std::string_view name, + const std::string_view value) { + if (!m_tag_open) { + throw std::runtime_error("no open tag to write an attribute to"); + } + *m_out << " " << name << "=\"" << escape_attribute(value) << "\""; +} + +void svg::SvgWriter::write_attribute(const std::string_view name, + const double value) { + write_attribute(name, format_number(value)); +} + +void svg::SvgWriter::write_style(const std::string_view property, + const std::string_view value) { + if (!m_tag_open) { + throw std::runtime_error("no open tag to write a style to"); + } + m_style += escape_attribute(property); + m_style += ":"; + m_style += escape_attribute(value); + m_style += ";"; +} + +void svg::SvgWriter::write_style(const std::string_view property, + const double value) { + write_style(property, format_number(value)); +} + +void svg::SvgWriter::write_text(const std::string_view text) { + close_tag(true); + *m_out << escape_text(text); +} + +} // namespace odr::internal diff --git a/src/odr/internal/svg/svg_writer.hpp b/src/odr/internal/svg/svg_writer.hpp new file mode 100644 index 000000000..df54b404a --- /dev/null +++ b/src/odr/internal/svg/svg_writer.hpp @@ -0,0 +1,61 @@ +#pragma once + +#include +#include +#include +#include + +namespace odr::internal::svg { + +/// Escapes @p text for element content, and drops the control characters xml +/// 1.0 does not allow. Not `html::escape_text`: that one emits ` `, which +/// no xml parser knows. +[[nodiscard]] std::string escape_text(std::string_view text); +/// As @ref escape_text, plus the quote that would end an attribute value. +[[nodiscard]] std::string escape_attribute(std::string_view value); + +/// @p value at the six significant digits a stream defaults to, but in the +/// classic locale and never in exponent notation - css reads neither `1,5` nor +/// `1.2e+3`. +[[nodiscard]] std::string format_number(double value); + +/// @brief Writes svg markup. +/// +/// Elements nest through @ref write_element_begin / @ref write_element_end; +/// attributes, style declarations and text go to the innermost open one. +/// Everything written is escaped, and numbers are formatted independently of +/// the stream's locale and precision. +/// +/// An element's `style` attribute is accumulated as declarations arrive and +/// written when the element takes content or ends, so attributes and style may +/// be written in any order. +class SvgWriter final { +public: + explicit SvgWriter(std::ostream &out); + + /// Opens `` where it took no content. + void write_element_end(); + + void write_attribute(std::string_view name, std::string_view value); + void write_attribute(std::string_view name, double value); + /// One declaration of the element's `style` attribute. + void write_style(std::string_view property, std::string_view value); + void write_style(std::string_view property, double value); + + void write_text(std::string_view text); + +private: + /// Ends the open tag, whether it takes content or not. + void close_tag(bool with_content); + + std::ostream *m_out{nullptr}; + std::vector m_stack; + /// The innermost element's tag is still open for attributes. + bool m_tag_open{false}; + /// The open tag's `style` declarations, unwritten. + std::string m_style; +}; + +} // namespace odr::internal::svg diff --git a/src/odr/internal/svm/AGENTS.md b/src/odr/internal/svm/AGENTS.md new file mode 100644 index 000000000..522cd982a --- /dev/null +++ b/src/odr/internal/svm/AGENTS.md @@ -0,0 +1,76 @@ +# AGENTS.md — `internal/svm` + +Read the root [`AGENTS.md`](../../../../AGENTS.md) first. This file covers what +svm does differently, and why. [`PLAN.md`](PLAN.md) has the roadmap. + +## What it is + +A StarView Metafile is the vector format StarOffice, OpenOffice and +LibreOffice write **object replacement images** in: the picture of a chart or +an OLE object that an odf or ooxml package carries alongside the object itself. +So an `.svm` rarely arrives on its own — it arrives inside a document, and it +is what the reader sees where a chart should be. + +The file is a signature (`VCLMTF`), a header, and then a flat list of *actions* +replayed against a graphics state, in the shape of a Windows metafile. Two +layers here: + +- `svm_format.*` — the binary reader: one `read_*` per object and per action. + Every record starts with a `VersionCompat` (version + length), and the + length is what lets the translator skip an action it does not implement. +- `svm_to_svg.*` — replays the actions against a `Context` (the graphics + state) and writes svg through `svg::SvgWriter`. + +`SvmFile` is an `abstract::ImageFile` and `is_decodable()` is false: there is +no element tree, and `html/image_file.cpp` renders it by translating it to svg +and embedding that as a data url. + +## There is no spec + +The format is not specified anywhere. The references, best first: + +- LibreOffice + [`SvmReader.cxx`](https://github.com/LibreOffice/core/blob/master/vcl/source/filter/svm/SvmReader.cxx) + — the authoritative binary layout, per action, per version. When a field's + meaning is in doubt, this is the answer. +- LibreOffice + [`svgwriter.cxx`](https://github.com/LibreOffice/core/blob/master/filter/source/svg/svgwriter.cxx) + — its own metafile → svg export, i.e. our problem already solved, for 52 + action types. The reference for *mapping* decisions. +- [`metaact.hxx`](https://github.com/LibreOffice/core/blob/master/include/vcl/metaact.hxx) + — what each action means. +- ONLYOFFICE's + [`SPEC`](https://github.com/ONLYOFFICE/core/blob/master/DesktopEditor/raster/Metafile/StarView/SPEC) + — a prose write-up modelled on [MS-WMF], reverse-engineered from the same + sources. Cheap to read, but incomplete: several FIXMEs, `Color` unfinished, + polygon flags explicitly unfinished. + +## Conventions + +- **An unimplemented action is skipped by its length, never guessed at.** The + loop checks how far the reader got: short means the rest is ignored (logged), + past the end means the file is malformed and we throw. +- **Everything unhandled is logged.** A metafile we cannot draw looks exactly + like one we drew correctly — a blank rectangle raises no error anywhere — so + the log is the only way to tell. Anything reached by `default:` says so. +- **The markup goes through `svg::SvgWriter`, never to the stream directly.** + Text in a chart label is arbitrary, so it is escaped, and svg is xml: an + unescaped `&` costs the whole image, not one label. Note that + `html::escape_text` is the *wrong* escape here — it emits ` `, which no + xml parser knows. + +## Testing + +`svm_test.cpp` builds its input as bytes inline through `SvmBuilder`, so an +action is testable without a fixture and the test says what the bytes mean. +The fixtures — `odr-public/svm/{chart-1,table-1}.svm`, +`odr-private/svm/{test,Vyplaty}.svm`, and the `odt`/`ods` files named `*svm*` +— stay the end-to-end check. + +LibreOffice renders the same file, which makes it the oracle for what the +drawing should look like: + +```sh +/Applications/LibreOffice.app/Contents/MacOS/soffice --headless \ + --convert-to svg --outdir /tmp test/data/input/odr-public/svm/chart-1.svm +``` diff --git a/src/odr/internal/svm/PLAN.md b/src/odr/internal/svm/PLAN.md new file mode 100644 index 000000000..636936960 --- /dev/null +++ b/src/odr/internal/svm/PLAN.md @@ -0,0 +1,65 @@ +# PLAN — `internal/svm` + +What is missing from the svm → svg translator and the order it gets fixed in. +Umbrella issue: [#772](https://github.com/opendocument-app/OpenDocument.core/issues/772), +sub-parts [#194](https://github.com/opendocument-app/OpenDocument.core/issues/194) +(bitmaps) and [#95](https://github.com/opendocument-app/OpenDocument.core/issues/95) +(font attributes). + +Read [`AGENTS.md`](AGENTS.md) for how the module is built and what the +references are. + +## Stages + +Each stage is one pull request, stacked on the one before it. + +1. **Infrastructure.** `svg::SvgWriter`, so markup is written by something that + escapes; a `Logger` through the translator, so an action we drop says so; + inline-bytes tests, so an action can be tested without a fixture. Fixes + #772's defects 1 (escaping), 9 (style dispatch) and 10 (silence). +2. **Fixes to what we already emit.** The graphics state stack (`PUSH`/`POP`), + poly-polygon fill rule, the font size and map-mode unit in the transform, + `LineInfo`. #772 defects 2, 3, 5, 6, 8. +3. **Text.** `TEXTALIGN`, the `TEXTARRAY` dx array, `TEXTRECT`, and the #95 + font attributes (bold, italic, underline, strikeout, family). +4. **Primitives.** `PIXEL`, `POINT`, `LINE`, `ROUNDRECT`, `ELLIPSE`, `ARC`, + `PIE`, `CHORD` — one `svgwriter.cxx` case each. +5. **Bitmaps** (#194). See the shortcut below. +6. **Fills, clipping, transparency.** `GRADIENT`, `GRADIENTEX`, `HATCH`, + `WALLPAPER`, the `CLIPREGION` family, `TRANSPARENT`, `FLOATTRANSPARENT`. +7. **Stretch.** Bézier flags (#772 defect 4), the `EPS` substitute metafile, + and version-1 (pre-`VCLMTF`) files via `SvmConverter.cxx`. + +## Shortcuts worth taking + +- **Bitmaps are `.bmp` files already.** `SvmReader` reads them with + `ReadDIB(…, bFileHeader=true)`, i.e. the action body holds a DIB *with* its + `BITMAPFILEHEADER` — `"BM"`, `bfSize`, `bfOffBits`. So a `BMP` action needs + no pixel decoding at all: read the header far enough to know the byte length, + hand the bytes to the browser as `data:image/bmp;base64,…` inside an + ``. Palettes, RLE4/RLE8 and bit fields are then the browser's problem, + not ours. Two cases still need work: `ZCOMPRESS` (a LibreOffice-only + compression — inflate with miniz, then rewrite the header), and the alpha + mask of `BMPEX` (a second DIB, `1` = transparent) which becomes an SVG + `` over an inverting `feColorMatrix`. +- **Béziers are cheap once the flags are read.** A polygon flag of + `PolyFlags::Control` marks a control point, so a flagged polygon maps onto an + SVG path's `C` segments directly. The reader is the part that is missing. +- **Gradients, hatches and dashes are declarative in SVG** — + ``, ``, ``, `stroke-dasharray`. No + rasterising, no tiling by hand. +- **`FLOATTRANSPARENT` nests a whole metafile**: translate it into a `` and + put the gradient on that group's `mask`. + +## Testing + +`svm_test.cpp` builds its input as bytes inline (`SvmBuilder`), so an action +gets a test without a fixture. The fixtures +(`odr-public/svm/{chart-1,table-1}.svm`, `odr-private/svm/{test,Vyplaty}.svm`) +stay the end-to-end check, and LibreOffice is the oracle for what the drawing +should look like: + +```sh +/Applications/LibreOffice.app/Contents/MacOS/soffice --headless \ + --convert-to svg --outdir /tmp test/data/input/odr-public/svm/chart-1.svm +``` diff --git a/src/odr/internal/svm/README.md b/src/odr/internal/svm/README.md deleted file mode 100644 index 798befa23..000000000 --- a/src/odr/internal/svm/README.md +++ /dev/null @@ -1,21 +0,0 @@ -# SVM implementation - -## Features - -- [ ] font - - [ ] size - - [ ] italic, bold - - [ ] alignment - - [ ] underline, strike through - - [ ] color - - [ ] family -- [ ] transform (e.g. flip, rotate) -- [ ] images - -## References - -- https://github.com/LibreOffice/core/blob/master/include/vcl/metaact.hxx - -### Related work - -- https://github.com/SoftarexTechnologies/svmconv diff --git a/src/odr/internal/svm/svm_format.cpp b/src/odr/internal/svm/svm_format.cpp index de9fc7c5c..cc6d41ec7 100644 --- a/src/odr/internal/svm/svm_format.cpp +++ b/src/odr/internal/svm/svm_format.cpp @@ -64,6 +64,121 @@ std::string svm::read_string_with_encoding(std::istream &in, return read_uint16_prefixed_ascii_string(in); } +std::string_view svm::action_type_name(const std::uint16_t type) { + switch (type) { + case META_NULL_ACTION: + return "META_NULL_ACTION"; + case META_PIXEL_ACTION: + return "META_PIXEL_ACTION"; + case META_POINT_ACTION: + return "META_POINT_ACTION"; + case META_LINE_ACTION: + return "META_LINE_ACTION"; + case META_RECT_ACTION: + return "META_RECT_ACTION"; + case META_ROUNDRECT_ACTION: + return "META_ROUNDRECT_ACTION"; + case META_ELLIPSE_ACTION: + return "META_ELLIPSE_ACTION"; + case META_ARC_ACTION: + return "META_ARC_ACTION"; + case META_PIE_ACTION: + return "META_PIE_ACTION"; + case META_CHORD_ACTION: + return "META_CHORD_ACTION"; + case META_POLYLINE_ACTION: + return "META_POLYLINE_ACTION"; + case META_POLYGON_ACTION: + return "META_POLYGON_ACTION"; + case META_POLYPOLYGON_ACTION: + return "META_POLYPOLYGON_ACTION"; + case META_TEXT_ACTION: + return "META_TEXT_ACTION"; + case META_TEXTARRAY_ACTION: + return "META_TEXTARRAY_ACTION"; + case META_STRETCHTEXT_ACTION: + return "META_STRETCHTEXT_ACTION"; + case META_TEXTRECT_ACTION: + return "META_TEXTRECT_ACTION"; + case META_BMP_ACTION: + return "META_BMP_ACTION"; + case META_BMPSCALE_ACTION: + return "META_BMPSCALE_ACTION"; + case META_BMPSCALEPART_ACTION: + return "META_BMPSCALEPART_ACTION"; + case META_BMPEX_ACTION: + return "META_BMPEX_ACTION"; + case META_BMPEXSCALE_ACTION: + return "META_BMPEXSCALE_ACTION"; + case META_BMPEXSCALEPART_ACTION: + return "META_BMPEXSCALEPART_ACTION"; + case META_MASK_ACTION: + return "META_MASK_ACTION"; + case META_MASKSCALE_ACTION: + return "META_MASKSCALE_ACTION"; + case META_MASKSCALEPART_ACTION: + return "META_MASKSCALEPART_ACTION"; + case META_GRADIENT_ACTION: + return "META_GRADIENT_ACTION"; + case META_HATCH_ACTION: + return "META_HATCH_ACTION"; + case META_WALLPAPER_ACTION: + return "META_WALLPAPER_ACTION"; + case META_CLIPREGION_ACTION: + return "META_CLIPREGION_ACTION"; + case META_ISECTRECTCLIPREGION_ACTION: + return "META_ISECTRECTCLIPREGION_ACTION"; + case META_ISECTREGIONCLIPREGION_ACTION: + return "META_ISECTREGIONCLIPREGION_ACTION"; + case META_MOVECLIPREGION_ACTION: + return "META_MOVECLIPREGION_ACTION"; + case META_LINECOLOR_ACTION: + return "META_LINECOLOR_ACTION"; + case META_FILLCOLOR_ACTION: + return "META_FILLCOLOR_ACTION"; + case META_TEXTCOLOR_ACTION: + return "META_TEXTCOLOR_ACTION"; + case META_TEXTFILLCOLOR_ACTION: + return "META_TEXTFILLCOLOR_ACTION"; + case META_TEXTALIGN_ACTION: + return "META_TEXTALIGN_ACTION"; + case META_MAPMODE_ACTION: + return "META_MAPMODE_ACTION"; + case META_FONT_ACTION: + return "META_FONT_ACTION"; + case META_PUSH_ACTION: + return "META_PUSH_ACTION"; + case META_POP_ACTION: + return "META_POP_ACTION"; + case META_RASTEROP_ACTION: + return "META_RASTEROP_ACTION"; + case META_TRANSPARENT_ACTION: + return "META_TRANSPARENT_ACTION"; + case META_EPS_ACTION: + return "META_EPS_ACTION"; + case META_REFPOINT_ACTION: + return "META_REFPOINT_ACTION"; + case META_TEXTLINECOLOR_ACTION: + return "META_TEXTLINECOLOR_ACTION"; + case META_TEXTLINE_ACTION: + return "META_TEXTLINE_ACTION"; + case META_FLOATTRANSPARENT_ACTION: + return "META_FLOATTRANSPARENT_ACTION"; + case META_GRADIENTEX_ACTION: + return "META_GRADIENTEX_ACTION"; + case META_LAYOUTMODE_ACTION: + return "META_LAYOUTMODE_ACTION"; + case META_TEXTLANGUAGE_ACTION: + return "META_TEXTLANGUAGE_ACTION"; + case META_OVERLINECOLOR_ACTION: + return "META_OVERLINECOLOR_ACTION"; + case META_COMMENT_ACTION: + return "META_COMMENT_ACTION"; + default: + return "UNKNOWN"; + } +} + svm::VersionLength svm::read_version_length(std::istream &in) { VersionLength result; read_primitive(in, result.version); diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index 043914672..087e697d2 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -5,6 +5,7 @@ #include #include #include +#include #include // https://github.com/LibreOffice/core/blob/master/include/vcl/metaact.hxx @@ -206,6 +207,10 @@ struct TextLineAction final { std::uint32_t overline{}; }; +/// The action type's name, as `metaact.hxx` spells it, or `"UNKNOWN"`. For +/// logging what we skipped. +[[nodiscard]] std::string_view action_type_name(std::uint16_t type); + /// Reads a fixed-size field. A short read leaves the destination untouched, so /// the stream ending mid-field is malformed input rather than a stale value. template void read_primitive(std::istream &in, T &out) { diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index b59a1bc3c..7e8213b32 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -1,7 +1,9 @@ #include #include +#include +#include #include #include @@ -10,10 +12,23 @@ namespace odr::internal::svm { namespace { + +/// Which of the graphics state's colours a shape draws with. +enum class StyleKind { + line, ///< stroke, no fill + fill, ///< fill, no stroke + text, ///< the text colour as fill, plus the font +}; + +/// The action type's name with its number, for a log line. +std::string action_name(const ActionHeader &action_header) { + return std::string(action_type_name(action_header.type)) + "(" + + std::to_string(action_header.type) + ")"; +} + struct Context final { - std::istream *in{}; - std::ostream *out{}; - const ActionHeader *action{}; + svg::SvgWriter *out{}; + const Logger *logger{}; MapMode map_mode; TextEncoding encoding{}; @@ -50,102 +65,96 @@ std::string get_svg_color_string(const std::uint32_t color) { std::to_string(blue) + ")"; } -void write_color_style(std::ostream &out, const std::string &prefix, +/// The colour, or the property turned off where the state does not set one. +void write_color_style(svg::SvgWriter &out, const std::string &property, const std::uint32_t color, const bool set) { if (set) { - out << prefix << ":" << get_svg_color_string(color); + out.write_style(property, get_svg_color_string(color)); } else { - out << prefix << "-opacity:0"; + out.write_style(property + "-opacity", "0"); } - out << ";"; } -void write_line_style(std::ostream &out, const Context &context) { +void write_line_style(svg::SvgWriter &out, const Context &context) { write_color_style(out, "stroke", context.line_rgb, context.line_rgb_set); - out << "vector-effect:non-scaling-stroke;"; - out << "fill:none;"; + out.write_style("vector-effect", "non-scaling-stroke"); + out.write_style("fill", "none"); } -void write_fill_style(std::ostream &out, const Context &context) { +void write_fill_style(svg::SvgWriter &out, const Context &context) { write_color_style(out, "fill", context.fill_rgb, context.fill_rgb_set); - out << "stroke:none;"; + out.write_style("stroke", "none"); } -void write_text_style(std::ostream &out, const Context &context) { +void write_text_style(svg::SvgWriter &out, const Context &context) { write_color_style(out, "fill", context.text_rgb, true); - out << "font-family:" << context.font.family_name << ";"; - out << "font-size:" << context.font.size.y << ";"; + out.write_style("font-family", context.font.family_name); + out.write_style("font-size", context.font.size.y); } -void write_style(std::ostream &out, const Context &context, const int styles) { - out << " style=\""; - switch (styles) { - case 0: +void write_style(svg::SvgWriter &out, const Context &context, + const StyleKind kind) { + switch (kind) { + case StyleKind::line: write_line_style(out, context); break; - case 1: + case StyleKind::fill: + // TODO the fill kills the stroke the line style just wrote write_line_style(out, context); write_fill_style(out, context); break; - case 2: + case StyleKind::text: write_text_style(out, context); break; - default: - // TODO log or throw - ; } - out << "\""; } -void write_rectangle(std::ostream &out, const Rectangle &rect, - const Context &context) { - out << ""; +void write_rectangle(const Rectangle &rect, const Context &context) { + svg::SvgWriter &out = *context.out; + + out.write_element_begin("rect"); + out.write_attribute("x", transform_x(rect.left, context)); + out.write_attribute("y", transform_y(rect.top, context)); + out.write_attribute("width", transform_x(rect.right, context) - + transform_x(rect.left, context)); + out.write_attribute("height", transform_y(rect.bottom, context) - + transform_y(rect.top, context)); + write_style(out, context, StyleKind::fill); + out.write_element_end(); } -void write_polygon(std::ostream &out, const std::string &tag, - const std::vector &points, const bool fill, - const Context &context) { - out << "<" << tag; - - out << " points=\""; - for (auto [x, y] : points) { - out << transform_x(x, context) << "," << transform_y(y, context); - out << " "; - } - out << "\""; +void write_polygon(const std::string &tag, const std::vector &points, + const bool fill, const Context &context) { + svg::SvgWriter &out = *context.out; - if (fill) { - write_style(out, context, 1); - } else { - write_style(out, context, 0); + std::string points_attribute; + for (const auto [x, y] : points) { + points_attribute += svg::format_number(transform_x(x, context)); + points_attribute += ","; + points_attribute += svg::format_number(transform_y(y, context)); + points_attribute += " "; } - out << " />"; + out.write_element_begin(tag); + out.write_attribute("points", points_attribute); + write_style(out, context, fill ? StyleKind::fill : StyleKind::line); + out.write_element_end(); } -void write_text(std::ostream &out, const IntPair &point, - const std::string &text, const Context &context) { - out << ""; - out << text; - out << ""; +void write_text(const IntPair &point, const std::string &text, + const Context &context) { + svg::SvgWriter &out = *context.out; + + out.write_element_begin("text"); + out.write_attribute("x", transform_x(point.x, context)); + out.write_attribute("y", transform_y(point.y, context)); + write_style(out, context, StyleKind::text); + out.write_text(text); + out.write_element_end(); } void translate_action(const ActionHeader &action_header, std::istream &in, - std::ostream &out, Context &context) { + Context &context) { switch (action_header.type) { case META_FILLCOLOR_ACTION: read_primitive(in, context.fill_rgb); @@ -173,95 +182,96 @@ void translate_action(const ActionHeader &action_header, std::istream &in, context.text_line = read_text_line_action(in, action_header.vl); break; case META_RECT_ACTION: { - Rectangle action = read_rectangle(in); - write_rectangle(out, action, context); + const Rectangle action = read_rectangle(in); + write_rectangle(action, context); } break; case META_MAPMODE_ACTION: { context.map_mode = read_map_mode(in); } break; case META_POLYLINE_ACTION: { auto [points, line_info] = read_poly_line_action(in, action_header.vl); - write_polygon(out, "polyline", points, false, context); + write_polygon("polyline", points, false, context); } break; case META_POLYGON_ACTION: { auto [points] = read_polygon_action(in, action_header.vl); - write_polygon(out, "polygon", points, true, context); + write_polygon("polygon", points, true, context); } break; case META_POLYPOLYGON_ACTION: { auto [polygons] = read_poly_polygon_action(in, action_header.vl); for (const auto &p : polygons) { - write_polygon(out, "polygon", p, true, context); + write_polygon("polygon", p, true, context); } } break; case META_TEXT_ACTION: { - TextAction action = + const TextAction action = read_text_action(in, action_header.vl, context.encoding); - write_text(out, action.point, action.text, context); + write_text(action.point, action.text, context); } break; case META_TEXTARRAY_ACTION: { - TextArrayAction action = + const TextArrayAction action = read_text_array_action(in, action_header.vl, context.encoding); - write_text(out, action.point, action.text, context); + write_text(action.point, action.text, context); } break; case META_STRETCHTEXT_ACTION: { - StretchTextAction action = + const StretchTextAction action = read_stretch_text_action(in, action_header.vl, context.encoding); - write_text(out, action.point, action.text, context); + write_text(action.point, action.text, context); } break; - case META_TEXTRECT_ACTION: - // TODO read_text_rectangle_action; the caller skips the body meanwhile - break; - case META_NULL_ACTION: - case META_PUSH_ACTION: - case META_POP_ACTION: - case META_TEXTLANGUAGE_ACTION: - case META_COMMENT_ACTION: - // TODO implement default: - // TODO log unhandled action + ODR_DEBUG(*context.logger, + "unhandled action " << action_name(action_header) << ", skipping " + << action_header.vl.length << " bytes"); in.ignore(action_header.vl.length); break; } } + } // namespace +} // namespace odr::internal::svm + +namespace odr::internal { -void Translator::svg(const SvmFile &file, std::ostream &out) { +void svm::translate_to_svg(const SvmFile &file, std::ostream &out, + const Logger &logger) { const auto istream = file.file()->stream(); auto &in = *istream; + svg::SvgWriter writer(out); + Context context; - context.in = ∈ - context.out = &out; + context.out = &writer; + context.logger = &logger; const Header header = read_header(in); context.encoding = RTL_TEXTENCODING_ASCII_US; context.map_mode = header.map_mode; - out << ""; + writer.write_element_begin("svg"); + writer.write_attribute("xmlns", "http://www.w3.org/2000/svg"); + writer.write_attribute("version", "1.1"); + writer.write_attribute("viewBox", "0 0 " + std::to_string(header.size.x) + + " " + std::to_string(header.size.y)); while (in.peek() != -1) { // TODO check length fields should never exceed file size (limited istream?) - ActionHeader action_header = read_action_header(in); + const ActionHeader action_header = read_action_header(in); const std::int64_t start = in.tellg(); - translate_action(action_header, in, out, context); + translate_action(action_header, in, context); const std::int64_t left = action_header.vl.length - (static_cast(in.tellg()) - start); if (left > 0) { - // TODO log skipping - in.ignore(left); + ODR_DEBUG(logger, "action " << action_name(action_header) << " skipping " + << left << " trailing bytes"); + in.ignore(static_cast(left)); } else if (left < 0) { throw MalformedSvmFile(); } } - out << ""; + writer.write_element_end(); } -} // namespace odr::internal::svm +} // namespace odr::internal diff --git a/src/odr/internal/svm/svm_to_svg.hpp b/src/odr/internal/svm/svm_to_svg.hpp index 8d79cdac8..b6f5bc6fe 100644 --- a/src/odr/internal/svm/svm_to_svg.hpp +++ b/src/odr/internal/svm/svm_to_svg.hpp @@ -1,11 +1,18 @@ #pragma once -#include +#include + +namespace odr { +class Logger; +} namespace odr::internal::svm { class SvmFile; -namespace Translator { -void svg(const SvmFile &file, std::ostream &out); -} +/// Translates @p file's actions into an svg document. Actions we do not +/// implement are logged and dropped. +/// @throws MalformedSvmFile where an action reads past its own length. +void translate_to_svg(const SvmFile &file, std::ostream &out, + const Logger &logger); + } // namespace odr::internal::svm diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 00a37bce1..9fac57a22 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -57,6 +57,7 @@ add_executable(odr_test "src/internal/csv/csv_file_test.cpp" "src/internal/encoding/text_encoding_test.cpp" "src/internal/svg/svg_file_test.cpp" + "src/internal/svg/svg_writer_test.cpp" "src/internal/xml/xml_file_test.cpp" "src/internal/markdown/markdown_file_test.cpp" diff --git a/test/src/internal/html/image_file_test.cpp b/test/src/internal/html/image_file_test.cpp index 55afa9f83..4c1101013 100644 --- a/test/src/internal/html/image_file_test.cpp +++ b/test/src/internal/html/image_file_test.cpp @@ -49,7 +49,7 @@ std::string write_path(const HtmlService &service, const std::string &path) { std::string image_src(const File &file) { std::ostringstream out; internal::html::translate_image_src(DecodedFile(file).as_image_file(), out, - HtmlConfig()); + HtmlConfig(), Logger::null()); return out.str(); } @@ -165,7 +165,7 @@ TEST(image_file, an_embedded_image_is_named_by_its_own_mime_type) { TEST(image_file, an_unrecognised_image_falls_back_to_a_guess) { std::ostringstream out; internal::html::translate_image_src(image_file("not an image at all"), out, - HtmlConfig()); + HtmlConfig(), Logger::null()); EXPECT_NE(out.str().find("data:image/jpg;base64,"), std::string::npos); } diff --git a/test/src/internal/svg/svg_writer_test.cpp b/test/src/internal/svg/svg_writer_test.cpp new file mode 100644 index 000000000..923d82a03 --- /dev/null +++ b/test/src/internal/svg/svg_writer_test.cpp @@ -0,0 +1,84 @@ +#include + +#include +#include +#include + +#include + +using namespace odr::internal::svg; + +namespace { + +std::string write(void (*body)(SvgWriter &)) { + std::ostringstream out; + SvgWriter writer(out); + body(writer); + return out.str(); +} + +} // namespace + +TEST(SvgWriter, an_element_without_content_closes_itself) { + EXPECT_EQ("", write([](SvgWriter &out) { + out.write_element_begin("rect"); + out.write_attribute("x", 1.0); + out.write_element_end(); + })); +} + +TEST(SvgWriter, style_declarations_are_collected_into_one_attribute) { + EXPECT_EQ("hi", + write([](SvgWriter &out) { + out.write_element_begin("text"); + out.write_style("fill", "red"); + out.write_attribute("x", 2.0); + out.write_style("font-size", 3.0); + out.write_text("hi"); + out.write_element_end(); + })); +} + +TEST(SvgWriter, elements_nest) { + EXPECT_EQ("", write([](SvgWriter &out) { + out.write_element_begin("g"); + out.write_element_begin("rect"); + out.write_element_end(); + out.write_element_end(); + })); +} + +/// An unescaped `&` or `<` makes the document malformed, and a malformed svg +/// renders as nothing at all. +TEST(SvgWriter, escape_text) { + EXPECT_EQ("a & b <c> \"d\"", escape_text("a & b \"d\"")); +} + +TEST(SvgWriter, escape_attribute) { + EXPECT_EQ("a"b & c", escape_attribute("a\"b & c")); +} + +/// Xml 1.0 has no way to write these, escaped or not. +TEST(SvgWriter, escape_drops_control_characters) { + EXPECT_EQ("ab\tc", escape_text(std::string("a\x01" + "b\tc"))); +} + +TEST(SvgWriter, format_number) { + EXPECT_EQ("0", format_number(0)); + EXPECT_EQ("1", format_number(1)); + EXPECT_EQ("-1.5", format_number(-1.5)); + EXPECT_EQ("10519.4", format_number(10519.375)); +} + +/// Css reads no exponent, so `font-size:1.2e+07` would be no size at all. +TEST(SvgWriter, format_number_never_uses_an_exponent) { + EXPECT_EQ("12345678", format_number(12345678.0)); + EXPECT_EQ("0", format_number(0.0000001)); +} + +/// Nothing sensible to draw at, and `nan` in an attribute drops the element. +TEST(SvgWriter, format_number_of_something_that_is_not_a_number) { + EXPECT_EQ("0", format_number(std::numeric_limits::quiet_NaN())); + EXPECT_EQ("0", format_number(std::numeric_limits::infinity())); +} diff --git a/test/src/internal/svm/svm_test.cpp b/test/src/internal/svm/svm_test.cpp index 3ae84b636..5fa6149b4 100644 --- a/test/src/internal/svm/svm_test.cpp +++ b/test/src/internal/svm/svm_test.cpp @@ -1,19 +1,129 @@ #include +#include #include #include +#include #include #include +#include #include #include +#include +#include #include using namespace odr::internal; using namespace odr::test; +namespace { + +/// Builds a metafile byte by byte, so an action can be tested without a +/// fixture. Mirrors what `SvmReader` expects to read back. +class SvmBuilder final { +public: + SvmBuilder &u8(const std::uint8_t value) { + m_data.push_back(static_cast(value)); + return *this; + } + + SvmBuilder &u16(const std::uint16_t value) { + return u8(value & 0xff).u8(value >> 8 & 0xff); + } + + SvmBuilder &u32(const std::uint32_t value) { + return u16(value & 0xffff).u16(value >> 16 & 0xffff); + } + + SvmBuilder &i32(const std::int32_t value) { + return u32(static_cast(value)); + } + + SvmBuilder &point(const std::int32_t x, const std::int32_t y) { + return i32(x).i32(y); + } + + SvmBuilder &rectangle(const std::int32_t left, const std::int32_t top, + const std::int32_t right, const std::int32_t bottom) { + return i32(left).i32(top).i32(right).i32(bottom); + } + + /// A pascal string, as `read_uint16_prefixed_ascii_string` reads it. + SvmBuilder &ascii_string(const std::string &value) { + u16(static_cast(value.size())); + m_data += value; + return *this; + } + + /// Opens a `VersionCompat` whose length is filled in by @ref end. + SvmBuilder &begin(const std::uint16_t version = 1) { + u16(version); + m_open.push_back(m_data.size()); + u32(0); + return *this; + } + + SvmBuilder &end() { + const std::size_t offset = m_open.back(); + m_open.pop_back(); + const std::size_t length = m_data.size() - offset - 4; + for (std::size_t i = 0; i < 4; ++i) { + m_data[offset + i] = static_cast(length >> (8 * i) & 0xff); + } + return *this; + } + + /// The action's type and `VersionCompat`; ends with @ref end. + SvmBuilder &action(const svm::MetaActionType type, + const std::uint16_t version = 1) { + return u16(static_cast(type)).begin(version); + } + + SvmBuilder &map_mode(const std::int32_t scale_numerator = 1, + const std::int32_t scale_denominator = 1) { + return begin() + .u16(0) // unit + .point(0, 0) // origin + .i32(scale_numerator) // scale x + .i32(scale_denominator) // + .i32(scale_numerator) // scale y + .i32(scale_denominator) // + .u8(0) // simple + .end(); + } + + /// `"VCLMTF"`, the header, and whatever actions follow. + [[nodiscard]] std::string file(const std::int32_t width = 100, + const std::int32_t height = 100) const { + SvmBuilder header; + header.m_data = "VCLMTF"; + header.begin(2) + .u32(0) // compression mode + .map_mode() + .point(width, height) + .u32(0) // action count + .u8(0) // render graphic replacements + .end(); + return header.m_data + m_data; + } + +private: + std::string m_data; + std::vector m_open; +}; + +std::string translate(const std::string &data) { + const svm::SvmFile file(std::make_shared(data)); + std::ostringstream out; + svm::translate_to_svg(file, out, odr::Logger::null()); + return out.str(); +} + +} // namespace + TEST(SvmFile, open) { const svm::SvmFile svm(std::make_shared( TestData::test_file_path("odr-public/svm/chart-1.svm"))); @@ -21,12 +131,114 @@ TEST(SvmFile, open) { EXPECT_EQ(odr::FileType::starview_metafile, svm.file_type()); } +TEST(SvmToSvg, empty) { + EXPECT_EQ("", + translate(SvmBuilder().file())); +} + +TEST(SvmToSvg, rectangle) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_FILLCOLOR_ACTION) + .u32(0x0000ff) + .u8(1) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(1, 2, 11, 22) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("hello")); +} + +/// Unescaped, the markup below is not a document and the browser draws +/// nothing. +TEST(SvmToSvg, text_is_escaped) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_TEXT_ACTION) + .point(0, 0) + .ascii_string("a & b ") + .u16(0) + .u16(9) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find(">a & b <c>")); +} + +/// A font family is written into an attribute, and a quote in it would end +/// that attribute. +TEST(SvmToSvg, font_family_is_escaped) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_FONT_ACTION) + .begin() + .ascii_string("a\"b") + .ascii_string("") + .point(0, 10) + .u16(11) // charset + .u16(0) // family + .u16(0) // pitch + .u16(0) // weight + .u16(0) // underline + .u16(0) // strikeout + .u16(0) // italic + .u16(0) // language + .u16(0) // width + .u16(0) // orientation + .u8(0) // wordline + .u8(0) // outline + .u8(0) // shadow + .u8(0) // kerning + .end() + .end() + .action(svm::META_TEXT_ACTION) + .point(0, 0) + .ascii_string("x") + .u16(0) + .u16(1) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("font-family:a"b")); +} + +/// An action we do not implement is skipped by its own length, so the ones +/// after it still read. +TEST(SvmToSvg, unhandled_action_is_skipped) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_ELLIPSE_ACTION) + .rectangle(0, 0, 10, 10) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(1, 1, 2, 2) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("( TestData::test_file_path("odr-public/svm/table-1.svm"))); std::stringstream out; - svm::Translator::svg(svm, out); + svm::translate_to_svg(svm, out, odr::Logger::null()); EXPECT_LT(0, out.str().size()); } From 8390f966be1586b96974dc8759f18246dab5f8e9 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 10:31:24 +0200 Subject: [PATCH 2/4] refactor(svg, html): one xml escaper and one number formatter for both `svg::format_number` was a second implementation of `util::number::to_string_significant`, whose own doc comment already says "never in scientific notation, which CSS and SVG lengths do not accept". The two agree on every value the svg tests cover; they parted only below `1e-4`, where the stream-based one collapsed `1e-07` to `0`. The classic locale the svg writer wanted moves into `number_util`, so `Measure::to_string` gets it too. `svg::escape_text` / `escape_attribute` and `html::escape_attribute` were the same escape written twice - the html one as four `replace_all` passes over a by-value string. Both are now `util::xml::escape_text` / `escape_attribute`, one pass, dropping the control characters xml 1.0 cannot carry. `html::escape_text` stays in `html`: its ` ` and ` ` are presentation, not escaping. Also stops a `;` in a style value from opening a declaration of its own. It is dropped before escaping, which writes `;` of its own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XQpLmJpJ87qbKoG8B7kbLY --- CHANGELOG.md | 17 +++-- src/odr/internal/html/common.cpp | 15 +--- src/odr/internal/html/common.hpp | 7 +- src/odr/internal/html/document_element.cpp | 10 ++- src/odr/internal/html/document_style.cpp | 15 ++-- src/odr/internal/html/filesystem.cpp | 15 ++-- src/odr/internal/html/frontend.cpp | 6 +- src/odr/internal/html/media_file.cpp | 3 +- src/odr/internal/html/pdf_file.cpp | 5 +- src/odr/internal/svg/AGENTS.md | 20 +++-- src/odr/internal/svg/svg_writer.cpp | 85 ++++------------------ src/odr/internal/svg/svg_writer.hpp | 30 ++------ src/odr/internal/util/number_util.cpp | 3 + src/odr/internal/util/number_util.hpp | 7 +- src/odr/internal/util/xml_util.cpp | 49 +++++++++++++ src/odr/internal/util/xml_util.hpp | 7 ++ test/src/internal/svg/svg_writer_test.cpp | 39 +++++----- test/src/internal/util/xml_util_test.cpp | 14 ++++ 18 files changed, 177 insertions(+), 170 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25e27c0ab..82312629a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,14 +19,15 @@ The release run heads these entries with the version and opens a fresh - A spreadsheet decodes in less memory: 626 MB peak instead of 914 MB on a 297 MB `content.xml`. Rendered output is unchanged. -- Text in a StarView metafile (the picture odf and ooxml packages carry for a - chart or an OLE object) is escaped on its way into the svg we generate from - it. An `&`, `<` or `>` in a label used to make the svg malformed, which costs - the whole image rather than the one label. - -- What a StarView metafile translation drops is logged: every action we do not - implement, and the failure that used to fall back to emitting the raw `.svm` - bytes as a data url no browser renders. +- Text in a StarView metafile is escaped into the svg it renders as. An `&`, + `<` or `>` in a label made the svg malformed, and a malformed svg renders as + nothing. + +- A StarView metafile translation logs what it drops: the actions it does not + implement, and a translation failure it used to fall back from silently. + +- An html attribute value drops the control characters xml forbids, rather + than carrying them through. One escaper writes both html and svg now. ## v6.12.0 - 2026-08-30 diff --git a/src/odr/internal/html/common.cpp b/src/odr/internal/html/common.cpp index a6dd17119..60233cde0 100644 --- a/src/odr/internal/html/common.cpp +++ b/src/odr/internal/html/common.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -31,7 +32,7 @@ void html::write_viewport_meta( const std::optional mode_override) { if (config.viewport_content.has_value()) { out.write_header_viewport( - escape_attribute(config.viewport_content.value())); + util::xml::escape_attribute(config.viewport_content.value())); return; } @@ -256,9 +257,7 @@ std::string html::escape_text(std::string text) { return text; } - util::string::replace_all(text, "&", "&"); - util::string::replace_all(text, "<", "<"); - util::string::replace_all(text, ">", ">"); + text = util::xml::escape_text(text); if (text.front() == ' ') { text = " " + text.substr(1); @@ -274,14 +273,6 @@ std::string html::escape_text(std::string text) { return text; } -std::string html::escape_attribute(std::string value) { - util::string::replace_all(value, "&", "&"); - util::string::replace_all(value, "\"", """); - util::string::replace_all(value, "<", "<"); - util::string::replace_all(value, ">", ">"); - return value; -} - html::UriKind html::uri_kind(const std::string_view uri) { std::string scheme; for (const char ch : uri) { diff --git a/src/odr/internal/html/common.hpp b/src/odr/internal/html/common.hpp index a2e4c2ca1..f55ffdfa7 100644 --- a/src/odr/internal/html/common.hpp +++ b/src/odr/internal/html/common.hpp @@ -82,12 +82,11 @@ void write_zoom_style(HtmlWriter &out, const HtmlConfig &config, WidthFit fits, /// length, which leaves those insets as shipped. void write_content_margin_style(HtmlWriter &out, const HtmlConfig &config); +/// @ref util::xml::escape_text, plus the ` ` and ` ` that keep html +/// from collapsing the run's own whitespace. An attribute value wants +/// @ref util::xml::escape_attribute instead, which leaves spaces intact. std::string escape_text(std::string text); -/// Escape a string for use as an HTML double-quoted attribute value (`&`, `"`, -/// `<`, `>`). Unlike `escape_text`, it leaves leading/trailing spaces intact. -std::string escape_attribute(std::string value); - /// What a target is, as an `href` would be dispatched. Whitespace and control /// bytes are skipped while reading the scheme, as browsers strip them first. enum class UriKind { diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index dea4c96cb..1e825b712 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -12,6 +12,7 @@ #include #include #include +#include namespace odr::internal { @@ -413,7 +414,7 @@ void html::translate_link(const Element &element, const WritingState &state) { // A refused target loses the attribute, not the element. HtmlAttributesVector attributes; if (kind != UriKind::refused) { - attributes.emplace_back("href", escape_attribute(href)); + attributes.emplace_back("href", util::xml::escape_attribute(href)); } HtmlElementOptions options = @@ -435,8 +436,8 @@ void html::translate_bookmark(const Element &element, state.out().write_element_begin( "a", - HtmlElementOptions().set_inline(true).set_attributes( - HtmlAttributesVector{{"id", escape_attribute(bookmark.name())}})); + HtmlElementOptions().set_inline(true).set_attributes(HtmlAttributesVector{ + {"id", util::xml::escape_attribute(bookmark.name())}})); state.out().write_element_end("a"); } @@ -565,7 +566,8 @@ void html::translate_image(const Element &element, const WritingState &state) { .set_attributes([&](const HtmlAttributeWriterCallback &clb) { clb("alt", "Error: image not found or unsupported"); if (resource_location.has_value()) { - clb("src", escape_attribute(resource_location.value())); + clb("src", + util::xml::escape_attribute(resource_location.value())); } else { clb("src", [&](std::ostream &o) { // reached only for internal images, which have a file diff --git a/src/odr/internal/html/document_style.cpp b/src/odr/internal/html/document_style.cpp index 28e290ac9..243b307d1 100644 --- a/src/odr/internal/html/document_style.cpp +++ b/src/odr/internal/html/document_style.cpp @@ -4,6 +4,7 @@ #include #include +#include namespace odr::internal { @@ -150,7 +151,7 @@ std::string html::translate_text_style(const TextStyle &text_style) { if (const std::optional font_name = text_style.font_name; font_name.has_value()) { result.append("font-family:") - .append(escape_attribute(std::string(*font_name))) + .append(util::xml::escape_attribute(std::string(*font_name))) .append(";"); } if (const std::optional font_size = text_style.font_size; @@ -178,7 +179,7 @@ std::string html::translate_text_style(const TextStyle &text_style) { if (const std::optional font_shadow = text_style.font_shadow; font_shadow.has_value()) { result.append("text-shadow:") - .append(escape_attribute(*font_shadow)) + .append(util::xml::escape_attribute(*font_shadow)) .append(";"); } if (const std::optional font_color = text_style.font_color; @@ -206,7 +207,7 @@ std::string html::translate_block_font_style(const TextStyle &text_style) { if (const std::optional font_name = text_style.font_name; font_name.has_value()) { result.append("font-family:") - .append(escape_attribute(std::string(*font_name))) + .append(util::xml::escape_attribute(std::string(*font_name))) .append(";"); } if (const std::optional font_size = text_style.font_size; @@ -356,27 +357,27 @@ html::translate_table_cell_style(const TableCellStyle &table_cell_style) { table_cell_style.border.right; border_right.has_value()) { result.append("border-right:") - .append(escape_attribute(*border_right)) + .append(util::xml::escape_attribute(*border_right)) .append(";"); } if (const std::optional border_top = table_cell_style.border.top; border_top.has_value()) { result.append("border-top:") - .append(escape_attribute(*border_top)) + .append(util::xml::escape_attribute(*border_top)) .append(";"); } if (const std::optional border_left = table_cell_style.border.left; border_left.has_value()) { result.append("border-left:") - .append(escape_attribute(*border_left)) + .append(util::xml::escape_attribute(*border_left)) .append(";"); } if (const std::optional border_bottom = table_cell_style.border.bottom; border_bottom.has_value()) { result.append("border-bottom:") - .append(escape_attribute(*border_bottom)) + .append(util::xml::escape_attribute(*border_bottom)) .append(";"); } if (const std::optional text_rotation = diff --git a/src/odr/internal/html/filesystem.cpp b/src/odr/internal/html/filesystem.cpp index e267df552..be8bcbf03 100644 --- a/src/odr/internal/html/filesystem.cpp +++ b/src/odr/internal/html/filesystem.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -226,8 +227,9 @@ class HtmlServiceImpl final : public HtmlService { if (location.has_value()) { out.write_element_begin( "a", HtmlElementOptions().set_inline(true).set_attributes( - HtmlAttributesVector{{"href", escape_attribute(*location)}, - {"title", escape_attribute(name)}})); + HtmlAttributesVector{ + {"href", util::xml::escape_attribute(*location)}, + {"title", util::xml::escape_attribute(name)}})); out.write_raw(escape_text(file_path.string())); out.write_element_end("a"); } else { @@ -248,14 +250,15 @@ class HtmlServiceImpl final : public HtmlService { HtmlElementOptions().set_inline(true).set_class("odr-files-action")); if (const std::optional href = location.has_value() - ? std::optional(escape_attribute(*location)) + ? std::optional(util::xml::escape_attribute(*location)) : entry_data_url(file, mime_type_of(file_path)); href.has_value()) { out.write_element_begin( "a", HtmlElementOptions().set_inline(true).set_attributes( - HtmlAttributesVector{{"href", *href}, - {"download", escape_attribute(name)}, - {"title", escape_attribute(name)}})); + HtmlAttributesVector{ + {"href", *href}, + {"download", util::xml::escape_attribute(name)}, + {"title", util::xml::escape_attribute(name)}})); out.write_raw("\u2193"); out.write_element_end("a"); } diff --git a/src/odr/internal/html/frontend.cpp b/src/odr/internal/html/frontend.cpp index 1f328f16d..ee85b59fd 100644 --- a/src/odr/internal/html/frontend.cpp +++ b/src/odr/internal/html/frontend.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -1560,7 +1561,8 @@ void write_style(const Asset &asset, const WritingState &state, if (const HtmlResourceLocation location = locate(asset, state.config(), state.resources()); location.has_value()) { - state.out().write_header_style(escape_attribute(*location), media); + state.out().write_header_style(util::xml::escape_attribute(*location), + media); return; } @@ -1579,7 +1581,7 @@ void write_script(const Asset &asset, const WritingState &state) { if (const HtmlResourceLocation location = locate(asset, state.config(), state.resources()); location.has_value()) { - state.out().write_script(escape_attribute(*location)); + state.out().write_script(util::xml::escape_attribute(*location)); return; } diff --git a/src/odr/internal/html/media_file.cpp b/src/odr/internal/html/media_file.cpp index ece12d551..4a340821d 100644 --- a/src/odr/internal/html/media_file.cpp +++ b/src/odr/internal/html/media_file.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -181,7 +182,7 @@ class HtmlServiceImpl final : public HtmlService { : "controls preload=\"metadata\"") .set_attributes([&](const HtmlAttributeWriterCallback &clb) { if (location.has_value()) { - clb("src", escape_attribute(*location)); + clb("src", util::xml::escape_attribute(*location)); } else { clb("src", [&](std::ostream &o) { o << file_to_url(*m_media_file.file().impl(), m_mime_type); diff --git a/src/odr/internal/html/pdf_file.cpp b/src/odr/internal/html/pdf_file.cpp index 794ea8114..575ec1851 100644 --- a/src/odr/internal/html/pdf_file.cpp +++ b/src/odr/internal/html/pdf_file.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -245,7 +246,7 @@ std::vector collect_page_links(const pdf::Page &page, link.top = std::min(p0[1], p1[1]); link.width = std::abs(p1[0] - p0[0]); link.height = std::abs(p1[1] - p0[1]); - link.href = escape_attribute(std::move(href)); + link.href = util::xml::escape_attribute(std::move(href)); link.internal = internal; links.push_back(std::move(link)); } @@ -674,7 +675,7 @@ std::string svg_image_fragment(const pdf::ImageElement &image, !blend.empty()) { f << " style=\"mix-blend-mode:" << blend << '"'; } - f << " href=\"" << escape_attribute(images.url(image)) << "\"/>"; + f << " href=\"" << util::xml::escape_attribute(images.url(image)) << "\"/>"; if (!clip_id.empty()) { f << ""; } diff --git a/src/odr/internal/svg/AGENTS.md b/src/odr/internal/svg/AGENTS.md index 11283c8bf..ecf2d5198 100644 --- a/src/odr/internal/svg/AGENTS.md +++ b/src/odr/internal/svg/AGENTS.md @@ -76,14 +76,18 @@ svg. Today that is `svm/svm_to_svg.cpp`, translating a StarView metafile. Two things it does that a raw `operator<<` does not: -- **It escapes.** svg is xml, so an unescaped `&` in a chart label does not - spoil one label, it costs the whole image — an xml parse error renders - nothing at all. `html::escape_text` is the wrong tool for it: that one emits - ` `, which is an html entity and undefined in xml. -- **It formats numbers itself**, in fixed notation via `std::to_chars`, - independent of whatever locale or precision the stream carries. A stream - imbued with a german locale would otherwise write `1,5` into a coordinate, - and a `style` declaration is css, where `1.2e+3` is not a length. +- **It escapes**, through `util::xml::escape` — the same single pass + `html::escape_attribute` uses, asked to drop the control characters xml + forbids as well. svg is xml, so an unescaped `&` in a chart label does not + spoil one label, it costs the whole image: an xml parse error renders nothing + at all. `html::escape_text` is the wrong tool for it — that one emits + ` `, an html entity undefined in xml. +- **It formats numbers through `util::number::to_string_significant`**, not + through the stream. A stream imbued with a german locale would otherwise + write `1,5` into a coordinate, and a `style` declaration is css, where + `1.2e+3` is not a length. `format_number` adds only svg's own answer for a + value that is not finite: `0`, because `nan` in an attribute drops the + element. ## The xml layer is not free diff --git a/src/odr/internal/svg/svg_writer.cpp b/src/odr/internal/svg/svg_writer.cpp index 86d2756b8..200d5777d 100644 --- a/src/odr/internal/svg/svg_writer.cpp +++ b/src/odr/internal/svg/svg_writer.cpp @@ -1,83 +1,20 @@ #include +#include +#include + #include -#include -#include #include -#include #include +#include namespace odr::internal { -namespace { - -/// Tab, line feed and carriage return are the only characters below `0x20` an -/// xml 1.0 document may contain. -bool is_xml_control(const char c) { - const auto value = static_cast(c); - return value < 0x20 && c != '\t' && c != '\n' && c != '\r'; -} - -std::string escape(const std::string_view text, const bool attribute) { - std::string result; - result.reserve(text.size()); - - for (const char c : text) { - switch (c) { - case '&': - result += "&"; - break; - case '<': - result += "<"; - break; - case '>': - result += ">"; - break; - case '"': - result += attribute ? """ : "\""; - break; - default: - if (!is_xml_control(c)) { - result += c; - } - break; - } - } - - return result; -} - -} // namespace - std::string svg::format_number(const double value) { if (!std::isfinite(value)) { return "0"; } - - // the classic locale, because a stream imbued with a german one writes - // `1,5`, and that is two coordinates rather than one - std::ostringstream stream; - stream.imbue(std::locale::classic()); - stream << value; - std::string result = stream.str(); - - if (result.find('e') != std::string::npos) { - // a `style` declaration is css, and css reads no exponent, so a number - // that big is written out in full - at which point its fraction is noise - stream.str({}); - stream << std::fixed << std::setprecision(0) << value; - result = stream.str(); - } - - return result; -} - -std::string svg::escape_text(const std::string_view text) { - return escape(text, false); -} - -std::string svg::escape_attribute(const std::string_view value) { - return escape(value, true); + return util::number::to_string_significant(value, 6); } svg::SvgWriter::SvgWriter(std::ostream &out) : m_out{&out} {} @@ -122,7 +59,7 @@ void svg::SvgWriter::write_attribute(const std::string_view name, if (!m_tag_open) { throw std::runtime_error("no open tag to write an attribute to"); } - *m_out << " " << name << "=\"" << escape_attribute(value) << "\""; + *m_out << " " << name << "=\"" << util::xml::escape_attribute(value) << "\""; } void svg::SvgWriter::write_attribute(const std::string_view name, @@ -135,9 +72,13 @@ void svg::SvgWriter::write_style(const std::string_view property, if (!m_tag_open) { throw std::runtime_error("no open tag to write a style to"); } - m_style += escape_attribute(property); + m_style += util::xml::escape_attribute(property); m_style += ":"; - m_style += escape_attribute(value); + // a `;` the file wrote would open a declaration of its own; dropped before + // escaping, which writes `;` of its own + std::string sanitized(value); + std::erase(sanitized, ';'); + m_style += util::xml::escape_attribute(sanitized); m_style += ";"; } @@ -148,7 +89,7 @@ void svg::SvgWriter::write_style(const std::string_view property, void svg::SvgWriter::write_text(const std::string_view text) { close_tag(true); - *m_out << escape_text(text); + *m_out << util::xml::escape_text(text); } } // namespace odr::internal diff --git a/src/odr/internal/svg/svg_writer.hpp b/src/odr/internal/svg/svg_writer.hpp index df54b404a..f25d70679 100644 --- a/src/odr/internal/svg/svg_writer.hpp +++ b/src/odr/internal/svg/svg_writer.hpp @@ -7,40 +7,27 @@ namespace odr::internal::svg { -/// Escapes @p text for element content, and drops the control characters xml -/// 1.0 does not allow. Not `html::escape_text`: that one emits ` `, which -/// no xml parser knows. -[[nodiscard]] std::string escape_text(std::string_view text); -/// As @ref escape_text, plus the quote that would end an attribute value. -[[nodiscard]] std::string escape_attribute(std::string_view value); - -/// @p value at the six significant digits a stream defaults to, but in the -/// classic locale and never in exponent notation - css reads neither `1,5` nor -/// `1.2e+3`. +/// @ref util::number::to_string_significant at six digits, and `0` where +/// @p value is not finite - `nan` in an attribute drops the element. [[nodiscard]] std::string format_number(double value); -/// @brief Writes svg markup. +/// @brief Writes escaped svg markup. /// -/// Elements nest through @ref write_element_begin / @ref write_element_end; -/// attributes, style declarations and text go to the innermost open one. -/// Everything written is escaped, and numbers are formatted independently of -/// the stream's locale and precision. -/// -/// An element's `style` attribute is accumulated as declarations arrive and -/// written when the element takes content or ends, so attributes and style may -/// be written in any order. +/// The `style` attribute is accumulated as declarations arrive and written when +/// the element takes content or ends, so attributes and style may be written in +/// any order. class SvgWriter final { public: explicit SvgWriter(std::ostream &out); - /// Opens `` where it took no content. void write_element_end(); void write_attribute(std::string_view name, std::string_view value); void write_attribute(std::string_view name, double value); - /// One declaration of the element's `style` attribute. + /// One declaration of the element's `style` attribute; a `;` in @p value is + /// dropped rather than allowed to open another. void write_style(std::string_view property, std::string_view value); void write_style(std::string_view property, double value); @@ -52,7 +39,6 @@ class SvgWriter final { std::ostream *m_out{nullptr}; std::vector m_stack; - /// The innermost element's tag is still open for attributes. bool m_tag_open{false}; /// The open tag's `style` declarations, unwritten. std::string m_style; diff --git a/src/odr/internal/util/number_util.cpp b/src/odr/internal/util/number_util.cpp index 0a07582aa..10226ad06 100644 --- a/src/odr/internal/util/number_util.cpp +++ b/src/odr/internal/util/number_util.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include namespace odr::internal::util { @@ -11,6 +12,7 @@ std::string number::to_string_significant(const double value, const int significant_digits) { if (!std::isfinite(value)) { std::ostringstream ss; + ss.imbue(std::locale::classic()); ss << value; return ss.str(); } @@ -25,6 +27,7 @@ std::string number::to_string_significant(const double value, const int decimals = std::clamp(significant_digits - integer_digits, 0, 15); std::ostringstream ss; + ss.imbue(std::locale::classic()); ss << std::fixed << std::setprecision(decimals) << value; std::string result = ss.str(); diff --git a/src/odr/internal/util/number_util.hpp b/src/odr/internal/util/number_util.hpp index 5e040d27f..89f17c817 100644 --- a/src/odr/internal/util/number_util.hpp +++ b/src/odr/internal/util/number_util.hpp @@ -5,9 +5,10 @@ namespace odr::internal::util::number { /// Renders @p value with @p significant_digits significant digits, without -/// trailing zeros and never in scientific notation, which CSS and SVG lengths -/// do not accept. Asking for more digits than the source has shows its noise: -/// a `float` carries about 7, beyond that `68.55` becomes `68.550003`. +/// trailing zeros, never in scientific notation, which CSS and SVG lengths do +/// not accept, and in the classic locale, where a german one would write +/// `1,5`. Asking for more digits than the source has shows its noise: a +/// `float` carries about 7, beyond that `68.55` becomes `68.550003`. std::string to_string_significant(double value, int significant_digits); } // namespace odr::internal::util::number diff --git a/src/odr/internal/util/xml_util.cpp b/src/odr/internal/util/xml_util.cpp index 074636260..88f3b849c 100644 --- a/src/odr/internal/util/xml_util.cpp +++ b/src/odr/internal/util/xml_util.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -22,6 +23,54 @@ namespace odr::internal::util { static_assert(sizeof(pugi::xml_node_struct) == 12); static_assert(sizeof(pugi::xml_attribute_struct) == 8); +namespace { + +/// Tab, line feed and carriage return are the only sub-`0x20` characters xml +/// 1.0 allows. +bool is_xml_control(const char c) { + const auto value = static_cast(c); + return value < 0x20 && c != '\t' && c != '\n' && c != '\r'; +} + +std::string escape(const std::string_view text, const bool attribute) { + std::string result; + result.reserve(text.size()); + + for (const char c : text) { + switch (c) { + case '&': + result += "&"; + break; + case '<': + result += "<"; + break; + case '>': + result += ">"; + break; + case '"': + result += attribute ? """ : "\""; + break; + default: + if (!is_xml_control(c)) { + result += c; + } + break; + } + } + + return result; +} + +} // namespace + +std::string xml::escape_text(const std::string_view text) { + return escape(text, false); +} + +std::string xml::escape_attribute(const std::string_view value) { + return escape(value, true); +} + pugi::xml_document xml::parse(const std::string &in) { pugi::xml_document result; if (const auto success = result.load_string(in.c_str()); !success) { diff --git a/src/odr/internal/util/xml_util.hpp b/src/odr/internal/util/xml_util.hpp index 7554e5fd8..858a53210 100644 --- a/src/odr/internal/util/xml_util.hpp +++ b/src/odr/internal/util/xml_util.hpp @@ -2,6 +2,7 @@ #include #include +#include #include namespace pugi { @@ -19,6 +20,12 @@ class AbsPath; namespace odr::internal::util::xml { +/// Escapes `&`, `<` and `>` for element content, and drops the control +/// characters xml 1.0 cannot carry at all. +[[nodiscard]] std::string escape_text(std::string_view text); +/// As @ref escape_text, plus the `"` that would end an attribute value. +[[nodiscard]] std::string escape_attribute(std::string_view value); + pugi::xml_document parse(const std::string &); /// Buffers @p in twice on the way in; prefer the @ref abstract::File overload, /// which reads once against the size the file knows. diff --git a/test/src/internal/svg/svg_writer_test.cpp b/test/src/internal/svg/svg_writer_test.cpp index 923d82a03..2c9dca743 100644 --- a/test/src/internal/svg/svg_writer_test.cpp +++ b/test/src/internal/svg/svg_writer_test.cpp @@ -39,6 +39,25 @@ TEST(SvgWriter, style_declarations_are_collected_into_one_attribute) { })); } +TEST(SvgWriter, a_style_value_cannot_open_another_declaration) { + EXPECT_EQ("", + write([](SvgWriter &out) { + out.write_element_begin("text"); + out.write_style("font-family", "a;b"); + out.write_style("fill", "red"); + out.write_element_end(); + })); +} + +TEST(SvgWriter, a_style_value_keeps_the_semicolon_of_its_own_escape) { + EXPECT_EQ("", + write([](SvgWriter &out) { + out.write_element_begin("text"); + out.write_style("font-family", "a\"b&c"); + out.write_element_end(); + })); +} + TEST(SvgWriter, elements_nest) { EXPECT_EQ("", write([](SvgWriter &out) { out.write_element_begin("g"); @@ -48,22 +67,6 @@ TEST(SvgWriter, elements_nest) { })); } -/// An unescaped `&` or `<` makes the document malformed, and a malformed svg -/// renders as nothing at all. -TEST(SvgWriter, escape_text) { - EXPECT_EQ("a & b <c> \"d\"", escape_text("a & b \"d\"")); -} - -TEST(SvgWriter, escape_attribute) { - EXPECT_EQ("a"b & c", escape_attribute("a\"b & c")); -} - -/// Xml 1.0 has no way to write these, escaped or not. -TEST(SvgWriter, escape_drops_control_characters) { - EXPECT_EQ("ab\tc", escape_text(std::string("a\x01" - "b\tc"))); -} - TEST(SvgWriter, format_number) { EXPECT_EQ("0", format_number(0)); EXPECT_EQ("1", format_number(1)); @@ -71,13 +74,11 @@ TEST(SvgWriter, format_number) { EXPECT_EQ("10519.4", format_number(10519.375)); } -/// Css reads no exponent, so `font-size:1.2e+07` would be no size at all. TEST(SvgWriter, format_number_never_uses_an_exponent) { EXPECT_EQ("12345678", format_number(12345678.0)); - EXPECT_EQ("0", format_number(0.0000001)); + EXPECT_EQ("0.0000001", format_number(0.0000001)); } -/// Nothing sensible to draw at, and `nan` in an attribute drops the element. TEST(SvgWriter, format_number_of_something_that_is_not_a_number) { EXPECT_EQ("0", format_number(std::numeric_limits::quiet_NaN())); EXPECT_EQ("0", format_number(std::numeric_limits::infinity())); diff --git a/test/src/internal/util/xml_util_test.cpp b/test/src/internal/util/xml_util_test.cpp index 0ed331dc5..04097ca04 100644 --- a/test/src/internal/util/xml_util_test.cpp +++ b/test/src/internal/util/xml_util_test.cpp @@ -3,9 +3,23 @@ #include #include +#include using namespace odr::internal::util::xml; +TEST(xml_util, escape_text) { + EXPECT_EQ("a & b <c> \"d\"", escape_text("a & b \"d\"")); +} + +TEST(xml_util, escape_attribute) { + EXPECT_EQ("a"b & c", escape_attribute("a\"b & c")); +} + +TEST(xml_util, escape_drops_control_characters) { + EXPECT_EQ("ab\tc", escape_text(std::string("a\x01" + "b\tc"))); +} + TEST(xml_util, tokenize_text) { auto example1 = tokenize_text("hello world!"); EXPECT_EQ(1, example1.size()); From bfc9e2bb249bcde2c80a76df54593008e39bb00c Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 10:31:38 +0200 Subject: [PATCH 3/4] fix(svm): log a translation failure that is not a `std::exception` too The catch narrowed to `std::exception` to report `what()`, which let anything else past `translate_image_src(const ImageFile &)`, where nothing catches it. A second catch keeps the old guarantee and still says the image is gone. Also widens the skip length explicitly, as its sibling call already did, and trims two comments. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XQpLmJpJ87qbKoG8B7kbLY --- src/odr/internal/html/image_file.cpp | 3 ++- src/odr/internal/svm/svm_to_svg.cpp | 7 +++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/odr/internal/html/image_file.cpp b/src/odr/internal/html/image_file.cpp index 17665daaa..c578c746c 100644 --- a/src/odr/internal/html/image_file.cpp +++ b/src/odr/internal/html/image_file.cpp @@ -41,8 +41,9 @@ void write_image_src(const ImageFile &image_file, std::ostream &out, out << file_to_url(svg_out.str(), "image/svg+xml"); return; } catch (const std::exception &e) { - // the raw bytes below render as nothing, so say why ODR_WARNING(logger, "svm translation failed: " << e.what()); + } catch (...) { + ODR_WARNING(logger, "svm translation failed"); } } diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index 7e8213b32..2a9a4f1e3 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -20,7 +20,6 @@ enum class StyleKind { text, ///< the text colour as fill, plus the font }; -/// The action type's name with its number, for a log line. std::string action_name(const ActionHeader &action_header) { return std::string(action_type_name(action_header.type)) + "(" + std::to_string(action_header.type) + ")"; @@ -65,7 +64,7 @@ std::string get_svg_color_string(const std::uint32_t color) { std::to_string(blue) + ")"; } -/// The colour, or the property turned off where the state does not set one. +/// The colour, or `-opacity:0` where the state sets none. void write_color_style(svg::SvgWriter &out, const std::string &property, const std::uint32_t color, const bool set) { if (set) { @@ -99,7 +98,7 @@ void write_style(svg::SvgWriter &out, const Context &context, write_line_style(out, context); break; case StyleKind::fill: - // TODO the fill kills the stroke the line style just wrote + // TODO the fill overrides the stroke just written write_line_style(out, context); write_fill_style(out, context); break; @@ -221,7 +220,7 @@ void translate_action(const ActionHeader &action_header, std::istream &in, ODR_DEBUG(*context.logger, "unhandled action " << action_name(action_header) << ", skipping " << action_header.vl.length << " bytes"); - in.ignore(action_header.vl.length); + in.ignore(static_cast(action_header.vl.length)); break; } } From bd30986d4eedd64d46fac311222f9c5c96883582 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 10:31:39 +0200 Subject: [PATCH 4/4] docs(svm): keep README.md as the feature matrix, and trim the comments `README.md` comes back rather than being replaced by `AGENTS.md` and `PLAN.md`: it is the checklist of what the translator does and does not draw, now checked against the code and extended with the shapes, fills, clipping and state handling that were never listed, plus the references completed with `SvmReader.cxx`, `SvmConverter.cxx`, `svgwriter.cxx` and the ONLYOFFICE spec. It also records the half of #772's escaping defect that escaping does not fix: `read_string_with_encoding` returns the file's own bytes for every encoding but `UCS2`, so a latin-1 label emits invalid utf-8 and costs the image exactly as an unescaped `&` did. Stage 3 in `PLAN.md` picks it up. The comments this branch added are cut back to the house standard - no restatement of the code, no narrative. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XQpLmJpJ87qbKoG8B7kbLY --- src/odr/internal/abstract/html_service.hpp | 1 - src/odr/internal/html/html_service.hpp | 1 - src/odr/internal/svm/AGENTS.md | 6 +- src/odr/internal/svm/PLAN.md | 14 +++-- src/odr/internal/svm/README.md | 70 ++++++++++++++++++++++ src/odr/internal/svm/svm_format.hpp | 3 +- src/odr/internal/svm/svm_to_svg.hpp | 4 +- test/src/internal/svm/svm_test.cpp | 9 +-- 8 files changed, 88 insertions(+), 20 deletions(-) create mode 100644 src/odr/internal/svm/README.md diff --git a/src/odr/internal/abstract/html_service.hpp b/src/odr/internal/abstract/html_service.hpp index 111c864ea..32192db97 100644 --- a/src/odr/internal/abstract/html_service.hpp +++ b/src/odr/internal/abstract/html_service.hpp @@ -21,7 +21,6 @@ class HtmlService { virtual ~HtmlService() = default; [[nodiscard]] virtual const HtmlConfig &config() const = 0; - /// The sink everything written through this service logs to. [[nodiscard]] virtual const Logger &logger() const = 0; [[nodiscard]] virtual const HtmlViews &list_views() const = 0; diff --git a/src/odr/internal/html/html_service.hpp b/src/odr/internal/html/html_service.hpp index 20029a4e4..71484aa04 100644 --- a/src/odr/internal/html/html_service.hpp +++ b/src/odr/internal/html/html_service.hpp @@ -32,7 +32,6 @@ class HtmlView : public abstract::HtmlView { [[nodiscard]] const HtmlConfig &config() const override; [[nodiscard]] const std::optional &sheet_cut() const override; [[nodiscard]] const abstract::HtmlService &service() const; - /// The service's sink. [[nodiscard]] const Logger &logger() const; HtmlResources write_html(HtmlWriter &out) const override; diff --git a/src/odr/internal/svm/AGENTS.md b/src/odr/internal/svm/AGENTS.md index 522cd982a..46ce9af80 100644 --- a/src/odr/internal/svm/AGENTS.md +++ b/src/odr/internal/svm/AGENTS.md @@ -1,7 +1,8 @@ # AGENTS.md — `internal/svm` Read the root [`AGENTS.md`](../../../../AGENTS.md) first. This file covers what -svm does differently, and why. [`PLAN.md`](PLAN.md) has the roadmap. +svm does differently, and why. [`README.md`](README.md) has the feature matrix +and the references, [`PLAN.md`](PLAN.md) the roadmap. ## What it is @@ -58,6 +59,9 @@ The format is not specified anywhere. The references, best first: unescaped `&` costs the whole image, not one label. Note that `html::escape_text` is the *wrong* escape here — it emits ` `, which no xml parser knows. +- **Escaping is not yet enough.** `read_string_with_encoding` hands back the + file's own bytes for every encoding but `UCS2`, so a latin-1 label emits + invalid utf-8 and the parser refuses the document all the same. ## Testing diff --git a/src/odr/internal/svm/PLAN.md b/src/odr/internal/svm/PLAN.md index 636936960..25ded3a6b 100644 --- a/src/odr/internal/svm/PLAN.md +++ b/src/odr/internal/svm/PLAN.md @@ -6,8 +6,8 @@ sub-parts [#194](https://github.com/opendocument-app/OpenDocument.core/issues/19 (bitmaps) and [#95](https://github.com/opendocument-app/OpenDocument.core/issues/95) (font attributes). -Read [`AGENTS.md`](AGENTS.md) for how the module is built and what the -references are. +Read [`AGENTS.md`](AGENTS.md) for how the module is built and +[`README.md`](README.md) for the feature matrix and the references. ## Stages @@ -16,12 +16,16 @@ Each stage is one pull request, stacked on the one before it. 1. **Infrastructure.** `svg::SvgWriter`, so markup is written by something that escapes; a `Logger` through the translator, so an action we drop says so; inline-bytes tests, so an action can be tested without a fixture. Fixes - #772's defects 1 (escaping), 9 (style dispatch) and 10 (silence). + #772's defects 1 (escaping, but see stage 3 for the encoding half of it), 9 + (style dispatch) and 10 (silence). 2. **Fixes to what we already emit.** The graphics state stack (`PUSH`/`POP`), poly-polygon fill rule, the font size and map-mode unit in the transform, `LineInfo`. #772 defects 2, 3, 5, 6, 8. -3. **Text.** `TEXTALIGN`, the `TEXTARRAY` dx array, `TEXTRECT`, and the #95 - font attributes (bold, italic, underline, strikeout, family). +3. **Text.** `TEXTALIGN`, the `TEXTARRAY` dx array, `TEXTRECT`, the #95 font + attributes (bold, italic, underline, strikeout, family), and decoding a + non-`UCS2` string instead of passing its bytes through — until then a + latin-1 label emits invalid utf-8, which costs the image exactly as an + unescaped `&` did. 4. **Primitives.** `PIXEL`, `POINT`, `LINE`, `ROUNDRECT`, `ELLIPSE`, `ARC`, `PIE`, `CHORD` — one `svgwriter.cxx` case each. 5. **Bitmaps** (#194). See the shortcut below. diff --git a/src/odr/internal/svm/README.md b/src/odr/internal/svm/README.md new file mode 100644 index 000000000..604a4cff3 --- /dev/null +++ b/src/odr/internal/svm/README.md @@ -0,0 +1,70 @@ +# SVM implementation + +StarView Metafile → SVG. See [`AGENTS.md`](AGENTS.md) for how the module is +built and [`PLAN.md`](PLAN.md) for the order the gaps below get closed in. + +## Features + +- [x] shapes + - [x] rectangle + - [x] polyline, polygon, poly-polygon + - [ ] fill rule of a poly-polygon (holes are painted over) + - [ ] pixel, point, line, rounded rectangle, ellipse, arc, pie, chord + - [ ] bézier segments (the polygon flags are not read) +- [x] colour + - [x] line, fill, text + - [ ] `LineInfo` (width, dash, join, cap) + - [ ] text fill, overline (read into the state, never drawn) +- [ ] font + - [x] size (in map-mode units, which the transform does not apply) + - [ ] italic, bold + - [ ] alignment + - [ ] underline, strike through + - [x] colour + - [x] family +- [x] text + - [x] `TEXT`, `TEXTARRAY`, `STRETCHTEXT` as plain text at a point + - [ ] the `TEXTARRAY` dx array, `STRETCHTEXT` width, `TEXTRECT` + - [ ] non-`UCS2` encodings (the bytes go out undecoded, see below) +- [ ] transform (e.g. flip, rotate) + - [x] map mode origin and scale + - [ ] map mode unit +- [ ] images (`BMP`, `BMPEX`, `MASK` and their scale/part variants) +- [ ] gradient, hatch, wallpaper +- [ ] clipping regions +- [ ] transparency (`TRANSPARENT`, `FLOATTRANSPARENT`) +- [ ] graphics state stack (`PUSH`/`POP`) +- [ ] `EPS` substitute metafile +- [ ] version 1 (pre-`VCLMTF`) files +- [x] output is escaped, so a `&` in a label cannot cost the whole image +- [x] every action we skip is logged by name + +Anything not implemented is skipped by the action's own length, so the actions +after it still read. + +### Known defect + +Text in a non-`UCS2` encoding is passed through as the bytes the file holds +(`read_ascii_string`). A latin-1 label therefore emits invalid utf-8, and an +xml parser refuses that exactly as hard as an unescaped `&`. Escaping alone +does not make every label safe. + +## References + +- [`metaact.hxx`](https://github.com/LibreOffice/core/blob/master/include/vcl/metaact.hxx) + — what each action means. +- [`SvmReader.cxx`](https://github.com/LibreOffice/core/blob/master/vcl/source/filter/svm/SvmReader.cxx) + — the binary layout, per action, per version. The authority when a field is + in doubt. +- [`SvmConverter.cxx`](https://github.com/LibreOffice/core/blob/master/vcl/source/filter/svm/SvmConverter.cxx) + — the version 1 format, converted to the current one on read. +- [`svgwriter.cxx`](https://github.com/LibreOffice/core/blob/master/filter/source/svg/svgwriter.cxx) + — LibreOffice's own metafile → svg export, i.e. our problem already solved. + The reference for mapping decisions. +- [`SPEC`](https://github.com/ONLYOFFICE/core/blob/master/DesktopEditor/raster/Metafile/StarView/SPEC) + — ONLYOFFICE's prose write-up, modelled on [MS-WMF]. Cheap to read, but + incomplete: several FIXMEs, `Color` and the polygon flags unfinished. + +### Related work + +- https://github.com/SoftarexTechnologies/svmconv diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index 087e697d2..177787360 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -207,8 +207,7 @@ struct TextLineAction final { std::uint32_t overline{}; }; -/// The action type's name, as `metaact.hxx` spells it, or `"UNKNOWN"`. For -/// logging what we skipped. +/// The action type's name as `metaact.hxx` spells it, or `"UNKNOWN"`. [[nodiscard]] std::string_view action_type_name(std::uint16_t type); /// Reads a fixed-size field. A short read leaves the destination untouched, so diff --git a/src/odr/internal/svm/svm_to_svg.hpp b/src/odr/internal/svm/svm_to_svg.hpp index b6f5bc6fe..cb5e92e2f 100644 --- a/src/odr/internal/svm/svm_to_svg.hpp +++ b/src/odr/internal/svm/svm_to_svg.hpp @@ -9,8 +9,8 @@ class Logger; namespace odr::internal::svm { class SvmFile; -/// Translates @p file's actions into an svg document. Actions we do not -/// implement are logged and dropped. +/// Translates @p file into an svg document; unimplemented actions are logged +/// and dropped. /// @throws MalformedSvmFile where an action reads past its own length. void translate_to_svg(const SvmFile &file, std::ostream &out, const Logger &logger); diff --git a/test/src/internal/svm/svm_test.cpp b/test/src/internal/svm/svm_test.cpp index 5fa6149b4..64d06f6f9 100644 --- a/test/src/internal/svm/svm_test.cpp +++ b/test/src/internal/svm/svm_test.cpp @@ -21,8 +21,7 @@ using namespace odr::test; namespace { -/// Builds a metafile byte by byte, so an action can be tested without a -/// fixture. Mirrors what `SvmReader` expects to read back. +/// Builds a metafile byte by byte, as `SvmReader` reads it back. class SvmBuilder final { public: SvmBuilder &u8(const std::uint8_t value) { @@ -167,8 +166,6 @@ TEST(SvmToSvg, text) { EXPECT_NE(std::string::npos, svg.find(">hello")); } -/// Unescaped, the markup below is not a document and the browser draws -/// nothing. TEST(SvmToSvg, text_is_escaped) { const std::string svg = translate(SvmBuilder() .action(svm::META_TEXT_ACTION) @@ -182,8 +179,6 @@ TEST(SvmToSvg, text_is_escaped) { EXPECT_NE(std::string::npos, svg.find(">a & b <c>")); } -/// A font family is written into an attribute, and a quote in it would end -/// that attribute. TEST(SvmToSvg, font_family_is_escaped) { const std::string svg = translate(SvmBuilder() .action(svm::META_FONT_ACTION) @@ -218,8 +213,6 @@ TEST(SvmToSvg, font_family_is_escaped) { EXPECT_NE(std::string::npos, svg.find("font-family:a"b")); } -/// An action we do not implement is skipped by its own length, so the ones -/// after it still read. TEST(SvmToSvg, unhandled_action_is_skipped) { const std::string svg = translate(SvmBuilder() .action(svm::META_ELLIPSE_ACTION)