sc/inc/SheetView.hxx | 10 ++- sc/inc/SheetViewManager.hxx | 69 +++++++++++++++++++++++-- sc/qa/unit/tiledrendering/SheetViewTest.cxx | 25 ++++----- sc/source/core/data/SheetViewManager.cxx | 44 +++++++++------ sc/source/core/data/document.cxx | 4 - sc/source/ui/cctrl/SheetViewBox.cxx | 15 +---- sc/source/ui/dialogs/SelectSheetViewDialog.cxx | 30 +++------- sc/source/ui/operation/Operation.cxx | 13 +--- sc/source/ui/view/viewfun2.cxx | 4 - sc/source/ui/view/viewfun3.cxx | 2 10 files changed, 136 insertions(+), 80 deletions(-)
New commits: commit c56bc5bb98d9789993509bdba82fda3addcf5d72 Author: Tomaž Vajngerl <[email protected]> AuthorDate: Thu Feb 19 12:40:24 2026 +0900 Commit: Miklos Vajna <[email protected]> CommitDate: Fri Feb 20 13:18:36 2026 +0100 sc: Add iterator that iterates through valid sheet views only On SheetViewManager we now have iterateValidSheetViews() method instead of getSheetViews(), that will iterate through valid (non-null) sheet view in a ranged for loop. This prevents common problems of forgetting to check if the sheet view is null that we had previously. Also add counter for the valid sheet views, so methods size() and isEmpty() can use it to return the number of valid sheet views. Another issue that happened is that we remove the sheet views 2 times, because in ScViewFunc::RemoveCurrentSheetView we remove the sheet view from the manager, but the same happens internally when DeleteTable is called. Change-Id: I0e25e839ffc19c2f4e8689eee17b5932446409dd Reviewed-on: https://gerrit.libreoffice.org/c/core/+/199677 Tested-by: Jenkins CollaboraOffice <[email protected]> Reviewed-by: Miklos Vajna <[email protected]> diff --git a/sc/inc/SheetView.hxx b/sc/inc/SheetView.hxx index 03e4c87acb90..df560b121678 100644 --- a/sc/inc/SheetView.hxx +++ b/sc/inc/SheetView.hxx @@ -76,9 +76,10 @@ public: ScTable* getTablePointer() const; SCTAB getTableNumber() const; + SheetViewID getID() const { return mnID; } - OUString const& GetName() { return maName; } + OUString const& GetName() const { return maName; } /** A sheet view is valid if the pointer to the table is set */ bool isValid() const; @@ -101,14 +102,17 @@ public: /** Merges the reorder parameters */ void mergeReorderParameters(ReorderParam const& rReorderParameters); - std::optional<ReorderParam> const& getReorderParameters() { return moOriginalReorderParams; } + std::optional<ReorderParam> const& getReorderParameters() const + { + return moOriginalReorderParams; + } /** Reverses the complete (sheet view and default view) sorting order for the input row */ SCROW reverseSortingToDefaultView(SCROW nRow, SCCOL nColumn) const; // Last used sort parameters - std::optional<ScSortParam> const& getSortParam() { return moSortParam; } + std::optional<ScSortParam> const& getSortParam() const { return moSortParam; } /// Remember last used sort parameters when sheet view was sorted. void setSortParam(ScSortParam const& rSortParam) { moSortParam = rSortParam; } diff --git a/sc/inc/SheetViewManager.hxx b/sc/inc/SheetViewManager.hxx index 30afd0bc86c8..d57efa328068 100644 --- a/sc/inc/SheetViewManager.hxx +++ b/sc/inc/SheetViewManager.hxx @@ -16,6 +16,7 @@ #include <o3tl/safeint.hxx> #include <vector> #include <memory> +#include <iterator> class ScTable; @@ -26,6 +27,7 @@ class SC_DLLPUBLIC SheetViewManager { private: std::vector<std::shared_ptr<SheetView>> maViews; + size_t mnSheetViewCount = 0; sal_Int32 maNameCounter = 0; std::optional<SortOrderReverser> moSortOrder; @@ -37,6 +39,64 @@ private: OUString generateName(); + /** Forward iterator over valid (non-null) sheet views only. */ + class SheetViewIterator + { + private: + using VectorIterator = std::vector<std::shared_ptr<SheetView>>::const_iterator; + VectorIterator maCurrentIt; + VectorIterator maEndIt; + + void skipEmpty() + { + while (maCurrentIt != maEndIt && !(*maCurrentIt)) + { + maCurrentIt++; + } + } + + public: + SheetViewIterator(VectorIterator aBegin, VectorIterator aEnd) + : maCurrentIt(aBegin) + , maEndIt(aEnd) + { + skipEmpty(); + } + + SheetView& operator*() const { return **maCurrentIt; } + + SheetViewIterator& operator++() + { + maCurrentIt++; + skipEmpty(); + return *this; + } + + bool operator==(SheetViewIterator const& rOther) const + { + return maCurrentIt == rOther.maCurrentIt; + } + }; + + /** Wrapper for SheetViewIterator so it is possible to use it with range-for loops. */ + class SheetViewRange + { + std::vector<std::shared_ptr<SheetView>> const& mrViews; + + public: + SheetViewRange(std::vector<std::shared_ptr<SheetView>> const& rViews) + : mrViews(rViews) + { + } + + SheetViewIterator begin() const + { + return SheetViewIterator(mrViews.begin(), mrViews.end()); + } + + SheetViewIterator end() const { return SheetViewIterator(mrViews.end(), mrViews.end()); } + }; + public: SheetViewManager(); @@ -47,7 +107,10 @@ public: std::shared_ptr<SheetView> get(SheetViewID nID) const; /** True if there are no sheet views. */ - bool isEmpty() const { return maViews.empty(); } + bool isEmpty() const { return mnSheetViewCount == 0; } + + /** Number of sheet views. */ + size_t size() const { return mnSheetViewCount; } /** Remove the sheet view with the ID. True if successful. */ bool remove(SheetViewID nID); @@ -55,8 +118,8 @@ public: /** Remove all sheet views. */ void removeAll(); - /** Return the list of sheet views. */ - std::vector<std::shared_ptr<SheetView>> const& getSheetViews() const { return maViews; } + /** Returns a range that iterates only over valid (non-null) sheet views. */ + SheetViewRange iterateValidSheetViews() const { return SheetViewRange(maViews); } /** Calculate the next sheet view ID from the current ID. */ SheetViewID getNextSheetView(SheetViewID nID); diff --git a/sc/qa/unit/tiledrendering/SheetViewTest.cxx b/sc/qa/unit/tiledrendering/SheetViewTest.cxx index 95a0bbe72238..246366e8f8cc 100644 --- a/sc/qa/unit/tiledrendering/SheetViewTest.cxx +++ b/sc/qa/unit/tiledrendering/SheetViewTest.cxx @@ -400,7 +400,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetView) // Sheet view must be present auto pSheetViewManager = pDocument->GetSheetViewManager(0); - CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->size()); // There should be 2 tables CPPUNIT_ASSERT_EQUAL(SCTAB(2), pDocument->GetTableCount()); @@ -413,9 +413,8 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetView) removeSheetViewInCurrentView(); Scheduler::ProcessEventsToIdle(); - // Sheet view is retained, but null - CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->getSheetViews().size()); - CPPUNIT_ASSERT(!pSheetViewManager->getSheetViews().at(0)); + // Sheet view was removed + CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->size()); // Only 1 table left CPPUNIT_ASSERT_EQUAL(SCTAB(1), pDocument->GetTableCount()); @@ -477,7 +476,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testSheetViewOperationRestrictions_DefaultVi // Sheet view must be present auto pSheetViewManager = pDocument->GetSheetViewManager(SCTAB(0)); - CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->size()); auto pSheetView1 = pSheetViewManager->get(0); CPPUNIT_ASSERT_EQUAL(true, pSheetView1->isSynced()); @@ -543,7 +542,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testSheetViewOperationRestrictions_SheetView // Sheet view must be present auto pSheetViewManager = pDocument->GetSheetViewManager(SCTAB(0)); - CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->size()); auto pSheetView1 = pSheetViewManager->get(0); CPPUNIT_ASSERT_EQUAL(true, pSheetView1->isSynced()); @@ -684,7 +683,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveTableWithSheetViews) { auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); CPPUNIT_ASSERT(pSheetViewManager); - CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->size()); // Check we have the correct table selected CPPUNIT_ASSERT_EQUAL(SCTAB(2), rDocument.GetTableCount()); @@ -700,7 +699,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveTableWithSheetViews) { auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); CPPUNIT_ASSERT(pSheetViewManager); - CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->size()); CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); auto pSheetView1 = pSheetViewManager->get(0); @@ -728,7 +727,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveTableWithSheetViews) auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); CPPUNIT_ASSERT(pSheetViewManager); - CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->size()); auto pSheetView1 = pSheetViewManager->get(0); CPPUNIT_ASSERT_EQUAL(SCTAB(2), pSheetView1->getTableNumber()); @@ -766,7 +765,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetViewHolderTable) { auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); CPPUNIT_ASSERT(pSheetViewManager); - CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->size()); } // Create first sheet views @@ -775,7 +774,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetViewHolderTable) { auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); CPPUNIT_ASSERT(pSheetViewManager); - CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->size()); CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); auto pSheetView1 = pSheetViewManager->get(0); @@ -801,7 +800,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetViewHolderTable) auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); CPPUNIT_ASSERT(pSheetViewManager); - CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->size()); auto pSheetView1 = pSheetViewManager->get(0); CPPUNIT_ASSERT_EQUAL(SCTAB(2), pSheetView1->getTableNumber()); @@ -839,7 +838,7 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetViewHolderTable) auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); CPPUNIT_ASSERT(pSheetViewManager); - CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->size()); auto pSheetView1 = pSheetViewManager->get(0); CPPUNIT_ASSERT_EQUAL(SCTAB(1), pSheetView1->getTableNumber()); diff --git a/sc/source/core/data/SheetViewManager.cxx b/sc/source/core/data/SheetViewManager.cxx index b4f3c6da4fd2..879bb9b66166 100644 --- a/sc/source/core/data/SheetViewManager.cxx +++ b/sc/source/core/data/SheetViewManager.cxx @@ -20,23 +20,34 @@ SheetViewID SheetViewManager::create(ScTable* pSheetViewTable) { SheetViewID nID(maViews.size()); maViews.emplace_back(std::make_shared<SheetView>(pSheetViewTable, generateName(), nID)); + mnSheetViewCount++; return nID; } bool SheetViewManager::remove(SheetViewID nID) { - if (isValidSheetViewID(nID)) - { - // We only reset the value and not actually remove if from the vector, because the SheetViewID - // also represents the index in the vector. If we removed the value it would make all the - // following indices / SheetViewIDs returning the wrong SheetView. - maViews[nID].reset(); - return true; - } - return false; + if (!isValidSheetViewID(nID)) + return false; + + // It's probably a bug if we want to remove a non-existent sheet view. + SAL_WARN_IF(!maViews[nID], "sc", "Removing a non-existing sheet view."); + + if (!maViews[nID]) + return false; + + // We only reset the value and not actually remove if from the vector, because the SheetViewID + // also represents the index in the vector. If we removed the value it would make all the + // following indices / SheetViewIDs returning the wrong SheetView. + maViews[nID].reset(); + mnSheetViewCount--; + return true; } -void SheetViewManager::removeAll() { maViews.clear(); } +void SheetViewManager::removeAll() +{ + maViews.clear(); + mnSheetViewCount = 0; +} std::shared_ptr<SheetView> SheetViewManager::get(SheetViewID nID) const { @@ -115,14 +126,9 @@ SheetViewID SheetViewManager::getPreviousSheetView(SheetViewID nID) void SheetViewManager::unsyncAllSheetViews() { - for (auto const& pSheetView : maViews) + for (auto& rSheetView : iterateValidSheetViews()) { - if (!pSheetView) - { - continue; - } - - pSheetView->unsync(); + rSheetView.unsync(); } } @@ -144,9 +150,9 @@ void SheetViewManager::addOrderIndices(SortOrderInfo const& rSortInfo) void SheetViewManager::mergeReorderParameters(ReorderParam const& rReorderParameters) { - for (auto& pSheetView : maViews) + for (auto& rSheetView : iterateValidSheetViews()) { - pSheetView->mergeReorderParameters(rReorderParameters); + rSheetView.mergeReorderParameters(rReorderParameters); } } } diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx index 9b2297ed9918..ebd41484f18a 100644 --- a/sc/source/core/data/document.cxx +++ b/sc/source/core/data/document.cxx @@ -733,10 +733,10 @@ bool ScDocument::DeleteTab( SCTAB nTab ) auto pManager = pTable->GetSheetViewManager(); if (pManager) { - for (auto const& pSheetView : pManager->getSheetViews()) + for (auto const& rSheetView : pManager->iterateValidSheetViews()) { // Set the table pointer to null as the table will be deleted - pSheetView->getTablePointer()->RemoveSheetViewTablePointer(); + rSheetView.getTablePointer()->RemoveSheetViewTablePointer(); } } } diff --git a/sc/source/ui/cctrl/SheetViewBox.cxx b/sc/source/ui/cctrl/SheetViewBox.cxx index 222d03681be1..5134f1029ae7 100644 --- a/sc/source/ui/cctrl/SheetViewBox.cxx +++ b/sc/source/ui/cctrl/SheetViewBox.cxx @@ -66,17 +66,12 @@ void SheetViewBox::Update(sc::SheetViewID nSelectedID) if (pSheetManager) { - sc::SheetViewID nSheetViewID = 0; - for (auto const& pSheetView : pSheetManager->getSheetViews()) + for (auto const& rSheetView : pSheetManager->iterateValidSheetViews()) { - if (pSheetView) - { - OUString sID = OUString::number(nSheetViewID); - if (nSheetViewID == nSelectedID) - sActiveID = sID; - m_xWidget->append(sID, pSheetView->GetName()); - } - nSheetViewID++; + OUString sID = OUString::number(rSheetView.getID()); + if (rSheetView.getID() == nSelectedID) + sActiveID = sID; + m_xWidget->append(sID, rSheetView.GetName()); } } m_xWidget->thaw(); diff --git a/sc/source/ui/dialogs/SelectSheetViewDialog.cxx b/sc/source/ui/dialogs/SelectSheetViewDialog.cxx index c3f31022814a..a367600c5493 100644 --- a/sc/source/ui/dialogs/SelectSheetViewDialog.cxx +++ b/sc/source/ui/dialogs/SelectSheetViewDialog.cxx @@ -10,7 +10,6 @@ #include <dialogs/SelectSheetViewDialog.hxx> #include <sal/config.h> #include <o3tl/safeint.hxx> -#include <memory> #include <viewdata.hxx> #include <SheetViewManager.hxx> @@ -19,17 +18,17 @@ namespace sc { class SheetViewListEntry { - std::shared_ptr<SheetView> mpSheetView; + OUString maName; sc::SheetViewID mnSheetViewID = sc::InvalidSheetViewID; public: - SheetViewListEntry(std::shared_ptr<SheetView> const& pSheetView, sc::SheetViewID nSheetViewID) - : mpSheetView(pSheetView) + SheetViewListEntry(OUString const& rName, sc::SheetViewID nSheetViewID) + : maName(rName) , mnSheetViewID(nSheetViewID) { } - OUString getName() const { return mpSheetView->GetName(); } + OUString const& getName() const { return maName; } sc::SheetViewID getSheetViewID() const { return mnSheetViewID; } }; @@ -43,7 +42,6 @@ SelectSheetViewDialog::SelectSheetViewDialog(weld::Window* pParent, ScViewData& ScDocument& rDocument = mrViewData.GetDocument(); - auto pSheetManager = rDocument.GetSheetViewManager(mrViewData.GetTabNumber()); m_xEntryTree->clear(); m_xEntryTree->freeze(); @@ -51,21 +49,15 @@ SelectSheetViewDialog::SelectSheetViewDialog(weld::Window* pParent, ScViewData& OUString sActiveID = u"-1"_ustr; m_xEntryTree->append(u"-1"_ustr, SheetViewManager::defaultViewName()); - if (pSheetManager) + if (auto pSheetManager = rDocument.GetSheetViewManager(mrViewData.GetTabNumber())) { - sc::SheetViewID nSheetViewID = 0; - - for (auto const& pSheetView : pSheetManager->getSheetViews()) + for (auto const& rSheetView : pSheetManager->iterateValidSheetViews()) { - if (pSheetView) - { - auto& aEntry = m_aEntries.emplace_back(pSheetView, nSheetViewID); - OUString sID = OUString::number(m_aEntries.size() - 1); - if (nSheetViewID == mrViewData.GetSheetViewID()) - sActiveID = sID; - m_xEntryTree->append(sID, aEntry.getName()); - } - nSheetViewID++; + auto& aEntry = m_aEntries.emplace_back(rSheetView.GetName(), rSheetView.getID()); + OUString sID = OUString::number(m_aEntries.size() - 1); + if (rSheetView.getID() == mrViewData.GetSheetViewID()) + sActiveID = sID; + m_xEntryTree->append(sID, aEntry.getName()); } } diff --git a/sc/source/ui/operation/Operation.cxx b/sc/source/ui/operation/Operation.cxx index db08818eeacf..e45da8687bb6 100644 --- a/sc/source/ui/operation/Operation.cxx +++ b/sc/source/ui/operation/Operation.cxx @@ -201,12 +201,9 @@ void Operation::syncSheetViews() if (!pManager || pManager->isEmpty()) return; - for (std::shared_ptr<SheetView> const& pSheetView : pManager->getSheetViews()) + for (auto& rSheetView : pManager->iterateValidSheetViews()) { - if (!pSheetView) - continue; - - SCTAB nSheetViewTab = pSheetView->getTableNumber(); + SCTAB nSheetViewTab = rSheetView.getTableNumber(); std::optional<ScQueryParam> oQueryParam; ScDBData* pNoNameData = rDocument.GetAnonymousDBData(nSheetViewTab); @@ -222,7 +219,7 @@ void Operation::syncSheetViews() rDocument.OverwriteContent(nTab, nSheetViewTab); // Reverse the sorting of the default view in the sheet view - if (auto const& rReorderParameters = pSheetView->getReorderParameters()) + if (auto const& rReorderParameters = rSheetView.getReorderParameters()) { sc::ReorderParam aReorderParameters(*rReorderParameters); aReorderParameters.maSortRange.aStart.SetTab(nSheetViewTab); @@ -231,11 +228,11 @@ void Operation::syncSheetViews() rDocument.Reorder(aReorderParameters); } - auto const& oSortParam = pSheetView->getSortParam(); + auto const& oSortParam = rSheetView.getSortParam(); if (oSortParam) { // We need to reset the sort order for the sheet view as we will sort again - pSheetView->resetSortOrder(); + rSheetView.resetSortOrder(); rDocument.Sort(nSheetViewTab, *oSortParam, false, false, nullptr, nullptr); } diff --git a/sc/source/ui/view/viewfun2.cxx b/sc/source/ui/view/viewfun2.cxx index 62172a3030ff..4c1ce6d4854d 100644 --- a/sc/source/ui/view/viewfun2.cxx +++ b/sc/source/ui/view/viewfun2.cxx @@ -2642,9 +2642,9 @@ bool ScViewFunc::DeleteTables(const vector<SCTAB>& rTabs, bool bRecord) if (auto pManager = rDoc.GetSheetViewManager(nTab)) { - for (auto const& pSheetView : pManager->getSheetViews()) + for (auto const& rSheetView : pManager->iterateValidSheetViews()) { - aSheetViewHolderTablesToDelete.push_back(pSheetView->getTablePointer()); + aSheetViewHolderTablesToDelete.push_back(rSheetView.getTablePointer()); } } diff --git a/sc/source/ui/view/viewfun3.cxx b/sc/source/ui/view/viewfun3.cxx index 3339766ece64..32edd74d3b18 100644 --- a/sc/source/ui/view/viewfun3.cxx +++ b/sc/source/ui/view/viewfun3.cxx @@ -2130,8 +2130,8 @@ void ScViewFunc::RemoveCurrentSheetView() GetViewData().SetSheetViewID(sc::DefaultSheetViewID); SCTAB nSheetViewTab = rDocument.GetSheetViewNumber(nTab, nSheetViewID); - pSheetManager->remove(nSheetViewID); + // DeleteTable also removes the sheet view from the manager GetViewData().GetDocFunc().DeleteTable(nSheetViewTab, true); SheetViewChanged();
