sw/source/core/tox/tox.cxx | 8 +++++++-
sw/source/ui/index/swuiidxmrk.cxx | 37 ++++++++++++++++++++-----------------
sw/source/uibase/index/toxmgr.cxx | 3 ++-
3 files changed, 29 insertions(+), 19 deletions(-)
New commits:
commit d22a86089edfcadbef5231525a2947b954f4784e
Author: Armin Le Grand (allotropia) <armin.le.grand.ext...@allotropia.de>
AuthorDate: Fri Jan 12 11:22:04 2024 +0100
Commit: Armin Le Grand <armin.le.gr...@me.com>
CommitDate: Fri Jan 12 15:22:36 2024 +0100
tdf#158783 Correct compares of SwTOXMark Items
That item was never 'pooled', so operator== was not really
ever used. It just compared the 'type', so pretty many
instances were assumed to be equal, what is wrong.
We discussed to implement it (there is quite some content),
but we came to the point that it's only safe to say
instances are equal when same instance -> fallback to ptr
compare.
This came into play since I identified/changed many (160?)
places where SfxPoolItems were ptr-compared when doing that
paradigm change in Items. This leads to the two methods
'areSfxPoolItemPtrsEqual' which just makes ptr compare and
'SfxPoolItem::areSame' which also will use op==. For the
initial adaption I chose the wrong function adapting
places where SwTOXMark were involved.
Change-Id: I7df029ad4542719681b1455de17ed5990d248395
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161963
Reviewed-by: Michael Stahl <michael.st...@allotropia.de>
Tested-by: Armin Le Grand <armin.le.gr...@me.com>
diff --git a/sw/source/core/tox/tox.cxx b/sw/source/core/tox/tox.cxx
index 04f43e5d41f8..b29bafde11f6 100644
--- a/sw/source/core/tox/tox.cxx
+++ b/sw/source/core/tox/tox.cxx
@@ -148,7 +148,13 @@ void SwTOXMark::RegisterToTOXType(SwTOXType& rType)
bool SwTOXMark::operator==( const SfxPoolItem& rAttr ) const
{
assert(SfxPoolItem::operator==(rAttr));
- return m_pType == static_cast<const SwTOXMark&>(rAttr).m_pType;
+ // tdf#158783 this item was never 'pooled', so operator== was not really
+ // ever used. We discussed to implement it (there is quite some
+ // content), but we came to the point that it's only safe to say
+ // instances are equal when same instance -> fallback to ptr compare.
+ // NOTE: Do *not* use areSfxPoolItemPtrsEqual here, with DBG_UTIL
+ // active the contol/test code there would again call operator==
+ return this == &rAttr;
}
SwTOXMark* SwTOXMark::Clone( SfxItemPool* ) const
diff --git a/sw/source/ui/index/swuiidxmrk.cxx
b/sw/source/ui/index/swuiidxmrk.cxx
index a0a8a2f93c0c..92c25f2e9aee 100644
--- a/sw/source/ui/index/swuiidxmrk.cxx
+++ b/sw/source/ui/index/swuiidxmrk.cxx
@@ -287,19 +287,20 @@ void SwIndexMarkPane::InitControls()
bool bShow = false;
pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_PRV );
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ // tdf#158783 ptr compare OK for SwTOXMark (more below)
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
{
m_pSh->GotoTOXMark( *pMoveMark, TOX_NXT );
bShow = true;
}
- m_xPrevBT->set_sensitive(!SfxPoolItem::areSame(pMoveMark, pMark));
+ m_xPrevBT->set_sensitive(!areSfxPoolItemPtrsEqual(pMoveMark, pMark));
pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_NXT );
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
{
m_pSh->GotoTOXMark( *pMoveMark, TOX_PRV );
bShow = true;
}
- m_xNextBT->set_sensitive(!SfxPoolItem::areSame(pMoveMark, pMark));
+ m_xNextBT->set_sensitive(!areSfxPoolItemPtrsEqual(pMoveMark, pMark));
if( bShow )
{
m_xPrevBT->show();
@@ -308,19 +309,19 @@ void SwIndexMarkPane::InitControls()
}
pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_SAME_PRV );
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
{
m_pSh->GotoTOXMark( *pMoveMark, TOX_SAME_NXT );
bShow = true;
}
- m_xPrevSameBT->set_sensitive(!SfxPoolItem::areSame(pMoveMark, pMark));
+ m_xPrevSameBT->set_sensitive(!areSfxPoolItemPtrsEqual(pMoveMark,
pMark));
pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_SAME_NXT );
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
{
m_pSh->GotoTOXMark( *pMoveMark, TOX_SAME_PRV );
bShow = true;
}
- m_xNextSameBT->set_sensitive(!SfxPoolItem::areSame(pMoveMark, pMark));
+ m_xNextSameBT->set_sensitive(!areSfxPoolItemPtrsEqual(pMoveMark,
pMark));
if( bShow )
{
m_xNextSameBT->show();
@@ -894,25 +895,26 @@ void SwIndexMarkPane::UpdateDialog()
if( m_xPrevBT->get_visible() )
{
const SwTOXMark* pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_PRV );
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ // tdf#158783 ptr compare OK for SwTOXMark (more below)
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
m_pSh->GotoTOXMark( *pMoveMark, TOX_NXT );
- m_xPrevBT->set_sensitive( !SfxPoolItem::areSame(pMoveMark, pMark) );
+ m_xPrevBT->set_sensitive( !areSfxPoolItemPtrsEqual(pMoveMark, pMark) );
pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_NXT );
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
m_pSh->GotoTOXMark( *pMoveMark, TOX_PRV );
- m_xNextBT->set_sensitive( !SfxPoolItem::areSame(pMoveMark, pMark) );
+ m_xNextBT->set_sensitive( !areSfxPoolItemPtrsEqual(pMoveMark, pMark) );
}
if (m_xPrevSameBT->get_visible())
{
const SwTOXMark* pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_SAME_PRV
);
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
m_pSh->GotoTOXMark( *pMoveMark, TOX_SAME_NXT );
- m_xPrevSameBT->set_sensitive( !SfxPoolItem::areSame(pMoveMark, pMark)
);
+ m_xPrevSameBT->set_sensitive( !areSfxPoolItemPtrsEqual(pMoveMark,
pMark) );
pMoveMark = &m_pSh->GotoTOXMark( *pMark, TOX_SAME_NXT );
- if (!SfxPoolItem::areSame( pMoveMark, pMark ))
+ if (!areSfxPoolItemPtrsEqual( pMoveMark, pMark ))
m_pSh->GotoTOXMark( *pMoveMark, TOX_SAME_PRV );
- m_xNextSameBT->set_sensitive( !SfxPoolItem::areSame(pMoveMark, pMark)
);
+ m_xNextSameBT->set_sensitive( !areSfxPoolItemPtrsEqual(pMoveMark,
pMark) );
}
const bool bEnable = !m_pSh->HasReadonlySel();
@@ -1014,7 +1016,8 @@ void SwIndexMarkPane::ReInitDlg(SwWrtShell& rWrtShell,
SwTOXMark const * pCurTOX
if(pCurTOXMark)
{
for(sal_uInt16 i = 0; i < m_pTOXMgr->GetTOXMarkCount(); i++)
- if (SfxPoolItem::areSame(m_pTOXMgr->GetTOXMark(i), pCurTOXMark))
+ // tdf#158783 ptr compare OK for SwTOXMark (more below)
+ if (areSfxPoolItemPtrsEqual(m_pTOXMgr->GetTOXMark(i), pCurTOXMark))
{
m_pTOXMgr->SetCurTOXMark(i);
break;
diff --git a/sw/source/uibase/index/toxmgr.cxx
b/sw/source/uibase/index/toxmgr.cxx
index 8b8ff6dbd8ec..c7cd813eb428 100644
--- a/sw/source/uibase/index/toxmgr.cxx
+++ b/sw/source/uibase/index/toxmgr.cxx
@@ -47,7 +47,8 @@ void SwTOXMgr::DeleteTOXMark()
if( m_pCurTOXMark )
{
pNext = const_cast<SwTOXMark*>(&m_pSh->GotoTOXMark( *m_pCurTOXMark,
TOX_NXT ));
- if (SfxPoolItem::areSame( pNext, m_pCurTOXMark ))
+ // tdf#158783 ptr compare OK for SwTOXMark (more below)
+ if (areSfxPoolItemPtrsEqual( pNext, m_pCurTOXMark ))
pNext = nullptr;
m_pSh->DeleteTOXMark( m_pCurTOXMark );
