smartart import: handle multiple <a:schemeClr> in <dgm:fillClrLst>

The TODO in the ColorFragmentHandler ctor was right: we only handled the
last <a:schemeClr> child, but there can be multiple one.

Use them based on the index of a shape in a <dgm:forEach> 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

Change-Id: I1c5c4f82e621f1110ef06b0490ff79f82f60f214
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/95015
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>

10 files changed, 105 insertions, 29 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 <oox/core/contexthandler2.hxx>
 
+#include <vector>
+
 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<Color>& rColors);
+
+    virtual ::oox::core::ContextHandlerRef
+    onCreateContext(sal_Int32 nElement, const ::oox::AttributeList& rAttribs) override;
+
+private:
+    std::vector<Color>& 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<Color>& 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<oox::drawingml::Color>& 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 <map>
 #include <memory>
+#include <vector>
 
 #include <rtl/ustring.hxx>
 
@@ -111,12 +112,15 @@ typedef std::map<OUString,DiagramStyle> 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<oox::drawingml::Color> maFillColors;
+    std::vector<oox::drawingml::Color> maLineColors;
+    std::vector<oox::drawingml::Color> maEffectColors;
+    std::vector<oox::drawingml::Color> maTextFillColors;
+    std::vector<oox::drawingml::Color> maTextLineColors;
+    std::vector<oox::drawingml::Color> maTextEffectColors;
+
+    static const oox::drawingml::Color&
+    getColorByIndex(const std::vector<oox::drawingml::Color>& rColors, sal_Int32 nIndex);
 };
 
 typedef std::map<OUString,DiagramColor> 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..0832bb1bfe7b 100644
--- a/oox/source/drawingml/diagram/diagramlayoutatoms.cxx
+++ b/oox/source/drawingml/diagram/diagramlayoutatoms.cxx
@@ -1275,7 +1275,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 +1413,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
+            // <dgm:forEach> 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);
diff --git a/sd/qa/unit/data/pptx/fill-color-list.pptx b/sd/qa/unit/data/pptx/fill-color-list.pptx
new file mode 100644
index 000000000000..341233ad5f78
--- /dev/null
+++ 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 d14c4bc6950a..273056c868d7 100644
--- a/sd/qa/unit/import-tests-smartart.cxx
+++ b/sd/qa/unit/import-tests-smartart.cxx
@@ -105,6 +105,7 @@ public:
     void testRecursion();
     void testDataFollow();
     void testOrgChart2();
+    void testFillColorList();
 
     CPPUNIT_TEST_SUITE(SdImportTestSmartArt);
 
@@ -149,6 +150,7 @@ public:
     CPPUNIT_TEST(testRecursion);
     CPPUNIT_TEST(testDataFollow);
     CPPUNIT_TEST(testOrgChart2);
+    CPPUNIT_TEST(testFillColorList);
 
     CPPUNIT_TEST_SUITE_END();
 };
@@ -1454,6 +1456,23 @@ void SdImportTestSmartArt::testOrgChart2()
     xDocShRef->DoClose();
 }
 
+void SdImportTestSmartArt::testFillColorList()
+{
+    sd::DrawDocShellRef xDocShRef
+        = loadURL(m_directories.getURLFromSrc("/sd/qa/unit/data/pptx/fill-color-list.pptx"), PPTX);
+    uno::Reference<drawing::XShape> xGroup(getShapeFromPage(0, 0, xDocShRef), uno::UNO_QUERY);
+    uno::Reference<drawing::XShape> xShape = getChildShape(getChildShape(xGroup, 1), 0);
+    uno::Reference<beans::XPropertySet> xPropertySet(xShape, uno::UNO_QUERY_THROW);
+    sal_Int32 nFillColor = 0;
+    xPropertySet->getPropertyValue("FillColor") >>= nFillColor;
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: 12603469 (0xc0504d)
+    // - 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);
+    xDocShRef->DoClose();
+}
+
 CPPUNIT_TEST_SUITE_REGISTRATION(SdImportTestSmartArt);
 
 CPPUNIT_PLUGIN_IMPLEMENT();