Improved/typesafe event handling patch.

[brianvanderburg]

This patch provides the changes to wxWidgets to support the improved event handling. Old event macros still work just fine but no type checking is done on them. The new event handling makes it possible to connect an event not only to a member function or a wxEvtHandler derived class, but also to a global function or even a functor object. This class also provides a wxConnect template function for dynamic connections. wxConnect/wxEvtHandler::Connect return a wxEventConnection object that can be used to disconnect the event or check if it is still connected.

This patch also changes the minimal sample to show it in operation.

[brianvanderburg]

New event handler with support to enable/disable it by wxUSE_MODERN_EVENTS

[brianvanderburg]

The change in the latest patch (newevent5.patch) makes it possible to enable/disable the templated versions of the event handler connections. Event when disabled it still uses wxEventFunctoMethod, but a generic non-template version that always uses wxEvtHandler* for the handler object and (wxEvtHandler::*method)(wxEvent&) for the method.

When enabled, the complete templates for wxEventFunctor including with method, normal functor, and arbitrary functor such as boost::function, also the global wxConnect as well as wxEvtHandler::Connect for normal connections

When disabled only wxEventFunctor and generic wxEventFunctorMethod to connect to other methods and the wxEvtHandler::Connect are available. They still return wxEventConnection.

I have yet to fully test this but compilation of event.cpp works both when enabled and disabled.

This also adds some macros which can declare the event types both when enabled and disabled: wxDECLARE_EVENT and wxDEFINE_EVENT. When enabled, it used wxTypedEventType<x>, when disabled, plain wxEventType.

The next step is to apply wxDECLARE_EVENT/wxDEFINE_EVENT to the rest of the code base.

To apply it, it seems all is needed is: all the event declarations should use wxDECLARE_EVENT, and all the definitions should be wxDEFINE_EVENT. The handler wrappers would need to be done like this:

#if wxUSE_MODERN_EVENTS

#define wxXyzEventHandler(x) x
....

#else

#define wxXyzEventHandler(x) (wxObjectEventFunction)(wxXyzEventFunction)(x)

#endif

This way the EVT_XYZ event table macros will work both ways.

When modern events is on, wxEVT_XYZ will be a wxTypedEventType<type>, and the macro will emit an entry that uses the template version of wxNewEventFunctor taking 'x' as is.

When modern events is off, wxEVT_XYZ will be a wxEventType, and the macro will emit an entry that uses the the plain wxNewEventFunctor casting x to wxObjectEventFunction.

Even when modern events are on, user-defined events should still work even if they are plain wxEventType, as the macro's wxNewEventFunctor will match the version that expects wxObjectEventFunction. In addition any user code that does:

window.Connect(id, id2, wxEVT_XYZ, wxXyzEventHandler(MyWindow::OnXyz))

Will mis-behave since wxXyzEventHandler will not do the cast. This can be fixed simply by adding extra template Connect methods that works a little like the global wxConnect except unlike the global connect which can also check the type of the sender at compiler time, it won't.

[brianvanderburg]

New event with events declared in event.h/event.cpp changed as possible. Only testing compiling baselib_event.o with wxUSE_TYPESAFE_EVENTS

[brianvanderburg]

Same patch fixing matching problem of using legacy event type with method not exactly wxObjectEventFunction or sink not exactly wxEvtHandler*

[brianvanderburg]

Improvements to event handler code

[brianvanderburg]

New event handler patch and changes to some other need files

[brianvanderburg]

This patch makes most of the changes to get wxGTK compiling as needed. I say wxGTK because:

spinbutt.h/spinctrl.h have their events in the headers, but since there is no 'common' file for them their declarations are in gtk/spinbutt.cpp, gtk/spinctrl.cpp. I'd recommend a common source file like spinbtcmn.cpp and spinctrcmn.cpp

Either way, in wxUSE_TYPESAFE_EVENTS build, the event types require the event object (wxSpinEvent/etc) to exist as well so should only be creatd #if wxUSE_SPINCTRL/etc

wxAuiNotebook changes were made, the event handlers now take a wxAuiNotebookEvent instead of a wxCommandEvent. A note about EVT_COMMAND_RANGE/EVT_NOTIFY_RANGE: in wxUSE_TYPESAFE_EVENTS they don't cast and the type is expected to be a wxTypedEventType<x> and the handler to match, so they can work with pretty much any modern event type. So in this code the type is wxTypedEventType<wxAuiNotebookEvent> so the handler function must match and get a wxAuiNotebookEvent& argument.

But in !wxUSE_TYPESAFE_EVENTS, they do cast and expect a wxCommandEventFunction not a wxAuiNotebookEventFunction. As a result, the code in auibook.cpp should probably be changed from:

EVT_COMMAND_RANGE(wxEVT_COMMAND_AUI_.., id1, id2, func)

to

DECLARE_EVENT_TABLE_ENTRY(wxEVT_COMMAND_AUI_..., id1, id2, wxEventTableCast(wxAuiNotebookEventHandler, func), NULL)

or

wxDECLARE_EVT2(wxEVT_COMMAND_AUI..., id1, id2, wxEventTableCast(wxAuiNotebookEventHandler, func))

[Peter Most]

I was asked to do a code review and posted it to the mailing list. To prevent it from getting lost I attach it here as well.

[Peter Most]

Test for dynamic_cast<> and define wx_dynamic_cast()

[vadz]

AFAIU this is not ready to be applied yet so I'll change its state to prevent it from appearing in the list of new patches for now.

[Peter Most]

Patch for type-safe Connect()

[Peter Most]

This patch implements everything to make Connect() type-safe.
- All samples compile. Only in the calendar sample I had to make one minor change.
- All tests compile. Only in textentrytest.cpp I had to make one minor change.
- All tests work and those that don't are the same as in the trunk version.

[vadz]

Peter, as I said before I appreciate a lot you have done on this so I've exceptionally applied the patch (as r58039) even though it doesn't contain any documentation because it touches so many files and so would probably become impossible to apply very quickly. But this absolutely doesn't change the fact that we really, really need all this new stuff to be documented! We need API documentation of the new Connect() overloads as well as the new (and now public) wxDEFINE_XXX symbols. We need the corresponding overview in the manual to be updated. And probably the event sample should be updated too. And it would be nice to have some tests for the new code, even if they only check that connecting different event handlers (functions, functors, whatever) compiles. Without having the documentation your code risks to be misunderstood and misused and without having the tests it has high chance of being broken too so this is really vital and I hope you can submit the patch to documentation too.

I also have a few questions about the code itself. Maybe the answers to them are obvious but unfortunately there are almost no comments in the code which is really a pity as it's quite non-trivial and would merit having them. So if you think you can answer my questions by submitting a patch adding comments it would be great. Otherwise please answer them here or on wx-dev and I'll comment the code myself.

Anyhow, here they are:

1. Why does wxObjectEventFunctor::operator() override the passed in handler with its m_handler? Shouldn't the passed in argument have priority? Besides, what does the case of handler != m_handler really correspond to anyhow? In fact what is wxObjectEventFunctor itself exactly?
2. The implementation of wxObjectEventFunctor::operator==() doesn't seem right. It's not expected that comparing 2 objects of different types throws std::bad_cast as your code did. I changed it to something more reasonable but I still have a big problem with it: this operator doesn't seem to do any comparison at all, it's more like a check for matching or something. Again, I don't know what exactly the intention was here but a normal comparison operator would just check for field equality instead of also checking for something being NULL. In particular your "comparison" operator is not even commutative! Please rename this to something making more sense and do explain how it works.
3. There are also several other dynamic_cast<>s in the code, I don't think it's a good idea to start throwing exception from what is (currently) not exception-safe code. I'd just settle for wxCHECK probably.
4. Why do we need a reinterpret_cast in wxEventFunctorMethod::GetMethod()?
5. In fact why do we need GetMethod() pure virtual and what is it for at all? Again, this probably could be explained by a simple comment near its declaration but the trouble is that there is no such comments so I don't understand what exactly should it return :-(
6. This is probably unavoidable but I'm rather horrified by the sheer amount of code needed, e.g. 300 lines for new Connect() overloads. Could we somehow cut down on this? The current version surely was very difficult to write but, worse, it's difficult to read/maintain too :-( In particular it seems like we ought to be able to avoid duplicating all stuff differing only in Construct/New use...
7. wxApp::CallEventHandler() should take a reference and not a pointer as it can't be NULL so I changed this. It should still be documented too.

To summarize, we really need to document and comment the new classes because the relationships between them is not always clear. I hope you can still find time to do this (and leave this not closed to remind about it), TIA!

[vadz]

Unfortunately there is a much worse problem: the current code doesn't build at all under MSVC 7.1:

------ Rebuild All started: Project: core, Configuration: Debug Win32 ------
Compiling...
dummy.cpp
..\..\include\wx\event.h(355) : error C2064: term does not evaluate to a function taking 1 arguments
        ..\..\include\wx\event.h(350) : while compiling class-template member function 'void wxEventFunctorAdapter<EventType,Functor>::operator ()(wxEvtHandler *,wxEvent &)'
        with
        [
            EventType=wxTypedEventType<wxPaintEvent>,
            Functor=void (__thiscall wxScrolled<wxPanel>::* )(wxPaintEvent &)
        ]
        ..\..\include\wx\event.h(2881) : see reference to class template instantiation 'wxEventFunctorAdapter<EventType,Functor>' being compiled
        with
        [
            EventType=wxTypedEventType<wxPaintEvent>,
            Functor=void (__thiscall wxScrolled<wxPanel>::* )(wxPaintEvent &)
        ]
        ..\..\include\wx\event.h(2894) : see reference to function template instantiation 'void wxEvtHandler::Connect<EventType,Functor>(int,int,const EventType &,Functor & ,wxObject *)' being compiled
        with
        [
            EventType=wxTypedEventType<wxPaintEvent>,
            Functor=void (__thiscall wxScrolled<wxPanel>::* )(wxPaintEvent &)
        ]
        ..\..\include\wx\scrolwin.h(366) : see reference to function template instantiation 'void wxEvtHandler::Connect<wxTypedEventType<Event>,void(__thiscall wxScrolled<T>::* )(wxPaintEvent &)>(const EventType &,Functor & ,wxObject *)' being compiled
        with
        [
            Event=wxPaintEvent,
            T=wxPanel,
            EventType=wxTypedEventType<wxPaintEvent>,
            Functor=void (__thiscall wxScrolled<wxPanel>::* )(wxPaintEvent &)
        ]
        ..\..\include\wx\scrolwin.h(359) : while compiling class-template member function 'bool wxScrolled<T>::Create(wxWindow *,wxWindowID,const wxPoint &,const wxSize &,long,const wxString &)'
        with
        [
            T=wxPanel
        ]
        ..\..\include\wx\scrolwin.h(423) : see reference to class template instantiation 'wxScrolled<T>' being compiled
        with
        [
            T=wxPanel
        ]

Build Time 0:05
core - 1 error(s), 0 warning(s)

It looks like it should compile and I don't know why does it use the more generic template overload instead of the intended one but the fact is that it does not and so the "new" events have to be disabled for at least MSVC 7 too (and I wonder about 8) which makes them much less useful as this is probably still the most commonly used compiler for wxMSW development. It would be great if you could look into fixing this.

[vadz]

And there are errors (albeit link, not compile, time ones) with VC9 too:  http://buildbot.tt-solutions.com/wx/XPSP2%20VC9%20wxMSW%20trunk%20release/builds/477/step-compile/0

Unfortunately it looks like this patch just wasn't tested enough under Windows. I'll disable it there for MSVC for now but it probably won't build with any other compilers neither.

[vadz]

Aaaaaaargh and VC6 fails to build it even with new events disabled:  http://buildbot.tt-solutions.com/wx/XPSP2%20VC6%20wxMSW%20trunk%20release/builds/478/step-compile/0

I'm sorry but it looks like I'm going to revert this because I simply have no time to fix all these problems myself right now.

[jmsalli]

Using Visual C++ Express 2005 (VC8) here, SVN trunk (r58043) seemed to build just fine (with wxUSE_STL=1 however - I did not test otherwise).

[vadz]

Did you test static or DLL build? VC6 problems seem to be in DLL build and VC9 ones are also there. VC7 just doesn't want to compile this code and I don't think it can be fixed in any easy way :-(

[jmsalli]

Static build.

[vadz]

Ok, I fixed VC6 build and also renamed operator==() because this is not a comparison operator at all (addressing my point 2 in comment:7). It would still be great to fix VC7 build and DLL builds for VC9 but I really have no more time for this now.

[VZ]

(In [58049]) fix VC6 ICE; don't call the function which doesn't compare the objects operator==() (see #10000)

[Peter Most]

I've tried to change Connect() to either reduce the number or to make it compile with VC7, but as soon as I remove the void ( Class::*func )( typename EventType::CorrespondingEvent & ), argument, the compiler has trouble matching the Connect calls for the legacy events e.g.:

    template <typename EventType, typename Class, typename Derived>
    void Connect( int winid,
            int lastId,
            const EventType &eventType,
            void ( Class::*func )( typename EventType::CorrespondingEvent & ),
            wxObject *userData = NULL,
            Derived *eventSink = NULL )

to

    template <typename EventType, typename Class, typename Event, typename Derived>
    void Connect( int winid,
            int lastId,
            const EventType &eventType,
            void ( Class::*func )( Event & ),
            wxObject *userData = NULL,
            Derived *eventSink = NULL )

or to

    template <typename EventType, typename Class, typename Callee, typename Derived>
    void Connect( int winid,
            int lastId,
            const EventType &eventType,
            Callee func,
            wxObject *userData = NULL,
            Derived *eventSink = NULL )

And if you could make a unit test it would make testing whether it works
with VC7 (or any other untested before compiler) much simpler too.

The attached patch (unittests.2.patch (I named it unittest.patch, but Trac changed it somehow)) adds a couple of comments to event.h but also adds a unit test for the Connect calls so you can have a try. In the next couple of days I will make further patches to add more comments and especially more unit tests.

Remember:
- You don't have to compile to complete library to test the Connects. You can just do a 'make test_gui_evthandler.o'
- You have to enable the 'new events' in event.h again.

[VZ]

(In [58611]) add a unit test for new events (see #10000)

[vadz]

Peter, is there anything still left to apply here?

[Peter Most]

Nope, everything has been applied for now. Thanks for asking. But I'm planing more patches of course, so stay tuned ;-)

[Peter Most]

Replaced Connect<>()/Disconnect<>() with Bind<>()/Unbind<>()

[Peter Most]

The attached patch "bind_unbind.patch" replaces all template versions of Connect/Disconnect with the discussed Bind/Unbind methods.

[vadz]

Thanks, I've applied it with minor changes. Now Bind() must be documented. And I also wonder if it still makes sense to speak about wxEVENTS_COMPATIBILITY_2_8 if everything is perfectly compatible now (isn't it?). It probably should be replaced with wxUSE_EVENT_BIND or something like this.

[VZ]

(In [59134]) replace templae Connect() overloads with separate Bind() method to improve compatibility (see #10000, closes #10477)