Cleanup some ScPatternAttr specific stuff

Removed some special stuff in the ItemSet/Item tooling regarding
ScPatternAttr. There are still quite a view (grep for
isExceptionalSCItem), but all these are identified and isolated.

Only for that Item (of over 500) are these exceptions, so that raelly
does not fit into the Item schemata in any way. Not even the pool
default of ScPatternAttr is without trouble. It is the only and last
Item that needs to be 'pooled' in the sense to avoid copies on any
price. That is OK, but should not be done in the ItemSet schemata.

In short: It should not be an Item at all. It should be changed
and renamed to something describing it's role (CellAttributes..?)
and get helped/assisted by something called CellAttributeHelper at
SC's model or Pool. It should not even be derived from SetItem, it
could just contain an shared_ptr of SfxItemSet (allows more and better
optimizations - think about Clone() and op==).

In parallel, all these hacks in the ItemSet/Item stuff could be
removed, making that faster and easier. Also quite some usages of
DirectPutItemInPool could be cleaned-up, this only exists since
there *is* no defined place to hold that data (coud be
CellAttributeHelper in the future). Putting Items directly to the
pool (and in most cases never remove them again) is just nonsense
and another hint that all this does not fit to the Item/ItemSet
schema at all.

This is now - after hard isolation of problems and have all working -
doable. It may be one of the next things to do, but there are
other candidates, too. Doing this would mostly only help SC...

Found another hack that uses DirectPutItemInPool and *never* removes
any Item again, see comments framelinkarray.cxx

And another one: PoolItemTest::testPool() explicitely tests
DirectPutItemInPool stuff - which makes no sense long-term,
but for now keep it and make it work by marking the slots used
as _bNeedsPoolRegistration == true

Have now overhauled the framelinkarray stuff to work without
DirectPutItemInPool and Cell not being a SfxPoolItem. That will
be much less complex and use much less calls/checks. Since this
is the data structure created for every calc repaint that should
get faster, too.

Also for now and memory loss security I added code to
DirectPutItemInPool to behave the same as with the unchanged
implCreateItemEntry: register Items so that the garbage collection
still is used. This will/can be removed when all usages of
DirectPutItemInPool is cleaned up.

Directly registering in DirectPutItemInPoolImpl is more tricky
than thought, but a good thing to do. Looking at that I saw that
tryRegisterSfxPoolItem is not used anymore, so rearranged some stuff.

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

4 files changed, 34 insertions, 65 deletions

diff --git a/svl/qa/unit/items/test_itempool.cxx b/svl/qa/unit/items/test_itempool.cxx
index c8e313bb61e5..2cb751d4fd77 100644
--- a/svl/qa/unit/items/test_itempool.cxx
+++ b/svl/qa/unit/items/test_itempool.cxx
@@ -37,7 +37,7 @@ void PoolItemTest::testPool()
     SfxItemInfo const aItems[] =
     {
         // _nSID, _bNeedsPoolRegistration, _bShareable
-        { 4, false, true },
+        { 4, true,  true },
         { 3, true,  false /* test NeedsPoolRegistration */ },
         { 2, false, false },
         { 1, true,  false /* test NeedsPoolRegistration */}
diff --git a/svl/source/items/itempool.cxx b/svl/source/items/itempool.cxx
index 0d3275471eac..f119bc85304f 100644
--- a/svl/source/items/itempool.cxx
+++ b/svl/source/items/itempool.cxx
@@ -801,8 +801,19 @@ const SfxPoolItem& SfxItemPool::DirectPutItemInPoolImpl(const SfxPoolItem& rItem
     nRemainingDirectlyPooledSfxPoolItemCount++;
 #endif
 
+    // CAUTION: Do not register the problematic pool default
+    if (rItem.isExceptionalSCItem() && GetMasterPool()->newItem_UseDirect(rItem))
+        return rItem;
+
     // make sure to use 'master'-pool, that's the one used by SfxItemSets
-    return *implCreateItemEntry(*GetMasterPool(), &rItem, nWhich, bPassingOwnership, true);
+    const SfxPoolItem* pRetval(implCreateItemEntry(*GetMasterPool(), &rItem, nWhich, bPassingOwnership));
+
+    // For the moment, as long as DirectPutItemInPoolImpl is used, make sure that
+    // the changes in implCreateItemEntry do not change anything, that would
+    // risc memory leaks by not (ab)using the garbage collector aspect of the pool.
+    registerSfxPoolItem(*pRetval);
+
+    return *pRetval;
 }
 
 void SfxItemPool::DirectRemoveItemFromPool(const SfxPoolItem& rItem)
@@ -1058,7 +1069,7 @@ sal_uInt16 SfxItemPool::GetTrueSlotId( sal_uInt16 nWhich ) const
     return pItemInfos[nWhich - pImpl->mnStart]._nSID;
 }
 
-void SfxItemPool::tryRegisterSfxPoolItem(const SfxPoolItem& rItem, bool bPoolDirect)
+void SfxItemPool::registerSfxPoolItem(const SfxPoolItem& rItem)
 {
     assert(rItem.Which() != 0);
 
@@ -1067,7 +1078,7 @@ void SfxItemPool::tryRegisterSfxPoolItem(const SfxPoolItem& rItem, bool bPoolDir
         return;
 
     if (rItem.isRegisteredAtPool())
-        // already registered, done (should not happen)
+        // already registered, done
         return;
 
     if (!IsInRange(rItem.Which()))
@@ -1075,49 +1086,13 @@ void SfxItemPool::tryRegisterSfxPoolItem(const SfxPoolItem& rItem, bool bPoolDir
         // get to the right pool
         if (pImpl->mpSecondary)
         {
-            pImpl->mpSecondary->tryRegisterSfxPoolItem(rItem, bPoolDirect);
+            pImpl->mpSecondary->registerSfxPoolItem(rItem);
             return;
         }
 
         return;
     }
 
-    // get index (must exist due to checks above)
-    const sal_uInt16 nIndex(rItem.Which() - pImpl->mnStart);
-    bool bRegisterItem(bPoolDirect);
-
-    if (!bRegisterItem)
-    {
-        if (g_bItemClassicMode)
-        {
-            // classic mode: register in general *all* items
-            // ...but only shareable ones: for non-shareable registering for re-use
-            // makes no sense
-            bRegisterItem = Shareable_Impl(nIndex);
-        }
-        else
-        {
-            // experimental mode: only register items that are defined to be registered
-            bRegisterItem = NeedsPoolRegistration_Impl(nIndex);
-        }
-    }
-
-    if (bRegisterItem)
-        doRegisterSfxPoolItem(rItem);
-}
-
-void SfxItemPool::doRegisterSfxPoolItem(const SfxPoolItem& rItem)
-{
-    assert(rItem.Which() != 0);
-
-    if (IsSlot(rItem.Which()))
-        // do not register SlotItems
-        return;
-
-    if (rItem.isRegisteredAtPool())
-        // already registered, done
-        return;
-
     if (nullptr == ppRegisteredSfxPoolItems)
         // on-demand allocate array of registeredSfxPoolItems and init to nullptr
         ppRegisteredSfxPoolItems = new registeredSfxPoolItems*[GetSize_Impl()]{};
diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 6cf4e706e17d..abb264422828 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -128,7 +128,7 @@ SfxItemSet::SfxItemSet(SfxItemPool& pool, WhichRangesContainer wids)
     assert(svl::detail::validRanges2(m_pWhichRanges));
 }
 
-SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource, sal_uInt16 nWhich, bool bPassingOwnership, bool bPoolDirect)
+SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource, sal_uInt16 nWhich, bool bPassingOwnership)
 {
     if (nullptr == pSource)
         // SfxItemState::UNKNOWN aka current default (nullptr)
@@ -140,7 +140,7 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
         // just use pSource which equals INVALID_POOL_ITEM
         return pSource;
 
-    if (pSource->isNewItemCallback() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
+    if (pSource->isExceptionalSCItem() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
         // exceptional handling for *some* items, see SC
         // (do not copy item: use directly, it is a pool default)
         return pSource;
@@ -240,22 +240,14 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
         return pSource;
     }
 
-    // classic mode: try finding already existing item
-    // NOTE: bPoolDirect currently required due to DirectPutItemInPool and the
-    //   self-handled Item ScPatternAttr/ATTR_PATTERN in SC, else e.g.
-    //   testIteratorsDefPattern will fail in line 1306
+    // g_bItemClassicMode: try finding already existing item
     // NOTE: the UnitTest testIteratorsDefPattern claims that that Item "can be
     //   edited by the user" which explains why it breaks so many rules for Items,
     //   it behaves like an alien. That Item in the SC Pool claims to be a
     //   'StaticDefault' and gets changed (..?)
-    // NOTE: despite 1st thinking that this can be limited to ScPatternAttr/
-    //   ATTR_PATTERN it also has to be applied to the range
-    //   [ATTR_PATTERN_START, ATTR_PATTERN_END] *used* by ATTR_PATTERN, plus
-    //   it, so it's [100 .. 155] and [156] in SC. For now, just use bPoolDirect.
-    //   This needs to be cleaned-up somehow anyways
 
     // only do this if classic mode or required (calls from Pool::Direct*)
-    while(g_bItemClassicMode || bPoolDirect)
+    while(g_bItemClassicMode || pSource->isExceptionalSCItem())
     {
         if (!pTargetPool->Shareable_Impl(nIndex))
             // not shareable, so no need to search for identical item
@@ -308,29 +300,31 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
 
     // Unfortunately e,g, SC does 'special' things for some new Items,
     // so we need to give the opportunity for this. To limit this to
-    // the needed cases, use m_bNewItemCallback flag at item
-    if (pSource->isNewItemCallback())
+    // the needed cases, use m_bExceptionalSCItem flag at item
+    if (pSource->isExceptionalSCItem())
         pMasterPool->newItem_Callback(*pSource);
 
     // try to register @Pool (only needed if not yet registered)
     if (!pSource->isRegisteredAtPool())
     {
-        if (!bPoolDirect) // re-use bPoolDirect
+        bool bRegisterAtPool(pSource->isExceptionalSCItem());
+
+        if (!bRegisterAtPool)
         {
             if (g_bItemClassicMode)
             {
                 // in classic mode register only/all shareable items
-                bPoolDirect = pTargetPool->Shareable_Impl(nIndex);
+                bRegisterAtPool = pTargetPool->Shareable_Impl(nIndex);
             }
             else
             {
                 // in new mode register only/all items marked as need to be registered
-                bPoolDirect = pTargetPool->NeedsPoolRegistration_Impl(nIndex);
+                bRegisterAtPool = pTargetPool->NeedsPoolRegistration_Impl(nIndex);
             }
         }
 
-        if (bPoolDirect)
-            pTargetPool->doRegisterSfxPoolItem(*pSource);
+        if (bRegisterAtPool)
+            pTargetPool->registerSfxPoolItem(*pSource);
     }
 
     return pSource;
@@ -346,7 +340,7 @@ void implCleanupItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource)
         // nothing to do for invalid item entries
         return;
 
-    if (pSource->isNewItemCallback() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
+    if (pSource->isExceptionalSCItem() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
         // exceptional handling for *some* items, see SC
         // do not delete Item, it is a pool default
         return;
@@ -421,7 +415,7 @@ SfxItemSet::SfxItemSet( const SfxItemSet& rASet )
 
     for (const auto& rSource : rASet)
     {
-        *ppDst = implCreateItemEntry(*GetPool(), rSource, 0, false, false);
+        *ppDst = implCreateItemEntry(*GetPool(), rSource, 0, false);
         ppDst++;
     }
 
@@ -702,7 +696,7 @@ const SfxPoolItem* SfxItemSet::PutImpl(const SfxPoolItem& rItem, sal_uInt16 nWhi
     }
 
     // prepare new entry
-    SfxPoolItem const* pNew(implCreateItemEntry(*GetPool(), &rItem, nWhich, bPassingOwnership, false));
+    SfxPoolItem const* pNew(implCreateItemEntry(*GetPool(), &rItem, nWhich, bPassingOwnership));
 
     // Notification-Callback
     if(m_aCallback)
@@ -1311,7 +1305,7 @@ void SfxItemSet::MergeItem_Impl(const SfxPoolItem **ppFnd1, const SfxPoolItem *p
 
         else if ( pFnd2 && bIgnoreDefaults )
             // Decision table: default, set, doesn't matter, sal_True
-            *ppFnd1 = implCreateItemEntry(*GetPool(), pFnd2, 0, false, false);
+            *ppFnd1 = implCreateItemEntry(*GetPool(), pFnd2, 0, false);
             // *ppFnd1 = &GetPool()->Put( *pFnd2 );
 
         if ( *ppFnd1 )
diff --git a/svl/source/items/poolitem.cxx b/svl/source/items/poolitem.cxx
index 49990079367c..bf86b4ab740e 100644
--- a/svl/source/items/poolitem.cxx
+++ b/svl/source/items/poolitem.cxx
@@ -499,7 +499,7 @@ SfxPoolItem::SfxPoolItem(sal_uInt16 const nWhich)
     , m_bStaticDefault(false)
     , m_bPoolDefault(false)
     , m_bRegisteredAtPool(false)
-    , m_bNewItemCallback(false)
+    , m_bExceptionalSCItem(false)
     , m_bIsSetItem(false)
 #ifdef DBG_UTIL
     , m_bDeleted(false)