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 <quikee@gmail.com>
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>

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();
 

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)));
 
-    // Clear content - including sparkline
-    ScMarkData aMark(rDocument.GetSheetLimits());
-    aMark.SetMarkArea(aRange.aStart);
-    rDocFunc.DeleteContents(aMark, InsertDeleteFlags::CONTENTS, true, true);
+        // 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 Sparkline at cell A7 doesn't exists
-    pSparkline = rDocument.GetSparkline(aRange.aStart);
-    CPPUNIT_ASSERT(!pSparkline);
+        // 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(4), pSparklines.size());
+    }
+
+    // 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();
 }

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>
@@ -1996,6 +1997,29 @@ void ScColumn::DeleteEmptyBroadcasters()
 
 // 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);
@@ -2003,11 +2027,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);
 }
 
@@ -2016,6 +2045,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;
 }
@@ -2060,12 +2092,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));
     }
 };

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();
 }
 

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;