sc/inc/SparklineList.hxx | 1
sc/qa/unit/SparklineTest.cxx | 114 +++++++++++++++++++++++-------
sc/source/core/data/column2.cxx | 38 +++++++++-
sc/source/core/data/document.cxx | 3
sc/source/core/data/table2.cxx | 2
sc/source/ui/sparklines/SparklineList.cxx | 27 ++++++-
6 files changed, 156 insertions(+), 29 deletions(-)
New commits:
commit 2370689d331b7f1a22cc2c7f285a288cecbe1b6c
Author: Tomaž Vajngerl <tomaz.vajng...@collabora.co.uk>
AuthorDate: Sun Apr 3 15:53:55 2022 +0900
Commit: Tomaž Vajngerl <qui...@gmail.com>
CommitDate: Wed Apr 13 01:34:09 2022 +0200
sc: improve keeping track of sparklines in SparklineList
Issues can happen when saving a document with sparklines where a
sparkline is deleted and then undo-ed. The reason for this is
because the SparlineList wasn't correctly updated when deleting,
copying or adding sparklines. This change adds hooks when new
sparklines are created or when sparklines are deleted to report
this into SparlineList.
SparklineList garbage-collects itself but this is not enough when
we rely that the non-deleted weak pointers to the sparkline groups
contain the correct non-deleted weak pointers to the correct
sparklines.
Change-Id: I976cbe7e6168813d3dd5089c036cc7fe4e357fb2
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/132554
Tested-by: Tomaž Vajngerl <qui...@gmail.com>
Reviewed-by: Tomaž Vajngerl <qui...@gmail.com>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/132883
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com>
diff --git a/sc/inc/SparklineList.hxx b/sc/inc/SparklineList.hxx
index bab467a09469..e9ca8591237c 100644
--- a/sc/inc/SparklineList.hxx
+++ b/sc/inc/SparklineList.hxx
@@ -36,6 +36,7 @@ public:
SparklineList();
void addSparkline(std::shared_ptr<Sparkline> const& pSparkline);
+ void removeSparkline(std::shared_ptr<Sparkline> const& pSparkline);
std::vector<std::shared_ptr<SparklineGroup>> getSparklineGroups();
diff --git a/sc/qa/unit/SparklineTest.cxx b/sc/qa/unit/SparklineTest.cxx
index 99efd55f8209..f8d07edc53e6 100644
--- a/sc/qa/unit/SparklineTest.cxx
+++ b/sc/qa/unit/SparklineTest.cxx
@@ -458,47 +458,109 @@ void
SparklineTest::testUndoRedoClearContentForSparkline()
insertTestData(rDocument);
// Sparkline range
- ScRange aRange(0, 6, 0, 0, 6, 0);
+ ScRange aDataRange(0, 0, 0, 3, 5, 0); //A1:D6
+ ScRange aRange(0, 6, 0, 3, 6, 0); // A7:D7
- // Check Sparkline at cell A7 doesn't exists
- auto pSparkline = rDocument.GetSparkline(aRange.aStart);
- CPPUNIT_ASSERT(!pSparkline);
+ // Check Sparklines - none should exist
+ {
+ CPPUNIT_ASSERT(!rDocument.HasSparkline(ScAddress(0, 6, 0)));
+ CPPUNIT_ASSERT(!rDocument.HasSparkline(ScAddress(1, 6, 0)));
+ CPPUNIT_ASSERT(!rDocument.HasSparkline(ScAddress(2, 6, 0)));
+ CPPUNIT_ASSERT(!rDocument.HasSparkline(ScAddress(3, 6, 0)));
+ }
auto pSparklineGroup = std::make_shared<sc::SparklineGroup>();
- CPPUNIT_ASSERT(rDocFunc.InsertSparklines(ScRange(0, 0, 0, 0, 5, 0),
aRange, pSparklineGroup));
+ CPPUNIT_ASSERT(rDocFunc.InsertSparklines(aDataRange, aRange,
pSparklineGroup));
- // Check Sparkline at cell A7 exists
- pSparkline = rDocument.GetSparkline(aRange.aStart);
- CPPUNIT_ASSERT(pSparkline);
- CPPUNIT_ASSERT_EQUAL(SCCOL(0), pSparkline->getColumn());
- CPPUNIT_ASSERT_EQUAL(SCROW(6), pSparkline->getRow());
+ // Check Sparklines
+ {
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(0, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(1, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(2, 6, 0)));
+ // D7 exists
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(3, 6, 0)));
+
+ // Check D7
+ auto pSparkline = rDocument.GetSparkline(ScAddress(3, 6, 0));
+ CPPUNIT_ASSERT_EQUAL(SCCOL(3), pSparkline->getColumn());
+ CPPUNIT_ASSERT_EQUAL(SCROW(6), pSparkline->getRow());
+
+ // Check collections
+ auto* pSparklineList = rDocument.GetSparklineList(SCTAB(0));
+ auto pSparklineGroups = pSparklineList->getSparklineGroups();
+ CPPUNIT_ASSERT_EQUAL(size_t(1), pSparklineGroups.size());
- // Clear content - including sparkline
- ScMarkData aMark(rDocument.GetSheetLimits());
- aMark.SetMarkArea(aRange.aStart);
- rDocFunc.DeleteContents(aMark, InsertDeleteFlags::CONTENTS, true, true);
+ auto pSparklines =
pSparklineList->getSparklinesFor(pSparklineGroups[0]);
+ CPPUNIT_ASSERT_EQUAL(size_t(4), pSparklines.size());
+ }
- // Check Sparkline at cell A7 doesn't exists
- pSparkline = rDocument.GetSparkline(aRange.aStart);
- CPPUNIT_ASSERT(!pSparkline);
+ // Clear content of cell D7 - including sparkline
+ {
+ ScMarkData aMark(rDocument.GetSheetLimits());
+ aMark.SetMarkArea(ScAddress(3, 6, 0));
+ rDocFunc.DeleteContents(aMark, InsertDeleteFlags::CONTENTS, true,
true);
+ }
+
+ // Check Sparklines
+ {
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(1, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(1, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(2, 6, 0)));
+ // D7 is gone
+ CPPUNIT_ASSERT(!rDocument.HasSparkline(ScAddress(3, 6, 0)));
+
+ // Check collections
+ auto* pSparklineList = rDocument.GetSparklineList(SCTAB(0));
+ auto pSparklineGroups = pSparklineList->getSparklineGroups();
+ CPPUNIT_ASSERT_EQUAL(size_t(1), pSparklineGroups.size());
+
+ auto pSparklines =
pSparklineList->getSparklinesFor(pSparklineGroups[0]);
+ CPPUNIT_ASSERT_EQUAL(size_t(3), pSparklines.size());
+ }
// Undo
rDocument.GetUndoManager()->Undo();
- // Check Sparkline at cell A7 exists
- pSparkline = rDocument.GetSparkline(aRange.aStart);
- CPPUNIT_ASSERT(pSparkline);
- CPPUNIT_ASSERT_EQUAL(SCCOL(0), pSparkline->getColumn());
- CPPUNIT_ASSERT_EQUAL(SCROW(6), pSparkline->getRow());
+ // Check Sparkline
+ {
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(0, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(1, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(2, 6, 0)));
+ // D7 exists - again
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(3, 6, 0)));
+
+ // Check D7
+ auto pSparkline = rDocument.GetSparkline(ScAddress(3, 6, 0));
+ CPPUNIT_ASSERT_EQUAL(SCCOL(3), pSparkline->getColumn());
+ CPPUNIT_ASSERT_EQUAL(SCROW(6), pSparkline->getRow());
+
+ auto* pSparklineList = rDocument.GetSparklineList(SCTAB(0));
+ auto pSparklineGroups = pSparklineList->getSparklineGroups();
+ CPPUNIT_ASSERT_EQUAL(size_t(1), pSparklineGroups.size());
+
+ auto pSparklines =
pSparklineList->getSparklinesFor(pSparklineGroups[0]);
+ CPPUNIT_ASSERT_EQUAL(size_t(4), pSparklines.size());
+ }
// Redo
rDocument.GetUndoManager()->Redo();
- // Check Sparkline at cell A7 doesn't exists
- pSparkline = rDocument.GetSparkline(aRange.aStart);
- CPPUNIT_ASSERT(!pSparkline);
+ // Check Sparklines
+ {
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(1, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(1, 6, 0)));
+ CPPUNIT_ASSERT(rDocument.HasSparkline(ScAddress(2, 6, 0)));
+ // D7 is gone - again
+ CPPUNIT_ASSERT(!rDocument.HasSparkline(ScAddress(3, 6, 0)));
- CPPUNIT_ASSERT(!rDocument.HasSparkline(aRange.aStart));
+ // Check collections
+ auto* pSparklineList = rDocument.GetSparklineList(SCTAB(0));
+ auto pSparklineGroups = pSparklineList->getSparklineGroups();
+ CPPUNIT_ASSERT_EQUAL(size_t(1), pSparklineGroups.size());
+
+ auto pSparklines =
pSparklineList->getSparklinesFor(pSparklineGroups[0]);
+ CPPUNIT_ASSERT_EQUAL(size_t(3), pSparklines.size());
+ }
xDocSh->DoClose();
}
diff --git a/sc/source/core/data/column2.cxx b/sc/source/core/data/column2.cxx
index 2bde1b82a8bc..2fbf42388ead 100644
--- a/sc/source/core/data/column2.cxx
+++ b/sc/source/core/data/column2.cxx
@@ -43,6 +43,7 @@
#include <tokenstringcontext.hxx>
#include <sortparam.hxx>
#include <SparklineGroup.hxx>
+#include <SparklineList.hxx>
#include <editeng/eeitem.hxx>
#include <o3tl/safeint.hxx>
@@ -1973,6 +1974,29 @@ void ScColumn::PrepareBroadcastersForDestruction()
// Sparklines
+namespace
+{
+
+class DeletingSparklinesHandler
+{
+ ScDocument& m_rDocument;
+ SCTAB m_nTab;
+
+public:
+ DeletingSparklinesHandler(ScDocument& rDocument, SCTAB nTab)
+ : m_rDocument(rDocument)
+ , m_nTab(nTab)
+ {}
+
+ void operator() (size_t /*nRow*/, const sc::SparklineCell* pCell)
+ {
+ auto* pList = m_rDocument.GetSparklineList(m_nTab);
+ pList->removeSparkline(pCell->getSparkline());
+ }
+};
+
+} // end anonymous ns
+
sc::SparklineCell* ScColumn::GetSparklineCell(SCROW nRow)
{
return maSparklines.get<sc::SparklineCell*>(nRow);
@@ -1980,11 +2004,16 @@ sc::SparklineCell* ScColumn::GetSparklineCell(SCROW
nRow)
void ScColumn::CreateSparklineCell(SCROW nRow, std::shared_ptr<sc::Sparkline>
const& pSparkline)
{
+ auto* pList = GetDoc().GetSparklineList(GetTab());
+ pList->addSparkline(pSparkline);
maSparklines.set(nRow, new sc::SparklineCell(pSparkline));
}
void ScColumn::DeleteSparklineCells(sc::ColumnBlockPosition& rBlockPos, SCROW
nRow1, SCROW nRow2)
{
+ DeletingSparklinesHandler aFunction(GetDoc(), nTab);
+ sc::ParseSparkline(maSparklines.begin(), maSparklines, nRow1, nRow2,
aFunction);
+
rBlockPos.miSparklinePos =
maSparklines.set_empty(rBlockPos.miSparklinePos, nRow1, nRow2);
}
@@ -1993,6 +2022,9 @@ bool ScColumn::DeleteSparkline(SCROW nRow)
if (!GetDoc().ValidRow(nRow))
return false;
+ DeletingSparklinesHandler aFunction(GetDoc(), nTab);
+ sc::ParseSparkline(maSparklines.begin(), maSparklines, nRow, nRow,
aFunction);
+
maSparklines.set_empty(nRow, nRow);
return true;
}
@@ -2037,12 +2069,16 @@ public:
auto const& pSparkline = pCell->getSparkline();
auto const& pGroup = pCell->getSparklineGroup();
- auto pDestinationGroup =
mrDestColumn.GetDoc().SearchSparklineGroup(pGroup->getID());
+ auto& rDestDoc = mrDestColumn.GetDoc();
+ auto pDestinationGroup =
rDestDoc.SearchSparklineGroup(pGroup->getID());
if (!pDestinationGroup)
pDestinationGroup = std::make_shared<sc::SparklineGroup>(*pGroup);
// Copy the group
auto pNewSparkline =
std::make_shared<sc::Sparkline>(mrDestColumn.GetCol(), nDestRow,
pDestinationGroup);
pNewSparkline->setInputRange(pSparkline->getInputRange());
+ auto* pList = rDestDoc.GetSparklineList(mrDestColumn.GetTab());
+ pList->addSparkline(pNewSparkline);
+
miDestPosition = mrDestSparkline.set(miDestPosition, nDestRow, new
sc::SparklineCell(pNewSparkline));
}
};
diff --git a/sc/source/core/data/table2.cxx b/sc/source/core/data/table2.cxx
index d52be76e6cc3..c1e89152d3ae 100644
--- a/sc/source/core/data/table2.cxx
+++ b/sc/source/core/data/table2.cxx
@@ -1848,8 +1848,8 @@ sc::Sparkline* ScTable::CreateSparkline(SCCOL nCol, SCROW
nRow, std::shared_ptr<
ScColumn& rColumn = CreateColumnIfNotExists(nCol);
std::shared_ptr<sc::Sparkline> pSparkline(new sc::Sparkline(nCol, nRow,
pSparklineGroup));
- maSparklineList.addSparkline(pSparkline);
rColumn.CreateSparklineCell(nRow, pSparkline);
+
return pSparkline.get();
}
diff --git a/sc/source/ui/sparklines/SparklineList.cxx
b/sc/source/ui/sparklines/SparklineList.cxx
index 7ee52ac74e27..1cae3089616d 100644
--- a/sc/source/ui/sparklines/SparklineList.cxx
+++ b/sc/source/ui/sparklines/SparklineList.cxx
@@ -25,6 +25,30 @@ void SparklineList::addSparkline(std::shared_ptr<Sparkline>
const& pSparkline)
m_aSparklineGroups.push_back(pWeakGroup);
}
+void SparklineList::removeSparkline(std::shared_ptr<Sparkline> const&
pSparkline)
+{
+ auto pWeakGroup =
std::weak_ptr<SparklineGroup>(pSparkline->getSparklineGroup());
+ auto iteratorGroup = m_aSparklineGroupMap.find(pWeakGroup);
+ if (iteratorGroup != m_aSparklineGroupMap.end())
+ {
+ auto& rWeakSparklines = iteratorGroup->second;
+
+ for (auto iterator = rWeakSparklines.begin(); iterator !=
rWeakSparklines.end();)
+ {
+ auto pCurrentSparkline = iterator->lock();
+
+ if (pCurrentSparkline && pCurrentSparkline != pSparkline)
+ {
+ iterator++;
+ }
+ else
+ {
+ iterator = rWeakSparklines.erase(iterator);
+ }
+ }
+ }
+}
+
std::vector<std::shared_ptr<SparklineGroup>>
SparklineList::getSparklineGroups()
{
std::vector<std::shared_ptr<SparklineGroup>> toReturn;
commit ce598cbef4060db670c44a46b06c2bb8fcb6667e
Author: Tomaž Vajngerl <tomaz.vajng...@collabora.co.uk>
AuthorDate: Sun Apr 3 13:57:36 2022 +0900
Commit: Tomaž Vajngerl <qui...@gmail.com>
CommitDate: Wed Apr 13 01:33:57 2022 +0200
sc: prevent a crash when deleting a sparkline
Change-Id: Idf89d4bbdc2bd29ce55cc3a8fd6707ece345869c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/132553
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <qui...@gmail.com>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/132882
Tested-by: Tomaž Vajngerl <qui...@gmail.com>
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index 64adfd65e702..6765e8a5849e 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -6647,6 +6647,9 @@ std::shared_ptr<sc::SparklineGroup>
ScDocument::SearchSparklineGroup(tools::Guid
{
for (auto const& rTable : maTabs)
{
+ if (!rTable)
+ continue;
+
auto& rSparklineList = rTable->GetSparklineList();
for (auto const& pSparklineGroup : rSparklineList.getSparklineGroups())
diff --git a/sc/source/ui/sparklines/SparklineList.cxx
b/sc/source/ui/sparklines/SparklineList.cxx
index 744a58bce66e..7ee52ac74e27 100644
--- a/sc/source/ui/sparklines/SparklineList.cxx
+++ b/sc/source/ui/sparklines/SparklineList.cxx
@@ -31,7 +31,8 @@ std::vector<std::shared_ptr<SparklineGroup>>
SparklineList::getSparklineGroups()
for (auto iterator = m_aSparklineGroups.begin(); iterator !=
m_aSparklineGroups.end();)
{
- if (auto pSparklineGroup = iterator->lock())
+ auto pWeakGroup = *iterator;
+ if (auto pSparklineGroup = pWeakGroup.lock())
{
toReturn.push_back(pSparklineGroup);
iterator++;
