Opened 10 years ago

Closed 7 years ago

#12054 closed defect (fixed)

wxView can't provide handlers for associated wxWindow's events

Reported by: wsu Owned by: vadz
Priority: normal Milestone: 3.0.0
Component: GUI-all Version: stable-latest
Keywords: wxView events regression Cc: juliansmart
Blocked By: Blocking:
Patch: no

Description

I want to make my wxView to provide event handlers for many of its wxWindow's events, but it doesn't seem to receive them. I have even tried pushing the wxView onto the wxWindow's event handler stack, and I still don't receive the events.

Please see the attached patch to the docview sample, which shows that my view is not receiving the intended window's events, but is receiving other windows' events!

Attachments (2)

docview missing events.patch download (2.8 KB) - added by wsu 10 years ago.
docview-limit-forward.diff download (3.4 KB) - added by vadz 10 years ago.
Patch limiting forwarding of events to the few selected ones

Download all attachments as: .zip

Change History (12)

Changed 10 years ago by wsu

comment:1 Changed 10 years ago by wsu

This patch does what I expect when applied to r64258. I would infer that the recent event handling changes caused this to stop working. Is that intentional?

comment:2 Changed 10 years ago by vadz

  • Keywords regression added
  • Owner set to vadz
  • Status changed from new to accepted

I see the problem, will debug.

Thanks for reporting!

comment:3 Changed 10 years ago by vadz

  • Cc juliansmart added
  • Milestone set to 2.9.1

The reason you get the EVT_SIZE for the child document frame is that it explicitly forwards all events to its view. AFAICS this was broken some time ago but it seems like this did work before, e.g. in 2.8 you can see that wxDocChildFrame::ProcessEvent() sends all events to the view unconditionally.

The reason that you don't get the EVT_SIZE for the canvas window is that it actually never gets resized! This is another consequence of the above: all size events of the frame are sent to the view. The view is pushed as the canvas event handler. But the canvas already has an event handler which is the special wxScrollHelper used by wxWidgets to implement scrolling. And hence this handler becomes the next handler of the view. And as the view gets the frame size event, it's passed to the scroll helper which always processes size events (for good reasons AFAIR). So the default handler for the size as the frame level never gets executed and doesn't resize the canvas.

IMO it's completely unexpected to get size events for the frame in its view. It also results in subtle bugs like this one (notice that everything works with e.g. the text view which doesn't use wxScrollHelper). So I'd only forward command events (or only menu command events) to the view/document/whatever in the docview code as this was probably the intention. Unfortunately I don't think we can stop this forwarding madness completely because there is probably enough code relying on it.

Julian, I'd really appreciate if you could chime in because the intention of all this forwarding is complete mystery to me. I am guessing here and it would be great to have your point of view as the original author of this code. TIA!

comment:4 follow-up: Changed 10 years ago by wsu

As of r64358, the docview sample fails the assert in wxControlContainer::SetLastFocus() when the user tries to create a Drawing document.

comment:5 in reply to: ↑ 4 Changed 10 years ago by vadz

  • Milestone changed from 2.9.1 to 3.0

Replying to wsu:

As of r64358, the docview sample fails the assert in wxControlContainer::SetLastFocus() when the user tries to create a Drawing document.

Sorry, what exactly should I do to reproduce this?

comment:6 Changed 10 years ago by wsu

As of r64358: execute the following:

  1. Run Docview sample
  1. File | New
  1. Select "Drawing"
  1. Click "Ok" button

--> This tries to create a Drawing window, but hits the assert.

This is still true as of r64681.

This is running on Windows Vista SP2 with WXWIN_COMPATIBILITY_2_8 set to 0.

comment:7 Changed 10 years ago by juliansmart

(Sorry for the late reply, I'm not receiving messages sent to the juliansmart address for some reason.)

Forwarding all events for the frame to the view does seem a little unsafe, and I'm not sure how we've got away with this before. So I think we could limit this to command events (including update UI events) and I doubt much code would be affected. Apps that are affected can explicitly handle the events by e.g. pushing the view onto the frame's event handler stack.

Unfortunately the new docview and event handling code in 2.9 is a bit unfamiliar to me (I wonder why so much code has been stuck into headers - adding code is going to make those headers really bloated). But I guess we just need to test for event.IsCommandEvent before calling m_docManager->ProcessEventLocally(event) in wxDocParentFrameAny.

comment:8 Changed 10 years ago by wsu

I tried the experiment of only forwarding wxCommandEvents, but the wxControlContainer::SetLastFocus() assert still fails. It turns out that wxChildFocusEvent is derived from wxCommandEvent, so it still gets forwarded to the wxView despite really being intended for (I think) the wxMDIParentFrame.

I am certainly no expert in the doc/view implememtation, but maybe events need to have an explicit IsViewCommandEvent() in addition to IsCommandEvent(). Most events would have IsViewCommandEvent() return what IsCommandEvent() returns, but some, such as wxChildFocusEvent, could return something different. I was thinking along the lines of events holding a trinary member where the values are ViewWantsYes, ViewWantsNo, and ViewWantsIfCommand. Most events would have the value ViewWantsIfCommand, but events like wxChildFocusEvent could have the value ViewWantsNo.

comment:9 Changed 10 years ago by vadz

Ok, it's even worse than I thought. If the assert doesn't happen we die with a stack overflow due to infinite recursion because when you set up a view as the next handler when any upward-propagating event is received because it bubbles up from the view to the child frame from which (thanks to its overridden TryBefore()) it's reflected back to the view again. And if the child frame didn't do it, the parent frame would because it also overrides TryBefore() to send the event to the active view.

Frankly, at this stage I'm very tempted to just nuke all this event forwarding code completely. Do people really use it? Is it really preferable to pushing the view on top of whichever window you want to get the events for?

If this is unacceptable, here is a patch which makes propagation of some selected events to the view work but which would still crash if you push the view as an event handler for any window. I don't know if it's worth committing though.

Changed 10 years ago by vadz

Patch limiting forwarding of events to the few selected ones

comment:10 Changed 7 years ago by vadz

  • Resolution set to fixed
  • Status changed from accepted to closed

I think this can be closed now. The originally reported problem doesn't exist any more, you don't need to do anything special to get the events in the view as they're all forwarded there, so this works just fine:

  • samples/docview/view.cpp

    diff --git a/samples/docview/view.cpp b/samples/docview/view.cpp
    index 5b3743f..b6c40a0 100644
    a b  
    3737
    3838BEGIN_EVENT_TABLE(DrawingView, wxView)
    3939    EVT_MENU(wxID_CUT, DrawingView::OnCut)
     40    EVT_SIZE(DrawingView::OnSize)
    4041END_EVENT_TABLE()
    4142
    4243// What to do when a view is created. Creates actual
  • samples/docview/view.h

    diff --git a/samples/docview/view.h b/samples/docview/view.h
    index 3ba84d8..d44d5f2 100644
    a b class DrawingView : public wxView 
    7878
    7979private:
    8080    void OnCut(wxCommandEvent& event);
     81    void OnSize(wxSizeEvent& event)
     82    {
     83        wxLogMessage("In DrawingView::OnSize");
     84        event.Skip();
     85    }
    8186
    8287    MyCanvas *m_canvas;
    8388

The problem of comment:6 doesn't happen any more.

And I really don't want to change anything else after finally getting the event handling order right (I believe/hope) in r73928.

Note: See TracTickets for help on using tickets.