Posted by dxli on Apr 10, 2025; 7:39pm
URL: https://forum.librecad.org/fixed-thanks-please-a-tip-for-the-RS-Eventhandler-tp5726883p5726890.html

We got quite many bugs from the eventhandler: due to eventhandler mismanaged action(yes, an RS_ActionInterface, not QAction) lifetime.

The recent changes I added as fast fixes to add IsActive(), for example.

The fix is still bad, for example, the test of IsActive() and Is Inactive() are implementated independently. Actually, one of them should be calling the other, to ensure consistence and correctness.

The worse problem is the API in eventhandler:

void SetCurrentAction(RS_ActionInterface* action);

internally, the call would take over the ownership of the action, but not as implied by the API.

It really should be:

void SetCurrentAction(std::unique_ptr<RS_ActionInterface> action);

Then, the API is clear that the ownership is transferred, and correctness of the ownership/lifetime is by the API not, by implementation.

The current design was just one quick fix after another, mostly badly implemented by me. For example, trying to be a minimum change, used shared_ptr instead of unique_ptr, to avoid handling copying. Then, the code got ugly, and more importantly buggy over time.

It worthwhile to clean it up with QAction handling as well.

sand1024 wrote

@dxli
As for the ownership of actions - you mean ones inherited from RS_ActionInterface, not QAction, right?

Actually, I don't think the ownership will be a huge issues there - plus so far the codebase includes several places where actions are created outside of RS_EventHandler for various one-time operations.

So far I never seen any issues with the ownership. During the development some issues may occur with action's QAction checking/unchecking - but they are easy to deal with.  So far the overall actions's lifecycle management is fine, as for me.

Yet of course, some code in RS_EventHandler might be improved.  I've made some cleanup in that area already, I'll put these changes in nearest PR.