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
<http://web.archive.org/web/20111015151600/http://msdn.microsoft.com/en-us/magazine/cc163470.aspx>,
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.

Change-Id: Ideb76c1ddd1ffff8d2a217cddf81106d1bb97eb9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/94872
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins

3 files changed, 109 insertions, 20 deletions

diff --git a/oox/source/drawingml/diagram/diagramlayoutatoms.cxx b/oox/source/drawingml/diagram/diagramlayoutatoms.cxx
index 800c9b1f64af..5e36f08d06ab 100644
--- a/oox/source/drawingml/diagram/diagramlayoutatoms.cxx
+++ b/oox/source/drawingml/diagram/diagramlayoutatoms.cxx
@@ -429,6 +429,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<Constraint>& rConstraints )
 {
@@ -466,31 +502,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<LayoutNode*>(pDirectChild.get());
+                    if (!pLayoutNode)
+                    {
+                        continue;
+                    }
+
+                    if (pLayoutNode->getName() != aCurrShape->getInternalName())
+                    {
+                        continue;
+                    }
+
+                    for (const auto& pChild : pLayoutNode->getChildren())
+                    {
+                        auto pConstraintAtom = dynamic_cast<ConstraintAtom*>(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);
 
diff --git a/sd/qa/unit/data/pptx/fill-color-list.pptx b/sd/qa/unit/data/pptx/fill-color-list.pptx
index 341233ad5f78..c88434952631 100644
--- a/sd/qa/unit/data/pptx/fill-color-list.pptx
+++ b/sd/qa/unit/data/pptx/fill-color-list.pptx
diff --git a/sd/qa/unit/import-tests-smartart.cxx b/sd/qa/unit/import-tests-smartart.cxx
index 76659c0cf680..67db3deb4cc6 100644
--- a/sd/qa/unit/import-tests-smartart.cxx
+++ b/sd/qa/unit/import-tests-smartart.cxx
@@ -1488,6 +1488,16 @@ void SdImportTestSmartArt::testFillColorList()
     // - Actual  : 16225862 (0xf79646)
     // i.e. the background of the "A" shape was orange-ish, rather than red-ish.
     CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(0xC0504D), nFillColor);
+
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: 2239
+    // - Actual  : 5199
+    // i.e. the "A" shape's height/width aspect ratio was not 0.4 but rather close to 1.0, even if
+    // ppt/diagrams/layout1.xml's <dgm:constr type="h" refType="w" op="lte" fact="0.4"/> requested
+    // 0.4.
+    awt::Size aActualSize = xShape->getSize();
+    CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(2239), aActualSize.Height);
+
     xDocShRef->DoClose();
 }