sc/inc/SheetViewManager.hxx | 6 ++++++ sc/qa/unit/tiledrendering/SheetViewTest.cxx | 5 +++-- sc/source/core/data/SheetViewManager.cxx | 11 +++++++---- 3 files changed, 16 insertions(+), 6 deletions(-)
New commits: commit beb74efcb7a071643414c3f5f0aa2c65477d3803 Author: Tomaž Vajngerl <[email protected]> AuthorDate: Thu Sep 11 18:56:39 2025 +0200 Commit: Tomaž Vajngerl <[email protected]> CommitDate: Fri Sep 26 04:13:04 2025 +0200 sc: don't remove the sheet view entry from the vector SheetViewID is directly related to the index in the vector where we store the sheet views, so it could be catastrphic if we remove the entry from the vector as SheetViewID would not reference the correct SheetView. Instead just reset the shared_ptr, so it has not SheetView, but this will create a hole in the vector. It's however unlikely that this is a problem as we will not have a lot of sheet views for a table. Also remove duplication when checking if the SheetViewID is valid (inside the bounds of the vector). Change-Id: Ifd4c67f49def942d9b9afdd2fc63f3a690124457 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/190889 Tested-by: Jenkins Reviewed-by: Tomaž Vajngerl <[email protected]> diff --git a/sc/inc/SheetViewManager.hxx b/sc/inc/SheetViewManager.hxx index 51bd7036d694..a2e50e716810 100644 --- a/sc/inc/SheetViewManager.hxx +++ b/sc/inc/SheetViewManager.hxx @@ -12,6 +12,7 @@ #include "SheetView.hxx" #include "types.hxx" +#include <o3tl/safeint.hxx> #include <vector> #include <memory> @@ -25,6 +26,11 @@ class SheetViewManager private: std::vector<std::shared_ptr<SheetView>> maViews; + bool isValidSheetViewID(SheetViewID nID) const + { + return nID >= 0 && o3tl::make_unsigned(nID) < maViews.size(); + } + public: SheetViewManager(); diff --git a/sc/qa/unit/tiledrendering/SheetViewTest.cxx b/sc/qa/unit/tiledrendering/SheetViewTest.cxx index 2ecfcfa3f13c..57384347c249 100644 --- a/sc/qa/unit/tiledrendering/SheetViewTest.cxx +++ b/sc/qa/unit/tiledrendering/SheetViewTest.cxx @@ -252,8 +252,9 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetView) dispatchCommand(mxComponent, u".uno:RemoveSheetView"_ustr, {}); Scheduler::ProcessEventsToIdle(); - // No more sheet view - CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->getSheetViews().size()); + // Sheet view is retained, but null + CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->getSheetViews().size()); + CPPUNIT_ASSERT(!pSheetViewManager->getSheetViews().at(0)); // Only 1 table left CPPUNIT_ASSERT_EQUAL(SCTAB(1), pDocument->GetTableCount()); diff --git a/sc/source/core/data/SheetViewManager.cxx b/sc/source/core/data/SheetViewManager.cxx index 1285ffe29cd8..b642ae2f26fc 100644 --- a/sc/source/core/data/SheetViewManager.cxx +++ b/sc/source/core/data/SheetViewManager.cxx @@ -23,9 +23,12 @@ SheetViewID SheetViewManager::create(ScTable* pSheetViewTable) bool SheetViewManager::remove(SheetViewID nID) { - if (nID >= 0 && o3tl::make_unsigned(nID) < maViews.size()) + if (isValidSheetViewID(nID)) { - maViews.erase(maViews.begin() + 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; @@ -33,7 +36,7 @@ bool SheetViewManager::remove(SheetViewID nID) std::shared_ptr<SheetView> SheetViewManager::get(SheetViewID nID) const { - if (nID >= 0 && o3tl::make_unsigned(nID) < maViews.size()) + if (isValidSheetViewID(nID)) { return maViews[nID]; } @@ -56,7 +59,7 @@ SheetViewID SheetViewManager::getNextSheetView(SheetViewID nID) } // If we assume current ID is valid, so set the start to current + 1 to search // for then next valid sheet view in the for loop. - else if (nID >= 0 && o3tl::make_unsigned(nID) < maViews.size()) + else if (isValidSheetViewID(nID)) { startIndex = size_t(nID) + 1; }
