tdf#153874 writerfilter: fix anchoring of decorative shapes

Turns out as-char flys can be decorative too.

The confusing GraphicImport takes a GraphicImportType by value but
everything else is a reference to a DomainMapper_Impl member.

The latter appears to work better so handle GraphicImportType the same
way and remove the function parameter.

(regression from commit 31084ebb59093be7dfe5ab53a20fdb3bcfde34b6)

Change-Id: I18c1d47d39751e8ddcaa52498077d89c43a934e9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/147998
Tested-by: Michael Stahl <michael.stahl@allotropia.de>
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>

7 files changed, 56 insertions, 40 deletions

diff --git a/sw/qa/extras/ooxmlexport/data/image_through_shape.docx b/sw/qa/extras/ooxmlexport/data/image_through_shape.docx
new file mode 100644
index 000000000000..dd90f9d9bf29
--- /dev/null
+++ b/sw/qa/extras/ooxmlexport/data/image_through_shape.docx
diff --git a/sw/qa/extras/ooxmlexport/ooxmlexport16.cxx b/sw/qa/extras/ooxmlexport/ooxmlexport16.cxx
index 99c0b2c9ffe7..9eb7eabd2434 100644
--- a/sw/qa/extras/ooxmlexport/ooxmlexport16.cxx
+++ b/sw/qa/extras/ooxmlexport/ooxmlexport16.cxx
@@ -823,6 +823,15 @@ DECLARE_OOXMLEXPORT_TEST(testTdf133473_shadowSize, "tdf133473.docx")
     CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(200000), nSize1);
 }
 
+DECLARE_OOXMLEXPORT_TEST(testTdf153874, "image_through_shape.docx")
+{
+    uno::Reference<beans::XPropertySet> const xShape1(getShapeByName(u"Test1"),  uno::UNO_QUERY);
+    uno::Reference<beans::XPropertySet> const xShape2(getShapeByName(u"Rectangle 1"), uno::UNO_QUERY);
+    CPPUNIT_ASSERT_EQUAL(text::TextContentAnchorType_AS_CHARACTER, xShape1->getPropertyValue("AnchorType").get<text::TextContentAnchorType>());
+    CPPUNIT_ASSERT_EQUAL(text::TextContentAnchorType_AT_CHARACTER, xShape2->getPropertyValue("AnchorType").get<text::TextContentAnchorType>());
+    CPPUNIT_ASSERT_LESS(xShape2->getPropertyValue("ZOrder").get<sal_uInt64>(), xShape1->getPropertyValue("ZOrder").get<sal_uInt64>());
+}
+
 DECLARE_OOXMLEXPORT_TEST(testTextBoxZOrder, "testTextBoxZOrder.docx")
 {
     // Is load successful?
diff --git a/writerfilter/source/dmapper/DomainMapper.cxx b/writerfilter/source/dmapper/DomainMapper.cxx
index 242a0d655e39..573a1b41f4a5 100644
--- a/writerfilter/source/dmapper/DomainMapper.cxx
+++ b/writerfilter/source/dmapper/DomainMapper.cxx
@@ -776,8 +776,9 @@ void DomainMapper::lcl_attribute(Id nName, Value & val)
         {
             //looks a bit like a hack - and it is. The graphic import is split into the inline_inline part and
             //afterwards the adding of the binary data.
-            m_pImpl->GetGraphicImport( IMPORT_AS_DETECTED_INLINE )->attribute(nName, val);
-            m_pImpl->ImportGraphic( val.getProperties(), IMPORT_AS_DETECTED_INLINE );
+            m_pImpl->m_eGraphicImportType = IMPORT_AS_DETECTED_INLINE; // really ???
+            m_pImpl->GetGraphicImport()->attribute(nName, val);
+            m_pImpl->ImportGraphic(val.getProperties());
         }
         break;
         case NS_ooxml::LN_Value_math_ST_Jc_centerGroup:
@@ -1337,7 +1338,7 @@ void DomainMapper::lcl_attribute(Id nName, Value & val)
             }
             break;
         case NS_ooxml::LN_OfficeArtExtension_Decorative_val:
-            m_pImpl->GetGraphicImport(IMPORT_AS_DETECTED_ANCHOR)->attribute(nName, val);
+            m_pImpl->GetGraphicImport()->attribute(nName, val);
             break;
         default:
             SAL_WARN("writerfilter", "DomainMapper::lcl_attribute: unhandled token: " << nName);
@@ -2539,15 +2540,14 @@ void DomainMapper::sprmWithProps( Sprm& rSprm, const PropertyMapPtr& rContext )
         writerfilter::Reference<Properties>::Pointer_t pProperties = rSprm.getProps();
         if( pProperties )
         {
-            GraphicImportType eGraphicType =
+            m_pImpl->m_eGraphicImportType =
                 (NS_ooxml::LN_anchor_anchor ==
                  sal::static_int_cast<Id>(nSprmId)) ?
                 IMPORT_AS_DETECTED_ANCHOR :
                 IMPORT_AS_DETECTED_INLINE;
-            GraphicImportPtr pGraphicImport =
-                m_pImpl->GetGraphicImport(eGraphicType);
+            GraphicImportPtr pGraphicImport = m_pImpl->GetGraphicImport();
             pProperties->resolve(*pGraphicImport);
-            m_pImpl->ImportGraphic(pProperties, eGraphicType);
+            m_pImpl->ImportGraphic(pProperties);
             if( !pGraphicImport->IsGraphic() )
             {
                 m_pImpl->ResetGraphicImport();
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.cxx b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
index 558d71f90fe7..bc5b44ac4863 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.cxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.cxx
@@ -8218,10 +8218,12 @@ void DomainMapper_Impl::AddAnnotationPosition(
     m_aAnnotationPositions[ nAnnotationId ] = aAnnotationPosition;
 }
 
-GraphicImportPtr const & DomainMapper_Impl::GetGraphicImport(GraphicImportType eGraphicImportType)
+GraphicImportPtr const & DomainMapper_Impl::GetGraphicImport()
 {
     if(!m_pGraphicImport)
-        m_pGraphicImport = new GraphicImport( m_xComponentContext, m_xTextFactory, m_rDMapper, eGraphicImportType, m_aPositionOffsets, m_aAligns, m_aPositivePercentages );
+    {
+        m_pGraphicImport = new GraphicImport(m_xComponentContext, m_xTextFactory, m_rDMapper, m_eGraphicImportType, m_aPositionOffsets, m_aAligns, m_aPositivePercentages);
+    }
     return m_pGraphicImport;
 }
 /*-------------------------------------------------------------------------
@@ -8233,11 +8235,11 @@ void DomainMapper_Impl::ResetGraphicImport()
 }
 
 
-void  DomainMapper_Impl::ImportGraphic(const writerfilter::Reference< Properties >::Pointer_t& ref, GraphicImportType eGraphicImportType)
+void  DomainMapper_Impl::ImportGraphic(const writerfilter::Reference<Properties>::Pointer_t& ref)
 {
-    GetGraphicImport(eGraphicImportType);
-    if( eGraphicImportType != IMPORT_AS_DETECTED_INLINE && eGraphicImportType != IMPORT_AS_DETECTED_ANCHOR )
-    {
+    GetGraphicImport();
+    if (m_eGraphicImportType != IMPORT_AS_DETECTED_INLINE && m_eGraphicImportType != IMPORT_AS_DETECTED_ANCHOR)
+    {   // this appears impossible?
         //create the graphic
         ref->resolve( *m_pGraphicImport );
     }
@@ -8288,7 +8290,7 @@ void  DomainMapper_Impl::ImportGraphic(const writerfilter::Reference< Properties
 
         uno::Reference<drawing::XShape> xShape = m_pGraphicImport->GetXShapeObject();
         UpdateEmbeddedShapeProps(xShape);
-        if (eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
+        if (m_eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
         {
             uno::Reference<beans::XPropertySet> xEmbeddedProps(m_xEmbedded, uno::UNO_QUERY);
             xEmbeddedProps->setPropertyValue("AnchorType", uno::Any(text::TextContentAnchorType_AT_CHARACTER));
@@ -8313,7 +8315,7 @@ void  DomainMapper_Impl::ImportGraphic(const writerfilter::Reference< Properties
     {
         bool bAppend = true;
         // workaround for images anchored to characters: add ZWSPs around the anchoring point
-        if ( eGraphicImportType != IMPORT_AS_DETECTED_INLINE && !m_aRedlines.top().empty() )
+        if (m_eGraphicImportType != IMPORT_AS_DETECTED_INLINE && !m_aRedlines.top().empty())
         {
             uno::Reference< text::XTextAppend > xTextAppend = m_aTextAppendStack.top().xTextAppend;
             if(xTextAppend.is())
@@ -8342,7 +8344,7 @@ void  DomainMapper_Impl::ImportGraphic(const writerfilter::Reference< Properties
         if ( bAppend )
             appendTextContent( xTextContent, uno::Sequence< beans::PropertyValue >() );
 
-        if (eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR && !m_aTextAppendStack.empty())
+        if (m_eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR && !m_aTextAppendStack.empty())
         {
             // Remember this object is anchored to the current paragraph.
             AnchoredObjectInfo aInfo;
@@ -8355,7 +8357,7 @@ void  DomainMapper_Impl::ImportGraphic(const writerfilter::Reference< Properties
             }
             m_aTextAppendStack.top().m_aAnchoredObjects.push_back(aInfo);
         }
-        else if (eGraphicImportType == IMPORT_AS_DETECTED_INLINE)
+        else if (m_eGraphicImportType == IMPORT_AS_DETECTED_INLINE)
         {
             m_bParaWithInlineObject = true;
 
diff --git a/writerfilter/source/dmapper/DomainMapper_Impl.hxx b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
index 22e6e527d5c0..234ae9416eb1 100644
--- a/writerfilter/source/dmapper/DomainMapper_Impl.hxx
+++ b/writerfilter/source/dmapper/DomainMapper_Impl.hxx
@@ -827,10 +827,10 @@ public:
         return m_pSettingsTable;
     }
 
-    GraphicImportPtr const & GetGraphicImport( GraphicImportType eGraphicImportType );
+    GraphicImportPtr const & GetGraphicImport();
     void            ResetGraphicImport();
     // this method deletes the current m_pGraphicImport after import
-    void    ImportGraphic(const writerfilter::Reference< Properties>::Pointer_t&, GraphicImportType eGraphicImportType );
+    void    ImportGraphic(const writerfilter::Reference<Properties>::Pointer_t&);
 
     void InitTabStopFromStyle(const css::uno::Sequence<css::style::TabStop>& rInitTabStops);
     void    IncorporateTabStop( const DeletableTabStop &aTabStop );
@@ -1161,6 +1161,7 @@ public:
     std::pair<OUString, OUString> m_aAligns;
     /// ST_PositivePercentage values we received
     std::queue<OUString> m_aPositivePercentages;
+    enum GraphicImportType m_eGraphicImportType = {};
     bool isInIndexContext() const { return m_bStartIndex;}
     bool isInBibliographyContext() const { return m_bStartBibliography;}
     SmartTagHandler& getSmartTagHandler() { return m_aSmartTagHandler; }
diff --git a/writerfilter/source/dmapper/GraphicImport.cxx b/writerfilter/source/dmapper/GraphicImport.cxx
index f73c1e9de56f..c8c1e392f2c0 100644
--- a/writerfilter/source/dmapper/GraphicImport.cxx
+++ b/writerfilter/source/dmapper/GraphicImport.cxx
@@ -204,7 +204,7 @@ private:
     bool      m_bYSizeValid;
 
 public:
-    GraphicImportType m_eGraphicImportType;
+    GraphicImportType & m_rGraphicImportType;
     DomainMapper&   m_rDomainMapper;
 
     sal_Int32 m_nLeftPosition;
@@ -272,12 +272,15 @@ public:
     std::optional<sal_Int32> m_oEffectExtentRight;
     std::optional<sal_Int32> m_oEffectExtentBottom;
 
-    GraphicImport_Impl(GraphicImportType eImportType, DomainMapper& rDMapper, std::pair<OUString, OUString>& rPositionOffsets, std::pair<OUString, OUString>& rAligns, std::queue<OUString>& rPositivePercentages) :
-        m_nXSize(0)
+    GraphicImport_Impl(GraphicImportType & rImportType, DomainMapper& rDMapper,
+            std::pair<OUString, OUString>& rPositionOffsets,
+            std::pair<OUString, OUString>& rAligns,
+            std::queue<OUString>& rPositivePercentages)
+        : m_nXSize(0)
         ,m_bXSizeValid(false)
         ,m_nYSize(0)
         ,m_bYSizeValid(false)
-        ,m_eGraphicImportType( eImportType )
+        ,m_rGraphicImportType(rImportType)
         ,m_rDomainMapper( rDMapper )
         ,m_nLeftPosition(0)
         ,m_nTopPosition(0)
@@ -315,11 +318,6 @@ public:
         ,m_rAligns(rAligns)
         ,m_rPositivePercentages(rPositivePercentages)
     {
-        if (m_eGraphicImportType == GraphicImportType::IMPORT_AS_DETECTED_INLINE
-            && !rDMapper.IsInShape())
-        {
-            m_zOrder = 0;
-        }
     }
 
     void setXSize(sal_Int32 _nXSize)
@@ -388,14 +386,19 @@ public:
 
     void applyZOrder(uno::Reference<beans::XPropertySet> const & xGraphicObjectProperties) const
     {
-        if (m_zOrder >= 0)
+        sal_Int32 nZOrder = m_zOrder;
+        if (m_rGraphicImportType == GraphicImportType::IMPORT_AS_DETECTED_INLINE
+            && !m_rDomainMapper.IsInShape())
+        {
+            nZOrder = 0;
+        }
+        if (nZOrder >= 0)
         {
             // tdf#120760 Send objects with behinddoc=true to the back.
-            sal_Int32 nZOrder = m_zOrder;
             if (m_bBehindDoc && m_rDomainMapper.IsInHeaderFooter())
                 nZOrder -= SAL_MAX_INT32;
             GraphicZOrderHelper* pZOrderHelper = m_rDomainMapper.graphicZOrderHelper();
-            bool bOldStyle = m_eGraphicImportType == GraphicImportType::IMPORT_AS_DETECTED_INLINE;
+            bool const bOldStyle(m_rGraphicImportType == GraphicImportType::IMPORT_AS_DETECTED_INLINE);
             xGraphicObjectProperties->setPropertyValue(getPropertyName(PROP_Z_ORDER),
                 uno::Any(pZOrderHelper->findZOrder(nZOrder, bOldStyle)));
             pZOrderHelper->addItem(xGraphicObjectProperties, nZOrder);
@@ -456,14 +459,14 @@ public:
 GraphicImport::GraphicImport(uno::Reference<uno::XComponentContext> xComponentContext,
                              uno::Reference<lang::XMultiServiceFactory> xTextFactory,
                              DomainMapper& rDMapper,
-                             GraphicImportType eImportType,
+                             GraphicImportType & rImportType,
                              std::pair<OUString, OUString>& rPositionOffsets,
                              std::pair<OUString, OUString>& rAligns,
                              std::queue<OUString>& rPositivePercentages)
 : LoggedProperties("GraphicImport")
 , LoggedTable("GraphicImport")
 , LoggedStream("GraphicImport")
-, m_pImpl(new GraphicImport_Impl(eImportType, rDMapper, rPositionOffsets, rAligns, rPositivePercentages))
+, m_pImpl(new GraphicImport_Impl(rImportType, rDMapper, rPositionOffsets, rAligns, rPositivePercentages))
 , m_xComponentContext(std::move(xComponentContext))
 , m_xTextFactory(std::move(xTextFactory))
 {
@@ -1015,7 +1018,7 @@ void GraphicImport::lcl_attribute(Id nName, Value& rValue)
                             lcl_correctWord2007EffectExtent(nOOXAngle);
                         }
 
-                        if (m_pImpl->m_eGraphicImportType == IMPORT_AS_DETECTED_INLINE)
+                        if (m_pImpl->m_rGraphicImportType == IMPORT_AS_DETECTED_INLINE)
                         {
                             if (nOOXAngle == 0)
                             {
@@ -1235,7 +1238,7 @@ void GraphicImport::lcl_attribute(Id nName, Value& rValue)
                             m_pImpl->m_nBottomMargin = 0;
                     }
 
-                    if (bUseShape && m_pImpl->m_eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
+                    if (bUseShape && m_pImpl->m_rGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
                     {
                         // If we are here, this is a drawingML shape. For those, only dmapper (and not oox) knows the anchoring infos (just like for Writer pictures).
                         // But they aren't Writer pictures, either (which are already handled above).
@@ -1342,7 +1345,7 @@ void GraphicImport::lcl_attribute(Id nName, Value& rValue)
                         aInteropGrabBag.update(m_pImpl->getInteropGrabBag());
                         xShapeProps->setPropertyValue("InteropGrabBag", uno::Any(aInteropGrabBag.getAsConstPropertyValueList()));
                     }
-                    else if (bUseShape && m_pImpl->m_eGraphicImportType == IMPORT_AS_DETECTED_INLINE)
+                    else if (bUseShape && m_pImpl->m_rGraphicImportType == IMPORT_AS_DETECTED_INLINE)
                     {
                         uno::Reference< beans::XPropertySet > xShapeProps(m_xShape, uno::UNO_QUERY_THROW);
                         m_pImpl->applyMargins(xShapeProps);
@@ -1718,7 +1721,7 @@ uno::Reference<text::XTextContent> GraphicImport::createGraphicObject(uno::Refer
                 uno::UNO_QUERY_THROW);
             xGraphicObjectProperties->setPropertyValue(getPropertyName(PROP_GRAPHIC), uno::Any(rxGraphic));
             xGraphicObjectProperties->setPropertyValue(getPropertyName(PROP_ANCHOR_TYPE),
-                uno::Any( m_pImpl->m_eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR ?
+                uno::Any( m_pImpl->m_rGraphicImportType == IMPORT_AS_DETECTED_ANCHOR ?
                                     text::TextContentAnchorType_AT_CHARACTER :
                                     text::TextContentAnchorType_AS_CHARACTER ));
             xGraphicObject.set( xGraphicObjectProperties, uno::UNO_QUERY_THROW );
@@ -1793,7 +1796,7 @@ uno::Reference<text::XTextContent> GraphicImport::createGraphicObject(uno::Refer
                     uno::Any(true));
 
             sal_Int32 nWidth = - m_pImpl->m_nLeftPosition;
-            if (m_pImpl->m_eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
+            if (m_pImpl->m_rGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
             {
                 xGraphicObjectProperties->setPropertyValue(getPropertyName(PROP_DECORATIVE), uno::Any(m_pImpl->m_bDecorative));
                 //adjust margins
@@ -1917,7 +1920,8 @@ uno::Reference<text::XTextContent> GraphicImport::createGraphicObject(uno::Refer
             }
 
 
-            if(m_pImpl->m_eGraphicImportType == IMPORT_AS_DETECTED_INLINE || m_pImpl->m_eGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
+            if (m_pImpl->m_rGraphicImportType == IMPORT_AS_DETECTED_INLINE
+                || m_pImpl->m_rGraphicImportType == IMPORT_AS_DETECTED_ANCHOR)
             {
                 if( m_pImpl->getXSize() && m_pImpl->getYSize() )
                     xGraphicObjectProperties->setPropertyValue(getPropertyName(PROP_SIZE),
diff --git a/writerfilter/source/dmapper/GraphicImport.hxx b/writerfilter/source/dmapper/GraphicImport.hxx
index 9bd3f4aaaa3e..9729aecace62 100644
--- a/writerfilter/source/dmapper/GraphicImport.hxx
+++ b/writerfilter/source/dmapper/GraphicImport.hxx
@@ -84,7 +84,7 @@ public:
     explicit GraphicImport( css::uno::Reference<css::uno::XComponentContext>  xComponentContext,
                             css::uno::Reference<css::lang::XMultiServiceFactory> xTextFactory,
                             DomainMapper& rDomainMapper,
-                            GraphicImportType eGraphicImportType,
+                            GraphicImportType & rGraphicImportType,
                             std::pair<OUString, OUString>& rPositionOffsets,
                             std::pair<OUString, OUString>& rAligns,
                             std::queue<OUString>& rPositivePercentages);