bnc#821916: Better algorithm to find placeholder shape.

Placeholder type seems to be more relevant than index.

Change-Id: I9d6c6cad8e0a51b2385801f65d7d1c697ad7998e
Reviewed-on: https://gerrit.libreoffice.org/10844
Reviewed-by: David Tardon <dtardon@redhat.com>
Tested-by: David Tardon <dtardon@redhat.com>

3 files changed, 71 insertions, 74 deletions

diff --git a/oox/source/ppt/pptgraphicshapecontext.cxx b/oox/source/ppt/pptgraphicshapecontext.cxx
index b0772a87d7e0..47a808df7efe 100644
--- a/oox/source/ppt/pptgraphicshapecontext.cxx
+++ b/oox/source/ppt/pptgraphicshapecontext.cxx
@@ -53,13 +53,6 @@ PPTGraphicShapeContext::PPTGraphicShapeContext( ContextHandler2Helper& rParent,
 {
 }
 
-// if nFirstPlaceholder can't be found, it will be searched for nSecondPlaceholder
-static oox::drawingml::ShapePtr findPlaceholder( sal_Int32 nFirstPlaceholder, sal_Int32 nSecondPlaceholder, std::vector< oox::drawingml::ShapePtr >& rShapes )
-{
-    oox::drawingml::ShapePtr pPlaceholder = PPTShape::findPlaceholder( nFirstPlaceholder, rShapes );
-    return !nSecondPlaceholder || pPlaceholder.get() ? pPlaceholder : PPTShape::findPlaceholder( nSecondPlaceholder, rShapes );
-}
-
 ContextHandlerRef PPTGraphicShapeContext::onCreateContext( sal_Int32 aElementToken, const AttributeList& rAttribs )
 {
     switch( aElementToken )
@@ -135,12 +128,14 @@ ContextHandlerRef PPTGraphicShapeContext::onCreateContext( sal_Int32 aElementTok
                     if ( nFirstPlaceholder )
                     {
                         if ( eShapeLocation == Layout )     // for layout objects the referenced object can be found within the same shape tree
-                            pPlaceholder = findPlaceholder( nFirstPlaceholder, nSecondPlaceholder, mpSlidePersistPtr->getShapes()->getChildren() );
+                            pPlaceholder = PPTShape::findPlaceholder( nFirstPlaceholder, nSecondPlaceholder,
+                                    pPPTShapePtr->getSubTypeIndex(), mpSlidePersistPtr->getShapes()->getChildren(), true );
                         else if ( eShapeLocation == Slide ) // normal slide shapes have to search within the corresponding master tree for referenced objects
                         {
                             SlidePersistPtr pMasterPersist( mpSlidePersistPtr->getMasterPersist() );
                             if ( pMasterPersist.get() )
-                                pPlaceholder = findPlaceholder( nFirstPlaceholder, nSecondPlaceholder, pMasterPersist->getShapes()->getChildren() );
+                                pPlaceholder = PPTShape::findPlaceholder( nFirstPlaceholder, nSecondPlaceholder,
+                                        pPPTShapePtr->getSubTypeIndex(), pMasterPersist->getShapes()->getChildren() );
                         }
                     }
                 }
diff --git a/oox/source/ppt/pptshape.cxx b/oox/source/ppt/pptshape.cxx
index 0c5ebe0b74c4..70b2d79fce17 100644
--- a/oox/source/ppt/pptshape.cxx
+++ b/oox/source/ppt/pptshape.cxx
@@ -228,7 +228,7 @@ void PPTShape::addShape(
             if( mnSubType && getSubTypeIndex().has() && meShapeLocation == Layout ) {
                 oox::drawingml::ShapePtr pPlaceholder = PPTShape::findPlaceholderByIndex( getSubTypeIndex().get(), rSlidePersist.getShapes()->getChildren(), true );
                 if (!pPlaceholder.get())
-                    pPlaceholder = PPTShape::findPlaceholder( mnSubType, rSlidePersist.getShapes()->getChildren(), true );
+                    pPlaceholder = PPTShape::findPlaceholder( mnSubType, 0, getSubTypeIndex(), rSlidePersist.getShapes()->getChildren(), true );
 
                 if (pPlaceholder.get()) {
                     if( maSize.Width == 0 || maSize.Height == 0 ) {
@@ -378,26 +378,76 @@ void PPTShape::applyShapeReference( const oox::drawingml::Shape& rReferencedShap
     Shape::applyShapeReference( rReferencedShape, bUseText );
 }
 
-oox::drawingml::ShapePtr PPTShape::findPlaceholder( const sal_Int32 nMasterPlaceholder, std::vector< oox::drawingml::ShapePtr >& rShapes, bool bMasterOnly )
+namespace
+{
+    bool ShapeLocationIsMaster(oox::drawingml::Shape *pInShape)
+    {
+        PPTShape* pShape = dynamic_cast<PPTShape*>(pInShape);
+        return pShape && pShape->getShapeLocation() == Master;
+    }
+}
+
+// Function to find placeholder (ph) for a shape. No idea how MSO implements this, but
+// this order seems to work quite well (probably it's unnecessary complicated / wrong):
+// 1. ph with nFirstSubType and the same oSubTypeIndex
+// 2. ph with nFirstSubType
+// 3. ph with nSecondSubType and the same oSubTypeIndex
+// 4. ph with the same oSubTypeIndex
+oox::drawingml::ShapePtr PPTShape::findPlaceholder( sal_Int32 nFirstSubType, sal_Int32 nSecondSubType,
+    const OptValue< sal_Int32 >& oSubTypeIndex, std::vector< oox::drawingml::ShapePtr >& rShapes, bool bMasterOnly )
 {
     oox::drawingml::ShapePtr aShapePtr;
+    oox::drawingml::ShapePtr aChoiceShapePtr1;
+    oox::drawingml::ShapePtr aChoiceShapePtr2;
+    oox::drawingml::ShapePtr aChoiceShapePtr3;
     std::vector< oox::drawingml::ShapePtr >::reverse_iterator aRevIter( rShapes.rbegin() );
-    while( aRevIter != rShapes.rend() )
+    while (aRevIter != rShapes.rend())
     {
-        if ( (*aRevIter)->getSubType() == nMasterPlaceholder &&
-             ( !bMasterOnly ||
-               ( dynamic_cast< PPTShape* >( (*aRevIter).get() ) && dynamic_cast< PPTShape* >( (*aRevIter).get() )->getShapeLocation() == Master ) ) )
+        if (!bMasterOnly || ShapeLocationIsMaster((*aRevIter).get()))
         {
-            aShapePtr = *aRevIter;
-            break;
+            if ((*aRevIter)->getSubTypeIndex() == oSubTypeIndex)
+            {
+                if ((*aRevIter)->getSubType() == nFirstSubType)
+                {
+                    aShapePtr = *aRevIter;
+                    break;
+                }
+                else if ((*aRevIter)->getSubType() == nSecondSubType && !aChoiceShapePtr2.get())
+                    aChoiceShapePtr2 = *aRevIter;
+                else if (!aChoiceShapePtr3.get())
+                    aChoiceShapePtr3 = *aRevIter;
+            }
+            else if ((*aRevIter)->getSubType() == nFirstSubType && !aChoiceShapePtr1.get())
+                aChoiceShapePtr1 = *aRevIter;
         }
         std::vector< oox::drawingml::ShapePtr >& rChildren = (*aRevIter)->getChildren();
-        aShapePtr = findPlaceholder( nMasterPlaceholder, rChildren, bMasterOnly );
-        if ( aShapePtr.get() )
+        aChoiceShapePtr3 = findPlaceholder( nFirstSubType, nSecondSubType, oSubTypeIndex, rChildren, bMasterOnly );
+        if (aChoiceShapePtr3.get())
+        {
+            if (aChoiceShapePtr3->getSubType() == nFirstSubType)
+            {
+                if (aChoiceShapePtr3->getSubTypeIndex() == oSubTypeIndex)
+                    aShapePtr = aChoiceShapePtr3;
+                else
+                    aChoiceShapePtr1 = aChoiceShapePtr3;
+            }
+            else if (aChoiceShapePtr3->getSubType() == nSecondSubType &&
+                    aChoiceShapePtr3->getSubTypeIndex() == oSubTypeIndex)
+            {
+                aChoiceShapePtr2 = aChoiceShapePtr3;
+            }
+        }
+        if (aShapePtr.get())
             break;
         ++aRevIter;
     }
-    return aShapePtr;
+    if (aShapePtr.get())
+        return aShapePtr;
+    if (aChoiceShapePtr1.get())
+        return aChoiceShapePtr1;
+    if (aChoiceShapePtr2.get())
+        return aChoiceShapePtr2;
+    return aChoiceShapePtr3;
 }
 
 oox::drawingml::ShapePtr PPTShape::findPlaceholderByIndex( const sal_Int32 nIdx, std::vector< oox::drawingml::ShapePtr >& rShapes, bool bMasterOnly )
diff --git a/oox/source/ppt/pptshapecontext.cxx b/oox/source/ppt/pptshapecontext.cxx
index 64747a4f3e39..8a2e3ff38602 100644
--- a/oox/source/ppt/pptshapecontext.cxx
+++ b/oox/source/ppt/pptshapecontext.cxx
@@ -53,45 +53,6 @@ PPTShapeContext::PPTShapeContext( ContextHandler2Helper& rParent, const SlidePer
 {
 }
 
-oox::drawingml::ShapePtr findPlaceholder( const sal_Int32 nMasterPlaceholder, const OptValue< sal_Int32 >& oSubTypeIndex, std::vector< oox::drawingml::ShapePtr >& rShapes )
-{
-    oox::drawingml::ShapePtr aShapePtr;
-    oox::drawingml::ShapePtr aChoiceShapePtr1;
-    oox::drawingml::ShapePtr aChoiceShapePtr2;
-    std::vector< oox::drawingml::ShapePtr >::reverse_iterator aRevIter( rShapes.rbegin() );
-    while( aRevIter != rShapes.rend() )
-    {
-        if ( (*aRevIter)->getSubType() == nMasterPlaceholder )
-        {
-            if( !oSubTypeIndex.has() && aChoiceShapePtr1 == 0 )
-                aChoiceShapePtr1 = *aRevIter;
-            else if( aChoiceShapePtr2 == 0 )
-                aChoiceShapePtr2 = *aRevIter;
-            if( (*aRevIter)->getSubTypeIndex() == oSubTypeIndex )
-            {
-                aShapePtr = *aRevIter;
-                break;
-            }
-        }
-        std::vector< oox::drawingml::ShapePtr >& rChildren = (*aRevIter)->getChildren();
-        aShapePtr = findPlaceholder( nMasterPlaceholder, oSubTypeIndex, rChildren );
-        if ( aShapePtr.get() )
-            break;
-        ++aRevIter;
-    }
-    if( aShapePtr == 0 )
-        return aChoiceShapePtr1 ? aChoiceShapePtr1 : aChoiceShapePtr2;
-    return aShapePtr;
-}
-
-// if nFirstPlaceholder can't be found, it will be searched for nSecondPlaceholder
-oox::drawingml::ShapePtr findPlaceholder( sal_Int32 nFirstPlaceholder, sal_Int32 nSecondPlaceholder,
-    const OptValue< sal_Int32 >& oSubTypeIndex, std::vector< oox::drawingml::ShapePtr >& rShapes )
-{
-    oox::drawingml::ShapePtr pPlaceholder = findPlaceholder( nFirstPlaceholder, oSubTypeIndex, rShapes );
-    return !nSecondPlaceholder || pPlaceholder.get() ? pPlaceholder : findPlaceholder( nSecondPlaceholder, oSubTypeIndex, rShapes );
-}
-
 ContextHandlerRef PPTShapeContext::onCreateContext( sal_Int32 aElementToken, const AttributeList& rAttribs )
 {
     if( getNamespace( aElementToken ) == NMSP_dsp )
@@ -165,25 +126,16 @@ ContextHandlerRef PPTShapeContext::onCreateContext( sal_Int32 aElementToken, con
                               oox::drawingml::ShapePtr pPlaceholder;
                               if ( eShapeLocation == Layout )       // for layout objects the referenced object can be found within the same shape tree
                               {
-                                  if( pPPTShapePtr->getSubTypeIndex().has() )
-                                      pPlaceholder = PPTShape::findPlaceholderByIndex( pPPTShapePtr->getSubTypeIndex().get(), mpSlidePersistPtr->getShapes()->getChildren() );
-                                  if ( !pPlaceholder.get() )
-                                      pPlaceholder = findPlaceholder( nFirstPlaceholder, nSecondPlaceholder, pPPTShapePtr->getSubTypeIndex(),
-                                                                      mpSlidePersistPtr->getShapes()->getChildren() );
+                                  pPlaceholder = PPTShape::findPlaceholder( nFirstPlaceholder, nSecondPlaceholder,
+                                          pPPTShapePtr->getSubTypeIndex(), mpSlidePersistPtr->getShapes()->getChildren(), true );
                               }
                               else if ( eShapeLocation == Slide )   // normal slide shapes have to search within the corresponding master tree for referenced objects
                               {
                                   SlidePersistPtr pMasterPersist( mpSlidePersistPtr->getMasterPersist() );
-                                  if ( pMasterPersist.get() ) {
-                                      if( pPPTShapePtr->getSubTypeIndex().has() )
-                                          pPlaceholder = PPTShape::findPlaceholderByIndex( pPPTShapePtr->getSubTypeIndex().get(), pMasterPersist->getShapes()->getChildren() );
-                                      // TODO: Check if this is required for non-notes pages as well...
-                                      if ( !pPlaceholder.get() || ( pMasterPersist->isNotesPage() && pPlaceholder->getSubType() != nFirstPlaceholder &&
-                                                                                                     pPlaceholder->getSubType() != nSecondPlaceholder ) )
-                                      {
-                                          pPlaceholder = findPlaceholder( nFirstPlaceholder, nSecondPlaceholder,
-                                                                          pPPTShapePtr->getSubTypeIndex(), pMasterPersist->getShapes()->getChildren() );
-                                      }
+                                  if ( pMasterPersist.get() )
+                                  {
+                                      pPlaceholder = PPTShape::findPlaceholder( nFirstPlaceholder, nSecondPlaceholder,
+                                              pPPTShapePtr->getSubTypeIndex(), pMasterPersist->getShapes()->getChildren() );
                                   }
                               }
                               if ( pPlaceholder.get() )