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>

6 files changed, 126 insertions, 67 deletions

diff --git a/sc/inc/conditio.hxx b/sc/inc/conditio.hxx
index 93481f202236..327b8367dce9 100644
--- a/sc/inc/conditio.hxx
+++ b/sc/inc/conditio.hxx
@@ -244,6 +244,7 @@ public:
     virtual void SetParent( ScConditionalFormat* pNew ) = 0;
 
     bool operator==( const ScFormatEntry& ) const;
+    virtual bool IsEqual( const ScFormatEntry&, bool bIgnoreSrcPos ) const;
 
     virtual void startRendering();
     virtual void endRendering();
@@ -350,9 +351,7 @@ public:
             ScConditionEntry( ScDocument* pDocument, const ScConditionEntry& r );
     virtual ~ScConditionEntry() override;
 
-    bool            operator== ( const ScConditionEntry& r ) const;
-
-    bool            EqualIgnoringSrcPos( const ScConditionEntry& r ) const;
+    bool            IsEqual( const ScFormatEntry& r, bool bIgnoreSrcPos ) const override;
 
     virtual void SetParent( ScConditionalFormat* pNew ) override;
 
@@ -440,8 +439,6 @@ class SC_DLLPUBLIC ScCondFormatEntry : public ScConditionEntry
 {
     OUString                  aStyleName;
 
-    using ScConditionEntry::operator==;
-
 public:
             ScCondFormatEntry( ScConditionMode eOper,
                                 const OUString& rExpr1, const OUString& rExpr2,
@@ -459,7 +456,7 @@ public:
             ScCondFormatEntry( ScDocument* pDocument, const ScCondFormatEntry& r );
     virtual ~ScCondFormatEntry() override;
 
-    bool            operator== ( const ScCondFormatEntry& r ) const;
+    bool            IsEqual( const ScFormatEntry& r, bool bIgnoreSrcPos ) const override;
 
     const OUString&   GetStyle() const        { return aStyleName; }
     void            UpdateStyleName(const OUString& rNew)  { aStyleName=rNew; }
@@ -572,7 +569,7 @@ public:
 
     ScCondFormatData GetData( ScRefCellValue& rCell, const ScAddress& rPos ) const;
 
-    bool            EqualEntries( const ScConditionalFormat& r ) const;
+    bool            EqualEntries( const ScConditionalFormat& r, bool bIgnoreSrcPos = false ) const;
 
     void            DoRepaint();
 
diff --git a/sc/qa/unit/ucalc.hxx b/sc/qa/unit/ucalc.hxx
index ed8bf0c09e0e..74729debe9ea 100644
--- a/sc/qa/unit/ucalc.hxx
+++ b/sc/qa/unit/ucalc.hxx
@@ -482,6 +482,7 @@ public:
     void testCondCopyPaste();
     void testCondCopyPasteSingleCell(); //e.g. fdo#82503
     void testCondCopyPasteSingleCellToRange(); //e.g. fdo#82503
+    void testCondCopyPasteSingleCellIntoSameFormatRange(); // e.g., tdf#95295
     void testCondCopyPasteSingleRowToRange(); //e.g. tdf#106242
     void testCondCopyPasteSingleRowToRange2();
     void testCondCopyPasteSheetBetweenDoc();
@@ -780,6 +781,7 @@ public:
     CPPUNIT_TEST(testCondCopyPaste);
     CPPUNIT_TEST(testCondCopyPasteSingleCell);
     CPPUNIT_TEST(testCondCopyPasteSingleCellToRange);
+    CPPUNIT_TEST(testCondCopyPasteSingleCellIntoSameFormatRange);
     CPPUNIT_TEST(testCondCopyPasteSingleRowToRange);
     CPPUNIT_TEST(testCondCopyPasteSingleRowToRange2);
     CPPUNIT_TEST(testCondCopyPasteSheetBetweenDoc);
diff --git a/sc/qa/unit/ucalc_condformat.cxx b/sc/qa/unit/ucalc_condformat.cxx
index f98d2e94b8eb..e0f18ca1fd5b 100644
--- a/sc/qa/unit/ucalc_condformat.cxx
+++ b/sc/qa/unit/ucalc_condformat.cxx
@@ -288,14 +288,19 @@ void Test::testCondCopyPaste()
     ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(7,7,0);
     CPPUNIT_ASSERT(pPastedFormat);
 
-    CPPUNIT_ASSERT_EQUAL(ScRangeList(aTargetRange), pPastedFormat->GetRange());
-    CPPUNIT_ASSERT( nIndex != pPastedFormat->GetKey());
+    // Pasting the same conditional format must modify existing format, making its range
+    // combined of previous range and newly pasted range having the conditional format.
+    // No new conditional formats must be created.
+    CPPUNIT_ASSERT_EQUAL(size_t(1), m_pDoc->GetCondFormList(0)->size());
+    aRangeList.Join(aTargetRange);
+    CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
+    CPPUNIT_ASSERT_EQUAL(sal_uInt32(nIndex), pPastedFormat->GetKey());
     const SfxPoolItem* pItem = m_pDoc->GetAttr( 7, 7, 0, ATTR_CONDITIONAL );
     const ScCondFormatItem* pCondFormatItem = static_cast<const ScCondFormatItem*>(pItem);
 
     CPPUNIT_ASSERT(pCondFormatItem);
     CPPUNIT_ASSERT_EQUAL(size_t(1), pCondFormatItem->GetCondFormatData().size());
-    CPPUNIT_ASSERT( nIndex != pCondFormatItem->GetCondFormatData().at(0) );
+    CPPUNIT_ASSERT_EQUAL(sal_uInt32(nIndex), pCondFormatItem->GetCondFormatData().at(0));
 
     m_pDoc->DeleteTab(0);
 }
@@ -322,14 +327,19 @@ void Test::testCondCopyPasteSingleCell()
     ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(4,4,0);
     CPPUNIT_ASSERT(pPastedFormat);
 
-    CPPUNIT_ASSERT_EQUAL(ScRangeList(aTargetRange), pPastedFormat->GetRange());
-    CPPUNIT_ASSERT( nIndex != pPastedFormat->GetKey());
+    // Pasting the same conditional format must modify existing format, making its range
+    // combined of previous range and newly pasted range having the conditional format.
+    // No new conditional formats must be created.
+    CPPUNIT_ASSERT_EQUAL(size_t(1), m_pDoc->GetCondFormList(0)->size());
+    aRangeList.Join(aTargetRange);
+    CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
+    CPPUNIT_ASSERT_EQUAL(sal_uInt32(nIndex), pPastedFormat->GetKey());
     const SfxPoolItem* pItem = m_pDoc->GetAttr( 4, 4, 0, ATTR_CONDITIONAL );
     const ScCondFormatItem* pCondFormatItem = static_cast<const ScCondFormatItem*>(pItem);
 
     CPPUNIT_ASSERT(pCondFormatItem);
     CPPUNIT_ASSERT_EQUAL(size_t(1), pCondFormatItem->GetCondFormatData().size());
-    CPPUNIT_ASSERT( nIndex != pCondFormatItem->GetCondFormatData().at(0) );
+    CPPUNIT_ASSERT_EQUAL(sal_uInt32(nIndex), pCondFormatItem->GetCondFormatData().at(0) );
 
     m_pDoc->DeleteTab(0);
 }
@@ -352,7 +362,11 @@ void Test::testCondCopyPasteSingleCellToRange()
     ScRange aTargetRange(4,4,0,5,8,0);
     pasteOneCellFromClip(m_pDoc, aTargetRange, &aClipDoc);
 
-    std::set<sal_uLong> aCondFormatIndices;
+    // Pasting the same conditional format must modify existing format, making its range
+    // combined of previous range and newly pasted range having the conditional format.
+    // No new conditional formats must be created.
+    CPPUNIT_ASSERT_EQUAL(size_t(1), m_pDoc->GetCondFormList(0)->size());
+    aRangeList.Join(aTargetRange);
     for(SCROW nRow = 4; nRow <= 8; ++nRow)
     {
         for (SCCOL nCol = 4; nCol <= 5; ++nCol)
@@ -360,24 +374,58 @@ void Test::testCondCopyPasteSingleCellToRange()
             ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(nCol, nRow, 0);
             CPPUNIT_ASSERT(pPastedFormat);
 
-            CPPUNIT_ASSERT_EQUAL(ScRangeList(ScRange(nCol, nRow, 0)), pPastedFormat->GetRange());
+            CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
             sal_uLong nPastedKey = pPastedFormat->GetKey();
-            CPPUNIT_ASSERT( nIndex != nPastedKey);
+            CPPUNIT_ASSERT_EQUAL(nIndex, nPastedKey);
             const SfxPoolItem* pItem = m_pDoc->GetAttr( nCol, nRow, 0, ATTR_CONDITIONAL );
             const ScCondFormatItem* pCondFormatItem = static_cast<const ScCondFormatItem*>(pItem);
 
             CPPUNIT_ASSERT(pCondFormatItem);
             CPPUNIT_ASSERT_EQUAL(size_t(1), pCondFormatItem->GetCondFormatData().size());
-            CPPUNIT_ASSERT( nIndex != pCondFormatItem->GetCondFormatData().at(0) );
-            auto itr = aCondFormatIndices.find(nPastedKey);
-            CPPUNIT_ASSERT(bool(itr == aCondFormatIndices.end()));
-            aCondFormatIndices.insert(nPastedKey);
+            CPPUNIT_ASSERT_EQUAL(sal_uInt32(nIndex), pCondFormatItem->GetCondFormatData().at(0) );
         }
     }
 
     m_pDoc->DeleteTab(0);
 }
 
+void Test::testCondCopyPasteSingleCellIntoSameFormatRange()
+{
+    m_pDoc->InsertTab(0, "Test");
+
+    ScConditionalFormat* pFormat = new ScConditionalFormat(1, m_pDoc);
+    ScRange aCondFormatRange(0, 0, 0, 3, 3, 0);
+    ScRangeList aRangeList(aCondFormatRange);
+    pFormat->SetRange(aRangeList);
+
+    ScCondFormatEntry* pEntry = new ScCondFormatEntry(ScConditionMode::Direct, "=B2", "", m_pDoc, ScAddress(0, 0, 0), ScGlobal::GetRscString(STR_STYLENAME_RESULT));
+    pFormat->AddEntry(pEntry);
+    sal_uLong nIndex = m_pDoc->AddCondFormat(pFormat, 0);
+
+    ScDocument aClipDoc(SCDOCMODE_CLIP);
+    copyToClip(m_pDoc, ScRange(1, 1, 0, 1, 1, 0), &aClipDoc);
+
+    ScRange aTargetRange(2, 2, 0, 2, 2, 0);
+    pasteFromClip(m_pDoc, aTargetRange, &aClipDoc);
+
+    ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(2, 2, 0);
+    CPPUNIT_ASSERT(pPastedFormat);
+
+    // Pasting the same conditional format into the same range must not modify existing format,
+    // since it already covers the pasted range. No new conditional formats must be created.
+    CPPUNIT_ASSERT_EQUAL(size_t(1), m_pDoc->GetCondFormList(0)->size());
+    CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
+    CPPUNIT_ASSERT_EQUAL(sal_uInt32(nIndex), pPastedFormat->GetKey());
+    const SfxPoolItem* pItem = m_pDoc->GetAttr(2, 2, 0, ATTR_CONDITIONAL);
+    const ScCondFormatItem* pCondFormatItem = static_cast<const ScCondFormatItem*>(pItem);
+
+    CPPUNIT_ASSERT(pCondFormatItem);
+    CPPUNIT_ASSERT_EQUAL(size_t(1), pCondFormatItem->GetCondFormatData().size());
+    CPPUNIT_ASSERT_EQUAL(sal_uInt32(nIndex), pCondFormatItem->GetCondFormatData().at(0));
+
+    m_pDoc->DeleteTab(0);
+}
+
 void Test::testCondCopyPasteSingleRowToRange()
 {
     m_pDoc->InsertTab(0, "Test");
@@ -398,7 +446,7 @@ void Test::testCondCopyPasteSingleRowToRange()
 
     ScConditionalFormat* pNewFormat = m_pDoc->GetCondFormat(0, 4, 0);
     CPPUNIT_ASSERT(pNewFormat);
-    CPPUNIT_ASSERT(pNewFormat->GetKey() != pFormat->GetKey());
+    CPPUNIT_ASSERT_EQUAL(pNewFormat->GetKey(), pFormat->GetKey());
 
     for (SCCOL nCol = 1; nCol <= MAXCOL; ++nCol)
     {
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;
                 }