Page 1 of 1

wx 2.9.1 - I do not understand the event posting anymore

Posted: Thu Sep 16, 2010 4:40 pm
by ddv
Hi all!

I am currently testing a freshly built wxwidgets 2.9.1 and I do not understand the documentation concerning the events must be posted.
Until now, when I have a worker thread that post events to the main gui, for example a image loader thread that send the loaded image filename, I used:

Code: Select all

void ImageLoaderThread::OnImageLoaded(const wxString& imageFileName)
{
   ImageLoaderEvent evt(wxEVT_IMAGELOADER_IMAGEREADY,imageFileName);

   evt.SetId(_destWindow->GetId());
   _destWindow->AddPendingEvent(evt);
}
_destWindow is the wxWindow receicing messages.
ImageLoaderEvent is a wxNotifyEvent-inherited class.

From what I read in the doc, am I supposed to use this code ?

Code: Select all

   evt.SetId(_destWindow->GetId());
   _destWindow->ProcessWindowEvent(evt);
even if I don't understand why it is called ProcessWindowEvent and not PushWindowEvent or QueueWindowEvent, because I want the event to be process not immediately but when the window will have to process it.

Also I read in the doc stuff like wxPostEvent (which is not thread safe), wxQueueEvent (which is thread safe), wxThreadEvent (which is inter-thread safe ?), QueueEvent, and the fact that wxString::c_str() should be used to make a deep copy of a string to send in the event, but Clone() would be also important for this ?

Well, after some thoughts, I guess I can write my code like this:

Code: Select all

void ImageLoaderThread::OnImageLoaded(const wxString& imageFileName)
{
   ImageLoaderEvent evt(wxEVT_IMAGELOADER_IMAGEREADY,imageFileName);

   evt.SetId(_destWindow->GetId());
   wxQueueEvent(_destWindows, evt.Clone());
}
The Clone method of my custom event class ImageLoaderEvent does not make a deep copy of the string (the wxString = operator is used), but since I read that wxQueueEvent is safe to use with wxString members, this should be OK ?

Or do I need to pass imageFileName.c_str() instead of imageFileName in the ctor of ImageLoaderEvent ?

Any thoughts ?
Thanks.

Posted: Thu Sep 16, 2010 9:20 pm
by EzPresso
You should always use QueueEvent methods within secondary threads IMHO. Why don't you just create your evt object on heap?

Posted: Thu Sep 16, 2010 9:38 pm
by EzPresso
Doublechecked the wxWidgets docs:

Code: Select all

virtual void wxEvtHandler::QueueEvent(wxEvent* event) [virtual]
This method is similar to ProcessEvent() but while the latter is synchronous, i.e. the event is processed immediately, before the function returns, this one is asynchronous and returns immediately while the event will be processed at some later time (usually during the next event loop iteration).

Another important difference is that this method takes ownership of the event parameter, i.e. it will delete it itself. This implies that the event should be allocated on the heap and that the pointer can't be used any more after the function returns (as it can be deleted at any moment).

QueueEvent() can be used for inter-thread communication from the worker threads to the main thread, it is safe in the sense that it uses locking internally and avoids the problem mentioned in AddPendingEvent() documentation by ensuring that the event object is not used by the calling thread any more. Care should still be taken to avoid that some fields of this object are used by it, notably any wxString members of the event object must not be shallow copies of another wxString object as this would result in them still using the same string buffer behind the scenes. For example:

Code: Select all

            void FunctionInAWorkerThread(const wxString& str)
            {
                wxCommandEvent* evt = new wxCommandEvent;

                // NOT evt->SetString(str) as this would be a shallow copy
                evt->SetString(str.c_str()); // make a deep copy

                wxTheApp->QueueEvent( evt );
            }


Posted: Fri Sep 17, 2010 8:51 am
by ddv
EzPresso wrote:You should always use QueueEvent methods within secondary threads IMHO. Why don't you just create your evt object on heap?
QueueEvent, when used in wxWindow, is a protected method.
From the doc, and from your response, I read that wxTheApp->QueueEvent is (must be ?) used. It seems strange to have only this solution of using a global variable to post event...
I will do this, but I don't understand why wxTheApp->QueueEvent has been chosed instead of using the myWindow->QueueEvent way.

By the way, I discovered that myWindow->GetEvtHandler()->QueueEvent() is compiling, would this working ? (I hate global variables :wink: )

Sorry for my insistence, but it seems to me that the documentation is not clear concerning the right way to send events, depending on the context...

Thanks.

Posted: Fri Sep 17, 2010 10:02 am
by doublemax
I'm not using wx 2.9.x in production code yet, so i'm not 100% sure about this...

The new 2.9.x documentation about events is a little weird and regarding AddPendingEvent() and QueueEvent() i think it contradicts itself. It implies that using QueueEvent is threadsafe on its own, but you still have to ensure deep copies of all string instances yourself, so i don't really see a big advantage over AddPendingEvent. Additionally, the need to create the event object on the heap creates a worse runtime performance.

My personal opinion is: Using AddPendingEvent to send events from worker threads is still ok, if the Clone() method of your event class ensures deep copies of all wxStrings (and all other reference-counted objects).
Check the second part of this post for an example:
http://forums.wxwidgets.org/viewtopic.p ... 503#103503

If your code works fine on 2.8, it should work (and compile) on 2.9. The only change is:
Instead of using some_window_pointer->AddPendingEvent(), it's now recommended to use some_window_pointer->GetEventHandler()->AddPendingEvent().

If you want to be extra safe and to comply with the wx documentation, use some_window_pointer->GetEventHandler()->QueueEvent() (like you already found out yourself), but you still have to make sure that your Clone() methods creates deep copies of the strings.

Posted: Fri Sep 17, 2010 10:11 am
by EzPresso
QueueEvent, when used in wxWindow, is a protected method.
Yes, but it is public in wxEvtHandler. And you are right. You may use myWindow->GetEvtHandler()->QueueEvent()

The documentation for wxWindow class states:
... ProcessEvent() itself can't be called for wxWindow objects as it ignores the event handlers associated with the window...

Posted: Mon Sep 20, 2010 11:57 am
by ddv
OK. Thanks to both of you.

I will use:

some_window_pointer->GetEventHandler()->QueueEvent()

and will use a secured Clone function (I have my own custom event class).

Posted: Mon Sep 20, 2010 2:20 pm
by evstevemd
Excuse me guys,

but your talk on deep copy and shallow copy have left me down town, in amazing flashlight :) I mean I'm confused with it. Could somebody explain to me what it means? Thanks

Posted: Mon Sep 20, 2010 2:40 pm
by doublemax
Assume this code:

Code: Select all

wxString s1 = wxT("some text");
wxString s2 = s1;
In this case you will have two wxString objects (s1 and s2), but internally they will point to the same memory block containing the actual characters ("some text"). This is called "shallow copy". This saves memory and - as string assignments occur pretty often in normal code - performance. Only if one of the string is changed, the actual string content will be copied.

But wxString is not thread-safe, so in multithreaded applications this can cause problems.

Therefore, when creating a string that you know will be accessed from different threads (e.g. if you set the string member of an event that will be sent from a secondary thread to the main thread), you must make sure that the string data between two strings is not shared. This can be done by the following code:

Code: Select all

wxString s1 = wxT("some text");
wxString s2 = s1.c_str();
This creates a "deep copy".

Posted: Mon Sep 20, 2010 8:33 pm
by evstevemd
doublemax wrote:Assume this code:

Code: Select all

wxString s1 = wxT("some text");
wxString s2 = s1;
In this case you will have two wxString objects (s1 and s2), but internally they will point to the same memory block containing the actual characters ("some text"). This is called "shallow copy". This saves memory and - as string assignments occur pretty often in normal code - performance. Only if one of the string is changed, the actual string content will be copied.

But wxString is not thread-safe, so in multithreaded applications this can cause problems.

Therefore, when creating a string that you know will be accessed from different threads (e.g. if you set the string member of an event that will be sent from a secondary thread to the main thread), you must make sure that the string data between two strings is not shared. This can be done by the following code:

Code: Select all

wxString s1 = wxT("some text");
wxString s2 = s1.c_str();
This creates a "deep copy".
Simple, clear!
Big thanks!