sc/qa/unit/ucalc_condformat.cxx | 142 +++++++++++++++++++++++++++++++++++-----
sc/source/core/data/table2.cxx | 51 ++++++++++++++
2 files changed, 177 insertions(+), 16 deletions(-)
New commits:
commit 2274742b1d5a5340e64915f9343b7431fb374c69
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Sat Jan 25 13:06:35 2025 +0500
Commit: Xisco Fauli <xiscofa...@libreoffice.org>
CommitDate: Tue Feb 4 16:13:12 2025 +0100
tdf#164844: handle colorscale and friends specially when deduplicating CF
The conditional formats like colorscale, databar and iconset may take
their whole range to calculate their maximum / minimum, and each cell
format would depend on the CF range. Thus, automatic merge of ranges
of similar CFs when copying/pasting could create unexpected results.
E.g., merging CFs of two distinct color scales - one for a range from
1 to 10, and another from 100 to 500 - would change colors on both of
them, which is usually not what user wants, even when these ranges
are side by side in adjacent columns.
On the other hand, users still may want to have a way to expand the
range automatically. E.g., when there is a column from A1 to A10 with
a colorscale, and the user copies a cell from that range to A11, it
is very likely that the intention is to expand the existing CF, not
to create a separate colorscale individually for the cell A11.
This makes it necessary to create some complexity in deciding when to
deduplicate, and when not. This change introduces a function for that,
called isRangeDependentFormatNeedDeduplication; and implements some
simple algorithm that currently only depends on analysis of the two
CFs' ranges. If both ranges are one-dimensional vectors, and the new
CF range "continues" the existing range (the edges of the two vectors
are adjacent), then it decides to combine them into one expanded CF.
Also, if the new range is completely inside the old one (which may
easily happen, when a user copies a cell from one place to another
inside the same range), it also allows deduplication. In other cases,
it rejects deduplication, requiring creation of a new CF.
It may be extended as needed. One possible improvement could be e.g.
when a user has a two-dimensional colorscale block, and copies some
of its rows below, then it would be likely that the intention is to
expand the range of existing CF, rather than creating a new one for
the expanded range. This would need some specification how to tell
this case from others that must not deduplicate.
I must note that this problem is indeed difficult: e.g., in Excel,
there is also no simple logic around copying such CF. I saw very odd
cases, where the copy resulted in merging ranges of existing CF, or
in creation of new CF; also, it could even magically add cells to
existing CFs without copy - just by entering data below existing CF
(and even when there was some gap between the existing CF range and
the newly entered data - but not when there was two gaps). So there
likely will always be some corner cases where users expect something
else.
This changes the behavior established in commits
3f614f431475e1bf3bb3bbeac59b0681309628b7 (tdf#95295: don't add
duplicate conditional formats, 2017-12-11),
3fa15dd614bd72ddb36dbe033abeef5609d31f38 (tdf#154906 tdf#129813
tdf#129814 sc: fix conditional format color scale, 2023-04-26)
and 8af6c46a9c0e86bbbd908e96ff236ad1d6c4ddab (tdf#155319 sc: fix
conditional format data bar after copying, 2023-05-22) regarding the
mentioned types of CF.
Change-Id: Ie94167bbf66e448a6b56ad0962cb38221aa7d4b7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/180735
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
Tested-by: Jenkins
(cherry picked from commit 2d1acf7dd0558f5c2575cef58df7db4e59181a68)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/180741
Reviewed-by: Xisco Fauli <xiscofa...@libreoffice.org>
diff --git a/sc/qa/unit/ucalc_condformat.cxx b/sc/qa/unit/ucalc_condformat.cxx
index 87a169b1a2c5..56ebcd01e3eb 100644
--- a/sc/qa/unit/ucalc_condformat.cxx
+++ b/sc/qa/unit/ucalc_condformat.cxx
@@ -305,7 +305,7 @@ CPPUNIT_TEST_FIXTURE(TestCondformat,
testDataBarCondCopyPaste)
pDatabar->SetDataBarData(pFormatData);
pFormat->AddEntry(pDatabar);
- sal_uInt32 nIndex = m_pDoc->AddCondFormat(std::move(pFormat), 0);
+ sal_uInt32 nIndex0 = m_pDoc->AddCondFormat(std::move(pFormat), 0);
ScDocument aClipDoc(SCDOCMODE_CLIP);
copyToClip(m_pDoc, aCondFormatRange, &aClipDoc);
@@ -313,11 +313,11 @@ CPPUNIT_TEST_FIXTURE(TestCondformat,
testDataBarCondCopyPaste)
ScRange aTargetRange(0, 3, 0, 2, 3, 0);
pasteFromClip(m_pDoc, aTargetRange, &aClipDoc);
- // 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);
+ // Pasting the same conditional databar format into a non-adjacent range
must create a new
+ // format.
+ sal_uInt32 nIndex1 = m_pDoc->GetCondFormat(0, 3, 0)->GetKey();
+ CPPUNIT_ASSERT_EQUAL(size_t(2), m_pDoc->GetCondFormList(0)->size());
+ aRangeList = aTargetRange;
for (SCCOL nCol = 0; nCol < 3; ++nCol)
{
ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(nCol, 3, 0);
@@ -325,13 +325,68 @@ CPPUNIT_TEST_FIXTURE(TestCondformat,
testDataBarCondCopyPaste)
CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
sal_uInt32 nPastedKey = pPastedFormat->GetKey();
- CPPUNIT_ASSERT_EQUAL(nIndex, nPastedKey);
+ CPPUNIT_ASSERT(nIndex0 != nPastedKey);
+ CPPUNIT_ASSERT_EQUAL(nIndex1, nPastedKey);
const SfxPoolItem* pItem = m_pDoc->GetAttr(nCol, 3, 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(nIndex,
pCondFormatItem->GetCondFormatData().front());
+ CPPUNIT_ASSERT_EQUAL(nPastedKey,
pCondFormatItem->GetCondFormatData().front());
+ }
+
+ // Now paste next to the previous range (immediately below)
+ aTargetRange = ScRange(0, 4, 0, 2, 4, 0);
+ pasteFromClip(m_pDoc, aTargetRange, &aClipDoc);
+
+ // Pasting the same conditional databar format into an adjacent range (not
continuing the row)
+ // must create a new format.
+ sal_uInt32 nIndex2 = m_pDoc->GetCondFormat(0, 4, 0)->GetKey();
+ CPPUNIT_ASSERT_EQUAL(size_t(3), m_pDoc->GetCondFormList(0)->size());
+ aRangeList = aTargetRange;
+ for (SCCOL nCol = 0; nCol < 3; ++nCol)
+ {
+ ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(nCol, 4, 0);
+ CPPUNIT_ASSERT(pPastedFormat);
+ CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
+
+ sal_uInt32 nPastedKey = pPastedFormat->GetKey();
+ CPPUNIT_ASSERT(nIndex0 != nPastedKey);
+ CPPUNIT_ASSERT(nIndex1 != nPastedKey);
+ CPPUNIT_ASSERT_EQUAL(nIndex2, nPastedKey);
+
+ const SfxPoolItem* pItem = m_pDoc->GetAttr(nCol, 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_EQUAL(nPastedKey,
pCondFormatItem->GetCondFormatData().front());
+ }
+
+ // Now paste next to the previous range (immediately to the right)
+ aTargetRange = ScRange(3, 4, 0, 5, 4, 0);
+ pasteFromClip(m_pDoc, aTargetRange, &aClipDoc);
+
+ // Pasting the same conditional databar format into an adjacent range
(continuing the row) 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(3), m_pDoc->GetCondFormList(0)->size());
+ aRangeList.Join(aTargetRange);
+ for (SCCOL nCol = 3; nCol < 6; ++nCol)
+ {
+ ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(nCol, 4, 0);
+ CPPUNIT_ASSERT(pPastedFormat);
+ CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
+
+ sal_uInt32 nPastedKey = pPastedFormat->GetKey();
+ CPPUNIT_ASSERT(nIndex0 != nPastedKey);
+ CPPUNIT_ASSERT(nIndex1 != nPastedKey);
+ CPPUNIT_ASSERT_EQUAL(nIndex2, nPastedKey);
+
+ const SfxPoolItem* pItem = m_pDoc->GetAttr(nCol, 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_EQUAL(nPastedKey,
pCondFormatItem->GetCondFormatData().front());
}
m_pDoc->DeleteTab(0);
@@ -407,7 +462,7 @@ CPPUNIT_TEST_FIXTURE(TestCondformat,
testColorScaleCondCopyPaste)
pColorScaleFormat->AddEntry(pEntryRed);
pFormat->AddEntry(pColorScaleFormat);
- sal_uInt32 nIndex = m_pDoc->AddCondFormat(std::move(pFormat), 0);
+ sal_uInt32 nIndex0 = m_pDoc->AddCondFormat(std::move(pFormat), 0);
ScDocument aClipDoc(SCDOCMODE_CLIP);
copyToClip(m_pDoc, aCondFormatRange, &aClipDoc);
@@ -415,11 +470,11 @@ CPPUNIT_TEST_FIXTURE(TestCondformat,
testColorScaleCondCopyPaste)
ScRange aTargetRange(0, 3, 0, 2, 3, 0);
pasteFromClip(m_pDoc, aTargetRange, &aClipDoc);
- // 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);
+ // Pasting the same conditional databar format into a non-adjacent range
must create a new
+ // format.
+ sal_uInt32 nIndex1 = m_pDoc->GetCondFormat(0, 3, 0)->GetKey();
+ CPPUNIT_ASSERT_EQUAL(size_t(2), m_pDoc->GetCondFormList(0)->size());
+ aRangeList = aTargetRange;
for (SCCOL nCol = 0; nCol < 3; ++nCol)
{
ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(nCol, 3, 0);
@@ -427,13 +482,68 @@ CPPUNIT_TEST_FIXTURE(TestCondformat,
testColorScaleCondCopyPaste)
CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
sal_uInt32 nPastedKey = pPastedFormat->GetKey();
- CPPUNIT_ASSERT_EQUAL(nIndex, nPastedKey);
+ CPPUNIT_ASSERT(nIndex0 != nPastedKey);
+ CPPUNIT_ASSERT_EQUAL(nIndex1, nPastedKey);
const SfxPoolItem* pItem = m_pDoc->GetAttr(nCol, 3, 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().front());
+ CPPUNIT_ASSERT_EQUAL(nPastedKey,
pCondFormatItem->GetCondFormatData().front());
+ }
+
+ // Now paste next to the previous range (immediately below)
+ aTargetRange = ScRange(0, 4, 0, 2, 4, 0);
+ pasteFromClip(m_pDoc, aTargetRange, &aClipDoc);
+
+ // Pasting the same conditional databar format into an adjacent range (not
continuing the row)
+ // must create a new format.
+ sal_uInt32 nIndex2 = m_pDoc->GetCondFormat(0, 4, 0)->GetKey();
+ CPPUNIT_ASSERT_EQUAL(size_t(3), m_pDoc->GetCondFormList(0)->size());
+ aRangeList = aTargetRange;
+ for (SCCOL nCol = 0; nCol < 3; ++nCol)
+ {
+ ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(nCol, 4, 0);
+ CPPUNIT_ASSERT(pPastedFormat);
+ CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
+
+ sal_uInt32 nPastedKey = pPastedFormat->GetKey();
+ CPPUNIT_ASSERT(nIndex0 != nPastedKey);
+ CPPUNIT_ASSERT(nIndex1 != nPastedKey);
+ CPPUNIT_ASSERT_EQUAL(nIndex2, nPastedKey);
+
+ const SfxPoolItem* pItem = m_pDoc->GetAttr(nCol, 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_EQUAL(nPastedKey,
pCondFormatItem->GetCondFormatData().front());
+ }
+
+ // Now paste next to the previous range (immediately to the right)
+ aTargetRange = ScRange(3, 4, 0, 5, 4, 0);
+ pasteFromClip(m_pDoc, aTargetRange, &aClipDoc);
+
+ // Pasting the same conditional databar format into an adjacent range
(continuing the row) 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(3), m_pDoc->GetCondFormList(0)->size());
+ aRangeList.Join(aTargetRange);
+ for (SCCOL nCol = 3; nCol < 6; ++nCol)
+ {
+ ScConditionalFormat* pPastedFormat = m_pDoc->GetCondFormat(nCol, 4, 0);
+ CPPUNIT_ASSERT(pPastedFormat);
+ CPPUNIT_ASSERT_EQUAL(aRangeList, pPastedFormat->GetRange());
+
+ sal_uInt32 nPastedKey = pPastedFormat->GetKey();
+ CPPUNIT_ASSERT(nIndex0 != nPastedKey);
+ CPPUNIT_ASSERT(nIndex1 != nPastedKey);
+ CPPUNIT_ASSERT_EQUAL(nIndex2, nPastedKey);
+
+ const SfxPoolItem* pItem = m_pDoc->GetAttr(nCol, 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_EQUAL(nPastedKey,
pCondFormatItem->GetCondFormatData().front());
}
m_pDoc->DeleteTab(0);
diff --git a/sc/source/core/data/table2.cxx b/sc/source/core/data/table2.cxx
index 907f08a24cec..076c3f40992f 100644
--- a/sc/source/core/data/table2.cxx
+++ b/sc/source/core/data/table2.cxx
@@ -589,6 +589,52 @@ void ScTable::CopyCellToDocument(SCCOL nSrcCol, SCROW
nSrcRow, SCCOL nDestCol, S
namespace {
+bool isFormatDependentOnRange(const ScConditionalFormat& rFormat)
+{
+ for (size_t i = 0; i < rFormat.size(); ++i)
+ if (auto* entry = rFormat.GetEntry(i))
+ if (auto type = entry->GetType(); type ==
ScFormatEntry::Type::Colorscale
+ || type ==
ScFormatEntry::Type::Databar
+ || type ==
ScFormatEntry::Type::Iconset)
+ return true;
+ return false;
+}
+
+bool isRangeDependentFormatNeedDeduplication(const ScRangeList& rOld, const
ScRangeList& rNew)
+{
+ // Are they two adjacent vectors?
+ if (rOld.size() == 1 && rNew.size() == 1)
+ {
+ // Test vertical vectors
+ if (rOld[0].aStart.Col() == rOld[0].aEnd.Col() && rNew[0].aStart.Col()
== rNew[0].aEnd.Col()
+ && rNew[0].aStart.Col() == rOld[0].aStart.Col())
+ {
+ if (rOld[0].aEnd.Row() == rNew[0].aStart.Row() - 1
+ || rNew[0].aEnd.Row() == rOld[0].aStart.Row() - 1)
+ {
+ return true; // Two joining vertical vectors -> merge
+ }
+ }
+ // Test horizontal vectors
+ if (rOld[0].aStart.Row() == rOld[0].aEnd.Row() && rNew[0].aStart.Row()
== rNew[0].aEnd.Row()
+ && rNew[0].aStart.Row() == rOld[0].aStart.Row())
+ {
+ if (rOld[0].aEnd.Col() == rNew[0].aStart.Col() - 1
+ || rNew[0].aEnd.Col() == rOld[0].aStart.Col() - 1)
+ {
+ return true; // Two joining horizontal vectors -> merge
+ }
+ }
+ }
+
+ // Is the new one fully included into the old one?
+ for (auto& range : rNew)
+ if (!rOld.Contains(range))
+ return false; // Different ranges, no deduplication
+
+ return true; // New is completely inside old -> merge (in fact, this means
"nothing to do")
+}
+
bool CheckAndDeduplicateCondFormat(ScDocument& rDocument, ScConditionalFormat*
pOldFormat, const ScConditionalFormat* pNewFormat, SCTAB nTab)
{
if (!pOldFormat)
@@ -598,6 +644,11 @@ bool CheckAndDeduplicateCondFormat(ScDocument& rDocument,
ScConditionalFormat* p
{
const ScRangeList& rNewRangeList = pNewFormat->GetRange();
ScRangeList& rDstRangeList = pOldFormat->GetRangeList();
+
+ if (isFormatDependentOnRange(*pOldFormat)
+ && !isRangeDependentFormatNeedDeduplication(rDstRangeList,
rNewRangeList))
+ return false; // No deduplication, create new format
+
for (size_t i = 0; i < rNewRangeList.size(); ++i)
{
rDstRangeList.Join(rNewRangeList[i]);
