Merge pull request #332 from mcorino/develop

improve/correct event handler wrapping

ext/wxruby3/swig/mark_free_impl.i

@@ -50,6 +50,21 @@ WXRUBY_EXPORT void GC_SetWindowDeleted(void *ptr)
   // garbage collected at next phase
   wxRuby_ReleaseEvtHandlerProcs(ptr);
 
+  // wxWidgets requires any pushed event handlers to be popped before
+  // the window gets destroyed. Handle this automatically in wxRuby3.
+  wxWindowBase* wxwin = (wxWindowBase*)ptr;
+  wxEvtHandler* wxevh = wxwin->GetEventHandler();
+  while (wxevh && wxevh != wxwin)
+  {
+    wxEvtHandler* wxevh_next = wxevh->GetNextHandler();
+    VALUE rb_evh = SWIG_RubyInstanceFor(wxevh);
+    // only remove tracked Ruby instantiated handlers since others are
+    // handlers internally set by wxWidgets C++ code and will be removed there
+    if (!NIL_P(rb_evh))
+      wxwin->RemoveEventHandler(wxevh); // remove and forget
+    wxevh = wxevh_next;
+  }
+
   // Wx calls this by standard after the window destroy event is
   // handled, but we need to call it before while the object link is
   // still around
@@ -223,6 +238,23 @@ WXRUBY_EXPORT void GC_mark_wxWindow(void *ptr)
       GC_mark_SizerBelongingToWindow(wx_sizer, rb_sizer);
   }
 
+  // mark any pushed event handlers
+#ifdef __WXRB_DEBUG__
+  if (wxRuby_TraceLevel()>2)
+    std::wcout << "* GC_mark_wxWindow - getting event handler" << std::endl;
+#endif
+  wxEvtHandler* evh = wx_win->GetEventHandler();
+  while (evh && evh != wx_win)
+  {
+#ifdef __WXRB_DEBUG__
+    if (wxRuby_TraceLevel()>2)
+      std::wcout << "* GC_mark_wxWindow - marking event handler" << std::endl;
+#endif
+    VALUE rb_evh = SWIG_RubyInstanceFor(evh);
+	  rb_gc_mark(rb_evh);
+	  evh = evh->GetNextHandler();
+  }
+
 #ifdef __WXRB_DEBUG__
   if (wxRuby_TraceLevel()>2)
     std::wcout << "* GC_mark_wxWindow - getting caret" << std::endl;

lib/wx/core/evthandler.rb

@@ -20,6 +20,29 @@ class EvtHandler
     alias :get_client_data :get_client_object
     alias :client_data :get_client_object
 
+    # Simplify event chaining for wxRuby by handling double linking internally in  Ruby code.
+    #
+    wx_set_next_handler = instance_method :set_next_handler
+    wx_set_previous_handler = instance_method :set_previous_handler
+
+    define_method :set_next_handler do |evthnd|
+      # check if we have a different existing next handler
+      cur_next = get_next_handler
+      if cur_next && cur_next != evthnd
+        # cut the 'old' next handler from the chain
+        cur_next.unlink
+      end
+      wx_set_next_handler.bind(self).call(evthnd)
+      wx_set_previous_handler.bind(evthnd).call(self) if evthnd
+    end
+    alias :next_handler= :set_next_handler
+
+    # disable this in Ruby; set_next_handler handles double linking
+    define_method :set_previous_handler do |evthnd|
+      raise NoMethodError
+    end
+    alias :previous_handler= :set_previous_handler
+
     # EventType is an internal class that's used to set up event handlers
     # and mappings.
     # * 'name' is the name of the event handler method in ruby

rakelib/lib/director/event_handler.rb

@@ -32,23 +32,18 @@ def setup
           # Do not see much use for allowing overrides for these either as wxWidgets do not either
           spec.no_proxy 'wxEvtHandler::QueueEvent',
                         'wxEvtHandler::AddPendingEvent'
+          # Do not see much point in allowing/supporting these to be overridden
+          spec.no_proxy 'wxEvtHandler::SetNextHandler',
+                        'wxEvtHandler::SetPreviousHandler'
+          # no doc for set_previous_handler
+          spec.regard('wxEvtHandler::SetPreviousHandler', regard_doc: false)
           # make SWIG aware of these
           spec.regard 'wxEvtHandler::TryBefore', 'wxEvtHandler::TryAfter'
           # Special type mapping for wxEvtHandler::QueueEvent which assumes ownership of the C++ event.
           # We need to disown Ruby with respect to the C++ event object but remain tracking the pair to keep
           # the Ruby event object alive.
-          # Queued (pending) events are cleaned up (deleted) by wxWidgets after (failing) handling
-          # which will automatically unlink and un-track them releasing the Ruby instance to be GC-ed.
-          spec.map 'wxEvent *event' => 'Wx::Event' do
-            map_in code: <<~__CODE
-              // get the wrapped wxEvent*
-              wxEvent *wx_ev = (wxEvent*)DATA_PTR($input);
-              // disown Ruby; wxWidgets C++ now controls lifecycle wxEvent*
-              RDATA(argv[0])->dfree = 0; // disown
-              // Queue the C++ event
-              $1 = wx_ev;
-              __CODE
-          end
+          spec.disown 'wxEvent *event'
+
           # add special mapping for event filters so we can accept the app instance as well
           # although Wx::App is not derived from Wx::EventFilter in wxRuby (no multiple inheritance)
           spec.map 'wxEventFilter*' => 'Wx::EventFilter,Wx::App' do
@@ -410,6 +405,8 @@ def process(gendoc: false)
               spec.no_proxy "#{spec.class_name(citem)}::ProcessEvent"
               spec.no_proxy "#{spec.class_name(citem)}::QueueEvent"
               spec.no_proxy "#{spec.class_name(citem)}::AddPendingEvent"
+              spec.no_proxy "#{spec.class_name(citem)}::SetNextHandler"
+              spec.no_proxy "#{spec.class_name(citem)}::SetPreviousHandler"
               is_evt_handler = true
             end
           end

rakelib/lib/director/num_validator.rb

@@ -374,6 +374,8 @@ class WXFloatValidator : public wxFloatingPointValidator<double>, public wxRubyV
           spec.no_proxy "#{klass}::ProcessEvent"
           spec.no_proxy "#{klass}::QueueEvent"
           spec.no_proxy "#{klass}::AddPendingEvent"
+          spec.no_proxy "#{klass}::SetNextHandler"
+          spec.no_proxy "#{klass}::SetPreviousHandler"
         end
         spec.suppress_warning(473,
                               'WXIntegerValidator::Clone',

rakelib/lib/director/window.rb

@@ -66,7 +66,6 @@ def setup
             __CODE
           end
           spec.ignore [
-            'wxWindow::PopEventHandler',
             'wxWindow::SetConstraints',
             'wxWindow::GetHandle',
             'wxWindow::GetSize(int *,int *) const', # no need; prefer the wxSize version
@@ -83,6 +82,15 @@ def setup
             'wxWindow::Raise',
             'wxWindow::Lower'
           ]
+
+          # Event handler chaining methods; pushed handlers always remain owned by Ruby
+          # wxRuby3 will automatically pop any remaining handlers when windows get deleted
+          # also ignore this
+          spec.ignore 'wxWindow::PopEventHandler', ignore_doc: false
+          # and instead add an argument-less version (will use default argument of C++ impl)
+          # as we do want C++ side to delete any popped handler
+          spec.extend_interface 'wxWindow', 'wxEvtHandler *PopEventHandler()'
+
           # no real docs and can't find actual examples of usage; ignore
           spec.ignore 'wxWindow::GetConstraints', 'wxWindow::SetConstraints'
           # redefine these so a nil parent is accepted

rakelib/lib/generate/doc/evt_handler.yaml

@@ -30,16 +30,18 @@
       - :pattern: !ruby/regexp /Reimplemented\s+in.*/
         :subst: ''
 :wxEvtHandler.SetNextHandler:
+  :brief:
+    :post:
+      - :pattern: !ruby/regexp /Sets\s+the\s+pointer\s+to\s+the\s+next\s+handler\./
+        :subst: 'Inserts the given handler as the next handler in the chain.'
   :detail:
     :pre:
-      :programlisting:
-        - :pattern: !ruby/regexp /.*/
-          :replace: |
-            
-            ```ruby
-              handlerA.set_next_handler(handlerB)
-              handlerB.set_previous_handler(handlerA)
-            ```
+      :para:
+        - :pattern: !ruby/regexp /See.*ProcessEvent\(\)/
+          :replace:  |
+            In wxRuby this fully handles double linking, i.e. wxRuby will take care of pointing the handler given
+            back to this handler as well. There is no need to call set_previous_handler so this method is not supported 
+            in wxRuby3.
 :wxEvtHandler.TryBefore:
   :detail:
     :pre:

tests/test_event_handling.rb

@@ -279,6 +279,80 @@ def test_event_blocker
     Wx.get_app.yield
   end
 
+  class MyEventHandler < Wx::EvtHandler
+
+    def initialize
+      super
+      @test_event = false
+      @test_cmd_event = false
+      evt_test_event :on_test_event
+      evt_test_cmd_event :on_test_cmd_event
+    end
+
+    attr_reader :test_event, :test_cmd_event
+
+    def on_test_event(_evt)
+      @test_event = true
+    end
+
+    def on_test_cmd_event(evt)
+      @test_cmd_event = true
+    end
+  end
+
+  class EventSnooper < Wx::EvtHandler
+
+    def initialize
+      super
+      @test_event = false
+      @test_cmd_event = false
+      evt_test_event :on_test_event
+      evt_test_cmd_event :on_test_cmd_event
+    end
+
+    attr_reader :test_event, :test_cmd_event
+
+    def on_test_event(_evt)
+      @test_event = true
+    end
+
+    def on_test_cmd_event(evt)
+      @test_cmd_event = true
+      evt.skip # make sure other handler(s) get this too
+    end
+
+  end
+
+  def test_chained_event_handler
+    snooper = EventSnooper.new
+    my_handler = MyEventHandler.new
+    snooper.set_next_handler(my_handler)
+    assert_false(my_handler.test_event)
+    assert_false(snooper.test_event)
+    snooper.process_event(TestEvent.new)
+    assert_false(my_handler.test_event)
+    assert_true(snooper.test_event)
+  end
+
+  def test_pushed_event_handler
+    win = TestFrame.new
+    snooper = EventSnooper.new
+    win.push_event_handler(snooper)
+    assert_false(win.test_event)
+    assert_false(snooper.test_event)
+    win.process_event(TestEvent.new)
+    assert_false(win.test_event)
+    assert_true(snooper.test_event)
+    win.reset
+    assert_false(win.test_cmd_event)
+    assert_false(snooper.test_cmd_event)
+    win.process_event(TestCmdEvent.new)
+    assert_true(win.test_cmd_event)
+    assert_true(snooper.test_cmd_event)
+    win.destroy
+    Wx.get_app.yield
+  end
+
   class MyEventFilter < Wx::EventFilter
     def initialize
       super