Re: [lmi] wx_test regression

[Vadim Zeitlin]

On Wed, 29 Oct 2014 00:08:49 +0000 Greg Chicares <address@hidden> wrote:

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

 Only tangentially related, but one thing I wanted to do was to use gcov
with the test to see how much of the existing code does it cover (my
suspicion is that the answer is "not much") and possibly try to extend them
to cover more of it.

GC> Please take a look at what I committed 20141028T2246Z (revision 6006).

 I had actually already done it when I saw the svn commit message and I
agree that this is even better than my patch, thanks!

GC> Now the progress meter counts only the IllustrationView windows. That
GC> takes care of one TODO. As for the others:
GC> 
GC> // TODO ?? CALCULATION_SUMMARY Instead, why not just update the
GC> // topmost window first, then update other windows, putting some
GC> // progress indication on the statusbar?

 I had seen this comment as well but, to be honest, I don't really
understand it, mostly because I don't see what is meant by the "topmost
window" here. Illustration views don't have to be stacked with exactly one
of them being on top, so there isn't necessarily a single such window. If
anything, I'd display progress in the status bar of the parent frame, which
is the bottom most one in Z-order.

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

 FWIW the usual solution to this kind of problem, i.e. whether or not the
progress dialog is useful or just an unnecessary distraction, is to wait
for either some small fixed amount of time (ideal) or some small part of
the work being done (simpler) and check if the total time is going to be
long enough, e.g. more than 3 seconds say, to justify showing the dialog.

 But showing the progress non-intrusively is even better, of course,
especially if the process can't, or is unlikely to, be cancelled.

GC> The patch below replaces the progress dialog with wxBusyCursor [0],

 Just a gratuitous plug: you could also use, with about the same amount of
effort, wxBusyInfo which can show more information about what's going on,
especially with the latest wx, see
http://wxwidgets.blogspot.com/2014/10/being-busy-without-being-ugly.html

GC> and it passes the automated GUI test. I think I like this, but I don't
GC> want to throw away the "statusbar" idea above too lightly. This looks
GC> like what I was thinking of:
GC> 
GC> http://www.nntp.perl.org/group/perl.wxperl.users/2011/02/msg7861.html
GC> | | I have a status bar [...] and I want to add a Wx::Gauge progress bar
GC> | There is a (C++) example in the wxWidgets sources; IIRC, you need to
GC> | create the gauge as a child of the status bar and in the OnSize handler,
GC> | set its size/position to the size of
GC> |      $statusbar->GetFieldRect( 1 );
GC> 
GC> Does that seem like a good idea to you? E.g., if you have a burning desire 
to
GC> add a progress-gauge-in-statusbar class to wx, or it's a common way of doing
GC> things in some influential new OS, then I don't want to discard it too 
quickly.

 This is unlikely to be a common way of doing things in anything new
because in modern UIs status bars themselves, let along progress bars in
them, are not very common any longer. Personally I like to use a thin gauge
at the top (i.e. just under the toolbar, if any, or the menu bar) or the
bottom of the window, but I don't think this is really standard (except,
perhaps, under Android where such progress gauges at the top are pretty
common).

 But LMI UI is "classic" rather than "modern" anyhow, and it does have a
status bar. So I think it could indeed make sense to reuse it for showing a
wxGauge inside it, a it's pretty empty currently anyhow. It should be
possible to do it entirely within LMI without problems and I think it would
be preferable to adding a progress field to wxStatusBar because I have no
idea if this can be implemented under OS X (which LMI doesn't target and so
doesn't have to care about, but wxStatusBar does) and even GTK+ 3 doesn't
allow doing this any more (but it works fine in GTK+ 2).

 If you'd like me to do this, do you want to use a specific progress
indicator in this particular case or should we change progress meter
implementation to always behave like this?



GC> // TODO ?? CALCULATION_SUMMARY It would probably be in much better
GC> // taste to use wxView::OnUpdate() for this purpose.
...
GC> Do you see any merit in using OnUpdate() here? I guess we could write it in
GC> 'illustration_view.?pp', enabling us to simplify 'skeleton.cpp' thus:
GC> -  v->DisplaySelectedValuesAsHtml();
GC> +  v->OnUpdate(NULL);
GC> but that doesn't seem worthwhile.

 I don't know much about wxView::OnUpdate(), it predates the beginning of
my involvement with wx and hasn't been changed since then. The only
potentially helpful way to use it I can imagine is like this:

        // Just a unique class.
        struct UpdateAsHTML : wxObject {};

        void Skeleton::UpdateViews()
        {
                UpdateViews updateAsHTML;
                UpdateAllViews(NULL, &updateAsHTML);
        }

        void IllustrationView::OnUpdate(wxView*, wxObject* what)
        {
                if(dynamic_cast<UpdateAsHTML*>(what))
                        {
                        DisplaySelectedValuesAsHtml();
                        }
        }

This is slightly better because it avoids dynamic_cast in UpdateViews()
(the one in OnUpdate() is not as bad because it's done on a class which is
explicitly used just for this), but I'm not sure if it's worth the extra
complexity. And, of course, you lose the possibility to display the
progress if you wanted it with this approach.

 In short, I don't think OnUpdate() is really worth using here (or anywhere
else).

 Regards,
VZ