Hi Noel,
Had a quick read of:
https://gerrit.libreoffice.org/#/c/26516/
Looks rather good to me - all the instances which you highlight as
'dodgy' look really odd to me even in the pre-existing code ;-) deleting
items that still have private pointers lurking around in child
containers is ... ;-) 'interesting'.
"delete pFoo->LetMePokeInYourInnards()"
is a curious use of pFoo ;-)
What do you think of the attached to fix that ? (not compile tested
sadly - but hopefully squashes nicely and achieves what we want ?
Would love to have some of that automated UI testing stuff implemented
at this stage - but absent that, if it works with some manual testing -
it'd be good to get in I think; while an impressive change - it's far
smaller and less disruptive than the original VclPtr stuff I think =)
ATB,
Michael.
--
michael.me...@collabora.com <><, GM Collabora Productivity
Skype: mmeeks, Google Hangout: mejme...@gmail.com
(M) +44 7795 666 147 - timezone usually UK / Europe
diff --git a/cui/source/tabpages/numpages.cxx b/cui/source/tabpages/numpages.cxx
index 4eafff9..9f9deaf3 100644
--- a/cui/source/tabpages/numpages.cxx
+++ b/cui/source/tabpages/numpages.cxx
@@ -1258,8 +1258,7 @@ void SvxNumOptionsTabPage::dispose()
{
if (m_pBitmapMB)
{
-VclPtr p = m_pBitmapMB->GetPopupMenu()->GetPopupMenu(m_nGalleryId);
-p.disposeAndClear(); // NoelG: dodgy, this leaves a dangling pointer
+m_pBitmapMB->GetPopupMenu()->DisposePopupMenu(m_nGalleryId);
}
delete pActNum;
pActNum = nullptr;
diff --git a/cui/source/tabpages/tpline.cxx b/cui/source/tabpages/tpline.cxx
index ca323ab..99417a4 100644
--- a/cui/source/tabpages/tpline.cxx
+++ b/cui/source/tabpages/tpline.cxx
@@ -229,13 +229,11 @@ void SvxLineTabPage::dispose()
// Symbols on a line (e.g. StarCharts), dtor new!
if (m_pSymbolMB)
{
-VclPtr p = m_pSymbolMB->GetPopupMenu()->GetPopupMenu( MN_GALLERY );
-p.disposeAndClear(); // NoelG: dodgy, this leaves a dangling pointer
+m_pSymbolMB->GetPopupMenu()->DisposePopupMenu( MN_GALLERY );
if(m_pSymbolList)
{
-VclPtr p2 = m_pSymbolMB->GetPopupMenu()->GetPopupMenu( MN_SYMBOLS );
-p2.disposeAndClear(); // NoelG: dodgy, this leaves a dangling pointer
+m_pSymbolMB->GetPopupMenu()->DisposePopupMenu( MN_SYMBOLS );
}
m_pSymbolMB = nullptr;
}
diff --git a/include/vcl/menu.hxx b/include/vcl/menu.hxx
index 81bb0c5..46c675e 100644
--- a/include/vcl/menu.hxx
+++ b/include/vcl/menu.hxx
@@ -284,6 +284,7 @@ public:
void SetPopupMenu( sal_uInt16 nItemId, PopupMenu* pMenu );
PopupMenu* GetPopupMenu( sal_uInt16 nItemId ) const;
+void DisposePopupMenu( sal_uInt16 nItemId );
void SetAccelKey( sal_uInt16 nItemId, const vcl::KeyCode& rKeyCode );
vcl::KeyCode GetAccelKey( sal_uInt16 nItemId ) const;
diff --git a/svtools/source/contnr/svimpbox.cxx b/svtools/source/contnr/svimpbox.cxx
index 5a19237..ddd009e 100644
--- a/svtools/source/contnr/svimpbox.cxx
+++ b/svtools/source/contnr/svimpbox.cxx
@@ -2921,9 +2921,7 @@ static void lcl_DeleteSubPopups(PopupMenu* pPopup)
if(pSubPopup)
{
lcl_DeleteSubPopups(pSubPopup);
-// NoelG: this looks very dodgy to me, we are attempting to delete this, but we leave a dangling pointer
-// in the PopupMenu class?
-pSubPopup.disposeAndClear();
+pPopup->DisposePopupMenu( pPopup->GetItemId( i ));
}
}
}
diff --git a/svx/source/fmcomp/fmgridcl.cxx b/svx/source/fmcomp/fmgridcl.cxx
index 9a1fa11..8183f05 100644
--- a/svx/source/fmcomp/fmgridcl.cxx
+++ b/svx/source/fmcomp/fmgridcl.cxx
@@ -782,8 +782,7 @@ void FmGridHeader::PostExecuteColumnContextMenu(sal_uInt16 nColId, const PopupMe
sal_uInt16 nPos = GetModelColumnPos(nColId);
// remove and delete the menu we inserted in PreExecuteColumnContextMenu
-VclPtr pControlMenu = rMenu.GetPopupMenu(SID_FM_CHANGECOL);
-pControlMenu.disposeAndClear(); // NoelG: dodgy, this leaves a dangling pointer
+rMenu.DisposePopupMenu(SID_FM_CHANGECOL);
OUString aFieldType;
boolbReplace = false;
diff --git a/sw/source/uibase/ribbar/workctrl.cxx b/sw/source/uibase/ribbar/workctrl.cxx
index ad20752..12d9f81 100644
--- a/sw/source/uibase/ribbar/workctrl.cxx
+++ b/sw/source/uibase/ribbar/workctrl.cxx
@@ -164,8 +164,7 @@ void SwTbxAutoTextCtrl::DelPopup()
{
for( sal_uInt16 i = 0; i < pPopup->GetItemCount(); i ++ )
{
-VclPtr pSubPopup = pPopup->GetPopupMenu(pPopup->GetItemId(i));
-pSubPopup.disposeAndClear(); // NoelG: dodgy, this leaves a dangling pointer
+pPopup->DisposePopupMenu(pPopup->GetItemId(i));
}
pPopup.disposeAndClear();
}
diff --git a/vcl/source/window/menu.cxx b/vcl/source/window/menu.cxx
index c5e32f6..f146be4
