Re: [lmi] wx_test cleanup

[Vadim Zeitlin]

On Sun, 02 Nov 2014 15:03:41 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2014-11-02 01:31Z, Vadim Zeitlin wrote:
GC> > On Wed, 29 Oct 2014 21:23:52 +0000 Greg Chicares <address@hidden> wrote:
...
GC> > GC>   (d) If the test that created the file passes.
GC> > 
GC> >  Yes, this is another possibility which I thought was similar enough to 
(b)
GC> > to not warrant including it, but I'm, to spoil the answer to your question
GC> > below, perfectly fine with it.
GC> 
GC> Okay. I'll spoil my answer to your question below by saying I
GC> still favor (d), even over (e).

 And I'm still perfectly fine with this.

GC> > 1. What to do with {New,Calc}Ill{Full,Summary}.txt files created by the
GC> >    "calculation_summary" text? This is related to one of the questions in
GC> >    my email from 2014-09-08T11:07:20Z
GC> 
GC> Here, I'll have to defer to Wendy: I don't know the motivation for
GC> particular tests.

 For now I'm leaving these files, it's a bit annoying to have them, but
they can be added to .svnignore (or .gitignore in my case) and I just
really have no idea what should really be done here.

GC> > 2. Some PDF viewers, notably the default/official one, lock the PDF file
GC> >    while it's opened in them, meaning that deleting the PDF file created
GC> >    in "pdf_illustration" test case will usually fail
GC> 
GC> This is 'LMI_WX_TEST_CASE(pdf_illustration)' in 'wx_test_pdf_create.cpp'.
GC> I'm not sure I understand the rationale for this test, but it seems to
GC> simulate "File | Print preview" for a new '.ill' and then test only this
GC> single condition:
GC> 
GC>     // Finally check for the expected output file existence.
GC>     LMI_ASSERT(fs::exists(pdf_path));
GC> 
GC> IOW, it opens the system's PDF viewer, but actually tests only that the
GC> output file exists.

 It also indirectly tests for the absence of errors when launching the PDF
viewer (if any dialogs would appear, the test would fail), which, I think,
can be useful. It would, of course, be even better if it could check if the
PDF viewer (or at least something launched by "Print preview") is really
running, but this would require getting its PID somehow and there is no
simple way to do it without some help from the non-testing part of LMI.

GC> and this test can fail only if the write_ledger_as_pdf() part fails.
GC> Therefore, it would seem more economical to add menu and toolbar
GC> commands to write a PDF file without invoking the PDF viewer, which
GC> takes some time to load and then must be closed manually--and then
GC> test that new command instead.

 I'm not sure about this... The GUI test is supposed to check that the user
actions have the expected effects and it seems to me that previewing the
PDF is a more useful action than checking that it could be created. It also
has the advantage that the person running the test can look at the document
opened in the PDF viewer after the test end and verify that, at least at a
glance, it looks correct (e.g. it's not empty).

 Of course, I could be completely wrong about all this and maybe LMI users
never preview the PDFs and this test wasn't designed to specifically check
for this. But this is the impression I had.

GC> So I think this leads to a new and better question: should we offer
GC> menu and toolbar commands to write a PDF file? Isn't that something
GC> that a program such as lmi would normally provide?

 This depends on how frequently this operation is used. I can't really
answer this.


GC> > it would be very nice if I could override
GC> > wxDocManager::MakeNewDocumentName() in the testing code as I could
GC> > handle almost all new files creation in a single place then.
GC> 
GC> I'm perfectly okay with something like this:
GC> 
GC> -- 8< --
GC> Index: docmanager_ex.cpp
GC> ===================================================================
GC> --- docmanager_ex.cpp       (revision 6018)
GC> +++ docmanager_ex.cpp       (working copy)
GC> @@ -200,6 +200,11 @@
GC>      delete printout;
GC>  }
GC> 
GC> +wxString DocManagerEx::MakeNewDocumentName()
GC> +{
GC> +    return wxDocManager::MakeNewDocumentName();
GC> +}
GC> +
GC>  // Use a popup menu, instead of wxGetSingleChoiceData with strings
GC>  // that are not generally appropriate. Our users don't understand
GC>  // "Select a document template", they'd rather not have to hit
GC> Index: docmanager_ex.hpp
GC> ===================================================================
GC> --- docmanager_ex.hpp       (revision 6018)
GC> +++ docmanager_ex.hpp       (working copy)
GC> @@ -66,6 +66,7 @@
GC>      // deprecated unless it's wanted on other platforms?
GC> 
GC>      // wxDocManager overrides.
GC> +    virtual wxString       MakeNewDocumentName();
GC>      virtual wxDocTemplate* SelectDocumentType
GC>          (wxDocTemplate** templates
GC>          ,int             noTemplates
GC> -- >8 --
GC> 
GC> And I think that's all you need. It doesn't have to be changed
GC> from private to protected, because you'll have no need to call
GC> DocManagerEx::MakeNewDocumentName(): you can just call the wx
GC> base class's version directly. Should I commit this change?

 Sorry, I don't see how does it help. The problem is not with overriding
MakeNewDocumentName() per se, but rather with making LMI actually use the
document manager object overriding this method. As I tried to explain in
my http://lists.nongnu.org/archive/html/lmi/2014-11/msg00002.html email:

   The trouble is that to do it, I need to use a custom class deriving from
   DocManagerEx instead of DocManagerEx itself, i.e., again, modify some
   code outside of the tests themselves. This modification would be pretty
   minimal though and would consist in adding a new CreateDocManager()
   virtual method to Skeleton and calling it instead of doing

    doc_manager_ = new DocManagerEx;

   in Skeleton::InitInitDocManager().

To make this more precise, I suggest the following change:

----------------------------- >8 ---------------------------------
diff --git a/skeleton.cpp b/skeleton.cpp
index f2064ff..9d4dce3 100644
--- a/skeleton.cpp
+++ b/skeleton.cpp
@@ -267,13 +267,18 @@ wxMenuBar* Skeleton::AdjustMenus(wxMenuBar* argument)
     return argument;
 }

+DocManagerEx* Skeleton::DoCreateDocManager()
+{
+    return new DocManagerEx;
+}
+
 void Skeleton::InitDocManager()
 {
     // WX !! At least in wx-2.5.1, this can't be created in the
     // constructor, because that would try to create an instance of
     // class wxPageSetupDialogData, which apparently mustn't be done
     // before the application object is constructed.
-    doc_manager_ = new DocManagerEx;
+    doc_manager_ = DoCreateDocManager();
     doc_manager_->FileHistoryLoad(*config_);

     new(wx) wxDocTemplate
diff --git a/skeleton.hpp b/skeleton.hpp
index 8e7ac77..495fc26 100644
--- a/skeleton.hpp
+++ b/skeleton.hpp
@@ -73,6 +73,9 @@ class Skeleton
     virtual bool OnExceptionInMainLoop ();
     virtual bool OnInit                ();

+    // Skeleton virtual methods which may be overridden only in testing code.
+    virtual DocManagerEx* DoCreateDocManager();
+
   private:
     wxMenuBar* AdjustMenus(wxMenuBar*);

----------------------------- >8 ---------------------------------

GC> Oh, and at the same time, could you take a look at the comments
GC> marked "WX !!" in these two files, and let me know what you
GC> think about them? It's been many years since I wrote them, and
GC> maybe some are no longer relevant.

[I'll reply to this separately to keep this thread focused]


 To summarize, my outstanding questions are:

1. What to do with the files created in the calculation_summary test
   (this one is for Wendy)?

2. What to do with the PDF file opened in the PDF viewer in
   pdf_illustration test case?

3. Can we add DoCreateDocManager() or provide some other way of customizing
   wxDocManager used by the main program?

 Thanks,
VZ