Re[2]: [lmi] Different behavior observed with 'File | Save'

[Vadim Zeitlin]

On Tue, 13 Oct 2009 16:28:54 +0000 Greg Chicares <address@hidden> wrote:

GC> [begin snippet]
GC> /// This complete replacement for wxDocManager::OnUpdateFileSave()
GC> /// should not call Skip().
GC> 
GC> void mec_view::UponUpdateFileSave(wxUpdateUIEvent& e)
GC> {
GC>     e.Enable(document().IsModified());
GC> }
GC> 
GC> /// This complete replacement for wxDocManager::OnUpdateFileSaveAs()
GC> /// should not call Skip().
GC> 
GC> void mec_view::UponUpdateFileSaveAs(wxUpdateUIEvent& e)
GC> {
GC>     e.Enable(true);
GC> }
GC> [end snippet]
GC> 
GC> The best solution to that downstream problem would be to delete all
GC> the code quoted above, along with everything that refers to it,
GC> correct? [A patch follows below.]

 Yes, I believe there is no reason to keep these methods. They just raise
questions about why are they there at all.


GC> Skip() is an important and useful function whose name always confounds me.

 It probably doesn't help but you're not alone in this situation. Moreover,
even though somehow it does seem natural to me I don't like it as I'd
prefer to not modify the event object to convey information about how it
should be handled. But unfortunately I don't think there is anything we can
do about it in wx API, backwards compatibility weighs too heavily.
 
GC> This causes me some cognitive dissonance. If I don't want the other
GC> handlers to be skipped, then I must call Skip(); if I don't call Skip(),
GC> however, then they are skipped.

 As I believe I already wrote here, the dissonance comes from the fact that
you read it as "skip the rest of the handlers". While the intended meaning
is "skip this handler", i.e. "pretend it's not there at all".

GC> Perhaps out of superstition or just trepidation, I've adopted a personal
GC> rule that an event handler should either call Skip(), or document why it
GC> does not. I particularly dread a handler that Skip()s in some cases but
GC> not in others, so I'd write IllustrationView::UponUpdateFileSave() thus:
GC> 
GC>     e.Skip();
GC>     if(is_phony_)
GC>         {
GC>         e.Enable(false);
GC>         e.Skip(false);
GC>         }
GC> 
GC> and I'd write IllustrationView::UponUpdateFileSaveAs() the same way
GC> in order to inherit any possible change in the wx implementation.

 Personally I dislike it and would have to fight the urge to rewrite it as

        if(is_phony_)
                e.Enable(false);
        else
                e.Skip();

every time I see it, just as I struggle not to rewrite

        i++;
        i++;

as

        i += 2;

or, maybe more realistically,

        int i = 1;
        if(condition)
                i = 2;

as

        int i = condition ? 2 : 1;

IOW I don't see any reason to intentionally use redundant code. I don't
worry about any run-time performance, of course, but I get the same kind of
cognitive dissonance when I read something like this. But maybe it's just
me.


GC> Here's my patch; it can be applied if testing confirms that it works
GC> and there is no other objection.

 I didn't test it yet but it looks correct to me. I'd keep at least some
comments though, especially if you preserve the 2 Skip() calls which is
something rather unusual. At least a general comment that this class
intercepts update UI handling iff it's phony and uses the base class logic
otherwise.

 Regards,
VZ