wxQT and handling events

[sean d'epagnier]

I am not sure that this is needed, because with the limited samples there has never been a problem.  It may well be that our use of qt will never have additional pending signals when a slot is called, but it's hard to tell.

Consider winevent.h:29

    void EmitEvent( wxEvent &event ) const

    {

        wxWindow *handler = GetHandler();

        event.SetEventObject( handler );

        handler->HandleWindowEvent( event );

    }

Instead

    void EmitEvent( wxEvent *event ) const

    {

        wxWindow *handler = GetHandler();

        event->SetEventObject( handler );

        handler->GetEventHandler()->QueueEvent( event );

    }

I am asking because this difference could have many implications.  I'm not sure if it is better or not, but I think it avoids any cases where we would need to call deleteLater in the dtor.

Sean

[Vadim Zeitlin]

On Fri, 1 Aug 2014 09:17:21 +0800 sean d'epagnier wrote:

sde> Consider winevent.h:29
sde>
sde> void EmitEvent( wxEvent &event ) const
sde> {
sde> wxWindow *handler = GetHandler();
sde> event.SetEventObject( handler );
sde> handler->HandleWindowEvent( event );
sde> }
sde>
sde> Instead
sde>
sde> void EmitEvent( wxEvent *event ) const
sde> {
sde> wxWindow *handler = GetHandler();
sde> event->SetEventObject( handler );
sde> handler->GetEventHandler()->QueueEvent( event );
sde> }

I don't think we should do this for all events by default, this is
probably going to result in noticeable sluggishness when processing mouse
move events, for example. And if we don't do it for some events, we
shouldn't do it for any of them, because otherwise the order of event
handler calls won't be the same in wxQt as in the other ports.

sde> I am asking because this difference could have many implications. I'm
sde> not sure if it is better or not, but I think it avoids any cases where
sde> we would need to call deleteLater in the dtor.

Yes, I agree it would deal with this, but this seems like a too drastic
action to me. I think we could just implement wxWindow::Destroy() in wxQt
to set some internal m_needToDeleteLater flag to true, so that any handler
calling this->Destroy() would still avoid crashing but nothing would change
for the rest of the code (well, except that it will waste one more byte per
window...).

Regards,
VZ

[petah]

On Fri, 1 Aug 2014 09:17:21 +0800

"sean d'epagnier" <seandepagnier-Re5J...@public.gmane.org> wrote:
> I am not sure that this is needed, because with the limited samples there
> has never been a problem. It may well be that our use of qt will never
> have additional pending signals when a slot is called, but it's hard to
> tell.

I can't speak about Qt's implementation, only about sig/slot's theoretical model - but when a slot goes out of scope any and all signals that could potentially call it should get disconnected, so the situation you mention should never happen if destructor and signal disconnector are strictly coupled... but that may not always be the case with Destroy() and delayed destruction.

You may need explicit signal disconnection inside Destroy() and would probably have to manufacture such a corner case for testing (beyond vanilla samples)

Anyway re-queuing an event is no solution, for performance reasons Vadim mentioned or for correctness in general.

Cheers,

-- p

[sean d'epagnier]

I can't speak about Qt's implementation, only about sig/slot's theoretical model - but when a slot goes out of scope any and all signals that could potentially call it should get disconnected, so the situation you mention should never happen if destructor and signal disconnector are strictly coupled... but that may not always be the case with Destroy() and delayed destruction.

I also am not completely sure about the Qt implementation.  I know the signals get disconnected, but pending signals can still call the slot.  To handle this case, often if a thread were to emit several signals, and a slot in a different thread would receive them, then it's very possible they get queued up , and if the first call deletes the object, there may be a problem and deleteLater() should be used.

For non-threaded, the results are more vague.  The only examples I can find are something like, if you emit a signal which causes the object to be deleted, you cannot access the object anymore (obvious) but in some cases this has caused bugs which are hard to find because it wasn't obvious that emitting a given signal deleted the object.  In our case, we don't send more than one wxEvent before returning from the slot so I don't think it's possible.

I never did find a case that caused the program to crash with delete and not deleteLater which is why I am arguing so strongly against using deleteLater.

Anyway re-queuing an event is no solution, for performance reasons Vadim mentioned or for correctness in general.

I was a little suspicious of this, but not sure, it seemed simple enough.  The best solution I have then, is to allow compiling with delete or deleteLater, and if a case can be found where deleteLater does not crash and delete does, I would like to know about it.

[Vadim Zeitlin]

On Sat, 2 Aug 2014 00:12:59 -0700 (PDT) sean d'epagnier wrote:

sde> The best solution I have then, is to allow compiling with delete or
sde> deleteLater, and if a case can be found where deleteLater does not crash
sde> and delete does, I would like to know about it.

I'm not sure this can work in practice, nobody is going to recompile the
library whenever something crashes. If we're sure that calling delete() for
e.g. a button from its own wxEVT_BUTTON handler works fine, then we should
just always do it. If not, we should use the dynamic determination of which
function is safe to call as I wrote on Github (i.e. set some "in event
handler" flag in the object before invoking the event handler for an event
from it and call deleteLater() from Destroy() if this flag is set, delete()
otherwise).

Regards,
VZ

[Mariano Reingart]

I've sumarized the  "delete vs deleteLater" notes in:

https://github.com/reingart/wxWidgets/wiki/WxQtDeleteLaterNotes

BTW, killing itself from a signal seems to works (at least for a button).

The problem seems to be with controls that can trigger signals when children deleted (from example, notebook page changing signals or panels focus events )

If I understand Qt docs correctly, it don't expect deleting too much manually, specially not deleting children (i.e., deleting the parent should destroy the children automagically without issues). 

I also don't know what could happen if many signals/event are pending or if there could be more complex issues yet to be discovered with other widgets.

Sincerely, I don't understand why deleteLater is so wrong here, it just works and don't crashes, it is safer and it is even recommended by the qt doc for certain situations.

Anyway I prefer to focus in the TLW DeletePendingObjects by wx that seems more serious:

https://github.com/reingart/wxWidgets/issues/46

BTW2, it is not affected by delete vs deleteLater (at least in my tests)

Best regards

Mariano Reingart
http://www.sistemasagiles.com.ar
http://reingart.blogspot.com

 

[Vadim Zeitlin]

On Sat, 2 Aug 2014 18:12:11 -0300 Mariano Reingart wrote:

MR> If I understand Qt docs correctly, it don't expect deleting too much
MR> manually, specially not deleting children (i.e., deleting the parent should
MR> destroy the children automagically without issues).

This is understandable, it's the same in wx. But deleting children
manually still should work too... Anyhow, if the problem is indeed due to
this we could also avoid destroying the corresponding Qt widget if the
wxWindow owning it is destroyed as part of its parent destruction and then
delete all of them at once when deleting the parent. This is a bit more
complicated but at least it's still completely synchronous and
deterministic.

MR> Sincerely, I don't understand why deleteLater is so wrong here

I'm very reluctant to use it because it's asynchronous, meaning that it
makes the code harder to understand and debug and also makes the program
behaviour less deterministic as the exact moment of the destruction now
depends on when exactly the event loop will become idle which in turn
depends on the other events already in the queue. So instead of a single
destruction scenario to think about and debug, you now have many, and
potentially infinitely many, of them. This is not a problem in simple cases
but I'd be very surprised if we didn't run into some trouble due to this in
real, complex programs.

TL;DR: synchronous is simpler than asynchronous and simpler is always better.

Regards,
VZ