tdf#164745 fix SfxStateCache

Seems I did too much with tdf#162666 by also trying to
remember invalid and disabled states in pLastItem, so
went over it again. Also corrected SetVisibleState to
also take action when disabled item.

Not urgently necessary, but stumbled over it: The items
used for INVALID_POOL_ITEM and DISABLED_POOL_ITEM were
based on the same class. Not a problem technically,
these are 'special' static items that represent a state,
but comparing them would have claimed these to be equal
what should not be the case. Thus I based each of these
on an own derivation of SfxPoolItem.

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

3 files changed, 34 insertions, 36 deletions

diff --git a/include/svl/poolitem.hxx b/include/svl/poolitem.hxx
index 6d0105707077..440d152de9a0 100644
--- a/include/svl/poolitem.hxx
+++ b/include/svl/poolitem.hxx
@@ -122,8 +122,9 @@ enum class SfxItemType : sal_uInt16
     CntUInt32ItemType,
     DatabaseMapItemType,
     DbuTypeCollectionItemType,
+    DisabledItemType,
     DriverPoolingSettingsItemType,
-    InvalidOrDisabledItemType,
+    InvalidItemType,
     MediaItemType,
     NameOrIndexType,
     OfaPtrItemType,
@@ -562,6 +563,7 @@ class SfxItemPool;
 typedef struct _xmlTextWriter* xmlTextWriterPtr;
 class ItemInstanceManager;
 
+// macro to define overloaded ItemType function, see explanations below
 #define DECLARE_ITEM_TYPE_FUNCTION(T) \
     virtual SfxItemType ItemType() const override { return SfxItemType::T##Type; }
 
diff --git a/sfx2/source/control/statcach.cxx b/sfx2/source/control/statcach.cxx
index 4432f0bc37e6..34c2709af855 100644
--- a/sfx2/source/control/statcach.cxx
+++ b/sfx2/source/control/statcach.cxx
@@ -361,7 +361,8 @@ void SfxStateCache::SetVisibleState( bool bShow )
     bItemVisible = bShow;
     if ( bShow )
     {
-        if ( IsInvalidItem(pLastItem) || ( pLastItem == nullptr ))
+        // tdf#164745 also need to do this when disabled item
+        if ( nullptr == pLastItem || IsInvalidItem(pLastItem) || IsDisabledItem(pLastItem) )
         {
             pState = new SfxVoidItem( nId );
             bDeleteItem = true;
@@ -431,39 +432,17 @@ void SfxStateCache::SetState_Impl
             static_cast<SfxDispatchController_Impl *>(pInternalController)->StateChanged( nId, eState, pState, &aSlotServ );
 
         // Remember new value
-        if (!IsInvalidItem(pLastItem) && !IsDisabledItem(pLastItem))
+        if ( !IsInvalidItem(pLastItem) && !IsDisabledItem(pLastItem) )
         {
-            // tdf#162666 only delete if it *was* cloned
             delete pLastItem;
+            pLastItem = nullptr;
         }
 
-        // always reset to nullptr
-        pLastItem = nullptr;
-
-        if ( nullptr != pState)
-        {
-            if (IsInvalidItem(pState))
-            {
-                // tdf#162666 if invalid, use INVALID_POOL_ITEM
-                pLastItem = INVALID_POOL_ITEM;
-            }
-            else if (IsDisabledItem(pState))
-            {
-                // tdf#162666 if disabled, use DISABLED_POOL_ITEM
-                pLastItem = DISABLED_POOL_ITEM;
-            }
-            else
-            {
-                // tdf#162666 in all other cases, clone the Item. Note that
-                // due to not using a Pool here it is not possible to use a
-                // ItemSet or a ItemHolder, those would automatically handle
-                // these cases correctly. In this cache, this currently needs
-                // to be done handish. Also note that this may break again
-                // when changes in the Item/ItemSet/ItemHolder mechanism may
-                // be done
-                pLastItem = pState->Clone();
-            }
-        }
+        // tdf#164745 clone when it exists and it's really an item
+        if ( pState && !IsInvalidItem(pState) && !IsDisabledItem(pState) )
+            pLastItem = pState->Clone();
+        else
+            pLastItem = nullptr;
 
         eLastState = eState;
         bItemDirty = false;
diff --git a/svl/source/items/poolitem.cxx b/svl/source/items/poolitem.cxx
index 2aa31be8db24..917a5e425a18 100644
--- a/svl/source/items/poolitem.cxx
+++ b/svl/source/items/poolitem.cxx
@@ -293,7 +293,9 @@ bool SfxPoolItem::areSame(const SfxPoolItem& rItem1, const SfxPoolItem& rItem2)
 
 namespace
 {
-class InvalidOrDisabledItem final : public SfxPoolItem
+// tdf#164745 make InvalidItem and DisabledItem two classes to
+// avoi d that op== sees them as equal
+class InvalidItem final : public SfxPoolItem
 {
     virtual bool operator==(const SfxPoolItem&) const override { return true; }
     virtual SfxPoolItem* Clone(SfxItemPool*) const override { return nullptr; }
@@ -301,15 +303,30 @@ class InvalidOrDisabledItem final : public SfxPoolItem
 public:
     // make it StaticDefaultItem to process similar to these
     // which is plausible (never change and are not allowed to)
-    DECLARE_ITEM_TYPE_FUNCTION(InvalidOrDisabledItem)
-    InvalidOrDisabledItem()
+    DECLARE_ITEM_TYPE_FUNCTION(InvalidItem)
+    InvalidItem()
         : SfxPoolItem(0)
     {
         setStaticDefault();
     }
 };
-InvalidOrDisabledItem aInvalidItem;
-InvalidOrDisabledItem aDisabledItem;
+class DisabledItem final : public SfxPoolItem
+{
+    virtual bool operator==(const SfxPoolItem&) const override { return true; }
+    virtual SfxPoolItem* Clone(SfxItemPool*) const override { return nullptr; }
+
+public:
+    // make it StaticDefaultItem to process similar to these
+    // which is plausible (never change and are not allowed to)
+    DECLARE_ITEM_TYPE_FUNCTION(DisabledItem)
+    DisabledItem()
+        : SfxPoolItem(0)
+    {
+        setStaticDefault();
+    }
+};
+InvalidItem aInvalidItem;
+DisabledItem aDisabledItem;
 }
 
 SfxPoolItem const* const INVALID_POOL_ITEM = &aInvalidItem;