ITEM: Needed reworks on ItemSurrogate mechanism

ItemSurrogates are a possibility to iterate over
all SfxPoolItems associated with a WhichID at a
ItemPool to collect or change data.
It is in general not a good idea: the correct action
would be to iterate over the model data and change/
adapt/collect data on the Items of the type
in question. This is because the *assumtion* that you
iteate over all the Items associated with a document
model is *not* correct:
The ItemPool of the model is used for various ItemSets,
e.g. Dialogs/Sidebar and others, so you might get Items
not only from the DocumentModel but from elsewhere.
You might even get Items from *other* models, so
changing these might have unpredictable effects (!)
It is clear to me that this mechanism is more convenient
that iterating over the document models, and it might
have been invented due to this and then used in more
and maore cases. There should be a lambda/callback-based
mechanism in every document model to do this. Until then
we have to live with this 'compromize'. There are over
100 usages currrently, so no way to easily replace this.

For those reasons I changed this to be more safe: There
are two methods to do this now:

1: iterateItemSurrogates to allow read/write access. I
identified six places where that mechanism was used
to change SfxPoolItems, with the use of const_cast.
This now offers a lambda/callback mechanism and the
needed data in a helper (SurrogateData). Internally
it iterates over ItemSets and ItmHolders registered
and thus associated with the Pool. Changing an Item
means to set a changed Item at the Pool/replace the
holder.

2: GetItemSurrogates/FindItemSurrogate to allow
read-only iteration (2nd one with filter). This
collects the Items in a vector that you provide over
which you can then iterate. Do *not* use const_cast
and change the Item when using this (!)

Note that mechanism (2) pe-filters the Items so that
you get each only once: Of couse one Item can be set
in more than one ItemSet/Holder (but also in more than
one model). This filtering is not possible for (1), you
have to evtl. do the same replace action for the same
item, but this is the only way to not change Items that
are associated wth another model.

Note that (2) could also be changed to a lambda/callback
mechanism similar to (1), but there are too many places
that would beed to be adapted. That would have the
advantage to not need to pre-collect the candidates in a
first run.

Also removed/replaced FindItemSurrogate with using
GetItemSurrogates and locally filtering with that needle.
That also made me remove/cleanup CollectSurrogates, it's
only used in one place now.

Change-Id: I0fe2f51f4fca45e1e5aea032cb96bb77b4567f4d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162254
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

1 files changed, 93 insertions, 80 deletions

diff --git a/sd/source/core/drawdoc2.cxx b/sd/source/core/drawdoc2.cxx
index 328952c577c5..7804c98cb7b5 100644
--- a/sd/source/core/drawdoc2.cxx
+++ b/sd/source/core/drawdoc2.cxx
@@ -268,43 +268,53 @@ void SdDrawDocument::UpdatePageRelativeURLs(std::u16string_view aOldName, std::u
         return;
 
     SfxItemPool& rPool(GetPool());
-    ItemSurrogates aSurrogates;
-    rPool.GetItemSurrogates(aSurrogates, EE_FEATURE_FIELD);
-    for (const SfxPoolItem* pItem : aSurrogates)
+
+    rPool.iterateItemSurrogates(EE_FEATURE_FIELD, [&](SfxItemPool::SurrogateData& rData)
     {
-        const SvxFieldItem* pFldItem = dynamic_cast< const SvxFieldItem * > (pItem);
+        const SvxFieldItem* pFieldItem(dynamic_cast<const SvxFieldItem*>(&rData.getItem()));
 
-        if(pFldItem)
-        {
-            SvxURLField* pURLField = const_cast< SvxURLField* >( dynamic_cast<const SvxURLField*>( pFldItem->GetField() ) );
+        if (nullptr == pFieldItem)
+            return true; // continue callbacks
 
-            if(pURLField)
-            {
-                OUString aURL = pURLField->GetURL();
+        const SvxURLField* pURLField(dynamic_cast<const SvxURLField*>(pFieldItem->GetField()));
 
-                if (!aURL.isEmpty() && (aURL[0] == 35) && (aURL.indexOf(aOldName, 1) == 1))
-                {
-                    if (aURL.getLength() == sal_Int32(aOldName.size() + 1)) // standard page name
-                    {
-                        aURL = aURL.replaceAt(1, aURL.getLength() - 1, u"") +
-                            aNewName;
-                        pURLField->SetURL(aURL);
-                    }
-                    else
-                    {
-                        const OUString sNotes(SdResId(STR_NOTES));
-                        if (aURL.getLength() == sal_Int32(aOldName.size()) + 2 + sNotes.getLength()
-                            && aURL.indexOf(sNotes, aOldName.size() + 2) == sal_Int32(aOldName.size() + 2))
-                        {
-                            aURL = aURL.replaceAt(1, aURL.getLength() - 1, u"") +
-                                aNewName + " " + sNotes;
-                            pURLField->SetURL(aURL);
-                        }
-                    }
-                }
+        if (nullptr == pURLField)
+            return true; // continue callbacks
+
+        OUString aURL(pURLField->GetURL());
+
+        if (aURL.isEmpty() || (aURL[0] != 35) || (aURL.indexOf(aOldName, 1) != 1))
+            return true; // continue callbacks
+
+        bool bURLChange(false);
+
+        if (aURL.getLength() == sal_Int32(aOldName.size() + 1)) // standard page name
+        {
+            aURL = aURL.replaceAt(1, aURL.getLength() - 1, u"") +
+                aNewName;
+            bURLChange = true;
+        }
+        else
+        {
+            const OUString sNotes(SdResId(STR_NOTES));
+            if (aURL.getLength() == sal_Int32(aOldName.size()) + 2 + sNotes.getLength()
+                && aURL.indexOf(sNotes, aOldName.size() + 2) == sal_Int32(aOldName.size() + 2))
+            {
+                aURL = aURL.replaceAt(1, aURL.getLength() - 1, u"") +
+                    aNewName + " " + sNotes;
+                bURLChange = true;
             }
         }
-    }
+
+        if(bURLChange)
+        {
+            SvxFieldItem* pNewFieldItem(pFieldItem->Clone(&rPool));
+            const_cast<SvxURLField*>(static_cast<const SvxURLField*>(pNewFieldItem->GetField()))->SetURL(aURL);
+            rData.setItem(std::unique_ptr<SfxPoolItem>(pNewFieldItem));
+        }
+
+        return true; // continue callbacks
+    });
 }
 
 void SdDrawDocument::UpdatePageRelativeURLs(SdPage const * pPage, sal_uInt16 nPos, sal_Int32 nIncrement)
@@ -312,60 +322,63 @@ void SdDrawDocument::UpdatePageRelativeURLs(SdPage const * pPage, sal_uInt16 nPo
     bool bNotes = (pPage->GetPageKind() == PageKind::Notes);
 
     SfxItemPool& rPool(GetPool());
-    ItemSurrogates aSurrogates;
-    rPool.GetItemSurrogates(aSurrogates, EE_FEATURE_FIELD);
-    for (const SfxPoolItem* pItem : aSurrogates)
+    rPool.iterateItemSurrogates(EE_FEATURE_FIELD, [&](SfxItemPool::SurrogateData& rData)
     {
-        const SvxFieldItem* pFldItem;
+        const SvxFieldItem* pFieldItem(static_cast<const SvxFieldItem*>(&rData.getItem()));
 
-        if ((pFldItem = dynamic_cast< const SvxFieldItem * > (pItem)) != nullptr)
-        {
-            SvxURLField* pURLField = const_cast< SvxURLField* >( dynamic_cast<const SvxURLField*>( pFldItem->GetField() ) );
+        if (nullptr == pFieldItem)
+            return true; // continue callbacks
 
-            if(pURLField)
-            {
-                OUString aURL = pURLField->GetURL();
+        const SvxURLField* pURLField(dynamic_cast<const SvxURLField*>(pFieldItem->GetField()));
 
-                if (!aURL.isEmpty() && (aURL[0] == 35))
-                {
-                    OUString aHashSlide = "#" + SdResId(STR_PAGE);
-
-                    if (aURL.startsWith(aHashSlide))
-                    {
-                        OUString aURLCopy = aURL;
-                        const OUString sNotes(SdResId(STR_NOTES));
-
-                        aURLCopy = aURLCopy.replaceAt(0, aHashSlide.getLength(), u"");
-
-                        bool bNotesLink = ( aURLCopy.getLength() >= sNotes.getLength() + 3
-                            && aURLCopy.endsWith(sNotes) );
-
-                        if (bNotesLink != bNotes)
-                            continue; // no compatible link and page
-
-                        if (bNotes)
-                            aURLCopy = aURLCopy.replaceAt(aURLCopy.getLength() - sNotes.getLength(), sNotes.getLength(), u"");
-
-                        sal_Int32 number = aURLCopy.toInt32();
-                        sal_uInt16 realPageNumber = (nPos + 1)/ 2;
-
-                        if ( number >= realPageNumber )
-                        {
-                            // update link page number
-                            number += nIncrement;
-                            aURL = aURL.replaceAt(aHashSlide.getLength() + 1, aURL.getLength() - aHashSlide.getLength() - 1, u"") +
-                                OUString::number(number);
-                            if (bNotes)
-                            {
-                                aURL += " " + sNotes;
-                            }
-                            pURLField->SetURL(aURL);
-                        }
-                    }
-                }
-            }
+        if (nullptr == pURLField)
+            return true; // continue callbacks
+
+        OUString aURL(pURLField->GetURL());
+
+        if (aURL.isEmpty() || (aURL[0] != 35))
+            return true; // continue callbacks
+
+        OUString aHashSlide = "#" + SdResId(STR_PAGE);
+
+        if (!aURL.startsWith(aHashSlide))
+            return true; // continue callbacks
+
+        OUString aURLCopy = aURL;
+        const OUString sNotes(SdResId(STR_NOTES));
+
+        aURLCopy = aURLCopy.replaceAt(0, aHashSlide.getLength(), u"");
+
+        bool bNotesLink = ( aURLCopy.getLength() >= sNotes.getLength() + 3
+            && aURLCopy.endsWith(sNotes) );
+
+        if (bNotesLink != bNotes)
+            return true; // no compatible link and page, continue callbacks
+
+        if (bNotes)
+            aURLCopy = aURLCopy.replaceAt(aURLCopy.getLength() - sNotes.getLength(), sNotes.getLength(), u"");
+
+        sal_Int32 number = aURLCopy.toInt32();
+        sal_uInt16 realPageNumber = (nPos + 1)/ 2;
+
+        if ( number < realPageNumber )
+            return true; // continue callbacks
+
+        // update link page number
+        number += nIncrement;
+        aURL = aURL.replaceAt(aHashSlide.getLength() + 1, aURL.getLength() - aHashSlide.getLength() - 1, u"") +
+            OUString::number(number);
+        if (bNotes)
+        {
+            aURL += " " + sNotes;
         }
-    }
+
+        SvxFieldItem* pNewFieldItem(pFieldItem->Clone(&rPool));
+        const_cast<SvxURLField*>(static_cast<const SvxURLField*>(pNewFieldItem->GetField()))->SetURL(aURL);
+        rData.setItem(std::unique_ptr<SfxPoolItem>(pNewFieldItem));
+
+        return true; // continue callbacks
+    });
 }
 
 // Move page