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

3 files changed, 44 insertions, 80 deletions

diff --git a/xmloff/inc/xmloff/unointerfacetouniqueidentifiermapper.hxx b/xmloff/inc/xmloff/unointerfacetouniqueidentifiermapper.hxx
index 23f552536114..b664f7c38a3f 100644
--- a/xmloff/inc/xmloff/unointerfacetouniqueidentifiermapper.hxx
+++ b/xmloff/inc/xmloff/unointerfacetouniqueidentifiermapper.hxx
@@ -24,7 +24,6 @@
 #include "xmloff/dllapi.h"
 #include "sal/types.h"
 
-#include <deque>
 #include <map>
 #include <rtl/ustring.hxx>
 #include <com/sun/star/uno/XInterface.hpp>
@@ -32,12 +31,10 @@
 namespace comphelper
 {
 
-typedef ::std::map< rtl::OUString, const ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface > > IdMap_t;
+typedef ::std::map< rtl::OUString, ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface > > IdMap_t;
 
 class XMLOFF_DLLPUBLIC UnoInterfaceToUniqueIdentifierMapper
 {
-    typedef std::deque< rtl::OUString > Reserved_t;
-
 public:
     UnoInterfaceToUniqueIdentifierMapper();
 
@@ -53,16 +50,12 @@ public:
     */
     bool registerReference( const rtl::OUString& rIdentifier, const ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface >& rInterface );
 
-    /** reserves an identifier for later registration.
-
-        @returns
-            false, if the identifier already exists
-      */
-    bool reserveIdentifier( const rtl::OUString& rIdentifier );
+    /** always registers the given uno object with the given identifier.
 
-    /** registers the given uno object with reserved identifier.
-      */
-    bool registerReservedReference( const rtl::OUString& rIdentifier, const com::sun::star::uno::Reference< com::sun::star::uno::XInterface >& rInterface );
+        In contrast to registerReference(), this here overwrites any
+        earlier registration of the same identifier
+    */
+    void registerReferenceAlways( const rtl::OUString& rIdentifier, const ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface >& rInterface );
 
     /** @returns
             the identifier for the given uno object. If this uno object is not already
@@ -79,12 +72,10 @@ public:
 private:
     bool findReference( const ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface >& rInterface, IdMap_t::const_iterator& rIter ) const;
     bool findIdentifier( const OUString& rIdentifier, IdMap_t::const_iterator& rIter ) const;
-    bool findReserved( const OUString& rIdentifier ) const;
-    bool findReserved( const OUString& rIdentifier, Reserved_t::const_iterator& rIter ) const;
+    void insertReference( const OUString& rIdentifier, const ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface >& rInterface );
 
     IdMap_t	maEntries;
     sal_Int32 mnNextId;
-    Reserved_t maReserved;
 };
 
 }
diff --git a/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx b/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx
index 87eb2d3f8fa4..c0ddf8a135bf 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;
@@ -65,42 +63,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
@@ -159,42 +140,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 e4a07c9372e2..f0307c9f9806 100644
--- a/xmloff/source/draw/ximpshap.cxx
+++ b/xmloff/source/draw/ximpshap.cxx
@@ -3457,9 +3457,6 @@ SvXMLImportContext *SdXMLFrameShapeContext::CreateChildContext( sal_uInt16 nPref
 
         if(getSupportsMultipleContents() && dynamic_cast< SdXMLGraphicObjectShapeContext* >(pContext))
         {
-            if ( !maShapeId.isEmpty() )
-                GetImport().getInterfaceToIdentifierMapper().reserveIdentifier( maShapeId );
-
             addContent(*mxImplContext);
         }
     }
@@ -3536,11 +3533,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() )