[lmi] a less trivial patch: don't use deprecated MakeDefaultName() + refactoring

[Vadim Zeitlin]

On Sun, 11 May 2008 02:38:12 +0000 Greg Chicares <address@hidden> wrote:

GC> >  And I also should have probably mentioned in my previous email that I'm
GC> > going to submit a slightly less trivial patch fixing a similar problem 
with
GC> > MakeDefaultName() very soon so if you prefer to apply both of them at once
GC> > you should wait for it.
GC> 
GC> I was about to ask whether you had a replacement for that
GC> function, too.

 Attached is the patch which fixes this. I got a little carried away while
fixing this as I didn't want to duplicate my fix and so I also corrected a
TODO in census_document.hpp:

// TODO ?? This class and the illustration-document class have much in
// common that should be factored into a base class.

 In order to do this I introduced a new base class, LifeViewDocument, from
which both CensusDocument and IllustrationDocument inherit now and which
encapsulates the modification to wxDocument way of working which is used by
both of these classes. I'm not too happy with the (rather meaningless) name
of this class but unfortunately I couldn't find anything better, if you
have any suggestions I'd be grateful.

 Other than refactoring needed to fix this TODO there are only 2 real
changes in the patch:

1. Use MakeNewDocumentName() instead of deprecated MakeDefaultName().

   Unfortunately the new method will only exist in (yet unreleased) wx
   2.8.8 so I had to add an "#if wxCHECK_VERSION" check around it, this
   is a bit ugly but can be removed as soon as 2.8.8 is released and LMI
   starts using it.

2. Remove LMI_WX_CHILD_DOCUMENT before passing flags to wxDocument.

   I think it is wrong to use undefined (at wx level) flags with
   wxDocument, at the very least this may (and should, although currently
   doesn't) result in an assert about unknown flag.

The rest of the patch just moves the code around and the only questionable
construct in it may be the hack with pretending that wxDOC_NEW is set in
IllustrationDocument::OnCreate(), but OTOH is_phony_ itself is a kind of a
hack too and I prefer the current version which keeps everything related to
it in IllustrationDocument itself to introducing a IsPhony() virtual method
in LifeViewDocument class. And I don't see any other/better alternatives.

 As usual, looking forward to your comments!
VZ

P.S. Resending with the patch inline as the patch in attachment resulted in
     a bounce from lmi-owner with the error message "Attachment type
     explicitly disallowed". If Savannah uses mailman could you please
     enable TEXT/DIFF in its options? Or do you prefer that I make the
     patch available in some other way? Of course, as usual, you can also
     find the changes at http://lmi.tt-solutions.com/hg/lmi-tt/rev/3cf26ac25cf6

--- old/census_document.cpp     2008-03-23 02:53:24 +0000
+++ new/census_document.cpp     2008-05-11 11:01:07 +0000
@@ -29,16 +29,14 @@
 #include "census_document.hpp"
 #include "view_ex.tpp"
 
-#include "alert.hpp"
 #include "census_view.hpp"
-#include "miscellany.hpp"
-
-#include <fstream>
-
+
+// Our real base class is LifeViewDocument but this is just an implementation
+// detail and doesn't need to appear in wxRTTI classes chain.
 IMPLEMENT_DYNAMIC_CLASS(CensusDocument, wxDocument)
 
 CensusDocument::CensusDocument()
-    :wxDocument()
+    :LifeViewDocument()
 {
 }
 
@@ -54,69 +52,24 @@
         );
 }
 
-bool CensusDocument::OnCreate(wxString const& filename, long int flags)
+void CensusDocument::DoReadDocument(std::istream& is)
 {
-    // TODO ?? Why not offer doc_.read(filename)?
-    if(!(wxDOC_NEW & flags))
+    if(is)
         {
-        std::ifstream ifs(filename.c_str());
-        if(!ifs)
-            {
-            warning()
-                << "Unable to read file '"
-                << filename
-                << "'."
-                << LMI_FLUSH
-                ;
-            return false;
-            }
-        doc_.read(ifs);
+        doc_.read(is);
         }
 
     convert_from_ihs(doc_.case_parms_ , case_parms_ );
     convert_from_ihs(doc_.cell_parms_ , cell_parms_ );
     convert_from_ihs(doc_.class_parms_, class_parms_);
-
-    return wxDocument::OnCreate(filename, flags);
-}
-
-bool CensusDocument::OnNewDocument()
-{
-    Modify(true);
-    SetDocumentSaved(false);
-
-    wxString name;
-    GetDocumentManager()->MakeDefaultName(name);
-    SetTitle(name);
-    SetFilename(name, true);
-
-    return true;
-}
-
-/// See documentation for IllustrationDocument::DoOpenDocument().
-
-bool CensusDocument::DoOpenDocument(wxString const& filename)
-{
-    return true;
-}
-
-/// See documentation for IllustrationDocument::DoSaveDocument().
-
-bool CensusDocument::DoSaveDocument(wxString const& filename)
+}
+
+void CensusDocument::DoWriteDocument(std::ostream& os)
 {
     convert_to_ihs(doc_.case_parms_ , case_parms_ );
     convert_to_ihs(doc_.cell_parms_ , cell_parms_ );
     convert_to_ihs(doc_.class_parms_, class_parms_);
 
-    std::ofstream ofs(filename.c_str(), ios_out_trunc_binary());
-    doc_.write(ofs);
-    if(!ofs)
-        {
-        warning() << "Unable to save '" << filename << "'." << LMI_FLUSH;
-        return false;
-        }
-
-    status() << "Saved '" << filename << "'." << std::flush;
-    return true;
+    doc_.write(os);
 }
 

--- old/census_document.hpp     2008-03-25 16:46:18 +0000
+++ new/census_document.hpp     2008-05-11 10:54:59 +0000
@@ -28,18 +28,14 @@
 
 #include "input.hpp"
 #include "multiple_cell_document.hpp"
+#include "life_view_document.hpp"
 
 #include <boost/utility.hpp>
 
-#include <wx/docview.h>
-
 class WXDLLIMPEXP_FWD_CORE wxListView;
 
-// TODO ?? This class and the illustration-document class have much in
-// common that should be factored into a base class.
-
 class CensusDocument
-    :public wxDocument
+    :public LifeViewDocument
     ,private boost::noncopyable
 {
     friend class CensusView;
@@ -49,14 +45,12 @@
     virtual ~CensusDocument();
 
   private:
+    // Implement LifeViewDocument pure virtual methods.
+    virtual void DoReadDocument(std::istream& is);
+    virtual void DoWriteDocument(std::ostream& is);
+
     wxListView& PredominantViewWindow() const;
 
-    // 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);
-
     multiple_cell_document doc_;
 
     std::vector<Input> case_parms_;

--- old/illustration_document.cpp       2008-03-23 02:53:24 +0000
+++ new/illustration_document.cpp       2008-05-11 11:01:23 +0000
@@ -32,14 +32,14 @@
 #include "alert.hpp"
 #include "illustration_view.hpp"
 #include "inputillus.hpp"
-#include "miscellany.hpp"
-
-#include <fstream>
-
+
+// Our real base class is LifeViewDocument but this is just an implementation
+// detail and doesn't need to appear in wxRTTI classes chain.
 IMPLEMENT_DYNAMIC_CLASS(IllustrationDocument, wxDocument)
 
 IllustrationDocument::IllustrationDocument()
-    :is_phony_(false)
+    :LifeViewDocument(),
+     is_phony_(false)
 {
 }
 
@@ -60,95 +60,26 @@
         );
 }
 
-/// Class IllustrationView overloads wxView::OnCreate() to display a
-/// dialog that allow input parameters to be edited before the view is
-/// shown. Cancelling that dialog deliberately prevents that view from
-/// being created: it is not tasteful to show a blank view window
-/// underneath the dialog and destroy it on wxID_CANCEL.
-///
-/// But wxDocManager::CreateDocument() calls wxDocument::OnCreate()
-/// (which calls wxView::OnCreate() to create a view) before it calls
-/// wxDocument::DoOpenDocument() (where, by default, wx would read the
-/// document's data). Yet here it is required to read the document's
-/// data before the view is created.
-///
-/// Resolution: Read document data in IllustrationDocument::OnCreate()
-/// instead of in IllustrationDocument::DoOpenDocument(). Invoke
-/// wxView::OnCreate() from IllustrationView::OnCreate() only when the
-/// initial dialog is not cancelled.
-///
-/// Alternative not used: Because wxDocument::OnCreate() only creates
-/// a view and does nothing else, another resolution is to call that
-/// base-class function not from IllustrationDocument::OnCreate() but
-/// rather from IllustrationDocument::DoOpenDocument(). That is
-/// rejected for two reasons: it seems unnatural; and, far worse, it
-/// wreaks havoc on wx's object management--for instance, the document
-/// destroys itself when its last view ceases to exist, and much labor
-/// is required to prevent memory leaks or segfaults in that case.
-
 bool IllustrationDocument::OnCreate(wxString const& filename, long int flags)
 {
     if(LMI_WX_CHILD_DOCUMENT & flags)
         {
         is_phony_ = true;
+
+        // Remove the unknown (to wx) flag before passing it to the library.
+        flags &= ~LMI_WX_CHILD_DOCUMENT;
         }
 
-    // TODO ?? Why not offer doc_.read(filename)?
-    if(!(wxDOC_NEW & flags) && !(is_phony_))
+    if(is_phony_)
         {
-        std::ifstream ifs(filename.c_str());
-        if(!ifs)
-            {
-            warning()
-                << "Unable to read file '"
-                << filename
-                << "'."
-                << LMI_FLUSH
-                ;
-            return false;
-            }
-        doc_.read(ifs);
+        // Prevent the base class from reading the document contents (as child
+        // documents can't be opened nor saved) by always pretending that we
+        // create a new one.
+        flags |= wxDOC_NEW;
         }
-    convert_from_ihs(doc_.input_data(), input_);
-
-    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);
-
-    wxString name;
-    GetDocumentManager()->MakeDefaultName(name);
-    SetTitle(name);
-    SetFilename(name, true);
-
-    return true;
-}
-
-/// Override wx's built-in file management: doc_ handles that.
-///
-/// Override DoOpenDocument() instead of OnOpenDocument(): the latter
-/// doesn't permit customizing its diagnostic messages.
-
-bool IllustrationDocument::DoOpenDocument(wxString const& filename)
-{
-    return true;
-}
-
-/// Override wx's built-in file management: doc_ handles that.
-///
-/// Override DoSaveDocument() instead of OnSaveDocument(): the latter
-/// doesn't permit customizing its diagnostic messages.
+
+    return LifeViewDocument::OnCreate(filename, flags);
+}
 
 bool IllustrationDocument::DoSaveDocument(wxString const& filename)
 {
@@ -158,16 +89,23 @@
         return false;
         }
 
+    return LifeViewDocument::DoSaveDocument(filename);
+}
+
+void IllustrationDocument::DoReadDocument(std::istream& is)
+{
+    if(is)
+        {
+        doc_.read(is);
+        }
+
+    convert_from_ihs(doc_.input_data(), input_);
+
+}
+
+void IllustrationDocument::DoWriteDocument(std::ostream& os)
+{
     convert_to_ihs(*doc_.input_data_, input_);
-    std::ofstream ofs(filename.c_str(), ios_out_trunc_binary());
-    doc_.write(ofs);
-    if(!ofs)
-        {
-        warning() << "Unable to save '" << filename << "'." << LMI_FLUSH;
-        return false;
-        }
 
-    status() << "Saved '" << filename << "'." << std::flush;
-    return true;
+    doc_.write(os);
 }
-

--- old/illustration_document.hpp       2008-03-25 16:46:18 +0000
+++ new/illustration_document.hpp       2008-05-11 10:55:58 +0000
@@ -28,11 +28,10 @@
 
 #include "input.hpp"
 #include "single_cell_document.hpp"
+#include "life_view_document.hpp"
 
 #include <boost/utility.hpp>
 
-#include <wx/docview.h>
-
 /// WX !! The wx document-view implementation has no notion of 'child'
 /// documents, but sometimes lmi creates a document that logically is
 /// a 'child' of a parent CensusDocument: it corresponds to no actual,
@@ -50,7 +49,7 @@
 class WXDLLIMPEXP_FWD_CORE wxHtmlWindow;
 
 class IllustrationDocument
-    :public wxDocument
+    :public LifeViewDocument
     ,private boost::noncopyable
 {
     friend class IllustrationView;
@@ -61,15 +60,19 @@
 
     IllustrationView& PredominantView() const;
 
+    // Override the base class methods to add special treatment of phony case.
+    virtual bool OnCreate(wxString const& filename, long int flags);
+
+  protected:
+    virtual bool DoSaveDocument(wxString const& filename);
+
   private:
+    // Implement LifeViewDocument pure virtual methods.
+    virtual void DoReadDocument(std::istream& is);
+    virtual void DoWriteDocument(std::ostream& is);
+
     wxHtmlWindow& PredominantViewWindow() const;
 
-    // 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);
-
     single_cell_document doc_;
 
     Input input_;

--- old/life_view_document.cpp  1970-01-01 00:00:00 +0000
+++ new/life_view_document.cpp  2008-05-11 11:01:16 +0000
@@ -0,0 +1,139 @@
+// Base class for illustration and census LMI document objects.
+//
+// Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008 Gregory W. Chicares.
+//
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License version 2 as
+// published by the Free Software Foundation.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software Foundation,
+// Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+//
+// http://savannah.nongnu.org/projects/lmi
+// email: <address@hidden>
+// snail: Chicares, 186 Belle Woods Drive, Glastonbury CT 06033, USA
+
+// $Id: illustration_document.cpp,v 1.16 2008/03/23 02:53:24 chicares Exp $
+
+#ifdef __BORLANDC__
+#   include "pchfile.hpp"
+#   pragma hdrstop
+#endif
+
+#include "life_view_document.hpp"
+
+#include "alert.hpp"
+#include "miscellany.hpp"   // for ios_out_trunc_binary()
+
+#include <fstream>
+
+/// 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 LifeViewDocument::OnNewDocument()
+{
+    Modify(true);
+    SetDocumentSaved(false);
+
+#if wxCHECK_VERSION(2, 8, 8)
+    wxString const name = GetDocumentManager()->MakeNewDocumentName();
+#else // wx < 2.8.8
+    wxString name;
+    GetDocumentManager()->MakeDefaultName(name);
+#endif // wx >=/< 2.8.8
+    SetTitle(name);
+    SetFilename(name, true);
+
+    return true;
+}
+
+/// Class IllustrationView overloads wxView::OnCreate() to display a
+/// dialog that allow input parameters to be edited before the view is
+/// shown. Cancelling that dialog deliberately prevents that view from
+/// being created: it is not tasteful to show a blank view window
+/// underneath the dialog and destroy it on wxID_CANCEL.
+///
+/// But wxDocManager::CreateDocument() calls wxDocument::OnCreate()
+/// (which calls wxView::OnCreate() to create a view) before it calls
+/// wxDocument::DoOpenDocument() (where, by default, wx would read the
+/// document's data). Yet here it is required to read the document's
+/// data before the view is created.
+///
+/// Resolution: Read document data in IllustrationDocument::OnCreate()
+/// instead of in IllustrationDocument::DoOpenDocument(). Invoke
+/// wxView::OnCreate() from IllustrationView::OnCreate() only when the
+/// initial dialog is not cancelled.
+///
+/// Alternative not used: Because wxDocument::OnCreate() only creates
+/// a view and does nothing else, another resolution is to call that
+/// base-class function not from IllustrationDocument::OnCreate() but
+/// rather from IllustrationDocument::DoOpenDocument(). That is
+/// rejected for two reasons: it seems unnatural; and, far worse, it
+/// wreaks havoc on wx's object management--for instance, the document
+/// destroys itself when its last view ceases to exist, and much labor
+/// is required to prevent memory leaks or segfaults in that case.
+
+bool LifeViewDocument::OnCreate(wxString const& filename, long int flags)
+{
+    // TODO ?? Why not offer doc_.read(filename)?
+    std::ifstream ifs;
+    if(!(wxDOC_NEW & flags))
+        {
+        ifs.open(filename.c_str());
+        if(!ifs)
+            {
+            warning()
+                << "Unable to read file '"
+                << filename
+                << "'."
+                << LMI_FLUSH
+                ;
+            return false;
+            }
+        }
+
+    DoReadDocument(ifs);
+
+    return wxDocument::OnCreate(filename, flags);
+}
+
+/// Override wx's built-in file management: doc_ handles that.
+///
+/// Override DoOpenDocument() instead of OnOpenDocument(): the latter
+/// doesn't permit customizing its diagnostic messages.
+
+bool LifeViewDocument::DoOpenDocument(wxString const& filename)
+{
+    return true;
+}
+
+/// Override wx's built-in file management: doc_ handles that.
+///
+/// Override DoSaveDocument() instead of OnSaveDocument(): the latter
+/// doesn't permit customizing its diagnostic messages.
+
+bool LifeViewDocument::DoSaveDocument(wxString const& filename)
+{
+    std::ofstream ofs(filename.c_str(), ios_out_trunc_binary());
+    if(!ofs)
+        {
+        warning() << "Unable to save '" << filename << "'." << LMI_FLUSH;
+        return false;
+        }
+
+    status() << "Saved '" << filename << "'." << std::flush;
+    return true;
+}
+
+

--- old/life_view_document.hpp  1970-01-01 00:00:00 +0000
+++ new/life_view_document.hpp  2008-05-11 10:56:23 +0000
@@ -0,0 +1,56 @@
+// Document class for censuses.
+//
+// Copyright (C) 2004, 2005, 2006, 2007, 2008 Gregory W. Chicares.
+//
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License version 2 as
+// published by the Free Software Foundation.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software Foundation,
+// Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+//
+// http://savannah.nongnu.org/projects/lmi
+// email: <address@hidden>
+// snail: Chicares, 186 Belle Woods Drive, Glastonbury CT 06033, USA
+
+// $Id: census_document.hpp,v 1.11 2008/03/25 16:46:18 chicares Exp $
+
+#ifndef life_view_document_hpp
+#define life_view_document_hpp
+
+#include "config.hpp"
+
+#include <wx/docview.h>
+
+// Common abstract base class containing logic and functionality used by all
+// LMI document classes.
+
+class LifeViewDocument
+    :public wxDocument
+{
+  public:
+    // wxDocument overrides.
+    virtual bool OnCreate(wxString const& filename, long int flags);
+    virtual bool OnNewDocument();
+
+  protected:
+    virtual bool DoOpenDocument(wxString const& filename);
+    virtual bool DoSaveDocument(wxString const& filename);
+
+  private:
+    // Implement these methods in the derived class to read the document data
+    // from or write it to the specified stream. Notice that the stream in
+    // DoReadDocument() can be invalid indicating that a new document is being
+    // created instead of existing one being opened, the derived class must
+    // handle this case.
+    virtual void DoReadDocument(std::istream& is) = 0;
+    virtual void DoWriteDocument(std::ostream& is) = 0;
+};
+
+#endif // life_view_document_hpp

--- old/objects.make    2008-04-03 14:14:41 +0000
+++ new/objects.make    2008-05-11 10:58:53 +0000
@@ -301,6 +301,7 @@
   file_command_wx.o \
   illustration_document.o \
   illustration_view.o \
+  life_view_document.o \
   main_common.o \
   main_wx.o \
   msw_workarounds.o \

--- old/Makefile.am     2008-03-21 12:13:17 +0000
+++ new/Makefile.am     2008-05-11 12:42:20 +0000
@@ -156,6 +156,7 @@
     file_command_wx.cpp \
     illustration_document.cpp \
     illustration_view.cpp \
+    life_view_document.cpp \
     main_common.cpp \
     main_wx.cpp \
     msw_workarounds.cpp \