Re: [lmi] [lmi-commits] master 5bb0b55 2/3: Update statusbar smoothly (VZ)

[Greg Chicares]

On 2018-06-26 22:40, Vadim Zeitlin wrote:
> On Tue, 26 Jun 2018 18:36:54 -0400 (EDT) Greg Chicares <address@hidden> wrote:
[...]
> GC> --- a/alert_wx.cpp
> GC> +++ b/alert_wx.cpp
> GC> @@ -59,9 +59,14 @@ void status_alert(std::string const& s)
> GC>      if(wxTheApp && wxTheApp->GetTopWindow())
> GC>          {
> GC>          wxFrame* f = dynamic_cast<wxFrame*>(wxTheApp->GetTopWindow());
> GC> -        if(f && f->GetStatusBar())
> GC> +        if(f)
> GC>              {
> GC> -            f->SetStatusText(s);
> GC> +            wxStatusBar* b = f->GetStatusBar();
> GC> +            if(b)
> GC> +                {
> GC> +                f->SetStatusText(s);
> GC> +                f->GetStatusBar()->Update();
> GC> +                }
> GC>              }
> GC>          }
> GC>  }
> 
>  This is very minor, of course, but why call Update() on f->GetStatusBar()
> instead of calling it on "b" that we already have?

That's what I had intended to do. Thanks for pointing this out. I would
normally have put this group of changes aside and reviewed them afresh
the next day, but instead pushed them hastily in my eagerness to post a
message explaining the reasoning that led to them.

> And, if we use "b" on
> that line, why not use it on the one above too, for consistency?

I thought that odd, but didn't take time to look into it. Now I see that
class wxStatusBar also provides a SetStatusText() function, so I'll use
it directly.

BTW, the documentation says that calling wxFrame::SetStatusText() causes
the statusbar to be redrawn:

  
http://docs.wxwidgets.org/3.1/classwx_frame.html#a0026c883df35e1d1f8818e229c41249f
| Sets the status bar text and redraws the status bar.

whereas the documentation for wxStatusBar::SetStatusText
  
http://docs.wxwidgets.org/3.1/classwx_status_bar.html#a1f0fd75cce7f3f8c89c0b2f8b9b88dd1
makes no such claim about redrawing the status bar.

This seems to imply that if we use the wxFrame function, it's unnecessary
to call GetStatusBar()->Update(), but that is not the case: the implementation
in 'common/framecmn.cpp' just forwards to wxStatusBar (if a statusbar exists).

>  FWIW I'd also put this variable in the scope of the if statement itself,
> even if, again, it hardly matters here as the enclosing scope ends just
> after it anyhow -- but in general I think it's better hygiene to make the
> scope as small as possible

Agreed.

> i.e. my version would be
> 
>       if(auto b = f->GetStatusBar())
>           {
>           b->SetStatusText(s);
>           b->Update();
>           }
> 
>  What do you think?

I'm committing something a little different. I don't really like 'auto'
here: I can reason that a function named GetStatusBar() ought to return
something of a statusbar type, but it's not entirely obvious whether it
would be a reference or a pointer, and 'auto' obscures that. I'm allergic
to pointers, so I want to know when I'm using one.

I really wanted to write
  if(f && wxStatusBar* b = f->GetStatusBar())
but the language doesn't allow that: we can write a simple-declaration
inside if(), but not after &&. Similarly, we can't write one after ',':

    if
        (wxTheApp   // Fails on next line:
        ,wxFrame* f = wxTheApp && 
dynamic_cast<wxFrame*>(wxTheApp->GetTopWindow())
        ,wxStatusBar* b = f && f->GetStatusBar()
        )

Such ideas occur to me because I was recently looking at this elsewhere:

    bool const b =
                                  wxTheApp
        && dynamic_cast<wxFrame*>(wxTheApp->GetTopWindow())
        && dynamic_cast<wxFrame*>(wxTheApp->GetTopWindow())->GetStatusBar()
                                                           ^^ warning here
        ;

which '-Wnull-dereference' flagged for no legitimate reason I can see.
There really ought to be a clear way of expressing that, so that we
could evaluate a dereference chain like
  p0->p1->p2->p3
and return p3 if valid, else null_ptr if any of the pointers is null.