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

[Vadim Zeitlin]

[this reply is just about the missing scrollbars, I'll address the other
 points in another message]

On Mon, 11 Jan 2010 17:32:13 +0000 Greg Chicares <address@hidden> wrote:

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

 Yes, and I'll provide a patch fixing it at the end of the message. If
you'd like to know what exactly was wrong, please continue reading. If not,
please just skip to the end.

 There were several changes to the determination of the best window size in
2.9. In 2.8, whenever you called SetSize() (including doing it implicitly
at window creation by passing a non-default size to the ctor), it became
the window min size as well. This resulted in weird bugs: you could have a
normally laid out window initially, resize it making expand one of its
children and now shrinking the window back to its original size not only
wouldn't restore the original layout but could result in broken layout with
some windows not being given enough space to show themselves correctly (or
at all) because this one children didn't give back the extra size it
received because its min size now became much bigger than initially.

 So we don't set the size passed to SetSize() as min size any more. We
still do set the size passed to the ctor initially as the min size as this
is the expected behaviour and often does make sense. And I'm also wondering
if we shouldn't handle the first SetSize() call specially so that

        wxWindow *w = new wxWindow(..., wxSize(100, 100));

and

        wxWindow *w = new wxWindow(..., wxDefaultSize);
        w->SetSize(100, 100);
        
behave the same (right now they don't as the first version sets the min
size of the window to 100*100 and the second one doesn't), see this thread:
http://thread.gmane.org/gmane.comp.lib.wxwidgets.devel/118541

 To summarize, the min size of the wxHtmlWindows created in
about_dialog.cpp is not set at all any more with current wx 2.9. So if I
build LMI without any changes I simply don't see any HTML windows at all --
as they have no min size, they have 1*1 size i.e. are invisible. To correct
this we need to either create these windows with some initial size or call
SetMinSize() instead of SetSize(). The latter solution is preferable if you
want the window to exactly fit the HTML contents and it does work nicely
with the main about dialog.

 However it doesn't work with the full licence dialog because the minimal
height of its HTML cell is ~5000 pixels. This is more than the vertical
size of my screen and as all top level windows are constrained to fit on
the display by wx, calling SetMinSize(width, 5000) results in a dialog
spanning the entire display vertically and containing a 5000 pixel high
wxHtmlWindow inside it. This window still doesn't have scrollbars (it
doesn't need them because it's tall enough!) but, of course, all but its
first ~1000 pixels are hidden. And the button at the bottom of the dialog
is hidden as well. And this problem is also present in the version you
used, which predates SetSize/SetMinSize() changes.


 So the final solution is to use SetMinSize() instead of SetSize() as this
works with any wx version and also not use HTML size to size this window
but just make it reasonably big. This assumes that the licence text is long
but I think it's a safe assumption to make. If not, we could also
explicitly compare GetInternalRepresentation() result with
wxDisplay::GetClientArea() but IMO it's not worth complicating the code to
do it.


GC> Could you try to fix this, please?

 Here is my attempt, with the summary of the explanation above as commit
message:

commit afffae5185e131eed2b01c1067cbf6481b7009dc
Author: Vadim Zeitlin <address@hidden>
Date:   Tue Jan 12 17:50:35 2010 +0100

    Use wxHtmlWindow::SetMinSize() instead of SetSize() to set minimal size.

    The initial call to SetSize() used to set the size passed to it as the 
window
    minimal size as well in the previous wxWidgets versions, however it doesn't 
do
    it any longer for the latest wxWidgets 2.9.x.

    Use SetMinSize() explicitly to set the minimal acceptable size of the window
    instead, this works with all wxWidgets versions and is more clear.

    Also, don't use the total height of the HTML representation of the license
    text as the minimal size of the window as it will be almost certainly bigger
    than the display size and result in broken dialog layout due to 
impossibility
    of accommodating the requested minimal size.

diff --git a/about_dialog.cpp b/about_dialog.cpp
index 4272ab9..6c54c93 100644
--- a/about_dialog.cpp
+++ b/about_dialog.cpp
@@ -69,12 +69,19 @@ 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)
         ;
     int height = html_window->GetInternalRepresentation()->GetHeight();
-    html_window->SetSize(width, height);
+    html_window->SetMinSize(wxSize(width, height));

     wxButton* license_button = new(wx) wxButton
         (this
@@ -118,13 +125,20 @@ void AboutDialog::UponReadLicense(wxCommandEvent&)
         );
     html_window->SetBorders(0);
     html_window->SetPage(license_as_html());
+
     html_window->GetInternalRepresentation()->Layout(1);
     int width =
             html_window->GetInternalRepresentation()->GetWidth()
         +   wxSystemSettings::GetMetric(wxSYS_VSCROLL_X)
         ;
-    int height = html_window->GetInternalRepresentation()->GetHeight();
-    html_window->SetSize(width, height);
+
+    // Unlike in ShowModal() above, we don't want to size this window to fit
+    // its contents vertically as the license text is so long that it will
+    // almost surely not fit the display. Hence just (arbitrarily) make it
+    // slightly larger than the main dialog so that this one appears to be on
+    // top of it.
+    int height = (3*GetSize().y)/2;
+    html_window->SetMinSize(wxSize(width, height));

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


 Please let me know if you have any problems with this patch, thanks,
VZ