Re: [lmi] wx_test regression

[Vadim Zeitlin]

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(). And this is how the change I described as being
"obviously correct" turned out to be completely wrong :-(

 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.

 Anyhow, the simplest fix would be to revert this part of r6000 (the first
change in it is still correct AFAICS). Alternatively, the following patch
could be applied:

----------------------------------->8---------------------------------------
commit 3574456d0ae4f0ea039f6e01ff33304ccad582ee
Author: Vadim Zeitlin <address@hidden>
Date:   Tue Oct 28 14:40:13 2014 +0100

    Fix progress meter updating in UpdateViews().
    
    Ensure that the progress meter is set to exactly the number of views and not
    the total number of frame children, which is not really relevant and, worse,
    can change as the progress meter itself is created.

diff --git a/skeleton.cpp b/skeleton.cpp
index dc74af0..7a15357 100644
--- a/skeleton.cpp
+++ b/skeleton.cpp
@@ -1324,9 +1324,19 @@ bool Skeleton::ProcessCommandLine(int argc, char* argv[])
 void Skeleton::UpdateViews()
 {
     wxWindowList const& wl = frame_->GetChildren();
+
+    int num_views = 0;
+    for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); ++i)
+        {
+        if(dynamic_cast<wxDocMDIChildFrame*>(*i))
+            {
+            num_views++;
+            }
+        }
+
     boost::shared_ptr<progress_meter> meter
         (create_progress_meter
-            (wl.size()
+            (num_views
             ,"Updating calculation summaries"
             )
         );
@@ -1340,10 +1350,10 @@ void Skeleton::UpdateViews()
                 {
                 v->DisplaySelectedValuesAsHtml();
                 }
-            }
-        if(!meter->reflect_progress())
-            {
-            break;
+            if(!meter->reflect_progress())
+                {
+                break;
+                }
             }
         }
     meter->culminate();
----------------------------------->8---------------------------------------

 I've tested that it also fixed the problem and it's arguably more correct
as we don't care about the non-MDI children and skip them immediately
anyhow, so they shouldn't count for progress meter purposes.

 Sorry once again for the bug,
VZ