Fix recently broken forwarding of events between event handlers.

After the recent changes to the event processing logic, forwarding an event
from one event handler to another one stopped working correctly because the
per-event "process here only" flag prevented it from following the event
handler chain after forwarding. This notably broke keyboard navigation in
wxComboCtrl under MSW in wx itself and probably quite a few of other things in
user code.

Fix this by replacing the boolean flag with a pointer to the handler to which
the processing of this event should be restricted. This allows the full
processing to still take place if an event is forwarded to another handler.
So wxEvent::ShouldProcessHereOnly() is now called ShouldProcessOnlyIn() and
takes a wxEvtHandler parameter.

This made appear a problem in wxScrollHelperEvtHandler code that was hidden by
the bug above: the events were still processed multiple times in it. To fix
this, also add wxEvent::DidntHonourProcessOnlyIn() and take it into account in
the base class code. Did I mention that wxScrollHelperEvtHandler must die?

Add another unit test checking that forwarding works correctly.

git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@64464 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

diff --git a/include/wx/event.h b/include/wx/event.h
index e7536eb9ce3ac25d701159b12412defc9a484b02..f989454ce6f7c528b37c1c73f810ba182923632d 100644 (file)
--- a/include/wx/event.h
+++ b/include/wx/event.h
@@ -999,7 +999,21 @@ public:
     // This is also used only internally by ProcessEvent() to check if it
     // should process the event normally or only restrict the search for the
     // event handler to this object itself.
     // This is also used only internally by ProcessEvent() to check if it
     // should process the event normally or only restrict the search for the
     // event handler to this object itself.

-    bool ShouldProcessHereOnly() const { return m_processHereOnly; }
+    bool ShouldProcessOnlyIn(wxEvtHandler *h) const
+    {
+        return h == m_handlerToProcessOnlyIn;
+    }
+
+    // Called to indicate that the result of ShouldProcessOnlyIn() wasn't taken
+    // into account. The existence of this function may seem counterintuitive
+    // but unfortunately it's needed by wxScrollHelperEvtHandler, see comments
+    // there. Don't even think of using this in your own code, this is a gross
+    // hack and is only needed because of wx complicated history and should
+    // never be used anywhere else.
+    void DidntHonourProcessOnlyIn()
+    {
+        m_handlerToProcessOnlyIn = NULL;
+    }

 
 protected:
     wxObject*         m_eventObject;
 
 protected:
     wxObject*         m_eventObject;


@@ -1011,6 +1025,10 @@ public:
     // m_callbackUserData is for internal usage only
     wxObject*         m_callbackUserData;
 
     // m_callbackUserData is for internal usage only
     wxObject*         m_callbackUserData;
 

+private:
+    // If this handler
+    wxEvtHandler *m_handlerToProcessOnlyIn;
+

 protected:
     // the propagation level: while it is positive, we propagate the event to
     // the parent window (if any)
 protected:
     // the propagation level: while it is positive, we propagate the event to
     // the parent window (if any)


@@ -1025,11 +1043,6 @@ protected:
     // once for this event
     bool m_wasProcessed;
 
     // once for this event
     bool m_wasProcessed;
 

-    // this flag is used by ProcessEventLocally() to prevent ProcessEvent()
-    // from doing its usual stuff and force it to just call TryHere() instead,
-    // see the comment there explaining why is this needed
-    bool m_processHereOnly;
-

 protected:
     wxEvent(const wxEvent&);            // for implementing Clone()
     wxEvent& operator=(const wxEvent&); // for derived classes operator=()
 protected:
     wxEvent(const wxEvent&);            // for implementing Clone()
     wxEvent& operator=(const wxEvent&); // for derived classes operator=()


@@ -1038,8 +1051,8 @@ private:
     // it needs to access our m_propagationLevel
     friend class WXDLLIMPEXP_FWD_BASE wxPropagateOnce;
 
     // it needs to access our m_propagationLevel
     friend class WXDLLIMPEXP_FWD_BASE wxPropagateOnce;
 

-    // and this one needs to access our m_processHereOnly
-    friend class WXDLLIMPEXP_FWD_BASE wxEventProcessHereOnly;
+    // and this one needs to access our m_handlerToProcessOnlyIn
+    friend class WXDLLIMPEXP_FWD_BASE wxEventProcessInHandlerOnly;

 
 
     DECLARE_ABSTRACT_CLASS(wxEvent)
 
 
     DECLARE_ABSTRACT_CLASS(wxEvent)


@@ -1093,31 +1106,28 @@ private:
     wxDECLARE_NO_COPY_CLASS(wxPropagateOnce);
 };
 
     wxDECLARE_NO_COPY_CLASS(wxPropagateOnce);
 };
 

-// A helper used by ProcessEventLocally() to restrict the event processing
-// to this handler only.
-class WXDLLIMPEXP_BASE wxEventProcessHereOnly
+// A helper object used to temporarily make wxEvent::ShouldProcessOnlyIn()
+// return true for the handler passed to its ctor.
+class wxEventProcessInHandlerOnly

 {
 public:
 {
 public:

-    wxEventProcessHereOnly(wxEvent& event) : m_event(event)
+    wxEventProcessInHandlerOnly(wxEvent& event, wxEvtHandler *handler)
+        : m_event(event),
+          m_handlerToProcessOnlyInOld(event.m_handlerToProcessOnlyIn)

     {
     {

-        // This would be unexpected and would also restore the wrong value in
-        // this class dtor so if even does happen legitimately we'd need to
-        // store the value in ctor and restore it in dtor.
-        wxASSERT_MSG( !m_event.m_processHereOnly,
-                      "shouldn't be used twice for the same event" );
-
-        m_event.m_processHereOnly = true;
+        m_event.m_handlerToProcessOnlyIn = handler;

     }
 
     }
 

-    ~wxEventProcessHereOnly()
+    ~wxEventProcessInHandlerOnly()

     {
     {

-        m_event.m_processHereOnly = false;
+        m_event.m_handlerToProcessOnlyIn = m_handlerToProcessOnlyInOld;

     }
 
 private:
     wxEvent& m_event;
     }
 
 private:
     wxEvent& m_event;

+    wxEvtHandler * const m_handlerToProcessOnlyInOld;

 
 

-    wxDECLARE_NO_COPY_CLASS(wxEventProcessHereOnly);
+    wxDECLARE_NO_COPY_CLASS(wxEventProcessInHandlerOnly);

 };
 
 #if wxUSE_GUI
 };
 
 #if wxUSE_GUI

diff --git a/src/common/event.cpp b/src/common/event.cpp
index b6c0144752529d3bc6fae6a13971db34c6030464..c4f1ae81640e7652a7074d78398f7737f92f1320 100644 (file)
--- a/src/common/event.cpp
+++ b/src/common/event.cpp
@@ -363,10 +363,10 @@ wxEvent::wxEvent(int theId, wxEventType commandType)
     m_id = theId;
     m_skipped = false;
     m_callbackUserData = NULL;
     m_id = theId;
     m_skipped = false;
     m_callbackUserData = NULL;

+    m_handlerToProcessOnlyIn = NULL;

     m_isCommandEvent = false;
     m_propagationLevel = wxEVENT_PROPAGATE_NONE;
     m_wasProcessed = false;
     m_isCommandEvent = false;
     m_propagationLevel = wxEVENT_PROPAGATE_NONE;
     m_wasProcessed = false;

-    m_processHereOnly = false;

 }
 
 wxEvent::wxEvent(const wxEvent& src)
 }
 
 wxEvent::wxEvent(const wxEvent& src)


@@ -376,11 +376,11 @@ wxEvent::wxEvent(const wxEvent& src)
     , m_timeStamp(src.m_timeStamp)
     , m_id(src.m_id)
     , m_callbackUserData(src.m_callbackUserData)
     , m_timeStamp(src.m_timeStamp)
     , m_id(src.m_id)
     , m_callbackUserData(src.m_callbackUserData)

+    , m_handlerToProcessOnlyIn(NULL)

     , m_propagationLevel(src.m_propagationLevel)
     , m_skipped(src.m_skipped)
     , m_isCommandEvent(src.m_isCommandEvent)
     , m_wasProcessed(false)
     , m_propagationLevel(src.m_propagationLevel)
     , m_skipped(src.m_skipped)
     , m_isCommandEvent(src.m_isCommandEvent)
     , m_wasProcessed(false)

-    , m_processHereOnly(false)

 {
 }
 
 {
 }
 


@@ -393,11 +393,12 @@ wxEvent& wxEvent::operator=(const wxEvent& src)
     m_timeStamp = src.m_timeStamp;
     m_id = src.m_id;
     m_callbackUserData = src.m_callbackUserData;
     m_timeStamp = src.m_timeStamp;
     m_id = src.m_id;
     m_callbackUserData = src.m_callbackUserData;

+    m_handlerToProcessOnlyIn = NULL;

     m_propagationLevel = src.m_propagationLevel;
     m_skipped = src.m_skipped;
     m_isCommandEvent = src.m_isCommandEvent;
 
     m_propagationLevel = src.m_propagationLevel;
     m_skipped = src.m_skipped;
     m_isCommandEvent = src.m_isCommandEvent;
 

-    // don't change m_wasProcessed nor m_processHereOnly
+    // don't change m_wasProcessed

 
     return *this;
 }
 
     return *this;
 }


@@ -1378,7 +1379,7 @@ bool wxEvtHandler::ProcessEvent(wxEvent& event)
 
     // Short circuit the event processing logic if we're requested to process
     // this event in this handler only, see DoTryChain() for more details.
 
     // Short circuit the event processing logic if we're requested to process
     // this event in this handler only, see DoTryChain() for more details.

-    if ( event.ShouldProcessHereOnly() )
+    if ( event.ShouldProcessOnlyIn(this) )

         return TryHere(event);
 
 
         return TryHere(event);
 
 


@@ -1432,12 +1433,21 @@ bool wxEvtHandler::DoTryChain(wxEvent& event)
         // window to be called.
         //
         // So we must call ProcessEvent() but it must not do what it usually
         // window to be called.
         //
         // So we must call ProcessEvent() but it must not do what it usually

-        // does. To resolve this paradox we pass a special "process here only"
-        // flag to ProcessEvent() via the event object itself. This ensures
-        // that if our own, base class, version is called, it will just call
-        // TryHere() and won't do anything else, just as we want it to.
-        wxEventProcessHereOnly processHereOnly(event);
-        if ( h->ProcessEvent(event) )
+        // does. To resolve this paradox we set up a special flag inside the
+        // object itself to let ProcessEvent() know that it shouldn't do any
+        // pre/post-processing for this event if it gets it. Note that this
+        // only applies to this handler, if the event is passed to another one
+        // by explicitly calling its ProcessEvent(), pre/post-processing should
+        // be done as usual.
+        //
+        // Final complication is that if the implementation of ProcessEvent()
+        // called wxEvent::DidntHonourProcessOnlyIn() (as the gross hack that
+        // is wxScrollHelperEvtHandler::ProcessEvent() does) and ignored our
+        // request to process event in this handler only, we have to compensate
+        // for it by not processing the event further because this was already
+        // done by that rogue event handler.
+        wxEventProcessInHandlerOnly processInHandlerOnly(event, h);
+        if ( h->ProcessEvent(event) || !event.ShouldProcessOnlyIn(h) )

             return true;
     }
 
             return true;
     }
 

diff --git a/src/generic/scrlwing.cpp b/src/generic/scrlwing.cpp
index 0f1a4f57efc2034ca165399faadafe443450de21..6244d494a0400be6cbc5de450b2a22d51044dd5e 100644 (file)
--- a/src/generic/scrlwing.cpp
+++ b/src/generic/scrlwing.cpp
@@ -209,11 +209,6 @@ bool wxScrollHelperEvtHandler::ProcessEvent(wxEvent& event)
     // (as indicated by "process here only" flag being set) and we do want to
     // execute the handler defined in the window we're associated with right
     // now, without waiting until TryAfter() is called from wxEvtHandler.
     // (as indicated by "process here only" flag being set) and we do want to
     // execute the handler defined in the window we're associated with right
     // now, without waiting until TryAfter() is called from wxEvtHandler.

-    //
-    // Note that this means that the handler in the window will be called twice
-    // if there is a preceding event handler in the chain because we do it from
-    // here now and the base class DoTryChain() will also call it itself when
-    // we return. But this unfortunately seems unavoidable.

     bool processed = m_nextHandler->ProcessEvent(event);
 
     // always process the size events ourselves, even if the user code handles
     bool processed = m_nextHandler->ProcessEvent(event);
 
     // always process the size events ourselves, even if the user code handles


@@ -310,6 +305,17 @@ bool wxScrollHelperEvtHandler::ProcessEvent(wxEvent& event)
 
     event.Skip(wasSkipped);
 
 
     event.Skip(wasSkipped);
 

+    // We called ProcessEvent() on the next handler, meaning that we explicitly
+    // worked around the request to process the event in this handler only. As
+    // explained above, this is unfortunately really necessary but the trouble
+    // is that the event will continue to be post-processed by the previous
+    // handler resulting in duplicate calls to event handlers. Call the special
+    // function below to prevent this from happening, base class DoTryChain()
+    // will check for it and behave accordingly.
+    //
+    // And if we're not called from DoTryChain(), this won't do anything anyhow.
+    event.DidntHonourProcessOnlyIn();
+

     return processed;
 }
 
     return processed;
 }
 

diff --git a/tests/events/propagation.cpp b/tests/events/propagation.cpp
index 7ec1e50dd3860a0af4d32c49eb7fcd9496b2e539..49d6fac0a208d35856316bd34b1562eca3dfddaa 100644 (file)
--- a/tests/events/propagation.cpp
+++ b/tests/events/propagation.cpp
@@ -178,6 +178,7 @@ private:
         CPPUNIT_TEST( TwoHandlers );
         CPPUNIT_TEST( WindowWithoutHandler );
         CPPUNIT_TEST( WindowWithHandler );
         CPPUNIT_TEST( TwoHandlers );
         CPPUNIT_TEST( WindowWithoutHandler );
         CPPUNIT_TEST( WindowWithHandler );

+        CPPUNIT_TEST( ForwardEvent );

         CPPUNIT_TEST( ScrollWindowWithoutHandler );
         CPPUNIT_TEST( ScrollWindowWithHandler );
     CPPUNIT_TEST_SUITE_END();
         CPPUNIT_TEST( ScrollWindowWithoutHandler );
         CPPUNIT_TEST( ScrollWindowWithHandler );
     CPPUNIT_TEST_SUITE_END();


@@ -186,6 +187,7 @@ private:
     void TwoHandlers();
     void WindowWithoutHandler();
     void WindowWithHandler();
     void TwoHandlers();
     void WindowWithoutHandler();
     void WindowWithHandler();

+    void ForwardEvent();

     void ScrollWindowWithoutHandler();
     void ScrollWindowWithHandler();
 
     void ScrollWindowWithoutHandler();
     void ScrollWindowWithHandler();
 


@@ -262,6 +264,48 @@ void EventPropagationTestCase::WindowWithHandler()
     CPPUNIT_ASSERT_EQUAL( "oa2o1cpA", g_str );
 }
 
     CPPUNIT_ASSERT_EQUAL( "oa2o1cpA", g_str );
 }
 

+void EventPropagationTestCase::ForwardEvent()
+{
+    // The idea of this test is to check that the events explicitly forwarded
+    // to another event handler still get pre/post-processed as usual as this
+    // used to be broken by the fixes trying to avoid duplicate processing.
+    TestWindow * const win = new TestWindow(wxTheApp->GetTopWindow(), 'w');
+    wxON_BLOCK_EXIT_OBJ0( *win, wxWindow::Destroy );
+
+    TestEvtHandler h1('1');
+    win->PushEventHandler(&h1);
+    wxON_BLOCK_EXIT_OBJ1( *win, wxWindow::PopEventHandler, false );
+
+    class ForwardEvtHandler : public wxEvtHandler
+    {
+    public:
+        ForwardEvtHandler(wxEvtHandler& h) : m_h(&h) { }
+
+        virtual bool ProcessEvent(wxEvent& event)
+        {
+            g_str += 'f';
+
+            return m_h->ProcessEvent(event);
+        }
+
+    private:
+        wxEvtHandler *m_h;
+    } f(h1);
+
+    // First send the event directly to f.
+    wxCommandEvent event1(TEST_EVT);
+    f.ProcessEvent(event1);
+    CPPUNIT_ASSERT_EQUAL( "foa1wA", g_str );
+    g_str.clear();
+
+    // And then also test sending it to f indirectly.
+    wxCommandEvent event2(TEST_EVT);
+    TestEvtHandler h2('2');
+    h2.SetNextHandler(&f);
+    h2.ProcessEvent(event2);
+    CPPUNIT_ASSERT_EQUAL( "oa2fo1wAA", g_str );
+}
+

 void EventPropagationTestCase::ScrollWindowWithoutHandler()
 {
     TestWindow * const parent = new TestWindow(wxTheApp->GetTopWindow(), 'p');
 void EventPropagationTestCase::ScrollWindowWithoutHandler()
 {
     TestWindow * const parent = new TestWindow(wxTheApp->GetTopWindow(), 'p');