sc/inc/SheetView.hxx | 3 sc/inc/SheetViewManager.hxx | 3 sc/inc/table.hxx | 23 ++- sc/qa/unit/tiledrendering/SheetViewTest.cxx | 188 ++++++++++++++++++++++++++++ sc/source/core/data/SheetViewManager.cxx | 2 sc/source/core/data/document.cxx | 25 +++ sc/source/core/data/document10.cxx | 2 sc/source/ui/view/viewfun2.cxx | 46 +++++- sc/source/ui/view/viewfun3.cxx | 3 9 files changed, 280 insertions(+), 15 deletions(-)
New commits: commit 8573f255288b10cd74cf13d50824897be9dbd2f6 Author: Tomaž Vajngerl <[email protected]> AuthorDate: Wed Oct 15 09:47:33 2025 +0900 Commit: Miklos Vajna <[email protected]> CommitDate: Fri Oct 17 10:13:56 2025 +0200 sc: delete sheet views when the table is deleted This deletes all the sheet view holders when the table that has any sheet views is deleted. If a sheet view holder table is deleted, this change also makes sure that the sheet view gets properly deleted. This change also adds the test for deletion, that asserts step by step that the values tables and sheet views are inserted and delete correctly. Result of this is a couple of fixes to the delete operation. Change-Id: I29594c5e660fe8c42715b08bb9dcc3f25a611f29 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/192565 Reviewed-by: Miklos Vajna <[email protected]> Tested-by: Jenkins CollaboraOffice <[email protected]> diff --git a/sc/inc/SheetView.hxx b/sc/inc/SheetView.hxx index 4f4e2258004f..bff043b55812 100644 --- a/sc/inc/SheetView.hxx +++ b/sc/inc/SheetView.hxx @@ -9,6 +9,7 @@ #pragma once +#include "scdllapi.h" #include "types.hxx" #include <rtl/ustring.hxx> @@ -21,7 +22,7 @@ namespace sc * A sheet view is a special view of a sheet that can be filtered and sorted * independently from other views of the sheet. **/ -class SheetView +class SC_DLLPUBLIC SheetView { private: ScTable* mpTable = nullptr; diff --git a/sc/inc/SheetViewManager.hxx b/sc/inc/SheetViewManager.hxx index 3687197d3619..f516e3a76c22 100644 --- a/sc/inc/SheetViewManager.hxx +++ b/sc/inc/SheetViewManager.hxx @@ -50,6 +50,9 @@ public: /** Remove the sheet view with the ID. True if successful. */ bool remove(SheetViewID nID); + /** Remove all sheet views. */ + void removeAll(); + /** Return the list of sheet views. */ std::vector<std::shared_ptr<SheetView>> const& getSheetViews() const { return maViews; } diff --git a/sc/inc/table.hxx b/sc/inc/table.hxx index 487fc3c62d14..f3810ba98d18 100644 --- a/sc/inc/table.hxx +++ b/sc/inc/table.hxx @@ -255,7 +255,8 @@ private: bool mbForceBreaks:1; bool mbTotalsRowBelow:1; - bool mbIsSheetViewHolder : 1 = false; + sc::SheetViewID mnSheetViewID = sc::InvalidSheetViewID; + ScTable* mpSheetViewFor = nullptr; /** this is touched from formula group threading context */ std::atomic<bool> bStreamValid; @@ -1199,11 +1200,25 @@ public: std::shared_ptr<sc::SheetViewManager> const& GetSheetViewManager() const; - bool IsSheetViewHolder() const { return mbIsSheetViewHolder; } + bool IsSheetViewHolder() const { return bool(mpSheetViewFor); } - void SetSheetViewHolder(bool bValue) + ScTable* GetDefaultViewTable() const { return mpSheetViewFor; } + + sc::SheetViewID GetSheetViewID() const + { + return mnSheetViewID; + } + + + void SetSheetViewHolder(sc::SheetViewID nID, ScTable* pDefaultViewTable) + { + mnSheetViewID = nID; + mpSheetViewFor = pDefaultViewTable; + } + + void RemoveSheetViewTablePointer() { - mbIsSheetViewHolder = bValue; + mpSheetViewFor = nullptr; } private: diff --git a/sc/qa/unit/tiledrendering/SheetViewTest.cxx b/sc/qa/unit/tiledrendering/SheetViewTest.cxx index 988c0f6f519a..0d01c00674bd 100644 --- a/sc/qa/unit/tiledrendering/SheetViewTest.cxx +++ b/sc/qa/unit/tiledrendering/SheetViewTest.cxx @@ -510,6 +510,194 @@ CPPUNIT_TEST_FIXTURE(SheetViewTest, testCheckIfSheetViewIsSavedInDocument_OOXML) assertXPath(pXmlDoc, "/x:workbook/x:sheets/x:sheet", 1); } +CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveTableWithSheetViews) +{ + // Create a new sheet (in addition to the existing one), and create 2 sheet views of the new sheet. + // After that, delete the sheet and the 2 sheet view holder tables should also be deleted. + + ScModelObj* pModelObj = createDoc("empty.ods"); + pModelObj->initializeForTiledRendering(uno::Sequence<beans::PropertyValue>()); + ScDocument& rDocument = *pModelObj->GetDocument(); + + // Setup views + ScTestViewCallback aView1; + ScTabViewShell* pTabView1 = aView1.getTabViewShell(); + + // Insert a new sheet - "NewTab" + uno::Sequence<beans::PropertyValue> aArgsInsert(comphelper::InitPropertySequence( + { { "Name", uno::Any(u"NewTab"_ustr) }, { "Index", uno::Any(sal_Int16(1)) } })); + + dispatchCommand(mxComponent, u".uno:Insert"_ustr, aArgsInsert); + + { + auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); + CPPUNIT_ASSERT(pSheetViewManager); + CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->getSheetViews().size()); + + // Check we have the correct table selected + CPPUNIT_ASSERT_EQUAL(SCTAB(2), rDocument.GetTableCount()); + CPPUNIT_ASSERT_EQUAL(u"NewTab"_ustr, rDocument.GetAllTableNames()[0]); + CPPUNIT_ASSERT_EQUAL(u"Hoja1"_ustr, rDocument.GetAllTableNames()[1]); + + CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); + } + + // Create first sheet views + dispatchCommand(mxComponent, u".uno:NewSheetView"_ustr, {}); + + { + auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); + CPPUNIT_ASSERT(pSheetViewManager); + CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->getSheetViews().size()); + + CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); + auto pSheetView1 = pSheetViewManager->get(0); + CPPUNIT_ASSERT_EQUAL(SCTAB(1), pSheetView1->getTableNumber()); + + CPPUNIT_ASSERT_EQUAL(SCTAB(3), rDocument.GetTableCount()); + CPPUNIT_ASSERT_EQUAL(u"NewTab"_ustr, rDocument.GetAllTableNames()[0]); + CPPUNIT_ASSERT_EQUAL(u"NewTab_2"_ustr, rDocument.GetAllTableNames()[1]); + CPPUNIT_ASSERT_EQUAL(u"Hoja1"_ustr, rDocument.GetAllTableNames()[2]); + } + + // Create second sheet views + dispatchCommand(mxComponent, u".uno:NewSheetView"_ustr, {}); + + { + CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); + + CPPUNIT_ASSERT_EQUAL(SCTAB(4), rDocument.GetTableCount()); + CPPUNIT_ASSERT_EQUAL(u"NewTab"_ustr, rDocument.GetAllTableNames()[0]); + CPPUNIT_ASSERT_EQUAL(u"NewTab_3"_ustr, rDocument.GetAllTableNames()[1]); + CPPUNIT_ASSERT_EQUAL(u"NewTab_2"_ustr, rDocument.GetAllTableNames()[2]); + CPPUNIT_ASSERT_EQUAL(u"Hoja1"_ustr, rDocument.GetAllTableNames()[3]); + + // Sheet view must be present + auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); + CPPUNIT_ASSERT(pSheetViewManager); + + CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + + auto pSheetView1 = pSheetViewManager->get(0); + CPPUNIT_ASSERT_EQUAL(SCTAB(2), pSheetView1->getTableNumber()); + + auto pSheetView2 = pSheetViewManager->get(1); + CPPUNIT_ASSERT_EQUAL(SCTAB(1), pSheetView2->getTableNumber()); + } + + // Delete the table - index 0 + uno::Sequence<beans::PropertyValue> aArgs( + comphelper::InitPropertySequence({ { "Index", uno::Any(sal_uInt16(0)) } })); + + dispatchCommand(mxComponent, u".uno:Remove"_ustr, aArgs); + + { + CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); + + CPPUNIT_ASSERT_EQUAL(SCTAB(1), rDocument.GetTableCount()); + CPPUNIT_ASSERT_EQUAL(u"Hoja1"_ustr, rDocument.GetAllTableNames()[0]); + } +} + +CPPUNIT_TEST_FIXTURE(SheetViewTest, testRemoveSheetViewHolderTable) +{ + // Delete the sheet view holder table directly + + ScModelObj* pModelObj = createDoc("empty.ods"); + pModelObj->initializeForTiledRendering(uno::Sequence<beans::PropertyValue>()); + ScDocument& rDocument = *pModelObj->GetDocument(); + + // Setup views + ScTestViewCallback aView1; + ScTabViewShell* pTabView1 = aView1.getTabViewShell(); + + { + auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); + CPPUNIT_ASSERT(pSheetViewManager); + CPPUNIT_ASSERT_EQUAL(size_t(0), pSheetViewManager->getSheetViews().size()); + } + + // Create first sheet views + dispatchCommand(mxComponent, u".uno:NewSheetView"_ustr, {}); + + { + auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); + CPPUNIT_ASSERT(pSheetViewManager); + CPPUNIT_ASSERT_EQUAL(size_t(1), pSheetViewManager->getSheetViews().size()); + + CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); + auto pSheetView1 = pSheetViewManager->get(0); + CPPUNIT_ASSERT_EQUAL(SCTAB(1), pSheetView1->getTableNumber()); + + CPPUNIT_ASSERT_EQUAL(SCTAB(2), rDocument.GetTableCount()); + CPPUNIT_ASSERT_EQUAL(u"Hoja1"_ustr, rDocument.GetAllTableNames()[0]); + CPPUNIT_ASSERT_EQUAL(u"Hoja1_2"_ustr, rDocument.GetAllTableNames()[1]); + } + + // Create second sheet views + dispatchCommand(mxComponent, u".uno:NewSheetView"_ustr, {}); + + { + CPPUNIT_ASSERT_EQUAL(SCTAB(0), pTabView1->GetViewData().GetTabNumber()); + + CPPUNIT_ASSERT_EQUAL(SCTAB(3), rDocument.GetTableCount()); + CPPUNIT_ASSERT_EQUAL(u"Hoja1"_ustr, rDocument.GetAllTableNames()[0]); + CPPUNIT_ASSERT_EQUAL(u"Hoja1_3"_ustr, rDocument.GetAllTableNames()[1]); + CPPUNIT_ASSERT_EQUAL(u"Hoja1_2"_ustr, rDocument.GetAllTableNames()[2]); + + // Sheet view must be present + auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); + CPPUNIT_ASSERT(pSheetViewManager); + + CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + + auto pSheetView1 = pSheetViewManager->get(0); + CPPUNIT_ASSERT_EQUAL(SCTAB(2), pSheetView1->getTableNumber()); + + auto pSheetView2 = pSheetViewManager->get(1); + CPPUNIT_ASSERT_EQUAL(SCTAB(1), pSheetView2->getTableNumber()); + } + + // Unhide the sheet view holder tables (or they won't be deleted) + { + uno::Sequence<beans::PropertyValue> aArgs( + comphelper::InitPropertySequence({ { "aTableName", uno::Any(u"Hoja1_3"_ustr) } })); + dispatchCommand(mxComponent, u".uno:Show"_ustr, aArgs); + } + { + uno::Sequence<beans::PropertyValue> aArgs( + comphelper::InitPropertySequence({ { "aTableName", uno::Any(u"Hoja1_2"_ustr) } })); + dispatchCommand(mxComponent, u".uno:Show"_ustr, aArgs); + } + + // Delete the table + { + uno::Sequence<beans::PropertyValue> aArgs( + comphelper::InitPropertySequence({ { "Index", uno::Any(sal_uInt16(2)) } })); + + dispatchCommand(mxComponent, u".uno:Remove"_ustr, aArgs); + } + + { + CPPUNIT_ASSERT_EQUAL(SCTAB(2), rDocument.GetTableCount()); + CPPUNIT_ASSERT_EQUAL(u"Hoja1"_ustr, rDocument.GetAllTableNames()[0]); + CPPUNIT_ASSERT_EQUAL(u"Hoja1_2"_ustr, rDocument.GetAllTableNames()[1]); + + // Sheet view must be present + auto pSheetViewManager = rDocument.GetSheetViewManager(SCTAB(0)); + CPPUNIT_ASSERT(pSheetViewManager); + + CPPUNIT_ASSERT_EQUAL(size_t(2), pSheetViewManager->getSheetViews().size()); + + auto pSheetView1 = pSheetViewManager->get(0); + CPPUNIT_ASSERT_EQUAL(SCTAB(1), pSheetView1->getTableNumber()); + + // Not deleted but the sheet view is now null + auto pSheetView2 = pSheetViewManager->get(1); + CPPUNIT_ASSERT(!pSheetView2); + } +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sc/source/core/data/SheetViewManager.cxx b/sc/source/core/data/SheetViewManager.cxx index 5e56c1d5dc14..91333f633c3d 100644 --- a/sc/source/core/data/SheetViewManager.cxx +++ b/sc/source/core/data/SheetViewManager.cxx @@ -34,6 +34,8 @@ bool SheetViewManager::remove(SheetViewID nID) return false; } +void SheetViewManager::removeAll() { maViews.clear(); } + std::shared_ptr<SheetView> SheetViewManager::get(SheetViewID nID) const { if (isValidSheetViewID(nID)) diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx index e2a94a68b111..d1c157dece3c 100644 --- a/sc/source/core/data/document.cxx +++ b/sc/source/core/data/document.cxx @@ -101,6 +101,7 @@ #include <mtvelements.hxx> #include <sfx2/lokhelper.hxx> +#include <SheetViewManager.hxx> using ::editeng::SvxBorderLine; using namespace ::com::sun::star; @@ -716,6 +717,30 @@ bool ScDocument::DeleteTab( SCTAB nTab ) if (pTab) pTab->UpdateDeleteTab(aCxt); + // Delete sheet views holders + if (ScTable* pTable = FetchTable(nTab)) + { + if (pTable->IsSheetViewHolder()) + { + assert(pTable->GetDefaultViewTable() && "Sheet view holder but the pointer to the table is nullptr"); + auto pSheetViewManager = pTable->GetDefaultViewTable()->GetSheetViewManager(); + assert(pSheetViewManager && "Sheet view holder but the table has no SheetViewManager"); + pSheetViewManager->remove(pTable->GetSheetViewID()); + } + else + { + auto pManager = pTable->GetSheetViewManager(); + if (pManager) + { + for (auto const& pSheetView : pManager->getSheetViews()) + { + // Set the table pointer to null as the table will be deleted + pSheetView->getTablePointer()->RemoveSheetViewTablePointer(); + } + } + } + } + // tdf#149502 make sure ScTable destructor called after the erase is finished, when // maTabs[x].nTab==x is true again, as it should be always true. // In the end of maTabs.erase, maTabs indexes change, but nTab updated before erase. diff --git a/sc/source/core/data/document10.cxx b/sc/source/core/data/document10.cxx index 523e28780dcd..c97caa193460 100644 --- a/sc/source/core/data/document10.cxx +++ b/sc/source/core/data/document10.cxx @@ -1135,7 +1135,7 @@ std::pair<sc::SheetViewID, SCTAB> ScDocument::CreateNewSheetView(SCTAB nTab) { auto nSheetViewID = pTable->GetSheetViewManager()->create(pSheetViewTable); pSheetViewTable->SetVisible(false); - pSheetViewTable->SetSheetViewHolder(true); + pSheetViewTable->SetSheetViewHolder(nSheetViewID, pTable); return { nSheetViewID, nSheetViewTab }; } } diff --git a/sc/source/ui/view/viewfun2.cxx b/sc/source/ui/view/viewfun2.cxx index 5f8ef60bfe90..ce2197b062fb 100644 --- a/sc/source/ui/view/viewfun2.cxx +++ b/sc/source/ui/view/viewfun2.cxx @@ -83,6 +83,7 @@ #include <mergecellsdialog.hxx> #include <sheetevents.hxx> #include <columnspanset.hxx> +#include <SheetViewManager.hxx> #include <vector> #include <memory> @@ -2549,12 +2550,12 @@ void ScViewFunc::DeleteTables( const SCTAB nTab, SCTAB nSheets ) pSfxApp->Broadcast( SfxHint( SfxHintId::ScAreaLinksChanged ) ); } -bool ScViewFunc::DeleteTables(const vector<SCTAB> &TheTabs, bool bRecord ) +bool ScViewFunc::DeleteTables(const vector<SCTAB>& rTabs, bool bRecord) { ScDocShell* pDocSh = GetViewData().GetDocShell(); ScDocument& rDoc = pDocSh->GetDocument(); bool bVbaEnabled = rDoc.IsInVBAMode(); - SCTAB nNewTab = TheTabs.front(); + SCTAB nNewTab = rTabs.front(); weld::WaitObject aWait(GetViewData().GetDialogParent()); if (bRecord && !rDoc.IsUndoEnabled()) bRecord = false; @@ -2574,7 +2575,7 @@ bool ScViewFunc::DeleteTables(const vector<SCTAB> &TheTabs, bool bRecord ) OUString aOldName; bool isFirstTab = true; - for(SCTAB nTab : TheTabs) + for (SCTAB nTab : rTabs) { if (isFirstTab) { @@ -2628,25 +2629,52 @@ bool ScViewFunc::DeleteTables(const vector<SCTAB> &TheTabs, bool bRecord ) bool bDelDone = false; - for(int i=TheTabs.size()-1; i>=0; --i) + std::vector<ScTable*> aSheetViewHolderTablesToDelete; + + for (sal_Int32 i = rTabs.size() - 1; i >= 0; --i) { + SCTAB nTab = rTabs[i]; OUString sCodeName; - bool bHasCodeName = rDoc.GetCodeName( TheTabs[i], sCodeName ); - if (rDoc.DeleteTab(TheTabs[i])) + bool bHasCodeName = rDoc.GetCodeName(nTab, sCodeName); + + // If we delete the current viewed table, make sure that we exit any sheet views + if (GetViewData().GetTabNumber() == nTab && GetViewData().GetSheetViewID() != sc::DefaultSheetViewID) + GetViewData().SetSheetViewID(sc::DefaultSheetViewID); + + if (auto pManager = rDoc.GetSheetViewManager(nTab)) + { + for (auto const& pSheetView : pManager->getSheetViews()) + { + aSheetViewHolderTablesToDelete.push_back(pSheetView->getTablePointer()); + } + } + + if (rDoc.DeleteTab(nTab)) { bDelDone = true; if( bVbaEnabled && bHasCodeName ) { VBA_DeleteModule( *pDocSh, sCodeName ); } - pDocSh->Broadcast( ScTablesHint( SC_TAB_DELETED, TheTabs[i] ) ); + pDocSh->Broadcast(ScTablesHint(SC_TAB_DELETED, nTab)); } } + + // Delete sheet view holder tables if it's table has been deleted + for (auto* pTable : aSheetViewHolderTablesToDelete) + { + SCTAB nTab = pTable->GetTab(); + if (rDoc.DeleteTab(nTab)) + { + pDocSh->Broadcast(ScTablesHint(SC_TAB_DELETED, nTab)); + } + } + if (bRecord) { pDocSh->GetUndoManager()->AddUndoAction( - std::make_unique<ScUndoDeleteTab>( GetViewData().GetDocShell(), TheTabs, - std::move(pUndoDoc), std::move(pUndoData) )); + std::make_unique<ScUndoDeleteTab>(GetViewData().GetDocShell(), rTabs, + std::move(pUndoDoc), std::move(pUndoData))); } if (bDelDone) diff --git a/sc/source/ui/view/viewfun3.cxx b/sc/source/ui/view/viewfun3.cxx index 829a7f10c4bc..591f30722076 100644 --- a/sc/source/ui/view/viewfun3.cxx +++ b/sc/source/ui/view/viewfun3.cxx @@ -2091,6 +2091,9 @@ void ScViewFunc::MakeNewSheetView() rDocSh.Broadcast(ScTablesHint(SC_TAB_INSERTED, nSheetViewTab)); SfxGetpApp()->Broadcast(SfxHint(SfxHintId::ScTablesChanged)); + // Need to make sure we return to the main sheet, to make sure we are not at a different location after inserting + SetTabNo(nTab); + SheetViewChanged(); }
