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

[Greg Chicares]

On 2009-10-13 13:07Z, Vadim Zeitlin wrote:
> On Tue, 13 Oct 2009 04:53:17 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> // My ancient copy of wx-2.5.4:
> GC> void wxDocManager::OnUpdateFileSave(wxUpdateUIEvent& event)
> GC> {
> GC>     wxDocument *doc = GetCurrentDocument();
> GC>     event.Enable( doc && doc->IsModified() );
> GC> }
> GC> 
> GC> In 'illustration_view.cpp' I wanted to add an extra condition,
> GC> so I copied IsModified() from the wx default handler and
> GC> changed it thus:
> GC>     e.Enable(!is_phony_ && document().IsModified());
> GC> That's fragile because wx can change its condition--indeed, now:
> GC> 
> GC> // wx-2.9.0:
> GC> void wxDocManager::OnUpdateFileSave(wxUpdateUIEvent& event)
> GC> {
> GC>     wxDocument * const doc = GetCurrentDocument();
> GC>     event.Enable( doc && !doc->AlreadySaved() );
> GC> }

Fragility propagates. That class has been cloned--with 'is_phony_'
diligently removed, so that nothing remains except the mistake:

[begin snippet]
/// This complete replacement for wxDocManager::OnUpdateFileSave()
/// should not call Skip().

void mec_view::UponUpdateFileSave(wxUpdateUIEvent& e)
{
    e.Enable(document().IsModified());
}

/// This complete replacement for wxDocManager::OnUpdateFileSaveAs()
/// should not call Skip().

void mec_view::UponUpdateFileSaveAs(wxUpdateUIEvent& e)
{
    e.Enable(true);
}
[end snippet]

The best solution to that downstream problem would be to delete all
the code quoted above, along with everything that refers to it,
correct? [A patch follows below.]

>  Again, I didn't test this yet but this should do the trick:
> 
> --- illustration_view.cpp
> +++ illustration_view.cpp
> @@ -256,12 +256,19 @@
>          }
>  }
> 
> -/// This complete replacement for wxDocManager::OnUpdateFileSave()
> -/// should not call Skip().
> -
>  void IllustrationView::UponUpdateFileSave(wxUpdateUIEvent& e)
>  {
> -    e.Enable(!is_phony_ && document().IsModified());
> +    if ( is_phony_ )
> +    {
> +        // Replace the base class logic with our one: phony documents are 
> never
> +        // saved, so just disable the menu item and do not call Skip().
> +        e.Enable(false);
> +    }
> +    else
> +    {
> +        // For normal documents simply reuse the base class logic.
> +        e.Skip();
> +    }
>  }
> 
>  /// This complete replacement for wxDocManager::OnUpdateFileSaveAs()

Skip() is an important and useful function whose name always confounds me.
AIUI, there's a chain of event handlers, and adding our own handler
 - places it at the head of the chain; and
 - by default, prevents the event from propagating down the chain;
but, if we call Skip(), then it does propagate.

This causes me some cognitive dissonance. If I don't want the other
handlers to be skipped, then I must call Skip(); if I don't call Skip(),
however, then they are skipped.

Perhaps out of superstition or just trepidation, I've adopted a personal
rule that an event handler should either call Skip(), or document why it
does not. I particularly dread a handler that Skip()s in some cases but
not in others, so I'd write IllustrationView::UponUpdateFileSave() thus:

    e.Skip();
    if(is_phony_)
        {
        e.Enable(false);
        e.Skip(false);
        }

and I'd write IllustrationView::UponUpdateFileSaveAs() the same way
in order to inherit any possible change in the wx implementation.

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

Index: illustration_view.cpp
===================================================================
RCS file: /sources/lmi/lmi/illustration_view.cpp,v
retrieving revision 1.106
diff -U 3 -r1.106 illustration_view.cpp
--- illustration_view.cpp       14 Apr 2009 23:04:12 -0000      1.106
+++ illustration_view.cpp       13 Oct 2009 14:57:25 -0000
@@ -256,20 +256,24 @@
         }
 }

-/// This complete replacement for wxDocManager::OnUpdateFileSave()
-/// should not call Skip().
-
 void IllustrationView::UponUpdateFileSave(wxUpdateUIEvent& e)
 {
-    e.Enable(!is_phony_ && document().IsModified());
+    e.Skip();
+    if(is_phony_)
+        {
+        e.Enable(false);
+        e.Skip(false);
+        }
 }

-/// This complete replacement for wxDocManager::OnUpdateFileSaveAs()
-/// should not call Skip().
-
 void IllustrationView::UponUpdateFileSaveAs(wxUpdateUIEvent& e)
 {
-    e.Enable(!is_phony_);
+    e.Skip();
+    if(is_phony_)
+        {
+        e.Enable(false);
+        e.Skip(false);
+        }
 }

 void IllustrationView::UponUpdateInapplicable(wxUpdateUIEvent& e)
Index: mec_view.cpp
===================================================================
RCS file: /sources/lmi/lmi/mec_view.cpp,v
retrieving revision 1.26
diff -U 3 -r1.26 mec_view.cpp
--- mec_view.cpp        30 Jul 2009 22:30:44 -0000      1.26
+++ mec_view.cpp        13 Oct 2009 14:57:25 -0000
@@ -69,8 +69,6 @@

 BEGIN_EVENT_TABLE(mec_view, ViewEx)
     EVT_MENU(XRCID("edit_cell"             ),mec_view::UponProperties)
-    EVT_UPDATE_UI(wxID_SAVE                 ,mec_view::UponUpdateFileSave)
-    EVT_UPDATE_UI(wxID_SAVEAS               ,mec_view::UponUpdateFileSaveAs)
     EVT_UPDATE_UI(XRCID("edit_cell"        ),mec_view::UponUpdateProperties)

 // There has to be a better way to inhibit these inapplicable ids.
@@ -193,22 +191,6 @@
         }
 }

-/// This complete replacement for wxDocManager::OnUpdateFileSave()
-/// should not call Skip().
-
-void mec_view::UponUpdateFileSave(wxUpdateUIEvent& e)
-{
-    e.Enable(document().IsModified());
-}
-
-/// This complete replacement for wxDocManager::OnUpdateFileSaveAs()
-/// should not call Skip().
-
-void mec_view::UponUpdateFileSaveAs(wxUpdateUIEvent& e)
-{
-    e.Enable(true);
-}
-
 void mec_view::UponUpdateInapplicable(wxUpdateUIEvent& e)
 {
     e.Enable(false);
Index: mec_view.hpp
===================================================================
RCS file: /sources/lmi/lmi/mec_view.hpp,v
retrieving revision 1.3
diff -U 3 -r1.3 mec_view.hpp
--- mec_view.hpp        24 Jul 2009 14:14:26 -0000      1.3
+++ mec_view.hpp        13 Oct 2009 14:57:25 -0000
@@ -91,8 +91,6 @@
     virtual wxPrintout* OnCreatePrintout();

     void UponProperties        (wxCommandEvent&);
-    void UponUpdateFileSave    (wxUpdateUIEvent&);
-    void UponUpdateFileSaveAs  (wxUpdateUIEvent&);
     void UponUpdateInapplicable(wxUpdateUIEvent&);
     void UponUpdateProperties  (wxUpdateUIEvent&);