[fixed - thanks] please a tip for the RS_Eventhandler

Hello, I urgently need help.
I build an action with RS_Action interface, without a preview  (rendering of an AutoCad Slide).
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/actions/file/lc_actionfileviewslide.cpp

When I remove finish(false); from ::trigger, the action continues, but I don't get any events in the action.
Probably because the RS_EventHandler is put on hasAction.

What do I have to do to get the events in the action?

Just like here in the plugins:
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/main/doc_plugin_interface.cpp#L1232

I need this because the action is to be ended via events.
In AutoCAD, the command picture display in the GraphicView ended either via Zoom, Pan, or Redraw.

Hm... I'm not sure I understand the expected logic there.  

Is it a one-shoot action (so trigger is called in init and that's it)?

void LC_ActionFileViewSlide::init(int status) {
    RS_ActionInterface::init(status);
    trigger();
}


Or that action should do something else?  

Please expand a little on what you're going to achieve from the actions lifecycle point of view and probably I'll be able to point you to some existing action as reference or provide more info.

oh, now I see that you call finish(false); in trigger() method.

I"ll try to make some assumptions and guess  - but in general - I don't think that the current implementation is correct way from the overall actions' lifecycle.

If the action is not a one-shoot one that should be finished right after the invocation - instead of calling the trigger() in init(), you need to define the set of states for the action and maintain the state via setStatus() method.

It might be that only one state is supported by the action, that's also ok.  

Later, in event handlers (like onMouseMove etc.) - you'll do necessary processing according to the current state of the action.  

By default - if you'd like to finish the action, you can call setStatus(-1) (any negative value for the state, actually). Usually right button click returns the action to the previous state until it finish.

It reminds me the longer time design issues with eventhandler:

1. actions should be owned by eventhandler, but currently, actions are held by eventhandler as shared_ptr. Wondering whether it's better to use unique_ptr instead;
2. the setCurrentAction() api should all be in smart pointers.

sand1024 wrote

oh, now I see that you call finish(false); in trigger() method.

I"ll try to make some assumptions and guess  - but in general - I don't think that the current implementation is correct way from the overall actions' lifecycle.

If the action is not a one-shoot one that should be finished right after the invocation - instead of calling the trigger() in init(), you need to define the set of states for the action and maintain the state via setStatus() method.

It might be that only one state is supported by the action, that's also ok.  

Later, in event handlers (like onMouseMove etc.) - you'll do necessary processing according to the current state of the action.  

By default - if you'd like to finish the action, you can call setStatus(-1) (any negative value for the state, actually). Usually right button click returns the action to the previous state until it finish.

@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.

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.

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.

Thanks first. Now I can continue to test.

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.

I'm not really making any progress.

I have a class:

class LC_OverlaySlideAction:public RS_ActionInterface {
public:
    LC_OverlaySlideAction(
        const char *name,
        RS_EntityContainer &container,
        RS_GraphicView &graphicView,
        RS2::ActionType actionType = RS2::ActionNone);

    ~LC_OverlaySlideAction() override = default;

    void init(int status) override;
    void finish(bool updateTB = true) override;
    void trigger() override;
    void mouseMoveEvent(QMouseEvent* e) override;
    void mousePressEvent(QMouseEvent* e) override;
    void mouseReleaseEvent(QMouseEvent* e) override;

protected:
    void drawOverlaySlide(const QString &fileName);
private:
    bool middleButtonPressed{false};
};

but when I end the action with "ESC," RS_EventHandler::killAllActions() always calls RS_ActionInterface::finish(), never the overridden LC_OverlaySlideAction::finish().

Why doesn't this work? I have to reset values on finish.



Even weirder is, if I add just a virtual void bla() {} to the public: property of the RS_ActionInterface class, the entire LC crashes.
If I add it in different places, LC crashes in completely different places.
Once with QWidget attributes, or RS_Snapper.
Strange behavior.

class RS_ActionInterface : public RS_Snapper {
Q_OBJECT
public:
    RS_ActionInterface(const char* name,
                       RS_EntityContainer& container,
                       RS_GraphicView& graphicView,
                       RS2::ActionType actionType = RS2::ActionNone);
           ~RS_ActionInterface() override;

    virtual RS2::ActionType rtti() const;

    void setName(const char* _name);
    QString getName();

    virtual void init(int status);
   
    /* virtual void bla() {} <<<<<<< crashes QWidget attributes ??? */
   
    virtual void mouseMoveEvent(QMouseEvent*);
    virtual void mousePressEvent(QMouseEvent*);
    virtual void mouseReleaseEvent(QMouseEvent*);
    virtual void keyPressEvent(QKeyEvent* e);
    virtual void keyReleaseEvent(QKeyEvent* e);
    virtual void coordinateEvent(RS_CoordinateEvent*);
    virtual void commandEvent(RS_CommandEvent*);
    virtual QStringList getAvailableCommands();
    virtual void setStatus(int status);
    int getStatus() const;
    virtual void trigger();
    virtual bool isFinished() const;
    virtual void setFinished();
    virtual void finish(bool updateTB = true );
    virtual void setPredecessor(RS_ActionInterface* pre);
    void suspend() override;
    void resume() override;
    virtual void hideOptions();
    virtual void showOptions();
   
    /* virtual void bla() {} <<<<<<< crashes RS_Snapper ??? */
   
    ...

The crashing issue could hint an undefined behavior (buffer overflow, invalid pointer dereferenced, etc.)

Could you share a git branch with the issue? Of course, it helps to describe steps to reproduce.

The "finish()" issue is likely due to lifetime management as well.

Could you rebase to my latest changes to avoid raw pointers for actions?

emanuel wrote

I'm not really making any progress.

I have a class:

class LC_OverlaySlideAction:public RS_ActionInterface {
public:
    LC_OverlaySlideAction(
        const char *name,
        RS_EntityContainer &container,
        RS_GraphicView &graphicView,
        RS2::ActionType actionType = RS2::ActionNone);

    ~LC_OverlaySlideAction() override = default;

    void init(int status) override;
    void finish(bool updateTB = true) override;
    void trigger() override;
    void mouseMoveEvent(QMouseEvent* e) override;
    void mousePressEvent(QMouseEvent* e) override;
    void mouseReleaseEvent(QMouseEvent* e) override;

protected:
    void drawOverlaySlide(const QString &fileName);
private:
    bool middleButtonPressed{false};
};

but when I end the action with "ESC," RS_EventHandler::killAllActions() always calls RS_ActionInterface::finish(), never the overridden LC_OverlaySlideAction::finish().

Why doesn't this work? I have to reset values on finish.



Even weirder is, if I add just a virtual void bla() {} to the public: property of the RS_ActionInterface class, the entire LC crashes.
If I add it in different places, LC crashes in completely different places.
Once with QWidget attributes, or RS_Snapper.
Strange behavior.

class RS_ActionInterface : public RS_Snapper {
Q_OBJECT
public:
    RS_ActionInterface(const char* name,
                       RS_EntityContainer& container,
                       RS_GraphicView& graphicView,
                       RS2::ActionType actionType = RS2::ActionNone);
           ~RS_ActionInterface() override;

    virtual RS2::ActionType rtti() const;

    void setName(const char* _name);
    QString getName();

    virtual void init(int status);
   
    /* virtual void bla() {} <<<<<<< crashes QWidget attributes ??? */
   
    virtual void mouseMoveEvent(QMouseEvent*);
    virtual void mousePressEvent(QMouseEvent*);
    virtual void mouseReleaseEvent(QMouseEvent*);
    virtual void keyPressEvent(QKeyEvent* e);
    virtual void keyReleaseEvent(QKeyEvent* e);
    virtual void coordinateEvent(RS_CoordinateEvent*);
    virtual void commandEvent(RS_CommandEvent*);
    virtual QStringList getAvailableCommands();
    virtual void setStatus(int status);
    int getStatus() const;
    virtual void trigger();
    virtual bool isFinished() const;
    virtual void setFinished();
    virtual void finish(bool updateTB = true );
    virtual void setPredecessor(RS_ActionInterface* pre);
    void suspend() override;
    void resume() override;
    virtual void hideOptions();
    virtual void showOptions();
   
    /* virtual void bla() {} <<<<<<< crashes RS_Snapper ??? */
   
    ...

I don't know why either, but now suddenly everything works.
That was probably a stupid patch day yesterday...

Suddenly I can control all the events: I can now end the "vslide" command with the gestures "Pan", "Zoom", "ESC".
Unfortunately, it only works with the 2 switches here:
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/ui/view/qg_graphicview.h#L121
to get events like:
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/lib/actions/rs_actioninterface.h#L87

The result is very good, please take a look at the video:
simplescreenrecorder-2025-04-12_13.mp4
However, I have a shortcoming, I don't get to the background color, which is always "black".
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/lib/engine/overlays/overlay_slide/rs_overlayslide.cpp#L44

Do you have an idea why?

RS_Settings::background is "Black"

So, it's expected to be black. You can directly change RS_Color to get a different background color

emanuel wrote

I don't know why either, but now suddenly everything works.
That was probably a stupid patch day yesterday...

Suddenly I can control all the events: I can now end the "vslide" command with the gestures "Pan", "Zoom", "ESC".
Unfortunately, it only works with the 2 switches here:
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/ui/view/qg_graphicview.h#L121
to get events like:
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/lib/actions/rs_actioninterface.h#L87

The result is very good, please take a look at the video:
simplescreenrecorder-2025-04-12_13.mp4
However, I have a shortcoming, I don't get to the background color, which is always "black".
https://github.com/emanuel4you/LibreCAD/blob/developer/librecad/src/lib/engine/overlays/overlay_slide/rs_overlayslide.cpp#L44

Do you have an idea why?

I need the current setting, set by:

Then, use RS_Settings should be something like:


    LC_GROUP_GUARD("Colors");
    auto bgc = QColor(LC_GET_STR("background", RS_Settings::background));
    RS_Color fillBackground{bgc};

The RS_Settings::background is a static const, not actually set by the GUI user setting. Maybe, we should make it consistent with the user setting.

emanuel wrote

I need the current setting, set by:

Color works now, thanks!

Now I'm happy.
I built a cleanup function, getEventHandler()->killShownActions(); similar to killSelectActions(). This allowed me to reset 80% of the patch. I no longer need switches, the RS_ActionInterface is back to original, and I no longer need a wheelEvent in the action. And everything works, even the zoom buttons and the command line eliminate the slide display. :-)

well, yet the implementation may be improved a little:

1) Please note that the common approach for states management of actions - just create an enum that contains each state value and refer that enum values in setStatus()/getStatus() instead of raw int numbers.
This significantly improves readability.

2) the location of src\lib\actions\lc_overlayslideaction.cpp seems not the optimal - the \lib mostly contains base code. The lc_overlayboxaction.h  is there since it's used for the basic entities selection.
Yet that implementation... well, it's basically a subset of RS_PreviewActionInterface.

Basically lc_overlayslideaction may be eliminated at all, if you'll inherit LC_ActionFileViewSlide  from RS_PreviewActionInterface (plus, there are more specific methods - say, you can rely on onMouseRightButtonRelease method for handling right button click).

4) I didn't dig deeply into the implementation, but this code seems to be suspicious for me:


void RS_OverlaySlide::draw(RS_Painter* painter)
{
    LC_GROUP_GUARD("Colors");
    bool darkBackground = true;
    auto bgc = QColor(LC_GET_STR("background", RS_Settings::background));
    const RS_Color &fillBackground{bgc};
    if((bgc.red() * 299 + bgc.green() * 587 + bgc.blue() * 114) / 1000 >= 125)
    {
        darkBackground = false;
    }

    painter->fillRect(0, 0, m_width, m_height, fillBackground);

    slide_draw_qpainter(painter,
                        0,
                        0,
                        m_width,
                        m_height,
                        darkBackground,
                        qUtf8Printable(m_fileName));
}

Am I correct that the slide_draw_qpainter is called on each draw()/repaint invocation? An so each time the  the redraw is needed, the slide is loaded from the file?

Hope this not true, yet still please check.

5) Oh, and just forgot another thing - why you need to draw a slide as overlay? What is the logic of the action - you'd like just to let the user preview the slide?  

1) ok

2) ok

4) I'll check it

5) Slide is the Acad standard img format to set on DCL Buttons and Images. The "vslide" is only for displaying it on screen. The format can not be implemented in a dxf or dwg file. AutoCAD uses it for example to preview Blocks in Dialogs, befor inserting.

Sadly the command "mslde" is a lot of work more. It creates a slide from the drawing. only from lines and filled polygons.

The preview is not so good.
But is there another container where I can add that?
Where I don't have to redraw all the time.