Fix fdo#64512 Handle xml:id correctly on multi-image draw:frames

Fixes a regression from the pick-best-image from draw:frame in ODF,
where before sometimes the XShape got deleted that the
UnoInterfaceToUniqueIdentifierMapper::registerReference stored.

For that, added a
UnoInterfaceToUniqueIdentifierMapper::registerReferenceAlways
function, which overwrites potentially existing earlier entries
with the same identifier string.

This fix was originally much more messy, but then dtardon committed
30b248dfe5bfb8a0649e36f22c943b3feb2f1385 which also fixes this here
bug. Now only sneaking in slightly less involved interface map
handling and a safeguard in ximpshap.cxx.

Change-Id: I87501e43518a5fc2fee166c45a4e2f01718f5228

2 files changed, 37 insertions, 64 deletions

diff --git a/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx b/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx
index 568943a04092..317ad07301df 100644
--- a/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx
+++ b/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx
@@ -17,8 +17,6 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
-#include <algorithm>
-
 #include <xmloff/unointerfacetouniqueidentifiermapper.hxx>
 
 using namespace ::com::sun::star;
@@ -72,42 +70,25 @@ bool UnoInterfaceToUniqueIdentifierMapper::registerReference( const OUString& rI
     {
         return rIdentifier != (*aIter).first;
     }
-    else if( findIdentifier( rIdentifier, aIter ) || findReserved( rIdentifier ) )
+    else if( findIdentifier( rIdentifier, aIter ) )
     {
         return false;
     }
     else
     {
-        maEntries.insert( IdMap_t::value_type( rIdentifier, xRef ) );
-
-        // see if this is a reference like something we would generate in the future
-        const sal_Unicode *p = rIdentifier.getStr();
-        sal_Int32 nLength = rIdentifier.getLength();
-
-        // see if the identifier is 'id' followed by a pure integer value
-        if( nLength < 2 || p[0] != 'i' || p[1] != 'd' )
-            return true;
-
-        nLength -= 2;
-        p += 2;
-
-        while(nLength--)
-        {
-            if( (*p < '0') || (*p > '9') )
-                return true; // a custom id, that will never conflict with genereated id's
+        insertReference( rIdentifier, xRef );
+    }
 
-            p++;
-        }
+    return true;
+}
 
-        // the identifier is a pure integer value
-        // so we make sure we will never generate
-        // an integer value like this one
-        sal_Int32 nId = rIdentifier.copy(2).toInt32();
-        if( mnNextId <= nId )
-            mnNextId = nId + 1;
+void UnoInterfaceToUniqueIdentifierMapper::registerReferenceAlways( const OUString& rIdentifier, const Reference< XInterface >& rInterface )
+{
+    // Be certain that the references we store in our table are to the
+    // leading / primary XInterface - cf. findReference
+    uno::Reference< uno::XInterface > xRef( rInterface, uno::UNO_QUERY );
 
-        return true;
-    }
+    insertReference( rIdentifier, xRef );
 }
 
 const OUString& UnoInterfaceToUniqueIdentifierMapper::getIdentifier( const Reference< XInterface >& rInterface ) const
@@ -166,42 +147,35 @@ bool UnoInterfaceToUniqueIdentifierMapper::findIdentifier( const OUString& rIden
     return rIter != maEntries.end();
 }
 
-bool UnoInterfaceToUniqueIdentifierMapper::reserveIdentifier( const rtl::OUString& rIdentifier )
+void UnoInterfaceToUniqueIdentifierMapper::insertReference( const OUString& rIdentifier, const Reference< XInterface >& rInterface )
 {
-    if ( findReserved( rIdentifier ) )
-        return false;
+    maEntries[rIdentifier] = rInterface;
 
-    maReserved.push_back( rIdentifier );
-    return true;
-}
+    // see if this is a reference like something we would generate in the future
+    const sal_Unicode *p = rIdentifier.getStr();
+    sal_Int32 nLength = rIdentifier.getLength();
 
-bool UnoInterfaceToUniqueIdentifierMapper::registerReservedReference(
-        const rtl::OUString& rIdentifier,
-        const com::sun::star::uno::Reference< com::sun::star::uno::XInterface >& rInterface )
-{
-    Reserved_t::const_iterator aIt;
-    if ( !findReserved( rIdentifier, aIt ) )
-        return false;
+    // see if the identifier is 'id' followed by a pure integer value
+    if( nLength < 2 || p[0] != 'i' || p[1] != 'd' )
+        return;
 
-    Reserved_t::iterator aRemoveIt( maReserved.begin() + ( aIt - maReserved.begin() ) );
-    maReserved.erase( aRemoveIt );
-    registerReference( rIdentifier, rInterface );
+    nLength -= 2;
+    p += 2;
 
-    return true;
-}
+    while(nLength--)
+    {
+        if( (*p < '0') || (*p > '9') )
+            return; // a custom id, that will never conflict with genereated id's
 
-bool UnoInterfaceToUniqueIdentifierMapper::findReserved( const OUString& rIdentifier ) const
-{
-    Reserved_t::const_iterator aDummy;
-    return findReserved( rIdentifier, aDummy );
-}
+        p++;
+    }
 
-bool UnoInterfaceToUniqueIdentifierMapper::findReserved(
-        const OUString& rIdentifier,
-        Reserved_t::const_iterator& rIter ) const
-{
-    rIter = std::find( maReserved.begin(), maReserved.end(), rIdentifier );
-    return rIter != maReserved.end();
+    // the identifier is a pure integer value
+    // so we make sure we will never generate
+    // an integer value like this one
+    sal_Int32 nId = rIdentifier.copy(2).toInt32();
+    if( mnNextId <= nId )
+        mnNextId = nId + 1;
 }
 
 }
diff --git a/xmloff/source/draw/ximpshap.cxx b/xmloff/source/draw/ximpshap.cxx
index 75dae5d23e74..eb7171b90723 100644
--- a/xmloff/source/draw/ximpshap.cxx
+++ b/xmloff/source/draw/ximpshap.cxx
@@ -3455,9 +3455,6 @@ SvXMLImportContext *SdXMLFrameShapeContext::CreateChildContext( sal_uInt16 nPref
 
         if(getSupportsMultipleContents() && dynamic_cast< SdXMLGraphicObjectShapeContext* >(pContext))
         {
-            if ( !maShapeId.isEmpty() )
-                GetImport().getInterfaceToIdentifierMapper().reserveIdentifier( maShapeId );
-
             addContent(*mxImplContext);
         }
     }
@@ -3534,11 +3531,13 @@ void SdXMLFrameShapeContext::EndElement()
     // solve if multiple image child contexts were imported
     const SvXMLImportContext* const pSelectedContext(solveMultipleImages());
     const SdXMLGraphicObjectShapeContext* pShapeContext( dynamic_cast<const SdXMLGraphicObjectShapeContext*>( pSelectedContext ) );
-    if ( pShapeContext )
+    if ( pShapeContext && !maShapeId.isEmpty() )
     {
+        // fdo#64512 and fdo#60075 - make sure *this* shape is
+        // registered for given ID
         assert( mxImplContext.Is() );
         const uno::Reference< uno::XInterface > xShape( pShapeContext->getShape() );
-        GetImport().getInterfaceToIdentifierMapper().registerReservedReference( maShapeId, xShape );
+        GetImport().getInterfaceToIdentifierMapper().registerReferenceAlways( maShapeId, xShape );
     }
 
     if( !mxImplContext.Is() )