Re: [lmi] [PATCH] Skin appearance improvements

[Vadim Zeitlin]

On Fri, 1 Jul 2016 16:28:54 +0000 Greg Chicares <address@hidden> wrote:

GC> Here's an odd thing I notice, either with or without this patch.
GC> When I first open the tabbed dialog, the "Policy" listbox is
GC> narrow: perhaps thirty pixels wide. If I change the dialog's size
GC> in either dimension, the width grows to perhaps fifty pixels; it
GC> retains that width if I resize the dialog again.

 I don't see quite the same behaviour: when I create a new illustration
using the single premium skin, the listbox is initially _wider_ than it
becomes after resizing (it's ~140px wide initially and ~80px wide later).
This definitely looks wrong, so I spent some time on debugging this, but
only to conclude that it looks like things work correctly here. The
behaviour I observe is due to the fact that the initial listbox width is
arbitrarily set to 100px + some extra margins, see

https://github.com/wxWidgets/wxWidgets/blob/v3.1.0/src/msw/listbox.cpp#L619

However when the listbox is filled later, its width is computed using the
widths of the actual items which are added to it by then and it turns out
that it's much less than 100. So when the listbox is asked for its best
size later, it returns a smaller amount than it returned initially.

 The only problem I see here is that, arguably, wxListBox shouldn't claim
less size than it claimed initially when items are added to it. OTOH it
seems wrong to reserve 100px of width even for a listbox containing very
short strings. So I think the only reasonable solution is to reduce this
100px and I'll do this in wxWidgets itself. With this change the listbox
here can slightly grow in width when the dialog is resized after creation,
but this is IMO less surprising and thus preferable.

 To really fix this problem, I think something needs to be done at lmi
level. First idea is to apply a very simple fix at XRC level: just change
the size of the listbox to use something like

        <size>50,-1d</size>

to make the listbox sufficiently wide (50 dialog units is ~12 characters).
This works and I honestly don't see many drawbacks to doing this, but is
not particularly elegant.

 Unfortunately the only solution not requiring hard-coding anything that I
see is to perform the panel relayout after the listbox is initially filled
in the MvcController class and I'm not exactly sure how to do this for the
listboxes only and we definitely don't want to do it for all control
changes as this would make it much too sluggish. Please let me know if the
XRC hack above looks acceptable to you, if not I can try to propose
something else.

GC> The XRC file sets the width at fifty:
GC>   <size>-1,50</size>

 Sorry, this is a red herring, 50 is the height here and is ignored anyhow
as the listbox is expanded vertically now. This entire line could be
removed, unless we want to fix the minimal width as indicated above.

GC> so apparently that <size> element isn't honored until the dialog
GC> is resized. Merely moving the dialog doesn't affect the width.

 This is not surprising as only resizing, not moving, the dialog results in
resizing of its children.

GC> I see the same phenomenon with most other skins, but not with
GC> 'skin.xrc'

 This is a (un?)happy coincidence: in skin.xrc, the listbox is as wide as
the widest of the labels below it and there is one ("Extra separate-account
asset charge") which is wider than the initial empty listbox size, so it's
never going to be able to shrink below this size. If you make this label
very short, you can see the same problem in this skin as well.


 To summarize, I now explain everything you say except for the initial
size: for me it's bigger than necessary, not smaller. I have really no idea
what could explain the difference, I do use a slightly different wxWidgets
version but the code in wxListBox dates from 1999, so it must not be due to
this...


GC> >  The second one is much less clear as the borders are indeed inconsistent
GC> > but I don't know which one is correct and which one is wrong. After
GC> > examining this skin file, I think the intention was to use 4 pixels for 
the
GC> > outer border and 2 pixels for all internal borders, so I've adjusted all
GC> > borders to be consistent with this. As you can see
GC> > 
(https://github.com/vadz/lmi/pull/44/commits/d9355cebf5d655a56063834903d36561be3f1576),
GC> > this has resulted in quite a number of changes -- but less than there 
would
GC> > have been if I had done the converse.
GC> 
GC> Much better. I made a further one-character adjustment in
GC> commit ff0d8eeef08cb3433a73e464403758d684a72a20 (q.v.).

 FWIW I fully agree that borders should be used instead of spaces in the
labels.


 Thanks for applying these patches!
VZ