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

[Vadim Zeitlin]

On Tue, 13 Oct 2009 04:53:17 +0000 Greg Chicares <address@hidden> wrote:

GC> MakeNewIllustrationDocAndView() is documented thus:
GC> 
GC> /// Create a phantom child document and an associated corporeal view.
GC> 
GC> It's not being called in any case tested above. There's no "phantom"
GC> document here.

 Ok, so much for my psychic predictive powers... sorry, I should have
debugged it before posting. I still didn't do it though because I need to
fix a few problems in the latest cvs and so I'm waiting until we switch to
the new svn repository.

GC> Yet behavior differs with wx-2.9.0 versus wx-2.8.10 ...
GC> 
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> }
GC> 
GC> So is there a better way to write command enablers? I'm guessing
GC> that this is yet another case where I should have used Skip()
GC> somehow.

 Yes, you could use it here. There are 2 possibilities:

1. The document is phony. Then it never needs to be saved and hence you
   just need to do e.Enable(false)

2. The document isn't phony in which case you don't want to interfere with
   the base class logic at all. So you need to call e.Skip().

GC> If a tiny patch to accomplish that is obvious to you,
GC> please don't hesitate to show me.

 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()


Another possibility would be to use Connect() to only connect this handler
for phony documents in the first place. But I'm not sure if it's really
more clear in this case.


GC> My original intention was:
GC>   if "phantom"
GC>     then disable "File | Save" (and many other commands)
GC>   else
GC>     do whatever wx would do by default

 I believe the patch above does exactly this.


GC> To paraphrase: you've never saved it, so you ought to be able to save it.
GC> That's plausible, and it seems to be a pretty common behavior.
GC> 
GC> On the other hand, regardless of wx version, if you open a document that
GC> had previously been saved, "File | Save" is disabled until you modify it;
GC> so why should the behavior be any different for an unmodified new document?

 Simply because it's more convenient to the user. It may make sense to
create an empty new document file on disk. It doesn't make any sense to
save an existing file once again -- the disk file is already there.

 IOW using "Save" for a new document has observable effect. In the second
one it does not. So why provide it?

 OTOH, as you say, the world is illogical so I guess some people could
still be surprised to see "Save" disabled. But it's easy enough to always
enable it from the application code if this is what you want (and simply do
nothing if an already saved file is saved again) so I think the current wx
behaviour is optimal as it's reasonable by default and easy to override if
an alternative behaviour is wanted.

GC> Both alternatives are discussed here:
GC>   http://www.openoffice.org/issues/show_bug.cgi?id=65236
GC> | unchanged documents only can be saved using "save as". But I
GC> | see this as a feature and not as a bug

 Yes, but not everybody does. And I can't say I don't understand those who
don't: it's certainly logical but it's inconvenient.

GC> Here's an application that lets the user decide, through a
GC> configuration menu:

 Personally I think this is the worst solution by far.

GC> However, here's a third viewpoint:
GC>   "File | Save" should save immediately, and never bring up a dialog.
GC>   "File | Save as..." should always bring up a dialog.
GC>     Because there's a dialog iff the menu command says '...'.

 This is again logical but still inconvenient and IM[OE] convenience trumps
logic in UI all the time.

GC> So, unless someone strongly objects, I think we should just do (as
GC> above):
GC>   if "phantom"
GC>     then disable "File | Save" (and many other commands)
GC>   else
GC>     do whatever wx would do by default
GC> by some more robust means that won't need maintenance if wx's behavior
GC> ever changes again.

 I hope the above patch and/or proposal to use Connect() (I can make a
patch for it too, of course, but I'd really need to compile the program
first...) solves this problem then.

 Please let me know if it doesn't, thanks,
VZ