Re: [@dxli and @sand1024] please a tip for the RS_Eventhandler

Posted by dxli on
URL: https://forum.librecad.org/fixed-thanks-please-a-tip-for-the-RS-Eventhandler-tp5726883p5726896.html

I have pushed a baby step change to setCurrentAction() API:

everything is by shared_ptr now. It works for me during my manual test.

If it introduces too much troubles, we can revert it:




sand1024 wrote
Yes, there are some historical code there, and as for the ownership - I'd rather say that actions queue may be reviewed.

SetCurrentAction() - well, it's called just in one places from GraphicView (probably this is my local version only so far).

And in general, the RS_EventHandler is quite an internal class, it's hardly possible that someone will need to interact with it directly.

What is actually worse regarding the overall design and ownership issue - is the general approach that allows to set any arbitrary action via bool RS_GraphicView::setCurrentAction()... and I suppose this is the root of the most of ownership issues.  

For most of cases where action is created directly in code (like other action or so) - it is possible just to skip creation of one-short action at all (say, for zooming, scroll etc.).

If such arbitrary setting of the current action will be eliminated and the current action will be instantiated and set set from some managed place (like QG_ActionHandler) - the api there will be less important than now.

The RS_EventHandler will be just internal class without necessity to access it directly from outer code, plus the lifecycle could be defined better - with clear indication of responsibilities that defines who create the instance, who owns and deletes it.

I've planned to review this too, yet didn't completed this yet.