From 2b68d5ba440d17541075d3645dbfbf96fa054bf5a99a0802761bd17f1c69a261 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Chv=C3=A1tal?= Date: Tue, 2 Jun 2020 06:03:32 +0000 Subject: [PATCH] Accepting request 810676 from LibreOffice:6.4 - Fix bsc#1146025 - LO-L3: Colored textboxes in PPTX look very odd (SmartArt) * bsc1146025.diff OBS-URL: https://build.opensuse.org/request/show/810676 OBS-URL: https://build.opensuse.org/package/show/LibreOffice:Factory/libreoffice?expand=0&rev=883 --- bsc1146025.diff | 501 ++++++++++++++++++++++++++++++++++++++++++++ libreoffice.changes | 6 + libreoffice.spec | 3 + 3 files changed, 510 insertions(+) create mode 100644 bsc1146025.diff diff --git a/bsc1146025.diff b/bsc1146025.diff new file mode 100644 index 0000000..9032dbe --- /dev/null +++ b/bsc1146025.diff @@ -0,0 +1,501 @@ +From bc15a8241b1c8e113bcbb83343652b2c84820e87 Mon Sep 17 00:00:00 2001 +From: Miklos Vajna +Date: Fri, 22 May 2020 17:58:22 +0200 +Subject: [PATCH] bsc1146025.diff + +smartart import: handle multiple in + +The TODO in the ColorFragmentHandler ctor was right: we only handled the +last child, but there can be multiple one. + +Use them based on the index of a shape in a loop. + +Move the TODO to the only place which still assumes a single color in +the color list. + +(cherry picked from commit 12bea6c897822964ad4705418da54411cb15749e) + +Conflicts: + oox/source/drawingml/colorchoicecontext.cxx + oox/source/drawingml/diagram/diagram.cxx + sd/qa/unit/import-tests-smartart.cxx + +oox smartart import: fix aspect ratio of shape with composite algo + +The layout node has alg=composite, then a parTx and a desTx child layout +nodes. No matter what order is used (parent first, child first), the +result will be wrong, as the constraints refer to each other. I did not +spot any description in ISO 29500-1 that would describe what is the +expected behavior. + +Researching this, found "One other consideration when specifying +composite constraints is that the constraints must be specified in the +same order as the nested layout nodes." at +, +which suggests to handle constraints for each shape in a parent -> child +order, but keep a shared state when iterating over the children which +gives us: + +- parent node, all direct constraints +- for each child node: + - child's constraints from parent + - child's own constraints + +This way the desTx top value can depend on the parTx's height, and it's +supported to define parTx's height only in the parTx layout node, not in +the composite parent. + +And after all, it matches what PowerPoint does, so the column headings +in the bugdoc have a 4:10 height:width aspect ratio. + +(cherry picked from commit 414586649582e182b2603702f4f586f4beeed8a9) + +oox smartart import, composite alg: implement vertical centering + +The bugdoc's case was that the total height would be used by 2 shapes, +but then a constraint decreases the height of one shape, so not all +vertical space is used. + +We used to just count from the top, need to center vertically, as +PowerPoint does it. + +(cherry picked from commit acdde3c643fde015214c546b1567727272ea799e) + +Change-Id: I1c5c4f82e621f1110ef06b0490ff79f82f60f214 +deb76c1ddd1ffff8d2a217cddf81106d1bb97eb9 +436019e9e837b73130e387c9bcd309e20045b0f9 +--- + oox/inc/drawingml/colorchoicecontext.hxx | 15 ++ + oox/source/drawingml/colorchoicecontext.cxx | 25 +++ + oox/source/drawingml/diagram/diagram.cxx | 17 +- + oox/source/drawingml/diagram/diagram.hxx | 16 +- + .../diagram/diagramfragmenthandler.cxx | 15 +- + .../drawingml/diagram/diagramlayoutatoms.cxx | 162 ++++++++++++++---- + .../drawingml/diagram/diagramlayoutatoms.hxx | 3 +- + .../drawingml/diagram/layoutatomvisitors.cxx | 4 +- + 8 files changed, 208 insertions(+), 49 deletions(-) + +diff --git a/oox/inc/drawingml/colorchoicecontext.hxx b/oox/inc/drawingml/colorchoicecontext.hxx +index 29889f494e3d..dae8d7fb8e25 100644 +--- a/oox/inc/drawingml/colorchoicecontext.hxx ++++ b/oox/inc/drawingml/colorchoicecontext.hxx +@@ -22,6 +22,8 @@ + + #include + ++#include ++ + namespace oox { + namespace drawingml { + +@@ -65,6 +67,19 @@ private: + Color& mrColor; + }; + ++/// Same as ColorContext, but handles multiple colors. ++class ColorsContext : public ::oox::core::ContextHandler2 ++{ ++public: ++ explicit ColorsContext(::oox::core::ContextHandler2Helper const& rParent, ++ std::vector& rColors); ++ ++ virtual ::oox::core::ContextHandlerRef ++ onCreateContext(sal_Int32 nElement, const ::oox::AttributeList& rAttribs) override; ++ ++private: ++ std::vector& mrColors; ++}; + + } // namespace drawingml + } // namespace oox +diff --git a/oox/source/drawingml/colorchoicecontext.cxx b/oox/source/drawingml/colorchoicecontext.cxx +index cf6c17ecd3b4..a9e0d91ef32e 100644 +--- a/oox/source/drawingml/colorchoicecontext.cxx ++++ b/oox/source/drawingml/colorchoicecontext.cxx +@@ -149,6 +149,31 @@ ColorContext::ColorContext( ContextHandler2Helper const & rParent, Color& rColor + return nullptr; + } + ++ColorsContext::ColorsContext(ContextHandler2Helper const& rParent, std::vector& rColors) ++ : ContextHandler2(rParent) ++ , mrColors(rColors) ++{ ++} ++ ++::oox::core::ContextHandlerRef ColorsContext::onCreateContext(sal_Int32 nElement, ++ const AttributeList&) ++{ ++ switch (nElement) ++ { ++ case A_TOKEN(scrgbClr): ++ case A_TOKEN(srgbClr): ++ case A_TOKEN(hslClr): ++ case A_TOKEN(sysClr): ++ case A_TOKEN(schemeClr): ++ case A_TOKEN(prstClr): ++ { ++ mrColors.emplace_back(); ++ return new ColorValueContext(*this, mrColors.back()); ++ } ++ } ++ return nullptr; ++} ++ + } // namespace drawingml + } // namespace oox + +diff --git a/oox/source/drawingml/diagram/diagram.cxx b/oox/source/drawingml/diagram/diagram.cxx +index b2f3373ad113..a03a06c39125 100644 +--- a/oox/source/drawingml/diagram/diagram.cxx ++++ b/oox/source/drawingml/diagram/diagram.cxx +@@ -321,9 +321,11 @@ void loadDiagram( ShapePtr const & pShape, + if( !pData->getExtDrawings().empty() ) + { + const DiagramColorMap::const_iterator aColor = pDiagram->getColors().find("node0"); +- if( aColor != pDiagram->getColors().end() ) ++ if( aColor != pDiagram->getColors().end() && !aColor->second.maTextFillColors.empty()) + { +- pShape->setFontRefColorForNodes(aColor->second.maTextFillColor); ++ // TODO(F1): well, actually, there might be *several* color ++ // definitions in it, after all it's called list. ++ pShape->setFontRefColorForNodes(DiagramColor::getColorByIndex(aColor->second.maTextFillColors, -1)); + } + } + +@@ -425,6 +427,17 @@ void reloadDiagram(SdrObject* pObj, core::XmlFilterBase& rFilter) + child->addShape(rFilter, rFilter.getCurrentTheme(), xShapes, aTransformation, pShape->getFillProperties()); + } + ++const oox::drawingml::Color& ++DiagramColor::getColorByIndex(const std::vector& rColors, sal_Int32 nIndex) ++{ ++ assert(!rColors.empty()); ++ if (nIndex == -1) ++ { ++ return rColors[rColors.size() - 1]; ++ } ++ ++ return rColors[nIndex % rColors.size()]; ++} + } } + + /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ +diff --git a/oox/source/drawingml/diagram/diagram.hxx b/oox/source/drawingml/diagram/diagram.hxx +index 576c4007e29f..a674e248961e 100644 +--- a/oox/source/drawingml/diagram/diagram.hxx ++++ b/oox/source/drawingml/diagram/diagram.hxx +@@ -22,6 +22,7 @@ + + #include + #include ++#include + + #include + +@@ -111,12 +112,15 @@ typedef std::map DiagramQStyleMap; + + struct DiagramColor + { +- oox::drawingml::Color maFillColor; +- oox::drawingml::Color maLineColor; +- oox::drawingml::Color maEffectColor; +- oox::drawingml::Color maTextFillColor; +- oox::drawingml::Color maTextLineColor; +- oox::drawingml::Color maTextEffectColor; ++ std::vector maFillColors; ++ std::vector maLineColors; ++ std::vector maEffectColors; ++ std::vector maTextFillColors; ++ std::vector maTextLineColors; ++ std::vector maTextEffectColors; ++ ++ static const oox::drawingml::Color& ++ getColorByIndex(const std::vector& rColors, sal_Int32 nIndex); + }; + + typedef std::map DiagramColorMap; +diff --git a/oox/source/drawingml/diagram/diagramfragmenthandler.cxx b/oox/source/drawingml/diagram/diagramfragmenthandler.cxx +index 6e1000af3627..7eae543dc6f9 100644 +--- a/oox/source/drawingml/diagram/diagramfragmenthandler.cxx ++++ b/oox/source/drawingml/diagram/diagramfragmenthandler.cxx +@@ -196,21 +196,18 @@ ColorFragmentHandler::ColorFragmentHandler( ::oox::core::XmlFilterBase& rFilter, + { + // the actual colors - defer to color fragment handlers. + +- // TODO(F1): well, actually, there might be *several* color +- // definitions in it, after all it's called list. But +- // apparently ColorContext doesn't handle that anyway... + case DGM_TOKEN(fillClrLst): +- return new ColorContext( *this, maColorEntry.maFillColor ); ++ return new ColorsContext( *this, maColorEntry.maFillColors ); + case DGM_TOKEN(linClrLst): +- return new ColorContext( *this, maColorEntry.maLineColor ); ++ return new ColorsContext( *this, maColorEntry.maLineColors ); + case DGM_TOKEN(effectClrLst): +- return new ColorContext( *this, maColorEntry.maEffectColor ); ++ return new ColorsContext( *this, maColorEntry.maEffectColors ); + case DGM_TOKEN(txFillClrLst): +- return new ColorContext( *this, maColorEntry.maTextFillColor ); ++ return new ColorsContext( *this, maColorEntry.maTextFillColors ); + case DGM_TOKEN(txLinClrLst): +- return new ColorContext( *this, maColorEntry.maTextLineColor ); ++ return new ColorsContext( *this, maColorEntry.maTextLineColors ); + case DGM_TOKEN(txEffectClrLst): +- return new ColorContext( *this, maColorEntry.maTextEffectColor ); ++ return new ColorsContext( *this, maColorEntry.maTextEffectColors ); + } + break; + } +diff --git a/oox/source/drawingml/diagram/diagramlayoutatoms.cxx b/oox/source/drawingml/diagram/diagramlayoutatoms.cxx +index ff83dde63fa3..19b1d10679f4 100644 +--- a/oox/source/drawingml/diagram/diagramlayoutatoms.cxx ++++ b/oox/source/drawingml/diagram/diagramlayoutatoms.cxx +@@ -430,6 +430,42 @@ sal_Int32 AlgAtom::getVerticalShapesCount(const ShapePtr& rShape) + return nCount; + } + ++namespace ++{ ++/** ++ * Apply rConstraint to the rProperties shared layout state. ++ * ++ * Note that the order in which constraints are applied matters, given that constraints can refer to ++ * each other, and in case A depends on B and A is applied before B, the effect of A won't be ++ * updated when B is applied. ++ */ ++void ApplyConstraintToLayout(const Constraint& rConstraint, LayoutPropertyMap& rProperties) ++{ ++ const LayoutPropertyMap::const_iterator aRef = rProperties.find(rConstraint.msRefForName); ++ if (aRef != rProperties.end()) ++ { ++ const LayoutProperty::const_iterator aRefType = aRef->second.find(rConstraint.mnRefType); ++ if (aRefType != aRef->second.end()) ++ rProperties[rConstraint.msForName][rConstraint.mnType] ++ = aRefType->second * rConstraint.mfFactor; ++ else ++ { ++ // Values are never in EMU, while oox::drawingml::Shape position and size are always in ++ // EMU. ++ double fUnitFactor = 0; ++ if (isFontUnit(rConstraint.mnRefType)) ++ // Points -> EMU. ++ fUnitFactor = EMU_PER_PT; ++ else ++ // Millimeters -> EMU. ++ fUnitFactor = EMU_PER_HMM * 100; ++ rProperties[rConstraint.msForName][rConstraint.mnType] ++ = rConstraint.mfValue * fUnitFactor; ++ } ++ } ++} ++} ++ + void AlgAtom::layoutShape( const ShapePtr& rShape, + const std::vector& rConstraints ) + { +@@ -443,6 +479,11 @@ void AlgAtom::layoutShape( const ShapePtr& rShape, + LayoutProperty& rParent = aProperties[""]; + + sal_Int32 nParentXOffset = 0; ++ ++ // Track min/max vertical positions, so we can center everything at the end, if needed. ++ sal_Int32 nVertMin = std::numeric_limits::max(); ++ sal_Int32 nVertMax = 0; ++ + if (mfAspectRatio != 1.0) + { + rParent[XML_w] = rShape->getSize().Width; +@@ -467,31 +508,74 @@ void AlgAtom::layoutShape( const ShapePtr& rShape, + + for (const auto & rConstr : rConstraints) + { +- const LayoutPropertyMap::const_iterator aRef = aProperties.find(rConstr.msRefForName); +- if (aRef != aProperties.end()) ++ // Apply direct constraints for all layout nodes. ++ ApplyConstraintToLayout(rConstr, aProperties); ++ } ++ ++ for (auto& aCurrShape : rShape->getChildren()) ++ { ++ // Apply constraints from the current layout node for this child shape. ++ // Previous child shapes may have changed aProperties. ++ for (const auto& rConstr : rConstraints) + { +- const LayoutProperty::const_iterator aRefType = aRef->second.find(rConstr.mnRefType); +- if (aRefType != aRef->second.end()) +- aProperties[rConstr.msForName][rConstr.mnType] = aRefType->second * rConstr.mfFactor; +- else ++ if (rConstr.msForName != aCurrShape->getInternalName()) + { +- // Values are never in EMU, while oox::drawingml::Shape +- // position and size are always in EMU. +- double fUnitFactor = 0; +- if (isFontUnit(rConstr.mnRefType)) +- // Points -> EMU. +- fUnitFactor = EMU_PER_PT; +- else +- // Millimeters -> EMU. +- fUnitFactor = EMU_PER_HMM * 100; +- aProperties[rConstr.msForName][rConstr.mnType] +- = rConstr.mfValue * fUnitFactor; ++ continue; ++ } ++ ++ ApplyConstraintToLayout(rConstr, aProperties); ++ } ++ ++ // Apply constraints from the child layout node for this child shape. ++ // This builds on top of the own parent state + the state of previous shapes in the ++ // same composite algorithm. ++ const LayoutNode& rLayoutNode = getLayoutNode(); ++ for (const auto& pDirectChild : rLayoutNode.getChildren()) ++ { ++ auto pLayoutNode = dynamic_cast(pDirectChild.get()); ++ if (!pLayoutNode) ++ { ++ continue; ++ } ++ ++ if (pLayoutNode->getName() != aCurrShape->getInternalName()) ++ { ++ continue; ++ } ++ ++ for (const auto& pChild : pLayoutNode->getChildren()) ++ { ++ auto pConstraintAtom = dynamic_cast(pChild.get()); ++ if (!pConstraintAtom) ++ { ++ continue; ++ } ++ ++ const Constraint& rConstraint = pConstraintAtom->getConstraint(); ++ if (!rConstraint.msForName.isEmpty()) ++ { ++ continue; ++ } ++ ++ if (!rConstraint.msRefForName.isEmpty()) ++ { ++ continue; ++ } ++ ++ // Either an absolute value or a factor of a property. ++ if (rConstraint.mfValue == 0.0 && rConstraint.mnRefType == XML_none) ++ { ++ continue; ++ } ++ ++ Constraint aConstraint(rConstraint); ++ aConstraint.msForName = pLayoutNode->getName(); ++ aConstraint.msRefForName = pLayoutNode->getName(); ++ ++ ApplyConstraintToLayout(aConstraint, aProperties); + } + } +- } + +- for (auto & aCurrShape : rShape->getChildren()) +- { + awt::Size aSize = rShape->getSize(); + awt::Point aPos(0, 0); + +@@ -535,6 +619,24 @@ void AlgAtom::layoutShape( const ShapePtr& rShape, + aCurrShape->setSize(aSize); + aCurrShape->setChildSize(aSize); + aCurrShape->setPosition(aPos); ++ ++ nVertMin = std::min(aPos.Y, nVertMin); ++ nVertMax = std::max(aPos.Y + aSize.Height, nVertMax); ++ } ++ ++ // See if all vertical space is used or we have to center the content. ++ if (nVertMin >= 0 && nVertMax <= rParent[XML_h]) ++ { ++ sal_Int32 nDiff = rParent[XML_h] - (nVertMax - nVertMin); ++ if (nDiff > 0) ++ { ++ for (auto& aCurrShape : rShape->getChildren()) ++ { ++ awt::Point aPosition = aCurrShape->getPosition(); ++ aPosition.Y += nDiff / 2; ++ aCurrShape->setPosition(aPosition); ++ } ++ } + } + break; + } +@@ -1275,7 +1377,7 @@ void LayoutNode::accept( LayoutAtomVisitor& rVisitor ) + rVisitor.visit(*this); + } + +-bool LayoutNode::setupShape( const ShapePtr& rShape, const dgm::Point* pPresNode ) const ++bool LayoutNode::setupShape( const ShapePtr& rShape, const dgm::Point* pPresNode, sal_Int32 nCurrIdx ) const + { + SAL_INFO( + "oox.drawingml", +@@ -1413,15 +1515,17 @@ bool LayoutNode::setupShape( const ShapePtr& rShape, const dgm::Point* pPresNode + const DiagramColorMap::const_iterator aColor = mrDgm.getColors().find(aStyleLabel); + if( aColor != mrDgm.getColors().end() ) + { ++ // Take the nth color from the color list in case we are the nth shape in a ++ // loop. + const DiagramColor& rColor=aColor->second; +- if( rColor.maFillColor.isUsed() ) +- rShape->getShapeStyleRefs()[XML_fillRef].maPhClr = rColor.maFillColor; +- if( rColor.maLineColor.isUsed() ) +- rShape->getShapeStyleRefs()[XML_lnRef].maPhClr = rColor.maLineColor; +- if( rColor.maEffectColor.isUsed() ) +- rShape->getShapeStyleRefs()[XML_effectRef].maPhClr = rColor.maEffectColor; +- if( rColor.maTextFillColor.isUsed() ) +- rShape->getShapeStyleRefs()[XML_fontRef].maPhClr = rColor.maTextFillColor; ++ if( !rColor.maFillColors.empty() ) ++ rShape->getShapeStyleRefs()[XML_fillRef].maPhClr = DiagramColor::getColorByIndex(rColor.maFillColors, nCurrIdx); ++ if( !rColor.maLineColors.empty() ) ++ rShape->getShapeStyleRefs()[XML_lnRef].maPhClr = DiagramColor::getColorByIndex(rColor.maLineColors, nCurrIdx); ++ if( !rColor.maEffectColors.empty() ) ++ rShape->getShapeStyleRefs()[XML_effectRef].maPhClr = DiagramColor::getColorByIndex(rColor.maEffectColors, nCurrIdx); ++ if( !rColor.maTextFillColors.empty() ) ++ rShape->getShapeStyleRefs()[XML_fontRef].maPhClr = DiagramColor::getColorByIndex(rColor.maTextFillColors, nCurrIdx); + } + } + +diff --git a/oox/source/drawingml/diagram/diagramlayoutatoms.hxx b/oox/source/drawingml/diagram/diagramlayoutatoms.hxx +index 91028971473e..2e4551642389 100644 +--- a/oox/source/drawingml/diagram/diagramlayoutatoms.hxx ++++ b/oox/source/drawingml/diagram/diagramlayoutatoms.hxx +@@ -260,7 +260,8 @@ public: + { mpNodeShapes.push_back(pShape); } + + bool setupShape( const ShapePtr& rShape, +- const dgm::Point* pPresNode ) const; ++ const dgm::Point* pPresNode, ++ sal_Int32 nCurrIdx ) const; + + const LayoutNode* getParentLayoutNode() const; + +diff --git a/oox/source/drawingml/diagram/layoutatomvisitors.cxx b/oox/source/drawingml/diagram/layoutatomvisitors.cxx +index 4bfadc3affe8..c616ca3a9010 100644 +--- a/oox/source/drawingml/diagram/layoutatomvisitors.cxx ++++ b/oox/source/drawingml/diagram/layoutatomvisitors.cxx +@@ -73,7 +73,7 @@ void ShapeCreationVisitor::visit(LayoutNode& rAtom) + { + // reuse existing shape + ShapePtr pShape = rAtom.getExistingShape(); +- if (rAtom.setupShape(pShape, pNewNode)) ++ if (rAtom.setupShape(pShape, pNewNode, mnCurrIdx)) + { + pShape->setInternalName(rAtom.getName()); + rAtom.addNodeShape(pShape); +@@ -92,7 +92,7 @@ void ShapeCreationVisitor::visit(LayoutNode& rAtom) + "oox.drawingml", + "processing shape type " << (pShape->getCustomShapeProperties()->getShapePresetType())); + +- if (rAtom.setupShape(pShape, pNewNode)) ++ if (rAtom.setupShape(pShape, pNewNode, mnCurrIdx)) + { + pShape->setInternalName(rAtom.getName()); + pCurrParent->addChild(pShape); +-- +2.26.1 + diff --git a/libreoffice.changes b/libreoffice.changes index 4f57ba0..c416993 100644 --- a/libreoffice.changes +++ b/libreoffice.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Mon Jun 1 18:25:46 UTC 2020 - Andras Timar + +- Fix bsc#1146025 - LO-L3: Colored textboxes in PPTX look very odd (SmartArt) + * bsc1146025.diff + ------------------------------------------------------------------- Tue May 26 11:25:10 UTC 2020 - Andras Timar diff --git a/libreoffice.spec b/libreoffice.spec index 70c85e4..16e0e37 100644 --- a/libreoffice.spec +++ b/libreoffice.spec @@ -115,6 +115,8 @@ Patch17: bsc1160687-8.diff Patch18: bsc1165849-1.diff Patch19: bsc1165849-2.diff Patch20: bsc1165849-3.diff +# Bug 1146025 - LO-L3: Colored textboxes in PPTX look very odd (SmartArt) +Patch21: bsc1146025.diff # try to save space by using hardlinks Patch990: install-with-hardlinks.diff # save time by relying on rpm check rather than doing stupid find+grep @@ -984,6 +986,7 @@ Provides %{langname} translations and additional resources (help files, etc.) fo %patch18 -p1 %patch19 -p1 %patch20 -p1 +%patch21 -p1 %patch990 -p1 %patch991 -p1