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

[Greg Chicares]

[At last I have some time to attack the last few months' backlog...]

On 2009-10-20 15:46Z, Vadim Zeitlin wrote:
> 
>  AboutDialog::ShowModal() in about_dialog.cpp creates 2 sizes using this
> ctor: new wxFlexGridSizer(0, 1). This doesn't compile any more with the
> latest wx svn trunk and for a good reason: usually such 2 argument ctor is
> used by mistake and it seems that this was indeed the case here.

Yes, it does seem wrong.

And here's something else that's wrong:
  Help | About
  click "Read the GNU General Public License"
The license text doesn't scroll in the html window. There was a scrollbar
there, at least on a 20060313 build that I have at hand. Now, however,
there's no scrollbar, and the scrolling keys do nothing: only the preamble
can be read.

So something in AboutDialog::UponReadLicense() is now broken. Scrolling
did work in a 20080622 version, which used wx-2.8, and the changes in
'about_dialog.cpp' since then seem unremarkable...so perhaps there's some
defective code there that accidentally worked until wx-2.9 . It looks like
autolayout gives all available space to the html window; the pushbutton at
the bottom doesn't even appear (at least at 800x600 resolution in msw).
Could you try to fix this, please?

>  In previous wx version (until 2.9.0) using wxFlexGridSizer ctor with 2
> arguments used wxFlexGridSizer(int cols, int vgap = 0, int hgap = 0)
> overload. I.e. what this call did was creating a sizer with automatically
> determined number of columns, vertical gap between rows of 1 pixel and
> horizontal gap of 0 pixels.

That's not what I intended to do, of course. BTW, the documentation:
  http://docs.wxwidgets.org/trunk/classwx_flex_grid_sizer.html
isn't as clear as it might be--for instance:

  wxFlexGridSizer::wxFlexGridSizer
    (int           cols,
     const wxSize & gap = wxSize(0, 0)  
    )
  [...] If the number of rows is explicitly specified [...]

Apparently the same description is used for every ctor, but this ctor
offers no way to specify the number of rows.

>  However LMI code proceeds by calling AddGrowableCol(0) and
> AddGrowableCol(1) which seems to indicate that the intention was to create
> a sizer with 2 columns and one row. I.e. the constructor should be really
> replaced with wxFlexGridSizer(/* cols = */2).
> 
>  Similarly, the call to new wxFlexGridSizer(1, 0) below was probably meant
> to create a one column sizer and so should be replaced with
> wxFlexGridSizer(1). This would work with both 2.8, 2.9.0 and latest svn and
> do the right thing so here is the trivial patch which I suggest to apply:

I committed a change in that spirit on 20100111T1731Z. I used the
  wxFlexGridSizer(int, int, int, int)
ctor (which is backward compatible at least to wx-2.5.4) because it
seems more robust: rather than specifying two columns in the first
example, which will be wrong if we ever add a third button, I prefer
to specify that there's exactly one row. IOW, I'm using arguments of
1 and 0 only, to indicate horizontal or vertical orientation, kind of
like (part of) your suggestion following:

>  FWIW I, as usual, wouldn't use wxFlexGridSizer here at all: a simple
> combination of wxBoxSizer(wxHORIZONTAL) for the buttons and
> wxBoxSizer(wxVERTICAL) containing the HTML window and the buttons sizer
> would be simpler to understand IMO. I can, of course, make another patch if
> you agree.

Yes, please--for simplicity as well as uniformity (there's already a
wxBoxSizer in AboutDialog::UponReadLicense()).