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

[Vadim Zeitlin]

On Thu, 14 Jan 2010 17:33:40 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2010-01-13 21:51Z, Vadim Zeitlin wrote:
GC> > On Wed, 13 Jan 2010 13:45:44 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> The last line I can see is:
GC> > GC> | proprietary programs.  If your program is a subroutine library, you 
may
GC> > GC> and I can't see the "Close" button.
GC> 
GC> On 20100114T1631Z I made a set of changes that solves that problem AFAICT,
GC> and improves the layout of these 'about' dialogs IMO. See if you like it.

 Thanks! Looking at the diff with my version, the main difference between
my and your version is the removal of this comment from my patch:

@@ -69,13 +70,6 @@ int AboutDialog::ShowModal()
         );
     html_window->SetBorders(0);
     html_window->SetPage(license_notices_as_html());
-
-    // Size the window to exactly fit its contents. We assume here that this is
-    // reasonable, in particular that the HTML is short enough to fit entirely
-    // on the screen. If this is not the case, we'd break the dialog layout by
-    // requesting that one of its children be allocated more size than is
-    // available to the entire dialog itself, so if this ever becomes a concern
-    // we should explicitly compare the size of this window with display size.
     int width =
             html_window->GetInternalRepresentation()->GetWidth()
         +   wxSystemSettings::GetMetric(wxSYS_VSCROLL_X)


IMO it's not exactly obvious so I think the comment could be useful. I
don't know if you disagree with this entirely or just left it out
unintentionally.


 Also, there still remains unneeded, IMO, use of wxFlexGridSizer where a
simple wxBoxSizer would have been enough. So what do you think of this
patch [fragment]:

--- a/about_dialog.cpp
+++ b/about_dialog.cpp
@@ -83,33 +83,36 @@ int AboutDialog::ShowModal()
         ,"Read the GNU General Public License"
         );
     license_button->SetDefault();
+    license_button->SetFocus();
     wxButton* cancel_button = new(wx) wxButton
         (this
         ,wxID_CANCEL
         ,"Let me illustrate"
         );

-    wxFlexGridSizer* sizer1 = new(wx) wxFlexGridSizer(1, 0, 0, 0);
-    sizer1->AddGrowableCol(0);
-    sizer1->AddGrowableCol(1);
-    sizer1->Add(license_button, 1, wxALL|wxALIGN_LEFT , 2);
-    sizer1->Add(cancel_button , 1, wxALL|wxALIGN_RIGHT, 2);
+    wxBoxSizer* sizer_btn = new(wx) wxBoxSizer(wxHORIZONTAL);
+    sizer_btn->Add(license_button, wxALL, 2);
+    sizer_btn->Add(cancel_button, wxALL, 2);

-    wxFlexGridSizer* sizer0 = new(wx) wxFlexGridSizer(0, 1, 0, 0);
-    sizer0->AddGrowableRow(0);
-    sizer0->Add(html_window, 1, wxALL, 2);
-    sizer0->Add(sizer1     , 1, wxALL, 0); // Buttons have their own borders.
+    wxBoxSizer* sizer = new(wx) wxBoxSizer(wxVERTICAL);
+    sizer->Add(html_window, 1, wxALL|wxEXPAND, 2);
+    sizer->Add(sizer_btn);

-    SetAutoLayout(true);
-    SetSizer(sizer0);
-    sizer0->Fit(this);
+    SetSizerAndFit(sizer);
     Center();
     return wxDialog::ShowModal();
 }


Notice that I used SetSizerAndFit() instead of 2 calls as it does exactly
the same thing but it seems more clear to me to call one method of the
window object instead of 2 methods on 2 different objects. I also removed
the unnecessary SetAutoLayout() as this is done by SetSizer() anyhow. I
also added SetFocus() call as otherwise the focus is initially given to the
HTML window which has really no use for it (it doesn't even have scrollbars
so there is no need to scroll it, which might be convenient to do from
keyboard). These changes are unrelated to wxFlexGridSizer removal and
should IMO be done even if you finally prefer to keep wxFlexGridSizer.


 As for the code in UponReadLicese(), I still think that we could just size
the entire dialog to 80% of the screen size instead of making this the size
of just the HTML window. This would make the code shorter and simpler while
still looking just as nice (IMHO nicer actually as currently the dialog is
~90% of the height which is too close to 100% to my liking). E.g. would you
consider this patch:

--- a/about_dialog.cpp
+++ b/about_dialog.cpp
@@ -119,26 +119,20 @@ void AboutDialog::UponReadLicense(wxCommandEvent&)
         );
     html_window->SetBorders(0);
     html_window->SetPage(license_as_html());
-    html_window->GetInternalRepresentation()->Layout(1);
-
-    wxRect r = wxDisplay(wxDisplay::GetFromWindow(this)).GetClientArea();
-    // Using the whole client area would seem unnatural. Pushbuttons
-    // can't plausibly take more than twenty percent of the vertical
-    // space.
-    int width  = r.GetWidth () * 4 / 5;
-    int height = r.GetHeight() * 4 / 5;
-    html_window->SetMinSize(wxSize(width, height));

     wxButton* button = new(wx) wxButton(&dialog, wxID_CANCEL, "Close");
     button->SetDefault();

     wxBoxSizer* sizer = new(wx) wxBoxSizer(wxVERTICAL);
-    sizer->Add(html_window, 1, wxALL              , 2);
+    sizer->Add(html_window, 1, wxALL|wxEXPAND     , 2);
     sizer->Add(button     , 0, wxALL|wxALIGN_RIGHT, 2);

-    dialog.SetAutoLayout(true);
     dialog.SetSizer(sizer);
-    sizer->Fit(&dialog);
+
+    // Arbitrarily (but visually appealingly) size the dialog with a long
+    // license text to cover 80% of the screen size.
+    wxRect r = wxDisplay(wxDisplay::GetFromWindow(this)).GetClientArea();
+    dialog.SetSize(0.8*r.GetSize());
     dialog.Center();
     dialog.ShowModal();
 }

Notice that the calls to GetInternalRepresentation()->Layout() and (as
above) SetAutoLayout() are not needed in any case and could/should be
removed even if you don't change the code otherwise. Also notice that you
must use wxEXPAND for the html_window if you don't call SetMinSize()
explicitly as otherwise it would remain with its default 1 pixel width.

GC> I played with making it resizable, but didn't come up with anything I
GC> really liked. I added the wxRESIZE_BORDER style, but that caused the
GC> "Close" button to overlay part of the resize grip, which seemed wrong.

 This could be easily worked around by adding a small spacer to the end of
the size. Unfortunately I don't think wxWidgets can account for this
automatically. E.g. do this:

+    wxBoxSizer *sizer_btn = new(wx) wxBoxSizer(wxHORIZONTAL);
+    sizer_btn->Add(button);
+    sizer_btn->AddSpacer(10);
+
     wxBoxSizer* sizer = new(wx) wxBoxSizer(wxVERTICAL);
     sizer->Add(html_window, 1, wxALL              , 2);
-    sizer->Add(button     , 0, wxALL|wxALIGN_RIGHT, 2);
+    sizer->Add(sizer_btn  , 0, wxALL|wxALIGN_RIGHT, 2);

We could and probably should use wxSYS_VSCROLL_X size instead of the
hardcoded 10, I'd do it if you agreed to such change.

GC> I also added the wxMAXIMIZE_BOX style, but that gave me a grayed-out
GC> minimize box, which looked confusing; maybe msw just won't let us have
GC> one without the other?

 Yes, exactly.

GC> >  If we wanted to do this, then it would be enough to simply call
GC> > Maximize(), no complicated size calculations needed. OTOH I'm even less
GC> > sure if it's appropriate to open a full screen window in this case. 
Reading
GC> > 1600-pixel wide lines is not nice at all neither
GC> 
GC> Take a look at HEAD and see if you think it can be improved without
GC> too much work.

 This depends on what exactly do we want to do. AFAIK the ideal width of a
line of text from readability point of view is ~35em and it would be
trivial to just make the HTML window of the appropriate [minimal] width
[while still allowing the user to make it wider if he wants to]. And while
I'm pretty sure that nobody is ever reading licences anyhow, I still think
that showing lines of 200 characters (width of first line on my monitor
right now) is somewhat untidy. OTOH using 80% height with ~35em width
results in a very tall and narrow window (at least on my screen) which is
not nice neither.

 So what about a completely different criteria: wouldn't it make sense to
make the licence look closely to its "canonical representation" at
http://www.gnu.org/licenses/gpl.html? I didn't dive into their CSS to see
what exactly do they do but just replacing the end of my initial patch
above with this

-    dialog.SetAutoLayout(true);
     dialog.SetSizer(sizer);
-    sizer->Fit(&dialog);
+
+    wxRect r = wxDisplay(wxDisplay::GetFromWindow(this)).GetClientArea();
+    dialog.SetInitialSize(wxSize(85*dialog.GetCharWidth(), r.height * 4 / 5));

makes it appear reasonably close.

 Please find below the full patch combining all the changes above and
please let me know if you'd like me to change anything else.

 Thanks,
VZ

diff --git a/about_dialog.cpp b/about_dialog.cpp
index 0abfb4e..1cfa316 100644
--- a/about_dialog.cpp
+++ b/about_dialog.cpp
@@ -70,6 +70,13 @@ int AboutDialog::ShowModal()
         );
     html_window->SetBorders(0);
     html_window->SetPage(license_notices_as_html());
+
+    // Size the window to exactly fit its contents. We assume here that this is
+    // reasonable, in particular that the HTML is short enough to fit entirely
+    // on the screen. If this is not the case, we'd break the dialog layout by
+    // requesting that one of its children be allocated more size than is
+    // available to the entire dialog itself, so if this ever becomes a concern
+    // we should explicitly compare the size of this window with display size.
     int width =
             html_window->GetInternalRepresentation()->GetWidth()
         +   wxSystemSettings::GetMetric(wxSYS_VSCROLL_X)
@@ -83,33 +90,36 @@ int AboutDialog::ShowModal()
         ,"Read the GNU General Public License"
         );
     license_button->SetDefault();
+    license_button->SetFocus();
     wxButton* cancel_button = new(wx) wxButton
         (this
         ,wxID_CANCEL
         ,"Let me illustrate"
         );
 
-    wxFlexGridSizer* sizer1 = new(wx) wxFlexGridSizer(1, 0, 0, 0);
-    sizer1->AddGrowableCol(0);
-    sizer1->AddGrowableCol(1);
-    sizer1->Add(license_button, 1, wxALL|wxALIGN_LEFT , 2);
-    sizer1->Add(cancel_button , 1, wxALL|wxALIGN_RIGHT, 2);
+    wxBoxSizer* sizer_btn = new(wx) wxBoxSizer(wxHORIZONTAL);
+    sizer_btn->Add(license_button, wxALL, 2);
+    sizer_btn->Add(cancel_button, wxALL, 2);
 
-    wxFlexGridSizer* sizer0 = new(wx) wxFlexGridSizer(0, 1, 0, 0);
-    sizer0->AddGrowableRow(0);
-    sizer0->Add(html_window, 1, wxALL, 2);
-    sizer0->Add(sizer1     , 1, wxALL, 0); // Buttons have their own borders.
+    wxBoxSizer* sizer = new(wx) wxBoxSizer(wxVERTICAL);
+    sizer->Add(html_window, 1, wxALL|wxEXPAND, 2);
+    sizer->Add(sizer_btn);
 
-    SetAutoLayout(true);
-    SetSizer(sizer0);
-    sizer0->Fit(this);
+    SetSizerAndFit(sizer);
     Center();
     return wxDialog::ShowModal();
 }
 
 void AboutDialog::UponReadLicense(wxCommandEvent&)
 {
-    wxDialog dialog(this, wxID_ANY, std::string("GNU General Public License"));
+    wxDialog dialog
+        (this
+        ,wxID_ANY
+        ,std::string("GNU General Public License")
+        ,wxDefaultPosition
+        ,wxDefaultSize
+        ,wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER
+        );
     wxHtmlWindow* html_window = new(wx) wxHtmlWindow
         (&dialog
         ,wxID_ANY
@@ -119,26 +129,29 @@ void AboutDialog::UponReadLicense(wxCommandEvent&)
         );
     html_window->SetBorders(0);
     html_window->SetPage(license_as_html());
-    html_window->GetInternalRepresentation()->Layout(1);
-
-    wxRect r = wxDisplay(wxDisplay::GetFromWindow(this)).GetClientArea();
-    // Using the whole client area would seem unnatural. Pushbuttons
-    // can't plausibly take more than twenty percent of the vertical
-    // space.
-    int width  = r.GetWidth () * 4 / 5;
-    int height = r.GetHeight() * 4 / 5;
-    html_window->SetMinSize(wxSize(width, height));
 
     wxButton* button = new(wx) wxButton(&dialog, wxID_CANCEL, "Close");
     button->SetDefault();
 
+    wxBoxSizer *sizer_btn = new(wx) wxBoxSizer(wxHORIZONTAL);
+    sizer_btn->Add(button);
+    sizer_btn->AddSpacer(10);
+
     wxBoxSizer* sizer = new(wx) wxBoxSizer(wxVERTICAL);
-    sizer->Add(html_window, 1, wxALL              , 2);
-    sizer->Add(button     , 0, wxALL|wxALIGN_RIGHT, 2);
+    sizer->Add(html_window, 1, wxALL|wxEXPAND     , 2);
+    sizer->Add(sizer_btn  , 0, wxALL|wxALIGN_RIGHT, 2);
 
-    dialog.SetAutoLayout(true);
     dialog.SetSizer(sizer);
-    sizer->Fit(&dialog);
+
+    // The choice of the dialog size is rather arbitrary: we use 80% of the
+    // available display size vertically because the license text is rather
+    // long and so we want to show as much of it as possible while not quite
+    // maximizing the window (obscuring everything else the user has on his
+    // desktop would be impolite) and the choice of the width is motivated by
+    // the desire to wrap the license text similarly to how it appears at
+    // http://www.gnu.org/licenses/gpl.html
+    wxRect r = wxDisplay(wxDisplay::GetFromWindow(this)).GetClientArea();
+    dialog.SetInitialSize(wxSize(85*dialog.GetCharWidth(), r.height * 4 / 5));
     dialog.Center();
     dialog.ShowModal();
 }