tdf#95295: don't add duplicate conditional formats

This tries to make sure that on copy, any conditional formatting
that has same entries (conditions and formats) as another condition
in the table, is not appended to the conditions list, but updates
existing condition's range. This should prevent multiplication of
entries with duplicating conditions and formats, and fragmenting
ranges.

Existing unit tests had to be adjusted, because they had ensured
exactly that behaviour that is changed in this commit: that new
items are added to the list, and e.g. multiple elements are created
when a single cell is copied to a range.

Change-Id: I46adb883bab7249571b912d56fea0c7918b3516d
Reviewed-on: https://gerrit.libreoffice.org/45656
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Eike Rathke <erack@redhat.com>

3 files changed, 58 insertions, 46 deletions

diff --git a/sc/source/core/data/conditio.cxx b/sc/source/core/data/conditio.cxx
index 7bb41c29575d..eae43e7049c8 100644
--- a/sc/source/core/data/conditio.cxx
+++ b/sc/source/core/data/conditio.cxx
@@ -57,20 +57,15 @@ ScFormatEntry::ScFormatEntry(ScDocument* pDoc):
 
 bool ScFormatEntry::operator==( const ScFormatEntry& r ) const
 {
-    if(GetType() != r.GetType())
-        return false;
+    return IsEqual(r, false);
+}
 
-    switch(GetType())
-    {
-        case Type::Condition:
-            return static_cast<const ScCondFormatEntry&>(*this) == static_cast<const ScCondFormatEntry&>(r);
-        default:
-            // TODO: implement also this case
-            // actually return false for these cases is not that bad
-            // as soon as databar and color scale are tested we need
-            // to think about the range
-            return false;
-    }
+// virtual
+bool ScFormatEntry::IsEqual( const ScFormatEntry& /*r*/, bool /*bIgnoreSrcPos*/ ) const
+{
+    // By default, return false; this makes sense for all cases except ScConditionEntry
+    // As soon as databar and color scale are tested we need to think about the range
+    return false;
 }
 
 void ScFormatEntry::startRendering()
@@ -628,43 +623,31 @@ static bool lcl_IsEqual( const ScTokenArray* pArr1, const ScTokenArray* pArr2 )
         return !pArr1 && !pArr2; // Both 0? -> the same
 }
 
-bool ScConditionEntry::operator== ( const ScConditionEntry& r ) const
+// virtual
+bool ScConditionEntry::IsEqual( const ScFormatEntry& rOther, bool bIgnoreSrcPos ) const
 {
+    if (GetType() != rOther.GetType())
+        return false;
+
+    const ScCondFormatEntry& r = static_cast<const ScCondFormatEntry&>(rOther);
+
     bool bEq = (eOp == r.eOp && nOptions == r.nOptions &&
                 lcl_IsEqual( pFormula1, r.pFormula1 ) &&
                 lcl_IsEqual( pFormula2, r.pFormula2 ));
-    if (bEq)
+
+    if (!bIgnoreSrcPos)
     {
         // for formulas, the reference positions must be compared, too
         // (including aSrcString, for inserting the entries during XML import)
-        if ( ( pFormula1 || pFormula2 ) && ( aSrcPos != r.aSrcPos || aSrcString != r.aSrcString ) )
-            bEq = false;
-
-        // If not formulas, compare values
-        if ( !pFormula1 && ( nVal1 != r.nVal1 || aStrVal1 != r.aStrVal1 || bIsStr1 != r.bIsStr1 ) )
-            bEq = false;
-        if ( !pFormula2 && ( nVal2 != r.nVal2 || aStrVal2 != r.aStrVal2 || bIsStr2 != r.bIsStr2 ) )
+        if ( bEq && ( pFormula1 || pFormula2 ) && ( aSrcPos != r.aSrcPos || aSrcString != r.aSrcString ) )
             bEq = false;
     }
 
-    return bEq;
-}
-
-bool ScConditionEntry::EqualIgnoringSrcPos( const ScConditionEntry& r ) const
-{
-    bool bEq = (eOp == r.eOp && nOptions == r.nOptions &&
-                lcl_IsEqual( pFormula1, r.pFormula1 ) &&
-                lcl_IsEqual( pFormula2, r.pFormula2 ));
-    if (bEq)
-    {
-        // Here, ignore the aSrcPoses and aSrcStrings
-
-        // If not formulas, compare values
-        if ( !pFormula1 && ( nVal1 != r.nVal1 || aStrVal1 != r.aStrVal1 || bIsStr1 != r.bIsStr1 ) )
-            bEq = false;
-        if ( !pFormula2 && ( nVal2 != r.nVal2 || aStrVal2 != r.aStrVal2 || bIsStr2 != r.bIsStr2 ) )
-            bEq = false;
-    }
+    // If not formulas, compare values
+    if ( bEq && !pFormula1 && ( nVal1 != r.nVal1 || aStrVal1 != r.aStrVal1 || bIsStr1 != r.bIsStr1 ) )
+        bEq = false;
+    if ( bEq && !pFormula2 && ( nVal2 != r.nVal2 || aStrVal2 != r.aStrVal2 || bIsStr2 != r.bIsStr2 ) )
+        bEq = false;
 
     return bEq;
 }
@@ -1527,10 +1510,11 @@ ScCondFormatEntry::ScCondFormatEntry( ScDocument* pDocument, const ScCondFormatE
 {
 }
 
-bool ScCondFormatEntry::operator== ( const ScCondFormatEntry& r ) const
+// virtual
+bool ScCondFormatEntry::IsEqual( const ScFormatEntry& r, bool bIgnoreSrcPos ) const
 {
-    return ScConditionEntry::operator==( r ) &&
-            aStyleName == r.aStyleName;
+    return ScConditionEntry::IsEqual(r, bIgnoreSrcPos) &&
+        (aStyleName == static_cast<const ScCondFormatEntry&>(r).aStyleName);
 }
 
 ScCondFormatEntry::~ScCondFormatEntry()
@@ -1744,13 +1728,17 @@ ScConditionalFormat* ScConditionalFormat::Clone(ScDocument* pNewDoc) const
     return pNew;
 }
 
-bool ScConditionalFormat::EqualEntries( const ScConditionalFormat& r ) const
+bool ScConditionalFormat::EqualEntries( const ScConditionalFormat& r, bool bIgnoreSrcPos ) const
 {
     if( size() != r.size())
         return false;
 
     //TODO: Test for same entries in reverse order?
-    if ( ! ::comphelper::ContainerUniquePtrEquals(maEntries, r.maEntries) )
+    if (! std::equal(maEntries.begin(), maEntries.end(), r.maEntries.begin(),
+        [&bIgnoreSrcPos](const std::unique_ptr<ScFormatEntry>& p1, const std::unique_ptr<ScFormatEntry>& p2) -> bool
+            {
+                return p1->IsEqual(*p2, bIgnoreSrcPos);
+            }))
         return false;
 
     // right now don't check for same range
diff --git a/sc/source/core/data/table2.cxx b/sc/source/core/data/table2.cxx
index 626f58747571..751b7f0d5f4f 100644
--- a/sc/source/core/data/table2.cxx
+++ b/sc/source/core/data/table2.cxx
@@ -592,12 +592,36 @@ void ScTable::CopyConditionalFormat( SCCOL nCol1, SCROW nRow1, SCCOL nCol2, SCRO
         pNewFormat->UpdateReference(aRefCxt, true);
 
         sal_uLong nMax = 0;
+        bool bDuplicate = false;
         for(ScConditionalFormatList::const_iterator itrCond = mpCondFormatList->begin();
                 itrCond != mpCondFormatList->end(); ++itrCond)
         {
+            // Check if there is the same format in the destination
+            // If there is, then simply expand its range
+            if ((*itrCond)->EqualEntries(*pNewFormat, true))
+            {
+                bDuplicate = true;
+                pDocument->RemoveCondFormatData((*itrCond)->GetRange(), nTab, (*itrCond)->GetKey());
+                const ScRangeList& rNewRangeList = pNewFormat->GetRange();
+                ScRangeList& rDstRangeList = (*itrCond)->GetRangeList();
+                for (size_t i = 0; i < rNewRangeList.size(); ++i)
+                {
+                    rDstRangeList.Join(*rNewRangeList[i]);
+                }
+                pDocument->AddCondFormatData((*itrCond)->GetRange(), nTab, (*itrCond)->GetKey());
+                break;
+            }
+
             if ((*itrCond)->GetKey() > nMax)
                 nMax = (*itrCond)->GetKey();
         }
+        // Do not add duplicate entries
+        if (bDuplicate)
+        {
+            delete pNewFormat;
+            continue;
+        }
+
         pNewFormat->SetKey(nMax + 1);
         mpCondFormatList->InsertNew(pNewFormat);
 
diff --git a/sc/source/filter/xml/xmlcondformat.cxx b/sc/source/filter/xml/xmlcondformat.cxx
index 4925563b1502..f7b43c5cced4 100644
--- a/sc/source/filter/xml/xmlcondformat.cxx
+++ b/sc/source/filter/xml/xmlcondformat.cxx
@@ -310,7 +310,7 @@ void SAL_CALL ScXMLConditionalFormatContext::endFastElement( sal_Int32 /*nElemen
                     else
                         break;
                 }
-                else if (!pCacheCondFormatEntry->EqualIgnoringSrcPos(*pCondFormatEntry))
+                else if (!pCacheCondFormatEntry->IsEqual(*pCondFormatEntry, /*bIgnoreSrcPos*/true))
                 {
                     break;
                 }