[Libreoffice-commits] core.git: Branch 'feature/cib_contract138b' - 2 commits - sc/inc sc/qa sc/source
Libreoffice Gerrit user
logerrit at kemper.freedesktop.org
Mon Dec 3 10:02:40 UTC 2018
sc/inc/conditio.hxx | 9 ++--
sc/qa/unit/ucalc.hxx | 2 +
sc/qa/unit/ucalc_condformat.cxx | 76 +++++++++++++++++++++++++++++++--------
sc/source/core/data/conditio.cxx | 64 +++++++++++++++++---------------
sc/source/core/data/table2.cxx | 24 ++++++++++++
5 files changed, 127 insertions(+), 48 deletions(-)
New commits:
commit e012468d440a41f7c55fb1927341d4c86517bfdc
Author: Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Tue Dec 12 09:03:22 2017 +0100
Commit: Katarina Behrens <Katarina.Behrens at cib.de>
CommitDate: Mon Dec 3 10:46:06 2018 +0100
Fix unnecessary wrong downcast
...from ScFormatEntry to ScCondFormatEntry instead of just ScConditionEntry,
introduced with 3f614f431475e1bf3bb3bbeac59b0681309628b7 "tdf#95295: don't add
duplicate conditional formats". Caused e.g. CppunitTest_sc_bugfix_test to fail
under -fsanitize=vptr, when trying to downcast a ScValidationData.
Change-Id: I913076d3538bafbdb2ac57b6bb1bd89199b048a6
diff --git a/sc/source/core/data/conditio.cxx b/sc/source/core/data/conditio.cxx
index 27f0b0b4a693..0e59c3215dcc 100644
--- a/sc/source/core/data/conditio.cxx
+++ b/sc/source/core/data/conditio.cxx
@@ -698,7 +698,7 @@ bool ScConditionEntry::IsEqual( const ScFormatEntry& rOther, bool bIgnoreSrcPos
if (GetType() != rOther.GetType())
return false;
- const ScCondFormatEntry& r = static_cast<const ScCondFormatEntry&>(rOther);
+ const ScConditionEntry& r = static_cast<const ScConditionEntry&>(rOther);
bool bEq = (eOp == r.eOp && nOptions == r.nOptions &&
lcl_IsEqual( pFormula1, r.pFormula1 ) &&
commit 9d5c13f5aa8b292c658f6a3b55285be24baf2bc9
Author: Mike Kaganski <mike.kaganski at collabora.com>
AuthorDate: Fri Dec 1 15:25:56 2017 +0300
Commit: Katarina Behrens <Katarina.Behrens at cib.de>
CommitDate: Fri Nov 30 18:03:58 2018 +0100
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 at libreoffice.org>
Reviewed-by: Eike Rathke <erack at redhat.com>
diff --git a/sc/inc/conditio.hxx b/sc/inc/conditio.hxx
index 10906f0dbacb..1f5a08e8885a 100644
--- a/sc/inc/conditio.hxx
+++ b/sc/inc/conditio.hxx
@@ -157,6 +157,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();
@@ -231,7 +232,7 @@ public:
ScConditionEntry( ScDocument* pDocument, const ScConditionEntry& r );
virtual ~ScConditionEntry() override;
- bool operator== ( const ScConditionEntry& r ) const;
+ bool IsEqual( const ScFormatEntry& r, bool bIgnoreSrcPos ) const override;
virtual void SetParent( ScConditionalFormat* pNew ) override;
@@ -318,8 +319,6 @@ class SC_DLLPUBLIC ScCondFormatEntry : public ScConditionEntry
{
OUString aStyleName;
- using ScConditionEntry::operator==;
-
public:
ScCondFormatEntry( ScConditionMode eOper,
const OUString& rExpr1, const OUString& rExpr2,
@@ -337,7 +336,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; }
@@ -451,7 +450,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 eca7064b0119..27f0b0b4a693 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 condformat::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()
@@ -697,25 +692,32 @@ 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;
}
+ // 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;
}
@@ -1577,10 +1579,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()
@@ -1794,15 +1797,18 @@ 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?
- for (size_t i=0; i<size(); i++)
- if ( ! ::comphelper::ContainerUniquePtrEquals(maEntries, r.maEntries) )
- return false;
+ 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
// we only use this method to merge same conditional formats from
diff --git a/sc/source/core/data/table2.cxx b/sc/source/core/data/table2.cxx
index 4be3a7ec6840..cc57b8cba948 100644
--- a/sc/source/core/data/table2.cxx
+++ b/sc/source/core/data/table2.cxx
@@ -595,12 +595,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);
More information about the Libreoffice-commits
mailing list