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, 24 insertions, 18 deletions

diff --git a/svx/source/unodraw/UnoNameItemTable.cxx b/svx/source/unodraw/UnoNameItemTable.cxx
index ef7f193c40d5..2612bbf9b67d 100644
--- a/svx/source/unodraw/UnoNameItemTable.cxx
+++ b/svx/source/unodraw/UnoNameItemTable.cxx
@@ -39,8 +39,8 @@ using namespace ::cppu;
 namespace
 {
     // We need to override operator== here and specifically bypass the assert
-    // in SfxPoolItem::operator== in order to make the FindItemSurrogate call
-    // in SvxUnoNameItemTable::hasByName safe.
+    // in SfxPoolItem::operator== in order to make the GetItemSurrogates call
+    // and comparing it's results in SvxUnoNameItemTable::hasByName safe.
     class SampleItem : public NameOrIndex
     {
     public:
@@ -183,13 +183,15 @@ void SAL_CALL SvxUnoNameItemTable::replaceByName( const OUString& aApiName, cons
     {
         SampleItem aSample(mnWhich, aName);
         ItemSurrogates aSurrogates;
-        mpModelPool->FindItemSurrogate(aSurrogates, mnWhich, aSample);
+        mpModelPool->GetItemSurrogates(aSurrogates, mnWhich);
+
         for (const SfxPoolItem* pNameOrIndex : aSurrogates)
-            if (isValid(static_cast<const NameOrIndex*>(pNameOrIndex)))
-            {
-                const_cast<SfxPoolItem*>(pNameOrIndex)->PutValue( aElement, mnMemberId );
-                bFound = true;
-            }
+            if (aSample == *pNameOrIndex)
+                if (isValid(static_cast<const NameOrIndex*>(pNameOrIndex)))
+                {
+                    const_cast<SfxPoolItem*>(pNameOrIndex)->PutValue( aElement, mnMemberId );
+                    bFound = true;
+                }
     }
 
     if( !bFound )
@@ -213,14 +215,16 @@ uno::Any SAL_CALL SvxUnoNameItemTable::getByName( const OUString& aApiName )
     {
         SampleItem aSample(mnWhich, aName);
         ItemSurrogates aSurrogates;
-        mpModelPool->FindItemSurrogate(aSurrogates, mnWhich, aSample);
+        mpModelPool->GetItemSurrogates(aSurrogates, mnWhich);
+
         for (const SfxPoolItem* pFindItem : aSurrogates)
-            if (isValid(static_cast<const NameOrIndex*>(pFindItem)))
-            {
-                uno::Any aAny;
-                pFindItem->QueryValue( aAny, mnMemberId );
-                return aAny;
-            }
+            if (aSample == *pFindItem)
+                if (isValid(static_cast<const NameOrIndex*>(pFindItem)))
+                {
+                    uno::Any aAny;
+                    pFindItem->QueryValue( aAny, mnMemberId );
+                    return aAny;
+                }
     }
 
     throw container::NoSuchElementException();
@@ -266,10 +270,12 @@ sal_Bool SAL_CALL SvxUnoNameItemTable::hasByName( const OUString& aApiName )
 
     SampleItem aSample(mnWhich, aName);
     ItemSurrogates aSurrogates;
-    mpModelPool->FindItemSurrogate(aSurrogates, mnWhich, aSample);
+    mpModelPool->GetItemSurrogates(aSurrogates, mnWhich);
+
     for (const SfxPoolItem* pFindItem : aSurrogates)
-        if (isValid(static_cast<const NameOrIndex*>(pFindItem)))
-            return true;
+        if (aSample == *pFindItem)
+            if (isValid(static_cast<const NameOrIndex*>(pFindItem)))
+                return true;
     return false;
 }