[lmi] Event handling modernization (was: a slight access violation)

[Vadim Zeitlin]

On Sun, 09 Nov 2014 20:35:34 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2014-10-27 20:53Z, Vadim Zeitlin wrote:
GC> > 
GC> 
GC> [...the application receives menu events, not the MDI frame; and
GC> then, when the application becomes SkeletonTest, and the GUI test
GC> receives lmi's menu events, surprising things may happen...]
GC> 
GC> >  But then, personally, I wouldn't be doing this at wxApp level at all
GC> > anyhow, but rather in wxFrame containing the menus to be updated or the
GC> > commands to be processed, because I think that it is best to handle the
GC> > events as locally as possible.
GC> 
GC> That makes sense. When I ported old code from OWL to wx, I saw
GC> macros in wx samples that looked very much like OWL, so I used
GC> them with wx too, e.g.:
GC> 
GC>   BEGIN_EVENT_TABLE(Skeleton, wxApp)
GC>     EVT_MENU(wxID_ABOUT, Skeleton::UponAbout)
GC> 
GC> (I always had a sense that Julian must have known OWL very well.)

 Actually I strongly suspect that Julian knew MFC (as wx v1 was too similar
to it for this to be a simple coincidence) and I'm all but certain that MFC
programmers knew OWL (as it was quite like Microsoft to take an existing
product, study it, then make a clone of it and destroy the original using
better (if not necessarily legal) marketing strategy). But I won't go into
the details as I have been told (several times) that, incredible as it may
seem, not everyone is interested in software archaeology.


GC> However, I looked back at my old OWL code, and found:
GC> 
GC>   DEFINE_RESPONSE_TABLE1(PMMDIClient, TMDIClient)
GC>     EV_COMMAND(CM_FILEPRINT, CmFilePrint),
GC>     EV_COMMAND_ENABLE(CM_FILEPRINT, CmPrintEnable),
GC>   END_RESPONSE_TABLE;
GC> 
GC> Thus, I was certainly handling menu events at a lower level then,
GC> which is your point above; and I agree that it's a better idea
GC> now. How, specifically, would you recommend changing lmi?
GC>
GC> Derive a new MDI frame (or MDI client) class from a wx base?

 MDI client doesn't exist in wx public API because this is something too
Windows-specific and doesn't have any reasonable equivalent under other
platforms. So I'd indeed recommend doing it at MDI parent frame level.

 For completeness, here are the different levels at which this event could
be handled:

1. In a custom wxMenu subclass that we'd use for the "Window" menu. This is
   actually probably the best variant, abstractly speaking, as it ensures
   that the event is handled as close to its source as possible and
   entirely encapsulates the responsibility for enabling the menu items in
   the menu itself, which is nice. But the menu would need to "reach out"
   into the containing frame to get the total number of children, which
   somewhat spoils this encapsulation and it's relatively unusual to have
   event handling logic in the menu itself.

2. At child frame level. This is in the middle between (1) and (4) and so
   doesn't seem appealing. In practice, the only nice thing about this
   solution would have been that we could do it at DocMDIChildFrameEx
   level, i.e. avoid introducing a new wxDocMDIParentFrame-derived class
   just for this. But, as explained below, we can do this anyhow with
   dynamic event binding, so if you're fine with using it, this advantage
   is negated.

3. At parent frame menu bar level. I can't see any reason to prefer this
   to (4), so I mention this just for completeness.

4. At parent frame level. This does fully encapsulate this logic as well
   and it does seem logical to do something depending on the number of
   children of a frame in this frame itself.

 All of the above would work, but the last one seems the best to me, and so
this is what the first attached patch
(0001-Handle-only-the-menu-open-events-for-the-main-frame-.patch) does.

GC> I believe you prefer Connect() over event-table macros; would
GC> you recommend that sort of change, too?

 Yes, absolutely. With Bind() (which is a modern replacement for Connect()
whose only advantage compared to Bind() was that it could work even with
the pre C++98 MSVC 6 compiler), this code can remain in the same Skeleton
method but be invoked when the event happens in wxDocMDIParentFrame.
Without it, we would have needed deriving a new class just to define this
handler, which would be completely disproportionate.


 Before ending this message, let me describe the other patches I attached.
They are completely optional, but I couldn't prevent myself from making
them while looking at the code related to the MDI frames event handling.

 The 0002-Don-t-count-all-MDI-children-just-check-if-there-is-.patch one
addresses my urge to avoid performing unnecessary actions: we don't really
need the count of MDI children frame here at all, so why bother counting
all of them (even if "all" is probably virtually always just a few) when we
can stop after just the second one? Subsidiarily, why count them at all if
we don't have any active child anyhow.

 The 0003-Remove-unnecessary-DocMDIChildFrameEx-dtor.patch removes a
"WX !!" comment which is not relevant any more (and since quite some time,
too), bringing the total count of such remaining comments to "only" 45 (2%
decrease!).

 Finally, 0004-Remove-the-attempts-to-use-MDI-parent-status-bar-for.patch
is IMHO worth applying because this code is totally misleading -- and, in
fact, managed to confuse me quite thoroughly. The truth is, it doesn't do
absolutely anything and, to the best of my knowledge, never did. As the
commit message for this change explains, this event handler was never
executed and so all the efforts to provide the correct status bar to the
help string display code inside wxWidgets were in vain. As an aside, there
is a simpler and more direct way to do this, which is to override wxFrame
DoGiveHelp() virtual method to use another status bar. But we don't even
need to do this  because wxEVT_MENU_HIGHLIGHT is sent to the parent frame
anyhow and it, of course, already uses the correct status bar. So I don't
believe there is any reason to keep this code and less code is always good.

 And to really conclude this topic, I'd like to add that, while debugging
this, I did find a minor problem related to the help strings: they were not
shown at all for the items in a popup menu (e.g. the one shown when right
clicking in a census window). Unfortunately this is something that can't be
worked around at LMI level, it turned out to be a real bug in wxWidgets and
I had to fix it there. Nothing is required for this fix to work for LMI:
when you update to a later revision (I can't give the exact commit as I
haven't committed it just yet because the wxWidgets svn repository server
has some problems right now), whether you apply the above patch or not, you
will just see the help strings in the status bar for the popup menu items
as well.

 As always, please let me know if you have any questions about these
patches,
VZ

0001-Handle-only-the-menu-open-events-for-the-main-frame-.patch
Description: Text document

0002-Don-t-count-all-MDI-children-just-check-if-there-is-.patch
Description: Text document

0003-Remove-unnecessary-DocMDIChildFrameEx-dtor.patch
Description: Text document

0004-Remove-the-attempts-to-use-MDI-parent-status-bar-for.patch
Description: Text document