ITEM: Solve SfxVoidItem(0) situation

An instance of SfxVoidItem(0) was used to signal
the SfxItemState::DISABLED. This was done not only
using WhichID == 0, but using isVoidItem() at the
SfxPoolItem. Unfortunately this mixes up with usages
of SfxVoidItems, mostly for UI stuff/Slots.
This also means that all the time an SfxVoidItem
had to be cloned/delete when when added/removed
from ItemSet or ItemHolder. Much more action than
e.g. for INVALID_POOL_ITEM which we already use
by havong just a simple ptr to a single static
instance of an Item. Disabled should do the same
thing.
Unfortunately also the functionality was mixed
with non-SfxItemState::DISABLED purposes and these
were very hard to be separated.
But the current solution works now after some
quirks doing that. It even oes no more need the
isVoidItem() flag at the SfxPoolItem.

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

3 files changed, 43 insertions, 77 deletions

diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 17862d3ea71b..a7db03c68932 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -293,6 +293,11 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
         // just use pSource which equals INVALID_POOL_ITEM
         return pSource;
 
+    if (IsDisabledItem(pSource))
+        // SfxItemState::DISABLED aka invalid item
+        // just use pSource which equals DISABLED_POOL_ITEM
+        return pSource;
+
     // CAUTION: static default items are not *that* static as it seems
     // (or: should be). If they are freed with the Pool (see
     // ::ReleaseDefaults) they will be deleted. Same is true for
@@ -306,7 +311,8 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
 
     if (0 == pSource->Which())
     {
-        // These *should* be SfxVoidItem(0) the only Items with 0 == WhichID,
+        // There should be no Items with 0 == WhichID, but there are some
+        // constructed for dialog return values AKA result (look for SetReturnValue)
         // these need to be cloned (currently...)
         if (bPassingOwnership)
             return pSource;
@@ -441,9 +447,15 @@ void implCleanupItemEntry(SfxPoolItem const* pSource)
         // nothing to do for invalid item entries
         return;
 
+    if (IsDisabledItem(pSource))
+        // SfxItemState::DISABLED aka invalid item
+        // just use pSource which equals DISABLED_POOL_ITEM
+        return;
+
     if (0 == pSource->Which())
     {
-        // These *should* be SfxVoidItem(0) the only Items with 0 == WhichID
+        // There should be no Items with 0 == WhichID, but there are some
+        // constructed for dialog return values AKA result (look for SetReturnValue)
         // and need to be deleted
         delete pSource;
         return;
@@ -612,8 +624,8 @@ void SfxItemSet::checkRemovePoolRegistration(const SfxPoolItem* pItem)
         // no Item, done
         return;
 
-    if (IsInvalidItem(pItem) || pItem->isVoidItem() || 0 == pItem->Which())
-        // checks IsInvalidItem/SfxVoidItem(0)
+    if (IsInvalidItem(pItem) || IsDisabledItem(pItem))
+        // checks IsInvalidItem/IsDisabledItem
         return;
 
     if (SfxItemPool::IsSlot(pItem->Which()))
@@ -641,8 +653,8 @@ void SfxItemSet::checkAddPoolRegistration(const SfxPoolItem* pItem)
         // no Item, done
         return;
 
-    if (IsInvalidItem(pItem) || pItem->isVoidItem() || 0 == pItem->Which())
-        // checks IsInvalidItem/SfxVoidItem(0)
+    if (IsInvalidItem(pItem) || IsDisabledItem(pItem))
+        // checks IsInvalidItem/IsDisabledItem
         return;
 
     if (SfxItemPool::IsSlot(pItem->Which()))
@@ -669,7 +681,7 @@ sal_uInt16 SfxItemSet::ClearSingleItem_ForOffset( sal_uInt16 nOffset )
 {
     assert(nOffset < TotalCount());
     const_iterator aEntry(begin() + nOffset);
-    assert(nullptr == *aEntry || IsInvalidItem(*aEntry) || (*aEntry)->isVoidItem() || 0 != (*aEntry)->Which());
+    assert(nullptr == *aEntry || IsInvalidItem(*aEntry) || IsDisabledItem(*aEntry) || 0 != (*aEntry)->Which());
 
     if (nullptr == *aEntry)
         // no entry, done
@@ -789,7 +801,7 @@ SfxItemState SfxItemSet::GetItemState_ForOffset( sal_uInt16 nOffset, const SfxPo
         // Different ones are present
         return SfxItemState::DONTCARE;
 
-    if (pCandidate->isVoidItem())
+    if (IsDisabledItem(pCandidate))
         // Item is Disabled
         return SfxItemState::DISABLED;
 
@@ -840,10 +852,9 @@ const SfxPoolItem* SfxItemSet::PutImplAsTargetWhich(const SfxPoolItem& rItem, sa
 
 const SfxPoolItem* SfxItemSet::PutImpl(const SfxPoolItem& rItem, bool bPassingOwnership)
 {
-    if (0 == rItem.Which())
+    if (IsDisabledItem(&rItem))
     {
-        // no action needed: this *should* be SfxVoidItem(0) the only Items
-        // with 0 == WhichID -> only to be used by SfxItemSet::DisableItem
+        // no action needed: IsDisabledItem
         if (bPassingOwnership)
             delete &rItem;
         return nullptr;
@@ -933,7 +944,7 @@ bool SfxItemSet::Put(const SfxItemSet& rSource, bool bInvalidAsDefault)
                     }
                     else
                     {
-                        InvalidateItem_ForWhichID(nWhich);
+                        DisableOrInvalidateItem_ForWhichID(false, nWhich);
                     }
                 }
                 else
@@ -998,7 +1009,7 @@ void SfxItemSet::PutExtended
                             break;
 
                         case SfxItemState::DONTCARE:
-                            InvalidateItem_ForWhichID(nWhich);
+                            DisableOrInvalidateItem_ForWhichID(false, nWhich);
                             break;
 
                         default:
@@ -1025,7 +1036,7 @@ void SfxItemSet::PutExtended
                         break;
 
                     case SfxItemState::DONTCARE:
-                        InvalidateItem_ForWhichID(nWhich);
+                        DisableOrInvalidateItem_ForWhichID(false, nWhich);
                         break;
 
                     default:
@@ -1264,7 +1275,7 @@ const SfxPoolItem& SfxItemSet::Get( sal_uInt16 nWhich, bool bSrchInParent) const
                 return GetPool()->GetDefaultItem(nWhich);
             }
 #ifdef DBG_UTIL
-            if ((*aFoundOne)->isVoidItem())
+            if (IsDisabledItem(*aFoundOne))
                 SAL_INFO("svl.items", "SFX_WARNING: Getting disabled Item");
 #endif
             return **aFoundOne;
@@ -1598,8 +1609,8 @@ void SfxItemSet::MergeValues( const SfxItemSet& rSet )
 
 void SfxItemSet::MergeValue(const SfxPoolItem& rAttr, bool bIgnoreDefaults)
 {
-    if (0 == rAttr.Which())
-        // seems to be SfxVoidItem(0), nothing to do
+    if (IsDisabledItem(&rAttr))
+        // is IsDisabledItem, nothing to do
         return;
 
     const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(rAttr.Which()));
@@ -1610,17 +1621,17 @@ void SfxItemSet::MergeValue(const SfxPoolItem& rAttr, bool bIgnoreDefaults)
     }
 }
 
-void SfxItemSet::InvalidateItem_ForWhichID(sal_uInt16 nWhich)
+void SfxItemSet::DisableOrInvalidateItem_ForWhichID(bool bDisable, sal_uInt16 nWhich)
 {
     const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(nWhich));
 
     if (INVALID_WHICHPAIR_OFFSET != nOffset)
     {
-        InvalidateItem_ForOffset(nOffset);
+        DisableOrInvalidateItem_ForOffset(bDisable, nOffset);
     }
 }
 
-void SfxItemSet::InvalidateItem_ForOffset(sal_uInt16 nOffset)
+void SfxItemSet::DisableOrInvalidateItem_ForOffset(bool bDisable, sal_uInt16 nOffset)
 {
     // check and assert from invalid offset. The caller is responsible for
     // ensuring a valid offset (see callers, all checked & safe)
@@ -1634,65 +1645,22 @@ void SfxItemSet::InvalidateItem_ForOffset(sal_uInt16 nOffset)
     }
     else
     {
-        // entry is set
-        if (IsInvalidItem(*aFoundOne))
-            // already invalid item, done
+        if (bDisable && IsDisabledItem(*aFoundOne))
+            // already disabled item, done
             return;
 
-        // cleanup entry
-        checkRemovePoolRegistration(*aFoundOne);
-        implCleanupItemEntry(*aFoundOne);
-    }
-
-    // set new entry
-    *aFoundOne = INVALID_POOL_ITEM;
-}
-
-void SfxItemSet::DisableItem_ForWhichID(sal_uInt16 nWhich)
-{
-    const sal_uInt16 nOffset(GetRanges().getOffsetFromWhich(nWhich));
-
-    if (INVALID_WHICHPAIR_OFFSET != nOffset)
-    {
-        DisableItem_ForOffset(nOffset);
-    }
-}
-
-void SfxItemSet::DisableItem_ForOffset(sal_uInt16 nOffset)
-{
-    // check and assert from invalid offset. The caller is responsible for
-    // ensuring a valid offset (see callers, all checked & safe)
-    assert(nOffset < TotalCount());
-    const_iterator aFoundOne(begin() + nOffset);
-
-    if (nullptr == *aFoundOne)
-    {
-        // entry goes from nullptr -> invalid
-        ++m_nCount;
-    }
-    else
-    {
-        // entry is set
-        if ((*aFoundOne)->isVoidItem() && 0 == (*aFoundOne)->Which())
+        if (!bDisable && IsInvalidItem(*aFoundOne))
             // already invalid item, done
             return;
-    }
 
-    // prepare new entry
-    const SfxPoolItem* pNew(implCreateItemEntry(*GetPool(), new SfxVoidItem(0), true));
 
-    // Notification-Callback
-    if(m_aCallback)
-    {
-        m_aCallback(*aFoundOne, pNew);
+        // cleanup entry
+        checkRemovePoolRegistration(*aFoundOne);
+        implCleanupItemEntry(*aFoundOne);
     }
 
-    // cleanup entry (remove only)
-    checkRemovePoolRegistration(*aFoundOne);
-    implCleanupItemEntry(*aFoundOne);
-
     // set new entry
-    *aFoundOne = pNew;
+    *aFoundOne = bDisable ? DISABLED_POOL_ITEM : INVALID_POOL_ITEM;
 }
 
 sal_uInt16 SfxItemSet::GetWhichByOffset( sal_uInt16 nOffset ) const
diff --git a/svl/source/items/poolitem.cxx b/svl/source/items/poolitem.cxx
index 3b0ebd6fa882..60de354cdeba 100644
--- a/svl/source/items/poolitem.cxx
+++ b/svl/source/items/poolitem.cxx
@@ -494,7 +494,6 @@ SfxPoolItem::SfxPoolItem(sal_uInt16 const nWhich)
 #ifdef DBG_UTIL
     , m_nSerialNumber(nUsedSfxPoolItemCount)
 #endif
-    , m_bIsVoidItem(false)
     , m_bStaticDefault(false)
     , m_bPoolDefault(false)
     , m_bIsSetItem(false)
@@ -642,7 +641,7 @@ bool SfxPoolItem::areSame(const SfxPoolItem* pItem1, const SfxPoolItem* pItem2)
 {
     if (pItem1 == pItem2)
         // pointer compare, this handles already
-        // nullptr, INVALID_POOL_ITEM, SfxVoidItem
+        // nullptr, INVALID_POOL_ITEM, DISABLED_POOL_ITEM
         // and if any Item is indeed handed over twice
         return true;
 
@@ -689,14 +688,16 @@ bool SfxPoolItem::areSame(const SfxPoolItem& rItem1, const SfxPoolItem& rItem2)
 
 namespace
 {
-class InvalidItem final : public SfxPoolItem
+class InvalidOrDisabledItem final : public SfxPoolItem
 {
     virtual bool operator==(const SfxPoolItem&) const override { return true; }
     virtual SfxPoolItem* Clone(SfxItemPool*) const override { return nullptr; }
 };
-InvalidItem aInvalidItem;
+InvalidOrDisabledItem aInvalidItem;
+InvalidOrDisabledItem aDisabledItem;
 }
 
 SfxPoolItem const* const INVALID_POOL_ITEM = &aInvalidItem;
+SfxPoolItem const* const DISABLED_POOL_ITEM = &aDisabledItem;
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/svl/source/items/voiditem.cxx b/svl/source/items/voiditem.cxx
index 32057e1e2c75..2091359e7bb5 100644
--- a/svl/source/items/voiditem.cxx
+++ b/svl/source/items/voiditem.cxx
@@ -25,19 +25,16 @@ SfxPoolItem* SfxVoidItem::CreateDefault() { return new SfxVoidItem(0); }
 SfxVoidItem::SfxVoidItem(sal_uInt16 which)
     : SfxPoolItem(which)
 {
-    setIsVoidItem();
 }
 
 SfxVoidItem::SfxVoidItem(const SfxVoidItem& rCopy)
     : SfxPoolItem(rCopy.Which())
 {
-    setIsVoidItem();
 }
 
 SfxVoidItem::SfxVoidItem(SfxVoidItem&& rOrig)
     : SfxPoolItem(rOrig)
 {
-    setIsVoidItem();
 }
 
 bool SfxVoidItem::operator==(const SfxPoolItem& rCmp) const