Re: [lmi] Child documents

[Vadim Zeitlin]

[I'm returning to the old, old topic of child documents as I've finally
 done the necessary changes in wx and so we can discuss LMI changes now]

On Fri, 14 May 2010 10:51:56 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2010-05-13 14:53Z, Vadim Zeitlin wrote:
GC> > On Thu, 13 May 2010 01:56:24 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> > GC> Compared to that "far more limiting" concept, what lmi uses is 
still
GC> > GC> > GC> more limited, because it omits the Save[As]() part: all that 
remains is
GC> > GC> > GC> to "simply group TDocument [wxDocument] objects in a hierarchy" 
and to
GC> > GC> > GC> cascade only one operation, i.e., closing.
GC> > GC> > 
GC> > GC> >  I thought that this was indeed just a limitation and so it still 
made
GC> > GC> > sense. However now I'm less sure: if a child document can't be saved
GC> > GC> > independently of the main one
GC> > GC> 
GC> > GC> It can't be saved in any sense whatsoever.
GC> > 
GC> >  FWIW the implementation shown below doesn't really prevent saving child
GC> > documents.
GC> 
GC>   http://review.bakefile.org/r/204/diff/#index_header
GC> 1094        (usually when it's being created itself). They also can't be 
independently
GC> 1095        saved. A child document has its own view with the corresponding 
window.
GC> 
GC> Then I think you're documenting that they can't be saved independently,
GC> as a precaution to anyone who might hope to find support for that; and
GC> also saying that you haven't created obstacles to prevent a determined
GC> and clever user from circumventing that documentary restriction.

 This was indeed correct for the original version of the patch but in the
light of this message and after thinking more about it, I did implement
such obstacles now and neither "Save" nor "Save As" menu commands now do
anything for child documents. I.e. wxWidgets code takes care to disable
them when the child document window is active.

GC> If so, then that sounds okay to me, as long as I can implement such a
GC> restriction myself, e.g. with
GC>   EVT_UPDATE_UI(wxID_SAVE...

 You still can but this isn't necessary any longer.

GC> > It doesn't do anything in my example however and I'm not sure if
GC> > it can be reasonably implemented. But child documents are a rare concept
GC> > and I think that 99% of their use will be read-only anyhow so I'll just
GC> > postpone this problem for now. Except that it would be arguably reasonable
GC> > to disable wxID_SAVE[_AS] menu items for child documents inside wx itself
GC> > instead of doing it in your code.
GC> 
GC> It's okay with me if lmi does this for now, and then maybe someday wx
GC> will take care of it. But today lmi has:
GC> 
GC>       EVT_UPDATE_UI(wxID_SAVE                 
,IllustrationView::UponUpdateFileSave)
GC> 
GC>   void IllustrationView::UponUpdateFileSave(wxUpdateUIEvent& e)
GC>   {
GC>       e.Enable(!is_phony_ && document().IsModified());
GC>   }

 Again, this is not needed at all but if you still want to do it, you can
use

        e.Enable(!document().IsChildDocument() && document.IsModified());

However in my patch
GC> BTW, note the file-history comment above:
GC>   /// accordingly, it should never be added to any wxFileHistory. This
GC> Is that handled automatically already? I'm thinking that, if child
GC> documents can't be saved, then they can't be opened either, so they
GC> also can't be opened via file history.

 No, they're never added to the history because this only happens when the
document is either opened from a file (and child documents never are opened
like this) or saved to a file (and child documents are never saved).

GC> > GC> I suppose a separate document is a convenience rather than a 
necessity;
GC> > GC> or at least it seems convenient to an old OWL hand.
GC> > GC> 
GC> > GC> Here's a complete description of the additional behavior I seek.
GC> > GC>   File | New | Census  # creates window 'A'
GC> > GC>   Census | Run case    # creates window 'B'
GC> > GC>   Window | Next        # focuses window 'A'
GC> > GC>   [MDI-menu] | Close   # closes only 'A', and that's the problem.
GC> > GC> I want 'B' to close whenever 'A' closes for any reason.
...
GC> > In fact the next question I have is whether I should create a working 
patch
GC> > doing this for LMI?
GC> 
GC> Yes, please, and let me know when the supporting changes are available
GC> in a wx snapshot.

 I've committed the supporting changes as r68051 so they should be
available in the tomorrow snapshot.

 With these changes in wx and the patch below (against LMI HEAD) things
seem to work correctly for me, i.e. closing the window 'A' (in any way, not
only via MDI menu) does close window 'B'. The patch is mostly pretty
straightforward and simply replaces checks for is_phony_ with checks for
wxDocument::IsChildDocument(). The checks done by UponUpdateFileSave[As]()
methods and the methods themselves could be simply removed because, as I
wrote above, this is done by wx itself anyhow so while there would be no
harm in keeping them neither this just seemed unnecessary.

 The only not completely trivial part of the patch is the change to
MakeNewIllustrationDocAndView() itself. First, I had to change its
signature as I need the parent document to create a child one. And because
child documents don't use document templates, the document and its view are
now created manually. OTOH I removed the apparently unnecessary Modify()
and SetDocumentSaved() calls.

 Finally, the change in MakeNewIllustrationDocAndView() signature made it
necessary to change custom_io_0_run_if_file_exists() as well but this is
the [only] part that I'm really not sure about as I don't understand in
details what does this code do (i.e. does it really create a child document
here when we don't seem to have any parent one?) nor can I exercise it
(what exactly should I do to make it run?). So while I tried to preserve
the old behaviour by moving the old MakeNewIllustrationDocAndView() code
into this function, it's probably broken by this patch and I'd welcome any
advice about how to correct it.

 Any other comments about the patch below would be welcome too, of course.

 Thanks in advance,
VZ

--- a/census_view.cpp
+++ b/census_view.cpp
@@ -809,7 +809,7 @@ void CensusView::ViewOneCell(int index)
 {
     std::string const name(cell_parms()[index]["InsuredName"].str());
     IllustrationView& illview = MakeNewIllustrationDocAndView
-        (document().GetDocumentManager()
+        (document()
         ,serial_file_path(base_filename(), name, index, "ill").string().c_str()
         );
     illview.Run(&cell_parms()[index]);
@@ -830,7 +830,7 @@ void CensusView::ViewComposite()
         {
         std::string const name("composite");
         IllustrationView& illview = MakeNewIllustrationDocAndView
-            (document().GetDocumentManager()
+            (document()
             ,serial_file_path(base_filename(), name, -1, 
"ill").string().c_str()
             );
 
diff --git a/illustration_document.cpp b/illustration_document.cpp
index 90ef6bc..b70edd7 100644
--- a/illustration_document.cpp
+++ b/illustration_document.cpp
@@ -39,8 +39,8 @@
 
 IMPLEMENT_DYNAMIC_CLASS(IllustrationDocument, wxDocument)
 
-IllustrationDocument::IllustrationDocument()
-    :is_phony_(false)
+IllustrationDocument::IllustrationDocument(wxDocument* parent_document)
+    : wxDocument(parent_document)
 {
 }
 
@@ -89,11 +89,7 @@ wxHtmlWindow& IllustrationDocument::PredominantViewWindow() 
const
 
 bool IllustrationDocument::OnCreate(wxString const& filename, long int flags)
 {
-    if(LMI_WX_CHILD_DOCUMENT & flags)
-        {
-        is_phony_ = true;
-        }
-    else if(wxDOC_NEW & flags)
+    if(wxDOC_NEW & flags)
         {
         *doc_.input_data_ = default_cell();
         }
@@ -171,12 +167,6 @@ bool IllustrationDocument::DoOpenDocument(wxString const& 
filename)
 
 bool IllustrationDocument::DoSaveDocument(wxString const& filename)
 {
-    if(is_phony_)
-        {
-        warning() << "Impossible to save '" << filename << "'." << LMI_FLUSH;
-        return false;
-        }
-
     std::string f = ValidateAndConvertFilename(filename);
     std::ofstream ofs(f.c_str(), ios_out_trunc_binary());
     doc_.write(ofs);
diff --git a/illustration_document.hpp b/illustration_document.hpp
index 4768f7c..67aba95 100644
--- a/illustration_document.hpp
+++ b/illustration_document.hpp
@@ -32,19 +32,6 @@
 
 #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,
-/// distinct document, can't be opened or saved separately, and should
-/// be closed, along with all its views, when its parent closes; and,
-/// accordingly, it should never be added to any wxFileHistory. This
-/// set of behaviors is implemented here by implicitly defining a new
-/// document-creation flag, appropriating an unused bit in the flags
-/// word. This is brittle, but then again it seems unlikely that
-/// anyone will change this aspect of wx.
-
-enum {LMI_WX_CHILD_DOCUMENT = 8};
-
 class IllustrationView;
 class WXDLLIMPEXP_FWD_CORE wxHtmlWindow;
 
@@ -55,7 +42,7 @@ class IllustrationDocument
     friend class IllustrationView;
 
   public:
-    IllustrationDocument();
+    IllustrationDocument(wxDocument* parent_document = NULL);
     virtual ~IllustrationDocument();
 
     IllustrationView& PredominantView() const;
@@ -73,8 +60,6 @@ class IllustrationDocument
 
     single_cell_document doc_;
 
-    bool is_phony_;
-
     DECLARE_DYNAMIC_CLASS(IllustrationDocument)
 };
 
diff --git a/illustration_view.cpp b/illustration_view.cpp
index 86f1fa7..7f162d4 100644
--- a/illustration_view.cpp
+++ b/illustration_view.cpp
@@ -76,8 +76,6 @@ BEGIN_EVENT_TABLE(IllustrationView, ViewEx)
     EVT_MENU(XRCID("edit_cell"             ),IllustrationView::UponProperties)
     EVT_MENU(XRCID("copy_summary"          ),IllustrationView::UponCopySummary)
     EVT_MENU(wxID_COPY                      ,IllustrationView::UponCopyFull)
-    EVT_UPDATE_UI(wxID_SAVE                 
,IllustrationView::UponUpdateFileSave)
-    EVT_UPDATE_UI(wxID_SAVEAS               
,IllustrationView::UponUpdateFileSaveAs)
     EVT_UPDATE_UI(XRCID("edit_cell"        
),IllustrationView::UponUpdateProperties)
 
 // There has to be a better way to inhibit these inapplicable ids.
@@ -100,7 +98,6 @@ END_EVENT_TABLE()
 IllustrationView::IllustrationView()
     :ViewEx                  ()
     ,html_window_            (0)
-    ,is_phony_               (false)
 {
 }
 
@@ -125,7 +122,7 @@ wxWindow* IllustrationView::CreateChildWindow()
 
 int IllustrationView::EditProperties()
 {
-    if(is_phony_)
+    if(GetDocument()->IsChildDocument())
         {
 warning() << "That command should have been disabled." << LMI_FLUSH;
         return wxID_CANCEL;
@@ -174,9 +171,8 @@ wxMenuBar* IllustrationView::MenuBar() const
 
 bool IllustrationView::OnCreate(wxDocument* doc, long int flags)
 {
-    if(flags & LMI_WX_CHILD_DOCUMENT)
+    if(doc->IsChildDocument())
         {
-        is_phony_ = true;
         return ViewEx::OnCreate(doc, flags);
         }
 
@@ -244,34 +240,12 @@ void IllustrationView::UponPrintPdf(wxCommandEvent&)
 
 void IllustrationView::UponProperties(wxCommandEvent&)
 {
-// may have to check is_phony_ here--but that's bogus
-    if(is_phony_)
-        {
-//        return;
-        }
-
     if(wxID_OK == EditProperties())
         {
         Run();
         }
 }
 
-/// This complete replacement for wxDocManager::OnUpdateFileSave()
-/// should not call Skip().
-
-void IllustrationView::UponUpdateFileSave(wxUpdateUIEvent& e)
-{
-    e.Enable(!is_phony_ && document().IsModified());
-}
-
-/// This complete replacement for wxDocManager::OnUpdateFileSaveAs()
-/// should not call Skip().
-
-void IllustrationView::UponUpdateFileSaveAs(wxUpdateUIEvent& e)
-{
-    e.Enable(!is_phony_);
-}
-
 void IllustrationView::UponUpdateInapplicable(wxUpdateUIEvent& e)
 {
     e.Enable(false);
@@ -279,7 +253,7 @@ void 
IllustrationView::UponUpdateInapplicable(wxUpdateUIEvent& e)
 
 void IllustrationView::UponUpdateProperties(wxUpdateUIEvent& e)
 {
-    e.Enable(!is_phony_);
+    e.Enable(!GetDocument()->IsChildDocument());
 }
 
 void IllustrationView::Run(Input* overriding_input)
@@ -309,8 +283,6 @@ void IllustrationView::Run(Input* overriding_input)
 /// CensusView::ViewComposite() calls MakeNewIllustrationDocAndView()
 /// to view a composite whose values are not conveniently calculated
 /// in this TU, so they're passed via this function.
-///
-/// custom_io_0_run_if_file_exists() uses this function similarly.
 
 void IllustrationView::SetLedger(boost::shared_ptr<Ledger const> ledger)
 {
@@ -324,28 +296,27 @@ void IllustrationView::SetLedger(boost::shared_ptr<Ledger 
const> ledger)
 /// becomes useful.
 
 IllustrationView& MakeNewIllustrationDocAndView
-    (wxDocManager* dm
-    ,char const*   filename
+    (wxDocument& parent_document
+    ,char const* filename
     )
 {
-    LMI_ASSERT(0 != dm);
     LMI_ASSERT(0 != filename);
 
-    wxDocTemplate* dt = dm->FindTemplateForPath(filename);
-    LMI_ASSERT(0 != dt);
-
-    wxDocument* new_document = dt->CreateDocument
-        (filename
-        ,wxDOC_SILENT | LMI_WX_CHILD_DOCUMENT
+    // Child documents cannot be created using document templates so do it
+    // manually.
+    IllustrationDocument* const new_document = new IllustrationDocument
+        (&parent_document
         );
 
-    IllustrationDocument& illdoc =
-        safely_dereference_as<IllustrationDocument>(new_document)
-        ;
-    illdoc.SetFilename(filename, true);
-    illdoc.Modify(false);
-    illdoc.SetDocumentSaved(true);
-    return illdoc.PredominantView();
+    new_document->SetFilename(filename, true);
+
+    // As we don't use the template, we also need to manually create the
+    // associated view as well.
+    wxView* const view = new IllustrationView();
+    view->SetDocument(new_document);
+    view->OnCreate(new_document, wxDOC_SILENT);
+
+    return new_document->PredominantView();
 }
 
 /// Run an illustration from "custom" input.
@@ -378,10 +349,25 @@ bool custom_io_0_run_if_file_exists(wxDocManager* dm)
             else
                 {
                 LMI_ASSERT(0 != dm);
-                IllustrationView& illview = MakeNewIllustrationDocAndView
-                    (dm
-                    ,(c.custom_output_filename() + ".ill").c_str()
+
+                std::string const filename(c.custom_output_filename() + 
".ill");
+
+                wxDocTemplate* dt = dm->FindTemplateForPath(filename);
+                LMI_ASSERT(0 != dt);
+
+                wxDocument* new_document = dt->CreateDocument
+                    (filename
+                    ,wxDOC_SILENT
                     );
+
+                IllustrationDocument& illdoc =
+                    safely_dereference_as<IllustrationDocument>(new_document)
+                    ;
+                illdoc.SetFilename(filename, true);
+                illdoc.Modify(false);
+                illdoc.SetDocumentSaved(true);
+
+                IllustrationView& illview = illdoc.PredominantView();
                 illview.SetLedger(z.principal_ledger());
                 illview.DisplaySelectedValuesAsHtml();
                 safely_dereference_as<wxFrame>(illview.GetFrame()).Maximize();
diff --git a/illustration_view.hpp b/illustration_view.hpp
index e908252..b545975 100644
--- a/illustration_view.hpp
+++ b/illustration_view.hpp
@@ -88,15 +88,12 @@ class IllustrationView
     void UponPreviewPdf        (wxCommandEvent&);
     void UponPrintPdf          (wxCommandEvent&);
     void UponProperties        (wxCommandEvent&);
-    void UponUpdateFileSave    (wxUpdateUIEvent&);
-    void UponUpdateFileSaveAs  (wxUpdateUIEvent&);
     void UponUpdateInapplicable(wxUpdateUIEvent&);
     void UponUpdateProperties  (wxUpdateUIEvent&);
 
     Input& input_data();
 
     wxHtmlWindow* html_window_;
-    bool is_phony_;
     boost::shared_ptr<Ledger const> ledger_values_;
 
     DECLARE_DYNAMIC_CLASS(IllustrationView)
@@ -104,8 +101,8 @@ class IllustrationView
 };
 
 IllustrationView& MakeNewIllustrationDocAndView
-    (wxDocManager* dm
-    ,char const*   filename
+    (wxDocument& parent_document
+    ,char const* filename
     );
 
 bool custom_io_0_run_if_file_exists(wxDocManager*);
diff --git a/view_ex.cpp b/view_ex.cpp
index 5907851..2bcda2a 100644
--- a/view_ex.cpp
+++ b/view_ex.cpp
@@ -129,7 +129,10 @@ bool ViewEx::OnClose(bool delete_window)
         return false;
         }
 
-    DocManager().DissociateFileHistoryFromFileMenu(FrameWindow().GetMenuBar());
+    if (!GetDocument()->IsChildDocument())
+        {
+        
DocManager().DissociateFileHistoryFromFileMenu(FrameWindow().GetMenuBar());
+        }
 
     Activate(false);
 
@@ -153,7 +156,10 @@ bool ViewEx::OnClose(bool delete_window)
 bool ViewEx::OnCreate(wxDocument* doc, long int)
 {
     wxGetApp().CreateChildFrame(doc, this);
-    DocManager().AssociateFileHistoryWithFileMenu(FrameWindow().GetMenuBar());
+    if (!doc->IsChildDocument())
+        {
+        
DocManager().AssociateFileHistoryWithFileMenu(FrameWindow().GetMenuBar());
+        }
     GetFrame()->SetLabel("Loading document...");
 
     wxWindow* child = CreateChildWindow();