Re[2]: [lmi] UpdateUI problems in main_wx.cpp

[Vadim Zeitlin]

On Sat, 23 Aug 2008 16:48:27 +0000 Greg Chicares <address@hidden> wrote:

GC> Writing one EVT_UPDATE_UI_RANGE entry would be more appealing than
GC> writing fifteen distinct handlers that all map to the same function,
GC> if it were feasible and guaranteed to work. But there's no real
GC> problem if that's not feasible.

 A bit OT for this message but I have to say that I rather like Vaclav's
idea, I think it would fix it nicely, at least if you edit your XRC files
manually because I think it would be some time before the dialog editors
picked it up. Having 15 handlers definitely does work but seems extremely
inelegant, to borrow your terminology.


GC> However, there remains the real problem that this stuff doesn't work,

 To answer this briefly: this didn't work because of a bug in wx. The
event processing for the children of wxDocParentFrame was broken, i.e.
didn't correspond to the documentation nor made any sense.

 I've fixed the bug in wxWidgets svn trunk version and so the code works as
expected now, i.e. you can enable Skeleton::UponUpdateInapplicable()
handler and do event.Enable(false) in its body and it will disable the
toolbar tools if and only if there is no active CensusView which would have
enabled them first. I tested it with the latest wx svn and everything seems
to work fine.


 Now the question is how to solve this problem in LMI without using the
latest wx svn trunk. In principle, the fix reuses an existing virtual
function and so could be backported into 2.8 branch. It is too late to do
it for 2.8.9 though but I could do it after its release if you don't have
plans to switch to svn trunk (well, rather, some specific revision of svn
trunk, possibly the one called 2.9.0) any time soon. It's also possible to
work around the bug even with the 2.8 version currently being used by LMI
by defining the event handler at CensusDocument, instead of CensusView,
level if you want the problem to be fixed right now. Please let me know if
you'd like a patch for doing it like this -- I didn't make one yet because
it feels wrong to me to handle events in a document (a.k.a. model) class.
There are also other possibilities to fix the problem without changes to
wx, e.g. override ProcessEvent() in CensusView or in a new class deriving
from wxDocMDIParentFrame which would be used for Skeleton::frame_. But the
simplest remains to upgrade to a fixed wx version, of course. What would
you prefer to do?



 Finally, what follows is a longer explanation of what exactly went wrong
and how I changed it which can be read if you're interested but which is
strictly informational. The answers to your questions about the remaining
issues can be found after the "end explanation" marker near the end of the
message.

---------------------------- start explanation ----------------------------

[snip the experiment description]
GC> I'm mystified.

 So was I, to be honest. I needed to debug the code to understand what was
going on. This is where MSVC build really proves its usefulness BTW:
without having a [working (i.e. not Win32 version of gdb)] debugger I would
have spent much longer on this.

GC> I consult this page again:
GC> 
GC> 
http://docs.wxwidgets.org/trunk/overview_eventhandling.html#overview_eventhandling_processing
GC> 
GC> The sixth of six steps is
GC>   "Finally, ProcessEvent is called on the wxApp object."

 But the document view framework adds an extra twist not described (until
now) in this overview. Namely, wxDocParentFrame sends any events to its
m_docManager first. And wxDocManager sends any events to the current view
first. But wait, there is more: now the view sends any events to its
associated document.

 You might think that there is no problem so far: it's complicated, mostly
unnecessary and IMO in bad taste as it blurs the separation between
controllers, views and models (as any of them can be processing events) but
not wrong.

 However there is a bug right here because if the event wasn't processed by
the document itself (which is almost always true and certainly is the case
in LMI), the event is sent to wxTheApp by the default logic. So the handler
defined in the application class is ran before the handlers defined at the
view or frame level. The only opportunity to preempt the application level
handler is to define handlers in the document class and not the view one
(which explains one of the workarounds proposed above).


 This clearly needs to be fixed, but there is also the possibility that
people actually expect that when they send an event to wxDocument it ends
up in wxApp if it was unprocessed so we can't remove the event propagation
logic from wxDocument completely. Instead I added a new ProcessEventHere()
method to wxEvtHandler which does everything ProcessEvent() does except for
forwarding the event to the parent window/application object if it wasn't
processed. And I also changed the way the document-view classes hook into
the normal event processing logic: instead of overriding ProcessEvent()
itself, I now override TryValidator() method in them. This TryValidator()
is actually a misnomer (and I'd like to rename it in svn trunk, but the
patch I committed so far didn't do it as I wanted to be able to backport it
to 2.8 later if needed) and is just a generic hook which is called by
ProcessEvent() before examining the event handlers defined at the level of
this object itself.

 To summarize, the event handling now really corresponds to the (updated)
description at the link above.

----------------------------- end explanation -----------------------------


GC> At least two issues remain. First, it seems that counting child
GC> frames can't be the simplest solution: is there a better way?

 Not yet but wxMDIParentFrame::HasChildren() and GetChildrenCount() are
two of the functions which I added while working on my (still unfinished)
MDI patch. You won't need them in Skeleton::UponUpdateInapplicable(), of
course, but they could be useful elsewhere.

GC> Second, behavior is correct as long as no view class other than those
GC> named above is used, but incorrect with product-editor view classes.

 This shouldn't be the case any more, i.e. the behaviour should be always
correct when you use the fixed wxWidgets versions.

 Please let me know if you have any more questions about this,
VZ