diff --git a/common/tool/tool_manager.cpp b/common/tool/tool_manager.cpp index a4088035b4..e05b61a6ff 100644 --- a/common/tool/tool_manager.cpp +++ b/common/tool/tool_manager.cpp @@ -328,7 +328,7 @@ VECTOR2D TOOL_MANAGER::GetCursorPosition() const bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki::any& aParam, - COMMIT* aCommit ) + COMMIT* aCommit, bool aFromAPI ) { if( m_shuttingDown ) return true; @@ -336,10 +336,6 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki: bool retVal = false; TOOL_EVENT event = aAction.MakeEvent(); - // We initialize the SYNCHRONOUS state to finished so that tools that don't have an event - // loop won't hang if someone forgets to set the state. - std::atomic<SYNCRONOUS_TOOL_STATE> synchronousControl = STS_FINISHED; - if( event.Category() == TC_COMMAND ) event.SetMousePosition( GetCursorPosition() ); @@ -347,20 +343,20 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki: if( aParam.has_value() ) event.SetParameter( aParam ); - // Pass the commit (if any) - if( aCommit ) - { - event.SetSynchronous( &synchronousControl ); - event.SetCommit( aCommit ); - } - if( aNow ) { TOOL_STATE* current = m_activeState; + // An event with a commit must be run synchronously if( aCommit ) { - // An event with a commit must be run synchronously + // We initialize the SYNCHRONOUS state to finished so that tools that don't have an + // event loop won't hang if someone forgets to set the state. + std::atomic<SYNCRONOUS_TOOL_STATE> synchronousControl = STS_FINISHED; + + event.SetSynchronous( &synchronousControl ); + event.SetCommit( aCommit ); + processEvent( event ); while( synchronousControl == STS_RUNNING ) @@ -384,6 +380,17 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki: } else { + // It is really dangerous to pass a commit (whose lifetime we can't guarantee) to + // deferred event processing. There is a possibility that user actions will get run + // in between, which might either affect the lifetime of the commit or push or pop + // other commits. However, we don't currently have a better solution for the API. + if( aCommit ) + { + wxASSERT_MSG( aFromAPI, wxT( "Deferred actions have no way of guaranteeing the " + "lifetime of the COMMIT object" ) ); + event.SetCommit( aCommit ); + } + PostEvent( event ); } diff --git a/include/tool/tool_manager.h b/include/tool/tool_manager.h index 4fd7070981..1d0ce4b90d 100644 --- a/include/tool/tool_manager.h +++ b/include/tool/tool_manager.h @@ -256,9 +256,10 @@ public: * * @param aAction is the action to be invoked. * @param aParam is an optional parameter that might be used by the invoked action. Its meaning - * depends on the action. + * depends on the action. The parameter is not allowed to be a COMMIT as we have + * no way of guaranteeing the lifetime of the COMMIT object. */ - template<typename T> + template<typename T, std::enable_if_t<!std::is_convertible_v<T, COMMIT*>>* = nullptr> bool PostAction( const TOOL_ACTION& aAction, T aParam ) { // Use a cast to ensure the proper type is stored inside the parameter @@ -275,12 +276,15 @@ public: doRunAction( aAction, false, a, nullptr ); } - bool PostAction( const TOOL_ACTION& aAction, COMMIT* aCommit ) + // TODO: we need a way to guarantee that user events can't be processed in + // between API actions. Otherwise the COMMITs will get tangled up and we'll + // crash on bad pointers. + bool PostAPIAction( const TOOL_ACTION& aAction, COMMIT* aCommit ) { // Default initialize the parameter argument to an empty ki_any ki::any a; - return doRunAction( aAction, false, a, aCommit ); + return doRunAction( aAction, false, a, aCommit, true ); } /** @@ -538,7 +542,7 @@ private: * Helper function to actually run an action. */ bool doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki::any& aParam, - COMMIT* aCommit ); + COMMIT* aCommit, bool aFromAPI = false ); bool doRunAction( const std::string& aActionName, bool aNow, const ki::any& aParam, COMMIT* aCommit ); diff --git a/pcbnew/api/api_handler_pcb.cpp b/pcbnew/api/api_handler_pcb.cpp index 4e44b49b91..d1c313db3a 100644 --- a/pcbnew/api/api_handler_pcb.cpp +++ b/pcbnew/api/api_handler_pcb.cpp @@ -1138,7 +1138,7 @@ HANDLER_RESULT<Empty> API_HANDLER_PCB::handleInteractiveMoveItems( mgr->RunAction<EDA_ITEMS*>( PCB_ACTIONS::selectItems, &toSelect ); COMMIT* commit = getCurrentCommit( aCtx.ClientName ); - mgr->PostAction( PCB_ACTIONS::move, commit ); + mgr->PostAPIAction( PCB_ACTIONS::move, commit ); return Empty(); }