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, 113 insertions, 38 deletions

diff --git a/svl/source/items/itempool.cxx b/svl/source/items/itempool.cxx
index 341cbd3672ff..32b1c0499cf4 100644
--- a/svl/source/items/itempool.cxx
+++ b/svl/source/items/itempool.cxx
@@ -875,64 +875,139 @@ const SfxPoolItem *SfxItemPool::GetItem2Default(sal_uInt16 nWhich) const
     return (*mpStaticDefaults)[ GetIndex_Impl(nWhich) ];
 }
 
-void SfxItemPool::CollectSurrogates(std::unordered_set<const SfxPoolItem*>& rTarget, sal_uInt16 nWhich) const
+namespace
+{
+    class SurrogateData_ItemSet : public SfxItemPool::SurrogateData
+    {
+        const SfxPoolItem*  mpItem;
+        SfxItemSet*         mpSet;
+
+    public:
+        SurrogateData_ItemSet(const SfxPoolItem& rItem, SfxItemSet& rSet)
+        : SfxItemPool::SurrogateData()
+        , mpItem(&rItem)
+        , mpSet(&rSet)
+        {
+        }
+
+        SurrogateData_ItemSet(const SurrogateData_ItemSet&) = default;
+
+        virtual const SfxPoolItem& getItem() const override
+        {
+            return *mpItem;
+        }
+
+        virtual const SfxPoolItem* setItem(std::unique_ptr<SfxPoolItem> aNew) override
+        {
+            return mpSet->Put(std::unique_ptr<SfxPoolItem>(aNew.release()));
+        }
+    };
+
+    class SurrogateData_ItemHolder : public SfxItemPool::SurrogateData
+    {
+        SfxPoolItemHolder*  mpHolder;
+
+    public:
+        SurrogateData_ItemHolder(SfxPoolItemHolder& rHolder)
+        : SfxItemPool::SurrogateData()
+        , mpHolder(&rHolder)
+        {
+        }
+
+        SurrogateData_ItemHolder(const SurrogateData_ItemHolder&) = default;
+
+        virtual const SfxPoolItem& getItem() const override
+        {
+            return *mpHolder->getItem();
+        }
+
+        virtual const SfxPoolItem* setItem(std::unique_ptr<SfxPoolItem> aNew) override
+        {
+            *mpHolder = SfxPoolItemHolder(mpHolder->getPool(), aNew.release(), true);
+            return mpHolder->getItem();
+        }
+    };
+}
+
+void SfxItemPool::iterateItemSurrogates(
+    sal_uInt16 nWhich,
+    const std::function<bool(SurrogateData& rCand)>& rItemCallback) const
+{
+    // 1st source for surrogates
+    const registeredSfxItemSets& rSets(GetMasterPool()->maRegisteredSfxItemSets);
+
+    if(!rSets.empty())
+    {
+        const SfxPoolItem* pItem(nullptr);
+        std::vector<SurrogateData_ItemSet> aEntries;
+
+        // NOTE: this collects the callback data in a preparing run. This
+        //   is by purpose, else any write change may change the iterators
+        //   used at registeredSfxItemSets. I tied with direct feed and
+        //   that worked most of the time, but failed for ItemHolders due
+        //   to these being changed and being re-registered. I have avoided
+        //   this in SfxPoolItemHolder::operator=, but it's just a question
+        //   that in some scenario someone replaces an Item even with a
+        //   different type/WhichID that this will then break/crash
+        for (const auto& rCand : rSets)
+            if (SfxItemState::SET == rCand->GetItemState(nWhich, false, &pItem))
+                aEntries.emplace_back(*pItem, *rCand);
+
+        if (!aEntries.empty())
+            for (auto& rCand : aEntries)
+                if (!rItemCallback(rCand))
+                    return;
+    }
+
+    // 2nd source for surrogates
+    const registeredSfxPoolItemHolders& rHolders(GetMasterPool()->maRegisteredSfxPoolItemHolders);
+
+    if (!rHolders.empty())
+    {
+        std::vector<SurrogateData_ItemHolder> aEntries;
+
+        // NOTE: same as above, look there
+        for (auto& rCand : rHolders)
+            if (rCand->Which() == nWhich && nullptr != rCand->getItem())
+                aEntries.emplace_back(*rCand);
+
+        if (!aEntries.empty())
+            for (auto& rCand : aEntries)
+                if (!rItemCallback(rCand))
+                    return;
+    }
+}
+
+void SfxItemPool::GetItemSurrogates(ItemSurrogates& rTarget, sal_uInt16 nWhich) const
 {
     rTarget.clear();
 
     if (0 == nWhich)
         return;
 
+    // NOTE: This is pre-collected, in this case mainly to
+    //   remove all double listings of SfxPoolItems which can
+    //   of course be referenced multiple times in multiple
+    //   ItemSets/ItemHolders. It comes handy that
+    //   std::unordered_set does this by definition
+    std::unordered_set<const SfxPoolItem*> aNewSurrogates;
+
     // 1st source for surrogates
     const registeredSfxItemSets& rSets(GetMasterPool()->maRegisteredSfxItemSets);
     const SfxPoolItem* pItem(nullptr);
     for (const auto& rCand : rSets)
         if (SfxItemState::SET == rCand->GetItemState(nWhich, false, &pItem))
-            rTarget.insert(pItem);
+            aNewSurrogates.insert(pItem);
 
     // 2nd source for surrogates
     const registeredSfxPoolItemHolders& rHolders(GetMasterPool()->maRegisteredSfxPoolItemHolders);
     for (const auto& rCand : rHolders)
         if (rCand->Which() == nWhich && nullptr != rCand->getItem())
-            rTarget.insert(rCand->getItem());
-
-    // the 3rd source for surrogates is the list of direct put items
-    // but since these use SfxPoolItemHolder now they are automatically
-    // registered at 2nd source - IF NeedsSurrogateSupport is set. So
-    // as long as we have this DirectPutItem stuff, iterate here and
-    // warn if an Item was added
-    const directPutSfxPoolItemHolders& rDirects(GetMasterPool()->maDirectPutItems);
-#ifdef DBG_UTIL
-    const size_t aBefore(rTarget.size());
-#endif
-    for (const auto& rCand : rDirects)
-        if (rCand->Which() == nWhich && nullptr != rCand->getItem())
-            rTarget.insert(rCand->getItem());
-#ifdef DBG_UTIL
-    const size_t aAfter(rTarget.size());
-    if (aBefore != aAfter)
-    {
-        SAL_WARN("svl.items", "SfxItemPool: Found non-automatically registered Item for Surrogates in DirectPutItems (!)");
-    }
-#endif
-}
+            aNewSurrogates.insert(rCand->getItem());
 
-void SfxItemPool::GetItemSurrogates(ItemSurrogates& rTarget, sal_uInt16 nWhich) const
-{
-    std::unordered_set<const SfxPoolItem*> aNewSurrogates;
-    CollectSurrogates(aNewSurrogates, nWhich);
     rTarget = ItemSurrogates(aNewSurrogates.begin(), aNewSurrogates.end());
 }
 
-void SfxItemPool::FindItemSurrogate(ItemSurrogates& rTarget, sal_uInt16 nWhich, SfxPoolItem const & rSample) const
-{
-    std::unordered_set<const SfxPoolItem*> aNewSurrogates;
-    CollectSurrogates(aNewSurrogates, nWhich);
-    rTarget.clear();
-    for (const auto& rCand : aNewSurrogates)
-        if (rSample == *rCand)
-            rTarget.push_back(rCand);
-}
-
 sal_uInt16 SfxItemPool::GetWhich( sal_uInt16 nSlotId, bool bDeep ) const
 {
     if ( !IsSlot(nSlotId) )