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 | 44 +++++- sc/source/ui/view/viewfun3.cxx | 3 9 files changed, 279 insertions(+), 14 deletions(-)
New commits: commit 65fc7cb8387cd4249bde07b9ec20a10bb28028b1 Author: Tomaž Vajngerl <[email protected]> AuthorDate: Wed Oct 15 09:47:33 2025 +0900 Commit: Tomaž Vajngerl <[email protected]> CommitDate: Wed Oct 15 14:45:03 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/+/192423 Tested-by: Jenkins Reviewed-by: Tomaž Vajngerl <[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 9fb488eaaa81..3dd474ae86ca 100644 --- a/sc/inc/table.hxx +++ b/sc/inc/table.hxx @@ -253,7 +253,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; @@ -1189,11 +1190,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 761099707dfd..718e0c27b742 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 dee383e3ea05..82c50f985c55 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 e4eb11cc165c..aae3d7bbe3b0 100644 --- a/sc/source/core/data/document.cxx +++ b/sc/source/core/data/document.cxx @@ -102,6 +102,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 0331427d6b0c..e9edef8c98a6 100644 --- a/sc/source/core/data/document10.cxx +++ b/sc/source/core/data/document10.cxx @@ -1151,7 +1151,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 d2930d6c5090..ccb13f166a98 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> @@ -2546,12 +2547,12 @@ void ScViewFunc::DeleteTables( const SCTAB nTab, SCTAB nSheets ) pSfxApp->Broadcast( SfxHint( SfxHintId::ScAreaLinksChanged ) ); } -bool ScViewFunc::DeleteTables(const std::vector<SCTAB> &TheTabs, bool bRecord ) +bool ScViewFunc::DeleteTables(const std::vector<SCTAB>& rTabs, bool bRecord) { ScDocShell& rDocSh = GetViewData().GetDocShell(); ScDocument& rDoc = rDocSh.GetDocument(); bool bVbaEnabled = rDoc.IsInVBAMode(); - SCTAB nNewTab = TheTabs.front(); + SCTAB nNewTab = rTabs.front(); weld::WaitObject aWait(GetViewData().GetDialogParent()); if (bRecord && !rDoc.IsUndoEnabled()) bRecord = false; @@ -2571,7 +2572,7 @@ bool ScViewFunc::DeleteTables(const std::vector<SCTAB> &TheTabs, bool bRecord ) OUString aOldName; bool isFirstTab = true; - for(SCTAB nTab : TheTabs) + for (SCTAB nTab : rTabs) { if (isFirstTab) { @@ -2625,24 +2626,51 @@ bool ScViewFunc::DeleteTables(const std::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( rDocSh, sCodeName ); } - rDocSh.Broadcast( ScTablesHint( SC_TAB_DELETED, TheTabs[i] ) ); + rDocSh.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)) + { + rDocSh.Broadcast(ScTablesHint(SC_TAB_DELETED, nTab)); + } + } + if (bRecord) { rDocSh.GetUndoManager()->AddUndoAction( - std::make_unique<ScUndoDeleteTab>( GetViewData().GetDocShell(), TheTabs, + std::make_unique<ScUndoDeleteTab>( GetViewData().GetDocShell(), rTabs, std::move(pUndoDoc), std::move(pUndoData) )); } diff --git a/sc/source/ui/view/viewfun3.cxx b/sc/source/ui/view/viewfun3.cxx index 30f443a93441..1cd122a14115 100644 --- a/sc/source/ui/view/viewfun3.cxx +++ b/sc/source/ui/view/viewfun3.cxx @@ -2086,6 +2086,9 @@ void ScViewFunc::MakeNewSheetView() GetViewData().GetDocShell().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(); }
