Opened 9 years ago

Closed 7 years ago

#13873 closed defect (invalid)

wxDocParentFrameAny event routing

Reported by: troelsk Owned by:
Priority: normal Milestone: 2.9.5
Component: GUI-all Version: stable-latest
Keywords: wxDocParentFrameAny wxDocManager Cc:
Blocked By: Blocking:
Patch: no

Description

I have a button and a menu item directed to the same event handler, OnFileNew.

IMPLEMENT_DYNAMIC_CLASS(MyFrame, wxDocParentFrame)

BEGIN_EVENT_TABLE(MyFrame, wxDocManagerFrame)
   EVT_MENU(wxID_NEW  , MyFrame::OnFileNew)
   EVT_BUTTON(wxID_NEW, MyFrame::OnFileNew)
END_EVENT_TABLE()

If clicking the button my handler is called, as expected.
If selecting the menu item my handler is not called, wxDocManager::OnFileNew() is called instead.

Patch:
Ensure that EVT_MENU(MyFrame::OnFileNew) gets called.

Attachments (1)

wxDocParentFrameAny.patch download (587 bytes) - added by troelsk 9 years ago.
Trunk

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by troelsk

Trunk

comment:1 Changed 9 years ago by vadz

  • Component changed from GUI-generic to GUI-all
  • Milestone set to 3.0
  • Status changed from new to confirmed

This is almost certainly going to break other stuff, I don't remember any more why was TryBefore() used but I'm almost certain it was really needed.

The event handling in doc-view framework is just impossible to get right, for any value of "right" :-(

Anyhow, I'd need to try to remember why are we using TryBefore() here exactly. If anybody else remembers/knows this, please let me know.

Also, whatever we do, it would be very nice to have a unit test for this somewhere in tests/events/propagation.cpp, if you could make one (that would fail right now) it would be really appreciated.

comment:2 Changed 9 years ago by troelsk

I'll be using the patch locally for a while, see how it goes. I have several docview apps.

comment:3 Changed 9 years ago by troelsk

It seems that TryBefore() is used (throughout docview.h) for compatibility, sort of, because in 2.8 it looks like this, wxDocManager is called first.

// Extend event processing to search the view's event table
bool wxDocParentFrame::ProcessEvent(wxEvent& event)
{
    // Try the document manager, then do default processing
    if (!m_docManager || !m_docManager->ProcessEvent(event))
        return wxEvtHandler::ProcessEvent(event);
    else
        return true;
}

I'm not happy with the patch. I'm using the workaround below instead, in user space.
I'll suggest overriding ProcessEvent() instead of TryBefore() in trunk (throughout docview.h), for better compatibility with 2.8, and for keeping the user space workaround simpler (avoid the #if/#else and override only ProcessEvent)

#if (wxVERSION_NUMBER >= 2900) // trunk
bool MyFrame::TryBefore(wxEvent& event)
{
    switch (event.GetId())
    {
        case wxID_NEW: // avoid wxDocManager::OnFileNew()
        case wxID_OPEN: // avoid wxDocManager::OnFileOpen()
            return wxFrame::TryBefore(event);
        default:
            return wxDocParentFrame::TryBefore(event); // -> wxDocManager
    }
}
#else // 2.8 branch
bool MyFrame::ProcessEvent(wxEvent& event)
{
    switch (event.GetId())
    {
        case wxID_NEW: // avoid wxDocManager::OnFileNew()
        case wxID_OPEN: // avoid wxDocManager::OnFileOpen()
            return wxFrame::ProcessEvent(event);
        default:
            return wxDocParentFrame::ProcessEvent(event); // -> wxDocManager
    }
}
#endif

comment:4 Changed 9 years ago by vadz

  • Patch unset

Unfortunately overriding ProcessEvent() doesn't work at all because we need the ShouldProcessOnlyIn() check in the base version which, in turn, was added to fix multiple problems due to processing the same event several times when using pushed event handlers.

And you're right, we can't use TryAfter() here as otherwise the view would handle the event before the doc manager (I think?) which would be wrong.

So I still don't see how can this be fixed without breaking something else :-(

comment:5 Changed 8 years ago by troelsk

See ticket #14314

comment:6 Changed 8 years ago by troelsk

was added to fix multiple problems due to processing the same event several times

Yes, I think I myself created a ticket for this once. Appreciating your effort, but it might be that reverting to the old code compatible with 2.8 is better. Old user code is bound to break I fear (mine included probably).

comment:7 Changed 8 years ago by vadz

  • Milestone changed from 3.0 to 2.9.5

This must be fixed, yes. I still hope to do something better than just reverting the changes but I just don't have enough time/energy :-( It would be very helpful if someone could formulate all the scenarios that should work, i.e. "document manager handles event but skips it, the view handles event ==> the view gets it, the document manager doesn't" and so on. It could be expressed like this or directly as a patch to the event unit tests, which would be even more helpful. Once I know what exactly needs to be implemented I think I should be able to implement it but right now it's still hazy making it even more difficult.

comment:8 Changed 7 years ago by vadz

  • Status changed from confirmed to infoneeded_new

AFAICS, my proposed fix for #14314 won't change anything here as wxDocManager would still be getting the event before wxDocParentFrame. So what exactly is the problem here? Was the order different in 2.8? Or was it the same but you'd like to change it?

comment:9 Changed 7 years ago by troelsk

  • Status changed from infoneeded_new to new

Yes, this was different in 2.8, there I can grab wxID_NEW in the main window frame (wxDocParentFrame)

comment:10 Changed 7 years ago by vadz

  • Status changed from new to infoneeded_new

I just don't see how could it be possible, even in 2.9 -- the code in source:wxWidgets/branches/WX_2_8_BRANCH/src/common/docview.cpp#L2000 clearly passes the event to wxDocManager before passing it to the base class which searches for the handler in the event table.

And I can't reproduce this behaviour with 2.8, the parent frame handler defined in this patch is never called:

  • samples/docvwmdi/docview.cpp

     
    193193IMPLEMENT_CLASS(MyFrame, wxDocMDIParentFrame)
    194194BEGIN_EVENT_TABLE(MyFrame, wxDocMDIParentFrame)
    195195    EVT_MENU(DOCVIEW_ABOUT, MyFrame::OnAbout)
     196    EVT_MENU(wxID_NEW, MyFrame::OnFileNew)
    196197END_EVENT_TABLE()
    197198
    198199MyFrame::MyFrame(wxDocManager *manager, wxFrame *frame, const wxString& title,
  • samples/docvwmdi/docview.h

     
    4545  MyFrame(wxDocManager *manager, wxFrame *frame, const wxString& title, const wxPoint& pos, const wxSize& size,
    4646    long type);
    4747
     48  void OnFileNew(wxCommandEvent& event) { wxLogMessage("In MyFrame::OnFileNew()"); }
    4849  void OnAbout(wxCommandEvent& event);
    4950  MyCanvas *CreateCanvas(wxView *view, wxMDIChildFrame *parent);
    5051

What am I missing?

comment:11 Changed 7 years ago by troelsk

  • Resolution set to invalid
  • Status changed from infoneeded_new to closed

This ticket is almost entirely false. Had forgotten about this 2.8 hack I used, oops sorry.

bool MyFrame::ProcessEvent(wxEvent& event)
{
#if (wxVERSION_NUMBER >= 2904) // trac.wxwidgets.org/ticket/13873
    return base::ProcessEvent(event);
#else
    switch (event.GetId())
    {
        case wxID_NEW: // avoid wxDocManager::OnFileNew()
        case wxID_OPEN: // avoid wxDocManager::OnFileOpen()
            return wxFrame::ProcessEvent(event);
        default:
            return wxDocParentFrame::ProcessEvent(event); // -> wxDocManager
    }
#endif
}
Note: See TracTickets for help on using tickets.