Re[2]: [lmi] Wrong wxFlexGridSizer usage in about_dialog.cpp

[Vadim Zeitlin]

On Fri, 15 Jan 2010 15:32:55 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2010-01-14 22:37Z, Vadim Zeitlin wrote:
GC> > 
GC> >      wxButton* cancel_button = new(wx) wxButton
GC> >          (this
GC> >          ,wxID_CANCEL
GC> >          ,"Let me illustrate"
GC> >          );
GC> >  
GC> > -    wxFlexGridSizer* sizer1 = new(wx) wxFlexGridSizer(1, 0, 0, 0);
GC> > -    sizer1->AddGrowableCol(0);
GC> > -    sizer1->AddGrowableCol(1);
GC> > -    sizer1->Add(license_button, 1, wxALL|wxALIGN_LEFT , 2);
GC> > -    sizer1->Add(cancel_button , 1, wxALL|wxALIGN_RIGHT, 2);
GC> > +    wxBoxSizer* sizer_btn = new(wx) wxBoxSizer(wxHORIZONTAL);
GC> > +    sizer_btn->Add(license_button, wxALL, 2);
GC> > +    sizer_btn->Add(cancel_button, wxALL, 2);
GC> 
GC> Which override of Add() is being called on the last line?
GC> Wouldn't it be the following:
GC> 
GC>     wxSizerItem* Add(wxWindow *window,
GC>                      int proportion = 0,
GC>                      int flag = 0,
GC>                      int border = 0,
GC>                      wxObject* userData = NULL);

 I'm sorry, it's indeed this one and this is, of course, completely wrong.
These lines should have been written as

        sizer_btn->Add(license_button, 0, wxALL, 2);
        sizer_btn->Add(cancel_button,  0, wxALL, 2);

The fact that compiler doesn't catch this kind of bugs is one of the
reasons (the other one being that I also find it more clear to read) that I
introduced wxSizerFlags class in wx API several years ago and practically
never use other overloads of wxSizer::Add() any more.

 With wxSizerFlags, you'd write

        sizer_btn->Add(license_button, wxSizerFlags().Border());
        sizer_btn->Add(cancel_button, wxSizerFlags().Border());

(well, this would use border of standard size instead of 2 pixels but you
could use Border(wxALL, 2) as well, of course, it's just that I think that
using borders of consistent sizes everywhere is better anyhow). I didn't
want to mention wxSizerFlags in this thread as we were already discussing
several different topics in it but the above bug gave me a very good excuse
to mention it (I won't pretend that I introduced the bug on purpose though).

 To repeat, I think that using wxSizerFlags in new code is better because
it's easier to write (IMHO), to read (IMNSHO) and much more type-safe
(definitely).

 In the meanwhile, sorry once again for the original bug,
VZ