Re[2]: [lmi] Untimely "save" dialog [Was: wxmsw-2.9.0 regression: crash when a messagebox should appear]

[Vadim Zeitlin]

 Sorry for a longish post but there were several unrelated but not
completely independent problems/bugs there so they can't be explained
shortly nor can their explanation be completely separated. Nevertheless you
can see this post as a sum of 2 almost independent parts: first one is
about the "untimely prompt" and the second one is about the "zombie window"
(and there is also a couple of lines at the end about the crash when
modifying the most recently used menu).


On Mon, 09 Mar 2009 13:55:43 +0000 Greg Chicares <address@hidden> wrote:

GC> > In the "Comments" field, enter "xyz", then press OK. This
GC> > messagebox appears:
GC> > 
GC> > Do you want to save changes to sample.ill?
GC> > Yes   No   Cancel

 This problem should be fixed in normal case by the changes in
http://trac.wxwidgets.org/changeset/59449 which basically remove the calls
to OnSaveModified() from On{New,Open}Document() just as you suggested:
they don't seem to make sense there as these methods are only ever called
for the new documents which ought not to be saved in any case, even if they
were marked as modified.

 Because of this, overriding OnNewDocument() in LMI now becomes unnecessary
and so the following patch should be applied (with old code preserved
inside "#if !wxCHECK_VERSION(2,9,0)" if you want it to continue to work
with 2.8):

--- illustration_document.cpp   2008-12-27 02:57:00 +0000
+++ illustration_document.cpp   2009-03-09 17:38:15 +0000
@@ -115,31 +115,6 @@
     return wxDocument::OnCreate(filename, flags);
 }

-/// WX !! Oddly, wxDocument::OnNewDocument() calls OnSaveModified().
-/// That would seem appropriate if an existing document were being
-/// overlaid, but the function is designed to create an entirely new
-/// document. It's a problem here because this class sets the dirty
-/// flag earlier. wxDocument::OnNewDocument() also clears the dirty
-/// flag, but it seems more sensible to set it. For these and perhaps
-/// other reasons, the base-class function must not be called here.
-
-bool IllustrationDocument::OnNewDocument()
-{
-    Modify(true);
-    SetDocumentSaved(false);
-
-#if wxCHECK_VERSION(2,8,8)
-    wxString const name = GetDocumentManager()->MakeNewDocumentName();
-#else  // !wxCHECK_VERSION(2,8,8)
-    wxString name;
-    GetDocumentManager()->MakeDefaultName(name);
-#endif // !wxCHECK_VERSION(2,8,8)
-    SetTitle(name);
-    SetFilename(name, true);
-
-    return true;
-}
-
 /// Override wx's built-in file management: doc_ handles that.
 ///
 /// Override DoOpenDocument() instead of OnOpenDocument(): the latter

--- illustration_document.hpp   2008-12-27 02:57:00 +0000
+++ illustration_document.hpp   2009-03-09 17:38:25 +0000
@@ -66,7 +66,6 @@

     // wxDocument overrides.
     virtual bool OnCreate(wxString const& filename, long int flags);
-    virtual bool OnNewDocument();
     virtual bool DoOpenDocument(wxString const& filename);
     virtual bool DoSaveDocument(wxString const& filename);



 However the prompt still arises in the case when an error happens during
the document opening. I am however not sure if this is a problem at all in
this case: after all, the user did enter some data which you don't want to
lose (this is the reason for calling Modify(true) in the first place) so
maybe he should be given the possibility to save the .ill file even if
opening it failed? I'd like to hear your opinion about this. If you think
this is useful we should do at least 2 things:

1. Show the error message before the prompt for saving the file (as it's
   very confusing otherwise)
2. Remove the "Cancel" button from the dialog as closing the view can't
   be cancelled in this case

And if you don't think this is useful we need to decide whether it might
still be useful in some other situations, in which case the dirty flag
would need to be reset in LMI itself, or whether it can never be useful at
all in which case it would make sense to handle this at wx level.


GC> Click "OK". An MDI child window has opened; it has no apparent
GC> content, and its title is still "Loading document...".

 This is fixed by http://trac.wxwidgets.org/changeset/59455: now the window
is closed if the view creation fails.

 I also applied changes in http://trac.wxwidgets.org/changeset/59452 which
had to be revised in r59453 and r59454 to plug some memory leaks which
occurred if exceptions were thrown during the document creation but this
shouldn't affect LMI as IllustrationDocument doesn't seem to throw anything
anywhere. But now it could :-)

 The following patch can also now be applied to LMI to remove the
now unnecessary try/catch clause:

--- illustration_view.cpp       2009-01-06 15:01:23 +0000
+++ illustration_view.cpp       2009-03-09 18:26:59 +0000
@@ -175,11 +175,6 @@
 }

 /// This virtual function calls its base-class namesake explicitly.
-///
-/// Trap any exception thrown by EditProperties() to ensure that this
-/// function returns 'false' on failure, lest wx's doc-view framework
-/// create a zombie view. See:
-///   http://lists.nongnu.org/archive/html/lmi/2008-12/msg00017.html

 bool IllustrationView::OnCreate(wxDocument* doc, long int flags)
 {
@@ -189,16 +184,8 @@
         return ViewEx::OnCreate(doc, flags);
         }

-    try
-        {
-        if(wxID_OK != EditProperties())
-            {
-            return false;
-            }
-        }
-    catch(...)
-        {
-        report_exception();
+    if(wxID_OK != EditProperties())
+        {
         return false;
         }


Notice that an alternative could be to move Run() inside try/catch clause
instead but it seems inconsistent to catch only some exceptions and not all
of them, even if now (i.e. with my latest changes to wx) catching them or
letting them escape shouldn't change the observable behaviour.

 The only situation when it might be better to move Run() inside the
try/catch clause would be if we decide to keep the save prompt. In this
case the error message should be generated before it and so it would be
better to call report_exception() from here instead of waiting until it
bubbles up to wxApp::OnExceptionInMainLoop(). I don't attach the patch to
do it as it is very simple anyhow and I don't want to confuse matters
further by providing two mutually exclusive patches.



 Finally I was also able to reproduce the crash you reported. I didn't have
time to understand what's going on yet but it seems to be quite independent
from all the other bugs so I'll write about it in a separate message, this
one is already long enough.

 Please let me know if you see any problems with the changes discussed
above and what do you think about the proposed LMI patches.

 TIA,
VZ