Opened 2 years ago

Closed 16 months ago

Last modified 16 months ago

#14314 closed defect (fixed)

Menu event routing problem

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

Description

Seen with the docview sample in default mdi mode, patched to make one menu item disabled.

MDI parent menubar: the menu item is disabled

(...opening a mdi child...)

MDI child menubar: the menu item with the same id is enabled, and appears as enabled.
But you cannot select it!

If this is made with EVT_UPDATEUI() handlers, same unfortunate result.

I've attached a patch that seems to fix the problem but I doubt that this is the correct solution.

Attachments (9)

menuevent-problem.patch download (411 bytes) - added by troelsk 2 years ago.
Demonstration
menuevent-fix.patch download (950 bytes) - added by troelsk 2 years ago.
Fix
menuevent-problem.2.patch download (2.2 KB) - added by troelsk 2 years ago.
Problem
wx-docview-trace.diff download (2.4 KB) - added by vadz 18 months ago.
Patch to the sample that can be used for tracing event handling order
docview-single.patch download (656 bytes) - added by troelsk 18 months ago.
SetFrame() -> event processing gets blocked
docview-null.patch download (561 bytes) - added by troelsk 18 months ago.
NULL
toolbar.patch download (826 bytes) - added by troelsk 17 months ago.
Reproducing toolbar -> view problem, --mdi mode, text view
tbarbase.cpp.patch download (672 bytes) - added by troelsk 17 months ago.
Toolbar hack
menuevent-problem.3.patch download (3.4 KB) - added by troelsk 16 months ago.
Update UI demonstration

Download all attachments as: .zip

Change History (46)

Changed 2 years ago by troelsk

Demonstration

Changed 2 years ago by troelsk

Fix

comment:1 Changed 2 years ago by vadz

  • Status changed from new to confirmed

So what is actually the correct behaviour here? For the item to be disabled in the child menu bar too or for it to be enabled and work?

FWIW wxGTK currently does the latter and, if I understand the patch correctly, it does it as well. But is this really the right thing to do?

Changed 2 years ago by troelsk

Problem

comment:2 Changed 2 years ago by troelsk

The menu state dictated by the window in focus should "win" over the menu state from the main frame class (or app class) I think; currently it is vice versa.

I've added a patch with EVT_UPDATE_UI() handlers demonstrating this, with a EVT_UPDATE_UI(Enable) handler in the text view class and a EVT_UPDATE_UI(Disable) handler in the frame class (actually the app class because the sample has no derived frame class).

comment:3 Changed 2 years ago by troelsk

The menu state dictated by the window in focus should "win" over the menu state from the main frame

This is how it works with 2.8

Probably related to ticket #14314.

comment:4 Changed 2 years ago by troelsk

Probably related to ticket #13873

comment:5 Changed 2 years ago by troelsk

Maybe just reinstate the 2.8 behaviour which seems to work better. That is, overriding [the well-known] ProcessEvent() instead of [the mysterious] overrides of TryBefore() and TryAfter().

comment:6 Changed 2 years ago by vadz

  • Milestone set to 3.0

Unfortunately 2.8 behaviour has a ton of problems too. In particular, it called the same handler multiple times for the same event which was really unexpected. The TryBefore/After() functions were my attempt to fix this but the event handling is so complicated (and particularly so in doc/view code) that I clearly didn't manage to do it without breaking something else. But just restoring the old bugs is hardly ideal :-(

comment:7 Changed 18 months ago by vadz

  • Component changed from wxMSW to GUI-all
  • Keywords event added
  • Patch unset

I think the main problem here is that the menu event is sent to the parent frame instead of the child one in the first place, isn't it? I don't know how did it work in 2.8 but it would make sense to have the following order IMHO:

  1. wxView
  2. wxDocManager
  3. wxDocChildFrame
  4. wxDocParentFrame
  5. wxApp

What do you think?

comment:8 Changed 18 months ago by vadz

Oops, forgot:

  0. wxDocument

Whereas currently we get the following:

0. wxDocument
1. wxView
2. wxDocManager
3. wxDocument
4. wxView
5. wxDocChildFrame
6. wxDocParentFrame
7. wxApp

I.e. both the document and the view get the event twice which is clearly bad.

comment:9 Changed 18 months ago by vadz

OK, so actually there are 2 problems here:

  1. Menu events are sent to the wrong frame in the first place in wxMSW, this breaks the first example as the menu item is disabled in the parent frame even if it's enabled in the child. This could be fixed by your patch but I think it's better to just send the WM_COMMAND message to the right frame from the beginning.
  2. wxEvents were handled in the wrong order too which, I think, explained the second example. I've fixed the order to follow that of comment:7.

I'm going to retest my changes under all platforms and commit them soon, please test your code and let me know about any remaining problems and, especially, any regressions. TIA!

Changed 18 months ago by vadz

Patch to the sample that can be used for tracing event handling order

comment:10 Changed 18 months ago by vadz

The proposed changes are at http://review.bakefile.org/r/489/ if you'd like to comment on or simply test them. TIA!

comment:11 Changed 18 months ago by troelsk

Testing rb489.patch on MSW:

docview sample with the --single option:
File menu items (New/Open/Save/Save as) are nonresponsive
(without the patch the file menu items work except New)

About the order:

  1. wxDocument
  2. wxView
  3. wxDocManager
  4. wxDocChildFrame
  5. wxDocParentFrame
  6. wxApp

It seems strange that wxDocManager should get a crack before (focused) wxDocChildFrame.
Maybe consider moving wxDocManager down 2 steps,

  1. wxDocManager
  2. wxApp

PS: I prefer 2 steps, after wxDocParentFrame, because if you want to grab one of wxDocManager's menu id's (wxID_OPEN, wxID_NEW) it seems natural to grab them from the main window, together with the bunch of Bind()'s or EVT_MENU's you normally already have there.
This seems more convenient than deriving from the docmanager class and grabbing from there.

comment:12 Changed 18 months ago by vadz

I'll check the behaviour in the "--single" case, thanks.

As for the order: what you propose does make sense to me but I think the current (after patch) order should be the same as in 2.8 (except for absence of duplication), so I'd rather keep it for compatibility. Or am I wrong about this?

comment:13 Changed 18 months ago by troelsk

No, no problem. Compatibility with no duplication is fine.

comment:14 Changed 18 months ago by VZ

(In [73927]) Forward events to active child at MSW, not wx, level in wxMDIParentFrame.

We want to handle menu (and toolbar) events in the active MDI child before
handling them in the parent frame itself and the existing code achieved this
by forwarding wxEVT_MENU events at wx event processing level to the active
child. However this was not enough as the underlying MSW WM_COMMAND message
was still sent to the parent frame only and this could result in wx event not
being generated at all if the parent frame had a disabled menu item with the
same ID as (an enabled) item in the child frame, see #14314.

So forward WM_COMMAND directly to ensure that the correct window gets the
event in the first place. And this makes wxEVT_MENU forwarding in TryBefore()
unnecessary.

comment:15 Changed 18 months ago by VZ

(In [73928]) Fix event handling order in doc/view framework.

Ensure that the events are always (provided there is an open document)
processed in the following order:

  1. wxDocument
  2. wxView
  3. wxDocManager
  4. wxDocChildFrame
  5. wxDocParentFrame
  6. wxApp

Do this by forwarding the events from wxDocParentFrame to wxDocChildFrame
first and forward them from there to wxDocManager which -- and this part
remains unchanged -- in turn forwards them to the active wxView which finally
forwards them to wxDocument. This requires another condition in the event
handling code as we still must forward from wxDocParentFrame to wxDocManager
itself if there are no active children at all, but this is the only way to
have the same event order in all cases, whether the event is originally
received by wxDocChildFrame or wxDocParentFrame.

Document this and add a unit test verifying that things indeed work like this.

See #14314.

comment:16 Changed 18 months ago by vadz

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

I've fixed the problem with the "single" mode (and also the still duplicate handlers in "sdi" mode), thanks again for testing.

comment:17 Changed 18 months ago by troelsk

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, I have another problem with "single" mode. Added patch demonstrating this:

If, in single mode, you call view->SetFrame(toplevelframe) - which I do everywhere - event processing gets completely blocked, here, "return false":

bool wxDocParentFrameAnyBase::TryProcessEvent(wxEvent& event)
{
    if ( !m_docManager )
        return false;

    // If we have an active view, its associated child frame may have
    // already forwarded the event to wxDocManager, check for this:
    if ( wxView* const view = m_docManager->GetAnyUsableView() )
    {
        // Notice that we intentionally don't use wxGetTopLevelParent() here
        // because we want to check both for the case of a child "frame" (e.g.
        // MDI child frame or notebook page) inside this TLW and a separate
        // child TLW frame (as used in the SDI mode) here.
        for ( wxWindow* win = view->GetFrame(); win; win = win->GetParent() )
        {
            if ( win == m_frame )
                return false;  // DROPS OUT HERE!!!
        }
    }

    // But forward the event to wxDocManager ourselves if there are no views at
    // all or if we are the frame's view ourselves.
    return m_docManager->ProcessEventLocally(event);
}

In other words, the new event processing expects view->GetFrame() = NULL always in "single" mode.
Not sure what the fix is.

Changed 18 months ago by troelsk

SetFrame() -> event processing gets blocked

comment:18 Changed 18 months ago by vadz

  • Milestone changed from 3.0 to 2.9.5

I just don't understand "single" mode at all, I don't know what is the relationship between view/frame supposed to be in it :-(

Anyhow, it seems like this could be fixed by checking that only the parent of the view frame is this one, i.e.

  • src/common/docview.cpp

    diff --git a/src/common/docview.cpp b/src/common/docview.cpp
    index 246d247..4118192 100644
    a b bool wxDocParentFrameAnyBase::TryProcessEvent(wxEvent& event) 
    20752075    // already forwarded the event to wxDocManager, check for this: 
    20762076    if ( wxView* const view = m_docManager->GetAnyUsableView() ) 
    20772077    { 
    2078         // Notice that we intentionally don't use wxGetTopLevelParent() here 
    2079         // because we want to check both for the case of a child "frame" (e.g. 
    2080         // MDI child frame or notebook page) inside this TLW and a separate 
    2081         // child TLW frame (as used in the SDI mode) here. 
    2082         for ( wxWindow* win = view->GetFrame(); win; win = win->GetParent() ) 
     2078        wxWindow* win = view->GetFrame(); 
     2079        if ( win != m_frame ) 
    20832080        { 
    2084             if ( win == m_frame ) 
    2085                 return false; 
     2081            // Notice that we intentionally don't use wxGetTopLevelParent() 
     2082            // here because we want to check both for the case of a child 
     2083            // "frame" (e.g. MDI child frame or notebook page) inside this TLW 
     2084            // and a separate child TLW frame (as used in the SDI mode) here. 
     2085            for ( win = win->GetParent(); win; win = win->GetParent() ) 
     2086            { 
     2087                if ( win == m_frame ) 
     2088                    return false; 
     2089            } 
    20862090        } 
     2091        //else: This view is directly associated with the parent frame (which 
     2092        //      can happen in the so called "single" mode in which only one 
     2093        //      document can be opened and so is managed by the parent frame 
     2094        //      itself), there can be no child frame in play so we must forward 
     2095        //      the event to wxDocManager ourselves. 
    20872096    } 
    20882097 
    20892098    // But forward the event to wxDocManager ourselves if there are no views at 

Does this help/not harm anything else?

comment:19 Changed 18 months ago by troelsk

I see no harm only improvement, at the moment. I don't know if there is anything special to understand about the m_viewFrame member, in "single" mode, it is a simple placeholder as I see it...

comment:20 Changed 18 months ago by vadz

Well, the question is whether it's supposed to be NULL (because there is no child frame for it) or point to the parent frame (because it's the closest it has to the "associated frame"). I guess we'll just have to settle for "either" as the answer to this question though...

I'll commit the change above then, please let me know if it breaks anything else.

Thanks!

comment:21 Changed 18 months ago by VZ

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

(In [73943]) Fix for event propagation in "single document" doc/view mode.

Ensure that the events still get to wxDocManager even when we are using the
single document mode in which a view can be directly associated with the
parent frame.

Closes #14314.

comment:22 Changed 18 months ago by troelsk

Excellent! Many thanks for your effort in cleaning up docview.

comment:23 Changed 18 months ago by troelsk

Oops, needs to trap NULL because of the win->GetParent() call below, patch added

Changed 18 months ago by troelsk

NULL

comment:24 Changed 18 months ago by VZ

(In [73945]) Test for NULL associated frame in doc/view event handling code.

A view might not have any associated frame at all (this is probably a bad idea
but we don't seem to explicitly forbid this).

This should have been part of r73943, see #14314.

comment:25 Changed 17 months ago by troelsk

  • Resolution fixed deleted
  • Status changed from closed to reopened

Toolbar clicks never reach the view, in --mdi mode in wxMSW.
I'm fairly sure that it used to.
See new patch.

Changed 17 months ago by troelsk

Reproducing toolbar -> view problem, --mdi mode, text view

comment:26 Changed 17 months ago by troelsk

The MDI child frame receives the command, both when you click a menu item or click the corresponding toolbar button. But in the latter case, instead of allowing child frame+view+doc a crack at it (msw/window.cpp line 5314), it delegates the command to the toolbar, msw/window.cpp line 5301,

return win->MSWCommand(cmd, id);

The toolbar in turn seems to allow only the main frame window a crack at it, docmanager+child frame+view+doc gets no (second) crack at it. No solution from here, it is a bit hard to follow the event flow (esp after ProcessEvent was split up in 3 methods).

PS: slight comment typo in docview.cpp line 2030,

  • event handlers call order: first the document, then the new and then the

+ event handlers call order: first the document, then the view and then the

Changed 17 months ago by troelsk

Toolbar hack

comment:27 Changed 17 months ago by troelsk

The toolbar patch I just added makes the toolbar button clicks go the normal docview route.
This is not a good solution because unfortunately the EVT_UPDATE_UI handlers are not called.

comment:28 Changed 16 months ago by troelsk

Pain. The toolbar is entirely dead in MDI docview apps. Maybe open a new ticket for this instead of keeping it here? "Toolbar in wxDocMDIParentFrame is nonresponsive, and toolbar buttons are not updated"

comment:29 Changed 16 months ago by vadz

  • Owner set to vadz
  • Status changed from reopened to accepted

I'll look at this a.s.a.p. but I need to find enough contiguous free time for this first.

comment:30 Changed 16 months ago by vadz

Thanks once again for finding the bug, I do see it perfectly well.

Unfortunately I don't see how to fix it yet :-( The problem is that I now intercept all WM_COMMANDs at parent frame level and send them to the child frame. But for the toolbar commands the child frame sends them back to the toolbar itself for processing. And the toolbar has no knowledge of this frame, being child of the parent frame itself. So the child frame never gets the event.

With the update UI events it's even worse as their handling never involves the child frame at all.

So the changes of r73927 will probably need to be reverted and the problem it fixed (with the disabled menu items) be fixed in some other way. Although I'm not sure if this fixes the update UI problem, would you have some simple test for this by chance? And I still don't know how to fix that other problem with the menus if we do revert it... But it is less serious than toolbars not working at all, so in the worst case we'd just live with it -- but at least with working toolbars.

Changed 16 months ago by troelsk

Update UI demonstration

comment:31 Changed 16 months ago by troelsk

would you have some simple test for this by chance?

Added a little more to the sample,
menuevent-problem.3.patch
(includes the original patch, menuevent-problem.patch)

comment:32 Changed 16 months ago by VZ

(In [74274]) Undo "Forward events to active child at MSW, not wx, level in wxMDIParentFrame."

Unfortunately, forwarding MSW messages only takes care of the menu events but
not the toolbar ones -- which should be handled in the same way but were not.

So restore the old behaviour, the problem with menu items disabled in the
parent frame but enabled in the child one will be fixed differently.

This reverts r73927.

See #14314.

comment:33 Changed 16 months ago by VZ

(In [74275]) Use child MDI frame menu items in preference to the parent frame ones.

Look for the item with the given ID in the child frame menu bar first, before
looking in the parent frame menu bar. This ensures that if an item is disabled
by the parent frame but then reenabled by the child one, it still generates
commands as expected instead of being completely ignored.

See #14314.

comment:34 Changed 16 months ago by VZ

(In [74276]) Also propagate wxEVT_UPDATE_UI to the child MDI frame.

It seems to make sense to handle wxEVT_UPDATE_UI in the same way as wxEVT_MENU
as they are often used together. This allows to handle e.g. toolbar buttons
entirely in the child MDI frame, without any involvement from the parent.

See #14314.

comment:35 Changed 16 months ago by vadz

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

The commits above seem to fix all the problems reported so far for me.

The only remaining problem I'm aware of is that selecting "About" menu item results in processing the event twice in all of wxDocument, wxView, wxDocManager, wxChildFrame and wxParentFrame because it's first handled at the child level but then propagated upwards to the parent where it's handled again. Unfortunately I don't see any way to avoid this and from some point of view it actually makes sense as both the child and the parent have a menu item with this ID.

A more annoying problem is that completely different code paths are used for menu and toolbar event handling. This results in e.g. r73927 working for the menus but not the toolbars. It would be nice to somehow reconcile them but this is clearly not going to be simple...

Also, I suspect forwarding of wxEVT_UPDATE_UI events in r74276 might create problems too. But until it does, let's do it like this, it does seem convenient.

comment:36 Changed 16 months ago by troelsk

No problems seen here so far. Thanks!

comment:37 Changed 16 months ago by VZ

(In [74357]) Prevent duplicate menu event processing in MDI windows.

Record the object propagating the given event upwards in the event object
itself and use it in wxMDIParentFrame to determine whether the event being
handled is already coming from wxMDIChildFrame and avoid sending it back for
processing it there again in this case.

This is ugly and makes wx event processing even more complex but this is the
only way I could find to ensure that

(a) Both the child and the parent frames get the events from the toolbar

(even though the toolbar parent is the parent frame and hence normally
the child wouldn't get notified about them at all and so the forwarding
at wxMDIParentFrame level is required to make this work).

(b) The child gets the event only once, whether it comes from a toolbar (and

hence indirectly via the parent frame) or from the child menu (and hence
directly to the child, at least in wxMSW).

This commit fixes the event propagation unit test case, at least under MSW and
GTK.

See #14314.

Note: See TracTickets for help on using tickets.