Is there a reason why PushButton's onClicked event converts MouseEventArgs to WindowEventArgs?

Issue #1161 resolved
Constantine Tenma
created an issue

You can see it right here: https://bitbucket.org/cegui/cegui/src/b7f846e6d8926f09080c4326f60cf6049ba5f5fc/cegui/src/widgets/PushButton.cpp#lines-86:88

We use Lua bindings to CEGUI in a game, and so far in CEGUI all mouse events emit MouseEventArgs, so we always cast WindowEventArgs to MouseEventArgs with a lua helper CEGUI.toMouseEventArgs in the mouse event lua function callbacks.

It tripped us up when we subscribed to "Clicked" event of a button and args.button was containing nonsensical values. After some debugging we found out that we were in actuality receiving a WindowEventArgs and casting it to a MouseEventArgs (through tolua's internal casting mechanism which is used in CEGUI.toMouseEventArgs) resulted in reading a memory not belonging to a WindowEventArgs class!

So my question is, is there a deliberate reason for doing this? Because it is a very unobvious thing. Looking through the documentation for onClicked method I found two more such cases. The name of the event logically falls into the group of mouse events, so it makes sense to send a MouseEventArgs, I believe. Unless there is a specific reason not to, of which I am unfortunately unaware.

I've searched through issues, forums and wiki but didn't find anything useful, and most results were dated earlier than 2010 or marked obsolete.

I guess from now on I'll have to manually review the type of EventArgs that is passed to a callback for each event of each widget type just to be safe.

Comments (10)

  1. Lukas Meindl

    The way I see it, the expectation for the "Clicked" event here can easily be that it would be related to mouse input (only), therefore it should be a mouse event. Looking at the code, this is also exactly how it works and there is already a mouseeventargs present that could be used for firing this event. After all MouseEventArgs is deriving from WindowEventArgs so I believe this is not even ABI/API breaking and could be done in v0-8 without issues.

    I mean, the only way I see that making the change from WindowEvent to MouseEvent can affect "existing behaviour", is if somebody does a type-check for the event inside the event handler. Any static_cast will works as beforehand.

    So yea, I agree it should be a MouseEventArgs and I do not see any argument being brought up in the referenced thread against it. Of course you could use the MouseClick events instead but why would you have to do that?

  2. Lukas Meindl

    True, unfortunately the history does not go back in time far enough (ends at 2008) to be able to tell what the intention was here. It seems to be very intentional that only a window event args is used but I just cannot find or see a reason as to why that would have to be the case other than just reducing the event to the minimally relevant information (which in your case seems to have turned out insufficient).

    I also just realised that my last change does break ABI so I will look into a workaround that doesnt break ABI but has the same effect.

  3. Lukas Meindl

    I looked into it again a bit more and I think the intention here is that the "Clicked" event is only a "Triggered" event and therefore the naming is a bit unfortunate. What I mean is that this my or may not be fired due to a mouse event (although currently this is exclusively the case) and as such it makes sense to only have a WindowEvent here. Someone might add keyboard support and fire it via enter key, then a mouse event makes no sense. Imo, the naming should be changed, not the event type. I will revert my change.

  4. Lukas Meindl

    in CSS a "pressed" button is styled under the name "active"

    according to wikipedia: Terms for the "pushing" of a button include pressing, depressing, mashing, hitting, and punching.

    A,lthough it would be a descriptive term, "Activated" could easily be confused with what happens on a state change from disabled to enabled. So maybe "Pushed" would work better here or "ButtonPushed" (although that makes it confusing regarding the MenuItem which is probably not perceived as a "Button")

  5. Constantine Tenma reporter

    Why not "ButtonTriggered"? It makes sense with both mouse and keyboard and unlike "Pushed" it doesn't leave room for confusion with MouseDown/KeyDown event by implying it only fires when the button is in a pushed state (which is false, since the event fires if and only if the button was pushed and unpushed with cursor being on the button during both actions).

  6. Lukas Meindl

    Maybe "WidgetTriggered" is better since MenuItem for example may not be seen as a "button" neither by looks nor by code and it still keeps the naming generic.

    I of course can only make this change on default branch or I will break lot of user's code.

  7. Log in to comment