Re: [lmi] wx_test regression

[Greg Chicares]

On 2014-10-28 13:49Z, Vadim Zeitlin wrote:
> On Tue, 28 Oct 2014 12:54:11 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> With current lmi HEAD, compared to the results here:
> GC>   http://lists.nongnu.org/archive/html/lmi/2014-10/msg00069.html
> GC> I now see an additional failure, on a test that had passed:
> GC> 
> GC> calculation_summary: ERROR (Progress meter maximum count exceeded. [file 
> /lmi/src/lmi/progress_meter.cpp, line 98] )
> ...
> GC> so I now guess that the progress-meter problem may actually be in
> GC> Skeleton::UpdateViews()...which we did recently change:
> GC> 
> GC>     wxWindowList const& wl = frame_->GetChildren(); // changed to const&
> GC>     boost::shared_ptr<progress_meter> meter
> GC>         (create_progress_meter
> GC>             (wl.size()
> GC>             ,"Updating calculation summaries"
> GC>             )
> GC>         );
> GC>     for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); ++i)
> GC>         {
> GC>         wxDocMDIChildFrame const* c = 
> dynamic_cast<wxDocMDIChildFrame*>(*i);
> GC>         if(c)
> 
>  This is indeed the source of the problem: when the progress dialog is
> created, it becomes a child of the frame and so the actual number of the
> windows we iterate over is one greater than wl.size() passed to
> create_progress_meter().

Oh. I still don't automatically see something like wxWindowList as a
container...so I figured that adding 'const' wouldn't introduce the
possibility of its contents changing. But when I looked at the source
  const wxWindowList& GetChildren() const { return m_children; }
I understood.

>  And the really annoying thing is that I did test this change, but only
> with MSVC build, which uses native progress dialog that does not become a
> child of wxFrame and so didn't see this problem -- which I can reproduce
> perfectly well in MinGW build, where the generic progress dialog is used.

Well, the really encouraging thing is that the automated GUI test
has already paid its first dividend, by finding a defect quickly
and cheaply. When 'wx_test' is finished and integrated into our
normal pre-commit procedures, it'll prevent defects like this
from being committed in the first place.

>  Anyhow, the simplest fix would be to revert this part of r6000

But of course that's not satisfying.

> Alternatively, the following patch
> could be applied:
[...]
> +    int num_views = 0;
> +    for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); ++i)
> +        {
> +        if(dynamic_cast<wxDocMDIChildFrame*>(*i))
> +            {
> +            num_views++;
> +            }
> +        }

That works, but I want to feel like we've made a bigger improvement
in return for our pain--so let's attack the triple TODO marker right
above this function.

Please take a look at what I committed 20141028T2246Z (revision 6006).
Now the progress meter counts only the IllustrationView windows. That
takes care of one TODO. As for the others:

// TODO ?? CALCULATION_SUMMARY Instead, why not just update the
// topmost window first, then update other windows, putting some
// progress indication on the statusbar?

Here I thought it would be good to start with a measurement. I created
ten IllustrationView windows (I doubt an end user would ever want more),
then did "File | Preferences", toggled its checkbox, and hit OK. Even
though I did this numerous times, I never was able to see the progress
dialog clearly enough to read its estimate of the time I'd have to wait
for it to finish. Conclusions: the progress dialog is overkill; and
it's not worth the effort to update any topmost IllustrationView before
all the others.

The patch below replaces the progress dialog with wxBusyCursor [0], and
it passes the automated GUI test. I think I like this, but I don't want
to throw away the "statusbar" idea above too lightly. This looks like
what I was thinking of:

http://www.nntp.perl.org/group/perl.wxperl.users/2011/02/msg7861.html
| | I have a status bar [...] and I want to add a Wx::Gauge progress bar
| There is a (C++) example in the wxWidgets sources; IIRC, you need to
| create the gauge as a child of the status bar and in the OnSize handler,
| set its size/position to the size of
|      $statusbar->GetFieldRect( 1 );

Does that seem like a good idea to you? E.g., if you have a burning desire to
add a progress-gauge-in-statusbar class to wx, or it's a common way of doing
things in some influential new OS, then I don't want to discard it too quickly.

// TODO ?? CALCULATION_SUMMARY It would probably be in much better
// taste to use wxView::OnUpdate() for this purpose.

Searching the web, I conclude that this isn't the most widely used wx function.
Yet it does exist. Is it backed up by some thoughtful implementation inside wx
that we ought to be using, or is it just a bare hook? Well, wait...I think I
can figure that out quickly:

/opt/lmi/wx-scratch/wxWidgets-3.1.0/src[0]$grep -w OnUpdate **/* |less -S

There's almost nothing there. There is wxDocument::UpdateAllViews(), but that
just calls OnUpdate() for all views of a single document. That would be the
motivating purpose in MDI as it was originally conceived, AIUI. But here we
want to update all views belonging to a particular class, and they all have
distinct documents.

Do you see any merit in using OnUpdate() here? I guess we could write it in
'illustration_view.?pp', enabling us to simplify 'skeleton.cpp' thus:
-  v->DisplaySelectedValuesAsHtml();
+  v->OnUpdate(NULL);
but that doesn't seem worthwhile. I'm just trying to make sure we've sucked all
the juice out of that TODO comment before purging it.

---------

[0] "The patch below replaces the progress dialog with wxBusyCursor."


Index: skeleton.cpp
===================================================================
--- skeleton.cpp        (revision 6006)
+++ skeleton.cpp        (working copy)
@@ -73,7 +73,6 @@
 #include "policy_view.hpp"
 #include "preferences_model.hpp"
 #include "preferences_view.hpp"
-#include "progress_meter.hpp"
 #include "rounding_document.hpp"
 #include "rounding_view.hpp"
 #include "rounding_view_editor.hpp"     // RoundingButtonsXmlHandler
@@ -98,7 +97,7 @@
 #include <wx/textctrl.h>
 #include <wx/textdlg.h>                 // wxGetTextFromUser()
 #include <wx/toolbar.h>
-#include <wx/utils.h>                   // wxMilliSleep(), wxSafeYield()
+#include <wx/utils.h>                   // wxMilliSleep(), wxSafeYield(), 
wxBusyCursor
 #include <wx/xrc/xmlres.h>

 #include <iterator>
@@ -105,7 +104,6 @@
 #include <sstream>
 #include <stdexcept>
 #include <string>
-#include <vector>

 #if defined __WXGTK__
 #   include <gtk/gtk.h>
@@ -1313,14 +1311,10 @@

 // TODO ?? CALCULATION_SUMMARY It would probably be in much better
 // taste to use wxView::OnUpdate() for this purpose.
-//
-// TODO ?? CALCULATION_SUMMARY Instead, why not just update the
-// topmost window first, then update other windows, putting some
-// progress indication on the statusbar?

 void Skeleton::UpdateViews()
 {
-    std::vector<IllustrationView*> ivv;
+    wxBusyCursor wait;
     wxWindowList const& wl = frame_->GetChildren();
     for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); ++i)
         {
@@ -1330,26 +1324,9 @@
             IllustrationView* v = 
dynamic_cast<IllustrationView*>(c->GetView());
             if(v)
                 {
-                ivv.push_back(v);
+                v->DisplaySelectedValuesAsHtml();
                 }
             }
         }
-
-    boost::shared_ptr<progress_meter> meter
-        (create_progress_meter
-            (ivv.size()
-            ,"Updating calculation summaries"
-            )
-        );
-    typedef std::vector<IllustrationView*>::const_iterator vvci;
-    for(vvci i = ivv.begin(); i != ivv.end(); ++i)
-        {
-        (*i)->DisplaySelectedValuesAsHtml();
-        if(!meter->reflect_progress())
-            {
-            break;
-            }
-        }
-    meter->culminate();
 }