vcl/inc/wizdlg.hxx | 16 +----- vcl/source/control/roadmapwizard.cxx | 92 +++++++++++++++-------------------- 2 files changed, 44 insertions(+), 64 deletions(-)
New commits: commit e6073530d9058f8bf3176a15f32cb9ea5ec94a3a Author: Michael Weghorn <[email protected]> AuthorDate: Mon May 19 12:16:47 2025 +0200 Commit: Michael Weghorn <[email protected]> CommitDate: Mon May 19 23:31:12 2025 +0200 vcl: Have only one pointer to ORoadmap So far, RoadmapWizard had two pointers to the ORoadmap it uses (both pointing to the same object): RoadmapWizard::mpViewWindow had a vcl::Window pointer to the same object as the RoadmapWizardImpl::pRoadmap of its `m_xRoadmapImpl` was pointing to (both set in the ctor and disposed/unset in RoadmapWizard::dispose). Rename RoadmapWizard::mpViewWindow to RoadmapWizard::mpRoadmap and let it be the one and only pointer to the OWizard. Change-Id: I8d2becd4c9d84883d00222cccd35a9402df38ca5 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/185524 Tested-by: Jenkins Reviewed-by: Michael Weghorn <[email protected]> diff --git a/vcl/inc/wizdlg.hxx b/vcl/inc/wizdlg.hxx index 4805e8e2080c..5ba43579d6fc 100644 --- a/vcl/inc/wizdlg.hxx +++ b/vcl/inc/wizdlg.hxx @@ -57,7 +57,6 @@ namespace vcl struct RoadmapWizardImpl { - ScopedVclPtr<ORoadmap> pRoadmap; std::map<VclPtr<vcl::Window>, short> maResponses; Paths aPaths; PathId nActivePath; @@ -65,8 +64,7 @@ namespace vcl bool bActivePathIsDefinite; RoadmapWizardImpl() - :pRoadmap( nullptr ) - ,nActivePath( PathId::INVALID ) + :nActivePath( PathId::INVALID ) ,bActivePathIsDefinite( false ) { } @@ -109,7 +107,7 @@ namespace vcl VclPtr<TabPage> mpCurTabPage; VclPtr<PushButton> mpPrevBtn; VclPtr<PushButton> mpNextBtn; - VclPtr<vcl::Window> mpViewWindow; + VclPtr<ORoadmap> mpRoadmap; sal_uInt16 mnCurLevel; sal_Int16 mnLeftAlignCount; bool mbEmptyViewMargin; diff --git a/vcl/source/control/roadmapwizard.cxx b/vcl/source/control/roadmapwizard.cxx index 5300fa0c327e..7c2b5d427479 100644 --- a/vcl/source/control/roadmapwizard.cxx +++ b/vcl/source/control/roadmapwizard.cxx @@ -90,7 +90,6 @@ namespace vcl mpCurTabPage = nullptr; mpPrevBtn = nullptr; mpNextBtn = nullptr; - mpViewWindow = nullptr; mnCurLevel = 0; mbEmptyViewMargin = false; mnLeftAlignCount = 0; @@ -103,22 +102,21 @@ namespace vcl SetLeftAlignedButtonCount( 1 ); mbEmptyViewMargin = true; - m_xRoadmapImpl->pRoadmap.disposeAndReset( VclPtr<ORoadmap>::Create( this, WB_TABSTOP ) ); - m_xRoadmapImpl->pRoadmap->SetText( VclResId( STR_WIZDLG_ROADMAP_TITLE ) ); - m_xRoadmapImpl->pRoadmap->SetPosPixel( Point( 0, 0 ) ); - m_xRoadmapImpl->pRoadmap->SetItemSelectHdl( LINK( this, RoadmapWizard, OnRoadmapItemSelected ) ); + mpRoadmap = VclPtr<ORoadmap>::Create(this, WB_TABSTOP); + mpRoadmap->SetText( VclResId( STR_WIZDLG_ROADMAP_TITLE ) ); + mpRoadmap->SetPosPixel( Point( 0, 0 ) ); + mpRoadmap->SetItemSelectHdl( LINK( this, RoadmapWizard, OnRoadmapItemSelected ) ); Size aRoadmapSize = LogicToPixel(Size(85, 0), MapMode(MapUnit::MapAppFont)); aRoadmapSize.setHeight( GetSizePixel().Height() ); - m_xRoadmapImpl->pRoadmap->SetSizePixel( aRoadmapSize ); + mpRoadmap->SetSizePixel( aRoadmapSize ); - mpViewWindow = m_xRoadmapImpl->pRoadmap; - m_xRoadmapImpl->pRoadmap->Show(); + mpRoadmap->Show(); } void RoadmapWizard::ShowRoadmap(bool bShow) { - m_xRoadmapImpl->pRoadmap->Show(bShow); + mpRoadmap->Show(bShow); CalcAndSetSize(); } @@ -161,18 +159,18 @@ namespace vcl mpCurTabPage.clear(); mpPrevBtn.clear(); mpNextBtn.clear(); - mpViewWindow.clear(); + mpRoadmap.disposeAndClear(); Dialog::dispose(); } void RoadmapWizard::SetRoadmapHelpId( const OUString& _rId ) { - m_xRoadmapImpl->pRoadmap->SetHelpId( _rId ); + mpRoadmap->SetHelpId( _rId ); } void RoadmapWizard::SetRoadmapBitmap(const BitmapEx& rBmp) { - m_xRoadmapImpl->pRoadmap->SetRoadmapBitmap(rBmp); + mpRoadmap->SetRoadmapBitmap(rBmp); } void RoadmapWizard::SetLeftAlignedButtonCount( sal_Int16 _nCount ) @@ -204,9 +202,9 @@ namespace vcl rSize.AdjustHeight(nMaxHeight); // add in the view window size - if ( mpViewWindow && mpViewWindow->IsVisible() ) + if ( mpRoadmap && mpRoadmap->IsVisible() ) { - Size aViewSize = mpViewWindow->GetSizePixel(); + Size aViewSize = mpRoadmap->GetSizePixel(); // align left rSize.AdjustWidth(aViewSize.Width() ); } @@ -287,7 +285,7 @@ namespace vcl nOffY -= WIZARDDIALOG_BUTTON_OFFSET_Y; } - if ( !(mpViewWindow && mpViewWindow->IsVisible()) ) + if ( !(mpRoadmap && mpRoadmap->IsVisible()) ) return; tools::Long nViewOffX = 0; @@ -312,7 +310,7 @@ namespace vcl } nViewPosFlags |= PosSizeFlags::Height; } - mpViewWindow->setPosSizePixel( nViewOffX, nViewOffY, + mpRoadmap->setPosSizePixel( nViewOffX, nViewOffY, nViewWidth, nViewHeight, nViewPosFlags ); } @@ -355,9 +353,9 @@ namespace vcl aDlgSize.AdjustHeight( -nMaxHeight ); tools::Long nOffX = 0; tools::Long nOffY = 0; - if ( mpViewWindow && mpViewWindow->IsVisible() ) + if ( mpRoadmap && mpRoadmap->IsVisible() ) { - Size aViewSize = mpViewWindow->GetSizePixel(); + Size aViewSize = mpRoadmap->GetSizePixel(); // align left tools::Long nViewOffset = mbEmptyViewMargin ? 0 : WIZARDDIALOG_VIEW_DLGOFFSET_X; nOffX += aViewSize.Width() + nViewOffset; @@ -562,7 +560,7 @@ namespace vcl // synchronize the roadmap implUpdateRoadmap(); - m_xRoadmapImpl->pRoadmap->SelectRoadmapItemByID(getCurrentState()); + mpRoadmap->SelectRoadmapItemByID(getCurrentState()); ImplShowTabPage( ImplGetPage( mnCurLevel ) ); } @@ -858,10 +856,10 @@ namespace vcl // now, we have to remove all items after nCurrentStatePathIndex, and insert the items from the active // path, up to (excluding) nUpperStepBoundary - RoadmapTypes::ItemIndex nLoopUntil = ::std::max( nUpperStepBoundary, m_xRoadmapImpl->pRoadmap->GetItemCount() ); + RoadmapTypes::ItemIndex nLoopUntil = ::std::max( nUpperStepBoundary, mpRoadmap->GetItemCount() ); for ( RoadmapTypes::ItemIndex nItemIndex = nCurrentStatePathIndex; nItemIndex < nLoopUntil; ++nItemIndex ) { - bool bExistentItem = ( nItemIndex < m_xRoadmapImpl->pRoadmap->GetItemCount() ); + bool bExistentItem = ( nItemIndex < mpRoadmap->GetItemCount() ); bool bNeedItem = ( nItemIndex < nUpperStepBoundary ); bool bInsertItem = false; @@ -869,19 +867,19 @@ namespace vcl { if ( !bNeedItem ) { - while ( nItemIndex < m_xRoadmapImpl->pRoadmap->GetItemCount() ) - m_xRoadmapImpl->pRoadmap->DeleteRoadmapItem( nItemIndex ); + while ( nItemIndex < mpRoadmap->GetItemCount() ) + mpRoadmap->DeleteRoadmapItem( nItemIndex ); break; } else { // there is an item with this index in the roadmap - does it match what is requested by // the respective state in the active path? - RoadmapTypes::ItemId nPresentItemId = m_xRoadmapImpl->pRoadmap->GetItemID( nItemIndex ); + RoadmapTypes::ItemId nPresentItemId = mpRoadmap->GetItemID( nItemIndex ); WizardTypes::WizardState nRequiredState = rActivePath[ nItemIndex ]; if ( nPresentItemId != nRequiredState ) { - m_xRoadmapImpl->pRoadmap->DeleteRoadmapItem( nItemIndex ); + mpRoadmap->DeleteRoadmapItem( nItemIndex ); bInsertItem = true; } } @@ -895,7 +893,7 @@ namespace vcl WizardTypes::WizardState nState( rActivePath[ nItemIndex ] ); if ( bInsertItem ) { - m_xRoadmapImpl->pRoadmap->InsertRoadmapItem( + mpRoadmap->InsertRoadmapItem( nItemIndex, getStateDisplayName( nState ), nState, @@ -904,10 +902,10 @@ namespace vcl } const bool bEnable = m_xRoadmapImpl->aDisabledStates.find( nState ) == m_xRoadmapImpl->aDisabledStates.end(); - m_xRoadmapImpl->pRoadmap->EnableRoadmapItem( m_xRoadmapImpl->pRoadmap->GetItemID( nItemIndex ), bEnable ); + mpRoadmap->EnableRoadmapItem( mpRoadmap->GetItemID( nItemIndex ), bEnable ); } - m_xRoadmapImpl->pRoadmap->SetRoadmapComplete( !bIncompletePath ); + mpRoadmap->SetRoadmapComplete( !bIncompletePath ); } WizardTypes::WizardState RoadmapWizard::determineNextState( WizardTypes::WizardState _nCurrentState ) const @@ -942,7 +940,7 @@ namespace vcl IMPL_LINK_NOARG(RoadmapWizard, OnRoadmapItemSelected, LinkParamNone*, void) { - RoadmapTypes::ItemId nCurItemId = m_xRoadmapImpl->pRoadmap->GetCurrentRoadmapItemID(); + RoadmapTypes::ItemId nCurItemId = mpRoadmap->GetCurrentRoadmapItemID(); if ( nCurItemId == getCurrentState() ) // nothing to do return; @@ -984,28 +982,28 @@ namespace vcl void RoadmapWizard::InsertRoadmapItem(int nItemIndex, const OUString& rText, int nItemId, bool bEnable) { - m_xRoadmapImpl->pRoadmap->InsertRoadmapItem(nItemIndex, rText, nItemId, bEnable); + mpRoadmap->InsertRoadmapItem(nItemIndex, rText, nItemId, bEnable); } void RoadmapWizard::SelectRoadmapItemByID(int nItemId, bool bGrabFocus) { - m_xRoadmapImpl->pRoadmap->SelectRoadmapItemByID(nItemId, bGrabFocus); + mpRoadmap->SelectRoadmapItemByID(nItemId, bGrabFocus); } void RoadmapWizard::DeleteRoadmapItems() { - while (m_xRoadmapImpl->pRoadmap->GetItemCount()) - m_xRoadmapImpl->pRoadmap->DeleteRoadmapItem(0); + while (mpRoadmap->GetItemCount()) + mpRoadmap->DeleteRoadmapItem(0); } void RoadmapWizard::SetItemSelectHdl( const Link<LinkParamNone*,void>& _rHdl ) { - m_xRoadmapImpl->pRoadmap->SetItemSelectHdl(_rHdl); + mpRoadmap->SetItemSelectHdl(_rHdl); } int RoadmapWizard::GetCurrentRoadmapItemID() const { - return m_xRoadmapImpl->pRoadmap->GetCurrentRoadmapItemID(); + return mpRoadmap->GetCurrentRoadmapItemID(); } FactoryFunction RoadmapWizard::GetUITestFactory() const @@ -1062,7 +1060,7 @@ namespace vcl { vcl::Window* pChild = GetChild(i); - if (!isButton(pChild->GetType()) && pChild != mpViewWindow) + if (!isButton(pChild->GetType()) && pChild != mpRoadmap) { auto childNode = rJsonWriter.startStruct(); pChild->DumpAsPropertyTree(rJsonWriter); commit 9aca8ef0dac45808823a3912131d9ba69f9c3327 Author: Michael Weghorn <[email protected]> AuthorDate: Mon May 19 11:27:18 2025 +0200 Commit: Michael Weghorn <[email protected]> CommitDate: Mon May 19 23:31:06 2025 +0200 vcl: Drop always true return values + fallback path RoadmapWizard::skipUntil always returns true since Change-Id: Ieea7ca5b6f0ce67ad6f3fac4942a6a62921bfe95 Author: Michael Weghorn <[email protected]> Date: Mon May 19 11:20:55 2025 +0200 vcl: Switch DBG_ASSERT and OSL_FAIL to assert, drop early return , RoadmapWizard::skipBackwardUntil already did earlier. Change-Id: Ifde2dbcbfd09b8602d28b131437722baaa86cb6c Reviewed-on: https://gerrit.libreoffice.org/c/core/+/185515 Tested-by: Jenkins Reviewed-by: Michael Weghorn <[email protected]> diff --git a/vcl/inc/wizdlg.hxx b/vcl/inc/wizdlg.hxx index edec50ab0b82..4805e8e2080c 100644 --- a/vcl/inc/wizdlg.hxx +++ b/vcl/inc/wizdlg.hxx @@ -210,13 +210,10 @@ namespace vcl The skipped states appear in the state history, so <method>travelPrevious</method> will make use of them. - @return - <TRUE/> if and only if traveling was successful - @see skip @see skipBackwardUntil */ - bool skipUntil(WizardTypes::WizardState nTargetState); + void skipUntil(WizardTypes::WizardState nTargetState); /** moves back one or more states, until a given state is reached @@ -228,13 +225,10 @@ namespace vcl since you're interested in the target page only. Using <member>skipBackwardUntil</member> relieves you of this. - @return - <TRUE/> if and only if traveling was successful - @see skipUntil @see skip */ - bool skipBackwardUntil(WizardTypes::WizardState nTargetState); + void skipBackwardUntil(WizardTypes::WizardState nTargetState); /** returns the current state of the machine diff --git a/vcl/source/control/roadmapwizard.cxx b/vcl/source/control/roadmapwizard.cxx index 46858c0d165d..5300fa0c327e 100644 --- a/vcl/source/control/roadmapwizard.cxx +++ b/vcl/source/control/roadmapwizard.cxx @@ -703,7 +703,7 @@ namespace vcl Finish( RET_OK ); } - bool RoadmapWizard::skipBackwardUntil( WizardTypes::WizardState _nTargetState ) + void RoadmapWizard::skipBackwardUntil(WizardTypes::WizardState _nTargetState) { // don't travel directly on m_xWizardImpl->aStateHistory, in case something goes wrong std::stack< WizardTypes::WizardState > aTravelVirtually = m_xWizardImpl->aStateHistory; @@ -717,10 +717,9 @@ namespace vcl } m_xWizardImpl->aStateHistory = std::move(aTravelVirtually); ShowPage(_nTargetState); - return true; } - bool RoadmapWizard::skipUntil( WizardTypes::WizardState _nTargetState ) + void RoadmapWizard::skipUntil(WizardTypes::WizardState _nTargetState) { WizardTypes::WizardState nCurrentState = getCurrentState(); @@ -740,7 +739,6 @@ namespace vcl m_xWizardImpl->aStateHistory = std::move(aTravelVirtually); // show the target page ShowPage(nCurrentState); - return true; } void RoadmapWizard::travelNext() @@ -964,10 +962,9 @@ namespace vcl return; } - bool bResult = true; if ( nNewIndex > nCurrentIndex ) { - bResult = skipUntil( static_cast<WizardTypes::WizardState>(nCurItemId) ); + skipUntil(static_cast<WizardTypes::WizardState>(nCurItemId)); WizardTypes::WizardState nTemp = static_cast<WizardTypes::WizardState>(nCurItemId); while( nTemp ) { @@ -976,10 +973,7 @@ namespace vcl } } else - bResult = skipBackwardUntil( static_cast<WizardTypes::WizardState>(nCurItemId) ); - - if ( !bResult ) - m_xRoadmapImpl->pRoadmap->SelectRoadmapItemByID( getCurrentState() ); + skipBackwardUntil(static_cast<WizardTypes::WizardState>(nCurItemId)); } OUString RoadmapWizard::getStateDisplayName( WizardTypes::WizardState /* _nState */) commit 176bfa25ab34e4943880d8cad7a04ca95fd3a98e Author: Michael Weghorn <[email protected]> AuthorDate: Mon May 19 11:20:55 2025 +0200 Commit: Michael Weghorn <[email protected]> CommitDate: Mon May 19 23:30:59 2025 +0200 vcl: Switch DBG_ASSERT and OSL_FAIL to assert, drop early return Take those serious so if there are any invalid calls, they can be detected and fixed. Change-Id: Ieea7ca5b6f0ce67ad6f3fac4942a6a62921bfe95 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/185514 Tested-by: Jenkins Reviewed-by: Michael Weghorn <[email protected]> diff --git a/vcl/source/control/roadmapwizard.cxx b/vcl/source/control/roadmapwizard.cxx index 5eb8bb677c52..46858c0d165d 100644 --- a/vcl/source/control/roadmapwizard.cxx +++ b/vcl/source/control/roadmapwizard.cxx @@ -711,7 +711,7 @@ namespace vcl WizardTypes::WizardState nCurrentRollbackState = getCurrentState(); while ( nCurrentRollbackState != _nTargetState ) { - DBG_ASSERT( !aTravelVirtually.empty(), "RoadmapWizard::skipBackwardUntil: this target state does not exist in the history!" ); + assert(!aTravelVirtually.empty() && "RoadmapWizard::skipBackwardUntil: this target state does not exist in the history!"); nCurrentRollbackState = aTravelVirtually.top(); aTravelVirtually.pop(); } @@ -729,11 +729,7 @@ namespace vcl while ( nCurrentState != _nTargetState ) { WizardTypes::WizardState nNextState = determineNextState( nCurrentState ); - if ( WZS_INVALID_STATE == nNextState ) - { - OSL_FAIL( "RoadmapWizard::skipUntil: the given target state does not exist!" ); - return false; - } + assert(nNextState != WZS_INVALID_STATE && "RoadmapWizard::skipUntil: the given target state does not exist!"); // remember the skipped state in the history aTravelVirtually.push( nCurrentState );
