diff --git a/CHANGELOG.md b/CHANGELOG.md index af3f0bab4..ba8f3caa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,14 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- **Breaking**: `AnchorType` gains `none` as its first value, so every later + ordinal shifts by one. `Frame::anchor_type()` answers it for a frame that + does not exist, instead of `as_char`, which a real frame also answers. + +- **Fix**: a percentage `fo:margin` on an odf paragraph is resolved against the + parent style ([OpenDocument] 16.2) instead of being dropped. LibreOffice + writes `fo:margin="100%"` for "whatever the parent had". + - **Fix**: `Text::set_content` on an `.xlsx` run did nothing and said nothing. An xlsx declares `edit`, so a caller had no way to learn the write was dropped; it now throws `UnsupportedOperation`, as every other engine that diff --git a/apple/include/OdrCoreObjC/ODRDocumentElement.h b/apple/include/OdrCoreObjC/ODRDocumentElement.h index bf8ddc572..08c010cd4 100644 --- a/apple/include/OdrCoreObjC/ODRDocumentElement.h +++ b/apple/include/OdrCoreObjC/ODRDocumentElement.h @@ -54,7 +54,9 @@ typedef NS_ENUM(NSInteger, ODRShapeType) { } NS_SWIFT_NAME(ShapeType); typedef NS_ENUM(NSInteger, ODRAnchorType) { - ODRAnchorTypeAsChar = 0, + /// The frame does not exist, so it is anchored nowhere. + ODRAnchorTypeNone = 0, + ODRAnchorTypeAsChar, ODRAnchorTypeAtChar, ODRAnchorTypeAtFrame, ODRAnchorTypeAtPage, diff --git a/apple/src/ODRDocumentElement.mm b/apple/src/ODRDocumentElement.mm index d90e4d1bc..4af49bbe5 100644 --- a/apple/src/ODRDocumentElement.mm +++ b/apple/src/ODRDocumentElement.mm @@ -49,6 +49,7 @@ ODR_SAME_ENUM(ODRShapeTypeLine, odr::ShapeType::line); ODR_SAME_ENUM(ODRShapeTypeCustom, odr::ShapeType::custom); +ODR_SAME_ENUM(ODRAnchorTypeNone, odr::AnchorType::none); ODR_SAME_ENUM(ODRAnchorTypeAsChar, odr::AnchorType::as_char); ODR_SAME_ENUM(ODRAnchorTypeAtChar, odr::AnchorType::at_char); ODR_SAME_ENUM(ODRAnchorTypeAtFrame, odr::AnchorType::at_frame); diff --git a/jni/java/app/opendocument/core/AnchorType.java b/jni/java/app/opendocument/core/AnchorType.java index 2c6800601..ae8375cc4 100644 --- a/jni/java/app/opendocument/core/AnchorType.java +++ b/jni/java/app/opendocument/core/AnchorType.java @@ -2,7 +2,13 @@ /** Mirrors {@code odr::AnchorType}; constant order must match the C++ declaration. */ public enum AnchorType { - AS_CHAR, AT_CHAR, AT_FRAME, AT_PAGE, AT_PARAGRAPH; + /** The frame does not exist, so it is anchored nowhere. */ + NONE, + AS_CHAR, + AT_CHAR, + AT_FRAME, + AT_PAGE, + AT_PARAGRAPH; static AnchorType fromNative(int code) { return code < 0 ? null : values()[code]; diff --git a/python/src/bind_document.cpp b/python/src/bind_document.cpp index 7526de280..e332b4f71 100644 --- a/python/src/bind_document.cpp +++ b/python/src/bind_document.cpp @@ -78,6 +78,7 @@ void odr_python::bind_document(py::module_ &m) { .value("custom", odr::ShapeType::custom); py::enum_(m, "AnchorType") + .value("none", odr::AnchorType::none) .value("as_char", odr::AnchorType::as_char) .value("at_char", odr::AnchorType::at_char) .value("at_frame", odr::AnchorType::at_frame) diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index 16bb32d8b..881ec5ba6 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -640,10 +640,8 @@ ShapeType Frame::shape_type() const { } AnchorType Frame::anchor_type() const { - // `AnchorType` has no neutral value, so a frame that does not exist answers - // the commonest one. Ask @ref Element::operator bool to tell the two apart. return exists_() ? m_adapter2->frame_anchor_type(m_identifier) - : AnchorType::as_char; + : AnchorType::none; } std::optional Frame::x() const { diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index fdb55b0ee..acdc5dca5 100644 --- a/src/odr/document_element.hpp +++ b/src/odr/document_element.hpp @@ -123,6 +123,7 @@ enum class ShapeType { /// Collection of anchor types. enum class AnchorType { + none, ///< the frame does not exist, so it is anchored nowhere as_char, at_char, at_frame, diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index 2ca8c1934..f79033d9d 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -32,6 +32,21 @@ std::optional read_measure(const pugi::xml_attribute attribute) { return {}; } +/// A percentage margin is of @p parent, the same margin in the parent style +/// ([OpenDocument] 16.2) - not of the containing block, which is what css +/// reads a `%` as. +std::optional resolve_margin(const Measure &margin, + const std::optional &parent) { + if (margin.unit().name() != "%") { + return margin; + } + if (!parent.has_value()) { + return {}; + } + return Measure(parent->magnitude() * margin.magnitude() * 1e-2, + parent->unit()); +} + std::optional read_font_weight(const pugi::xml_attribute attribute) { if (!attribute) { @@ -412,50 +427,29 @@ void Style::resolve_paragraph_style_(const pugi::xml_node node, paragraph_properties.attribute("style:writing-mode"))) { result.direction = direction; } + // the shorthand is the four sides, each against its own inherited value if (const std::optional margin = read_measure(paragraph_properties.attribute("fo:margin"))) { - // TODO a percentage margin is dropped. css takes `%` here with the same - // meaning, so passing it through would work - but it moves the reference - // output for every document that uses one. - if (margin->unit().name() != "%") { - result.margin = DirectionalStyle(margin); - } + result.margin.right = resolve_margin(*margin, result.margin.right); + result.margin.top = resolve_margin(*margin, result.margin.top); + result.margin.left = resolve_margin(*margin, result.margin.left); + result.margin.bottom = resolve_margin(*margin, result.margin.bottom); } if (const std::optional margin_right = read_measure(paragraph_properties.attribute("fo:margin-right"))) { - // TODO a percentage margin is dropped. css takes `%` here with the same - // meaning, so passing it through would work - but it moves the reference - // output for every document that uses one. - if (margin_right->unit().name() != "%") { - result.margin.right = margin_right; - } + result.margin.right = resolve_margin(*margin_right, result.margin.right); } if (const std::optional margin_top = read_measure(paragraph_properties.attribute("fo:margin-top"))) { - // TODO a percentage margin is dropped. css takes `%` here with the same - // meaning, so passing it through would work - but it moves the reference - // output for every document that uses one. - if (margin_top->unit().name() != "%") { - result.margin.top = margin_top; - } + result.margin.top = resolve_margin(*margin_top, result.margin.top); } if (const std::optional margin_left = read_measure(paragraph_properties.attribute("fo:margin-left"))) { - // TODO a percentage margin is dropped. css takes `%` here with the same - // meaning, so passing it through would work - but it moves the reference - // output for every document that uses one. - if (margin_left->unit().name() != "%") { - result.margin.left = margin_left; - } + result.margin.left = resolve_margin(*margin_left, result.margin.left); } if (const std::optional margin_bottom = read_measure(paragraph_properties.attribute("fo:margin-bottom"))) { - // TODO a percentage margin is dropped. css takes `%` here with the same - // meaning, so passing it through would work - but it moves the reference - // output for every document that uses one. - if (margin_bottom->unit().name() != "%") { - result.margin.bottom = margin_bottom; - } + result.margin.bottom = resolve_margin(*margin_bottom, result.margin.bottom); } if (const std::optional line_height = read_measure(paragraph_properties.attribute("fo:line-height"))) { diff --git a/test/src/enum_ordinals_test.cpp b/test/src/enum_ordinals_test.cpp index e11dde492..a569fc899 100644 --- a/test/src/enum_ordinals_test.cpp +++ b/test/src/enum_ordinals_test.cpp @@ -191,11 +191,12 @@ TEST(EnumOrdinals, shape_type) { } TEST(EnumOrdinals, anchor_type) { - EXPECT_EQ(ordinal(AnchorType::as_char), 0); - EXPECT_EQ(ordinal(AnchorType::at_char), 1); - EXPECT_EQ(ordinal(AnchorType::at_frame), 2); - EXPECT_EQ(ordinal(AnchorType::at_page), 3); - EXPECT_EQ(ordinal(AnchorType::at_paragraph), 4); + EXPECT_EQ(ordinal(AnchorType::none), 0); + EXPECT_EQ(ordinal(AnchorType::as_char), 1); + EXPECT_EQ(ordinal(AnchorType::at_char), 2); + EXPECT_EQ(ordinal(AnchorType::at_frame), 3); + EXPECT_EQ(ordinal(AnchorType::at_page), 4); + EXPECT_EQ(ordinal(AnchorType::at_paragraph), 5); } TEST(EnumOrdinals, value_type) { diff --git a/test/src/internal/odf/odf_flat_file_test.cpp b/test/src/internal/odf/odf_flat_file_test.cpp index bb73a80d8..bee7648c4 100644 --- a/test/src/internal/odf/odf_flat_file_test.cpp +++ b/test/src/internal/odf/odf_flat_file_test.cpp @@ -718,3 +718,91 @@ TEST(FlatOpenDocumentFile, it_saves_back_as_one_xml_file) { .content(), "Hello"); } + +namespace { + +/// One paragraph, styled by @p properties over @p parent_properties. +std::string one_paragraph(const std::string &properties, + const std::string &parent_properties) { + return flat_text( + R"(one)", + R"()" + R"()" + R"()" + R"()" + R"()" + R"()"); +} + +/// @p properties over a parent stating `fo:margin-left="2cm"`. +std::string margin_over_parent(const std::string &properties) { + return one_paragraph(properties, R"(fo:margin-left="2cm")"); +} + +DirectionalStyle margins_of(const std::string &source) { + const Document document = + open(File::from_memory(source)).as_document_file().document(); + return first_of_type(document.root_element(), ElementType::paragraph) + .as_paragraph() + .style() + .margin; +} + +std::optional margin_left_of(const std::string &source) { + return margins_of(source).left; +} + +} // namespace + +/// The shorthand states all four sides. +TEST(FlatOpenDocumentFile, a_margin_shorthand_states_every_side) { + const DirectionalStyle margin = + margins_of(one_paragraph(R"(fo:margin="1cm")", "")); + + for (const std::optional &side : + {margin.right, margin.top, margin.left, margin.bottom}) { + ASSERT_TRUE(side.has_value()); + EXPECT_EQ(side->to_string(), "1cm"); + } +} + +/// [OpenDocument] 16.2: a percentage is of the same property in the parent +/// style, not of the containing block. +TEST(FlatOpenDocumentFile, a_percentage_margin_is_of_the_parent_style) { + const std::optional margin = + margin_left_of(margin_over_parent(R"(fo:margin-left="50%")")); + + ASSERT_TRUE(margin.has_value()); + EXPECT_DOUBLE_EQ(margin->magnitude(), 1); + EXPECT_EQ(margin->unit().name(), "cm"); +} + +/// The shorthand resolves each side against its own inherited value. +TEST(FlatOpenDocumentFile, a_percentage_margin_shorthand_keeps_the_parent) { + const std::optional margin = + margin_left_of(margin_over_parent(R"(fo:margin="100%")")); + + ASSERT_TRUE(margin.has_value()); + EXPECT_DOUBLE_EQ(margin->magnitude(), 2); + EXPECT_EQ(margin->unit().name(), "cm"); +} + +/// Nothing to be a percentage of leaves no margin at all. +TEST(FlatOpenDocumentFile, a_percentage_margin_without_a_parent_states_none) { + EXPECT_FALSE( + margin_left_of(one_paragraph(R"(fo:margin-left="25%")", "")).has_value()); +} + +/// A percentage margin never reaches css, where `%` is of the containing +/// block's width. +TEST(FlatOpenDocumentFile, a_percentage_margin_is_resolved_before_the_css) { + const std::string html = render(margin_over_parent(R"(fo:margin="100%")")); + + EXPECT_EQ(0U, count(html, "margin-left:100%")); + EXPECT_EQ(1U, count(html, "margin-left:2cm")); +}