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

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.
-    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;
@@ -1011,6 +1025,10 @@ public:
     // 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)
@@ -1025,11 +1043,6 @@ protected:
     // 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=()
@@ -1038,8 +1051,8 @@ private:
     // it needs to access our m_propagationLevel
     friend class WXDLLIMPEXP_FWD_BASE wxPropagateOnce;
 
-    // and this one needs to access our m_processHereOnly
+    // and this one needs to access our m_handlerToProcessOnlyIn
-    friend class WXDLLIMPEXP_FWD_BASE wxEventProcessHereOnly;
+    friend class WXDLLIMPEXP_FWD_BASE wxEventProcessInHandlerOnly;
 
 
     DECLARE_ABSTRACT_CLASS(wxEvent)
@@ -1093,31 +1106,28 @@ private:
     wxDECLARE_NO_COPY_CLASS(wxPropagateOnce);
 };
 
-// A helper used by ProcessEventLocally() to restrict the event processing
+// A helper object used to temporarily make wxEvent::ShouldProcessOnlyIn()
-// to this handler only.
+// return true for the handler passed to its ctor.
-class WXDLLIMPEXP_BASE wxEventProcessHereOnly
+class wxEventProcessInHandlerOnly
 {
 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
+        m_event.m_handlerToProcessOnlyIn = handler;
-        // 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;
     }
 
-    ~wxEventProcessHereOnly()
+    ~wxEventProcessInHandlerOnly()
     {
-        m_event.m_processHereOnly = false;
+        m_event.m_handlerToProcessOnlyIn = m_handlerToProcessOnlyInOld;
     }
 
 private:
     wxEvent& m_event;
+    wxEvtHandler * const m_handlerToProcessOnlyInOld;
 
-    wxDECLARE_NO_COPY_CLASS(wxEventProcessHereOnly);
+    wxDECLARE_NO_COPY_CLASS(wxEventProcessInHandlerOnly);
 };
 
 #if wxUSE_GUI

src/common/event.cpp

@@ -363,10 +363,10 @@ wxEvent::wxEvent(int theId, wxEventType commandType)
     m_id = theId;
     m_skipped = false;
     m_callbackUserData = NULL;
+    m_handlerToProcessOnlyIn = NULL;
     m_isCommandEvent = false;
     m_propagationLevel = wxEVENT_PROPAGATE_NONE;
     m_wasProcessed = false;
-    m_processHereOnly = false;
 }
 
 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_handlerToProcessOnlyIn(NULL)
     , 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_handlerToProcessOnlyIn = NULL;
     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;
 }
@@ -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.
-    if ( event.ShouldProcessHereOnly() )
+    if ( event.ShouldProcessOnlyIn(this) )
         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
-        // does. To resolve this paradox we pass a special "process here only"
+        // does. To resolve this paradox we set up a special flag inside the
-        // flag to ProcessEvent() via the event object itself. This ensures
+        // object itself to let ProcessEvent() know that it shouldn't do any
-        // that if our own, base class, version is called, it will just call
+        // pre/post-processing for this event if it gets it. Note that this
-        // TryHere() and won't do anything else, just as we want it to.
+        // only applies to this handler, if the event is passed to another one
-        wxEventProcessHereOnly processHereOnly(event);
+        // by explicitly calling its ProcessEvent(), pre/post-processing should
-        if ( h->ProcessEvent(event) )
+        // 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;
     }
 

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.
-    //
-    // 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
@@ -310,6 +305,17 @@ bool wxScrollHelperEvtHandler::ProcessEvent(wxEvent& event)
 
     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;
 }
 

tests/events/propagation.cpp

@@ -178,6 +178,7 @@ private:
         CPPUNIT_TEST( TwoHandlers );
         CPPUNIT_TEST( WindowWithoutHandler );
         CPPUNIT_TEST( WindowWithHandler );
+        CPPUNIT_TEST( ForwardEvent );
         CPPUNIT_TEST( ScrollWindowWithoutHandler );
         CPPUNIT_TEST( ScrollWindowWithHandler );
     CPPUNIT_TEST_SUITE_END();
@@ -186,6 +187,7 @@ private:
     void TwoHandlers();
     void WindowWithoutHandler();
     void WindowWithHandler();
+    void ForwardEvent();
     void ScrollWindowWithoutHandler();
     void ScrollWindowWithHandler();
 
@@ -262,6 +264,48 @@ void EventPropagationTestCase::WindowWithHandler()
     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');