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 913b9f0cc6bae621468a81f6a63bb0b8e2534023 Author: Tomaž Vajngerl <[email protected]> AuthorDate: Thu Sep 11 18:56:39 2025 +0200 Commit: Miklos Vajna <[email protected]> CommitDate: Fri Sep 26 08:58:36 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/+/191521 Reviewed-by: Miklos Vajna <[email protected]> Tested-by: Jenkins CollaboraOffice <[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 0777efe4f412..44cfd7ff2a24 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 c94e4ed9a40a..1db9356ff782 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 currnet 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; }
