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

[Greg Chicares]

On 2009-10-09 00:49Z, Vadim Zeitlin wrote:
> On Thu, 8 Oct 2009 13:23:06 -0400 "Murphy, Kimberly" <address@hidden> wrote:
> 
> MK> While testing with wx-2.9.0, I observe a different behavior 
> MK> when going to save a newly created illustration.
> MK> 
> MK> Consider:
> MK>   F | N | I
> MK>   OK
> MK>   
> MK> I want to save the case and immediately observe that the 
> MK> 'Save' button (Ctrl-S) is disabled. Checking the File menu, 
> MK> 'Save as...' (Ctrl-A) is the only option enabled. Ctrl-S
> MK> is only enabled if a change is made to the default input 
> MK> prior to hitting "OK". 
> MK> 
> MK> Next, consider:
> MK>   F | N | C
> MK>   OK
> MK>     
> MK> Here, I observe that the 'Save' button is enabled. Again 
> MK> checking the File menu, both 'Save' and 'Save as...' 
> MK> options are available, as I would expect. 
> MK> 
> MK> This is different behavior than with lmi-20090929Z, which 
> MK> was built with wx-2.8.10, where both 'Save' and 'Save as...' 
> MK> are enabled for a newly created illustration and census 
> MK> having default input.

Is "File | Save" enabled for a newly-created document? AIUI:

                  '.cns'   '.ill'
    wx-2.8.10       yes      yes
    wx-2.9.0        yes      no

So something changed in wx. Perhaps it's something exotic that's
only visible with lmi.

>  Thanks for describing the problem so precisely, I can easily see it as
> well. And while I didn't have time to rebuild LMI to test it, I think I
> might know what is causing it: it may be due to SetDocumentSaved(true) call
> in MakeNewIllustrationDocAndView() in illustration_view.cpp. This basically
> tricks the document into thinking that it had already been saved even
> though it wasn't.

MakeNewIllustrationDocAndView() is documented thus:

/// Create a phantom child document and an associated corporeal view.

It's not being called in any case tested above. There's no "phantom"
document here.

> And "Save" is intentionally disabled when the document
> had been already saved and not modified since -- there is no reason to save
> it once again in this case, is there?

Correct, there's no reason to save it once again in that case.

Yet behavior differs with wx-2.9.0 versus wx-2.8.10 ...

// My ancient copy of wx-2.5.4:
void wxDocManager::OnUpdateFileSave(wxUpdateUIEvent& event)
{
    wxDocument *doc = GetCurrentDocument();
    event.Enable( doc && doc->IsModified() );
}

In 'illustration_view.cpp' I wanted to add an extra condition,
so I copied IsModified() from the wx default handler and
changed it thus:
    e.Enable(!is_phony_ && document().IsModified());
That's fragile because wx can change its condition--indeed, now:

// wx-2.9.0:
void wxDocManager::OnUpdateFileSave(wxUpdateUIEvent& event)
{
    wxDocument * const doc = GetCurrentDocument();
    event.Enable( doc && !doc->AlreadySaved() );
}

So is there a better way to write command enablers? I'm guessing
that this is yet another case where I should have used Skip()
somehow. If a tiny patch to accomplish that is obvious to you,
please don't hesitate to show me. I could do this:
-    e.Enable(!is_phony_ && document().IsModified());
+    e.Enable(!is_phony_ && document().AlreadySaved());
but that would just hardcode the wx-2.9.0 behavior, and create
a problem when wx-7.8.9 uses a different technique.

>  Maybe I'm wrong as this seems to be done for the "phantom" document and
> not the real one but this seems like the most likely explanation for now.

There's no "phantom" document in the reproducible testcase.
The behavior changed because the wx enablement handler changed
(which is okay), but lmi's modified copy of that handler didn't.
Copying snippets from wx into lmi is fragile: that's the problem.

> I'll try to really debug it a.s.a.p. In the meanwhile I have a question for
> Greg: do you know/remember why was this call needed?

My original intention was:
  if "phantom"
    then disable "File | Save" (and many other commands)
  else
    do whatever wx would do by default
and IllustrationView::UponUpdateFileSave() was written only to
implement that 'if "phantom"' behavior. The problem is that the
code in lmi for the 'else' case really means:
    do whatever wx-2.5.4 would do by default

[Here follows a digression that I think leads us back to the same place.]

Now that I think about it, though: for new documents, a case can
be made either for or against disabling "File | Save". Here's the
case for enabling, from the wx-2.9.0 sources:

// return true if the document hasn't been modified since the last time it
// was saved (implying that it returns false if it was never saved, even if
// the document is not modified)
bool AlreadySaved() const { return !IsModified() && GetDocumentSaved(); }
[where "returns false" means enable the command]

To paraphrase: you've never saved it, so you ought to be able to save it.
That's plausible, and it seems to be a pretty common behavior.

On the other hand, regardless of wx version, if you open a document that
had previously been saved, "File | Save" is disabled until you modify it;
so why should the behavior be any different for an unmodified new document?

Here's an earlier discussion that reached no definitive conclusion:
  http://lists.nongnu.org/archive/html/lmi/2008-03/msg00057.html

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

Here's an application that lets the user decide, through a
configuration menu:
  http://www.holophase.com/omniglyph/onlinehelp/sch/setup_options_general.htm
| Always enable File Save - The "Save" entry in the File menu will always
| be enabled for use, even if the file has not been modified.

However, here's a third viewpoint:
  "File | Save" should save immediately, and never bring up a dialog.
  "File | Save as..." should always bring up a dialog.
    Because there's a dialog iff the menu command says '...'.
That would seem logical to me. As a corollary, "File | Save" could never
be offered for any document that has never previously been saved: it has
no filename, so a dialog is necessary, and therefore "Save as..." should
be the only option. The problem is that the world is so illogical that
I can't find any application that behaves this way. So, unless someone
strongly objects, I think we should just do (as above):
  if "phantom"
    then disable "File | Save" (and many other commands)
  else
    do whatever wx would do by default
by some more robust means that won't need maintenance if wx's behavior
ever changes again.