Re: [lmi] Group-quote PDF: whitespace changes, and enhancement

[Vadim Zeitlin]

On Fri, 9 Feb 2018 13:08:25 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-02-09 02:25, Vadim Zeitlin wrote:
[...]
GC> >  I'll make a properly tested (including compiling it with MinGW, which I
GC> > didn't do yet) PR tomorrow, but I can already say that this trivial 
change:
GC> 
GC> Works for me.

 Here is the PR, tested with MinGW, with the changes discussed below:

                https://github.com/vadz/lmi/pull/65

As I mentioned in the last commit message, I think it might be worth to
improve it a bit further by adding an ellipsis to the end of the string if
it doesn't fit. This would require slightly more work, but still wouldn't
be really difficult, and I think it could make things more clear in case
the string is indeed truncated. I also have to say that the truncation was
clearer until I changed the code to subtract column_margin_ from the
clipping rectangle width, but it also was very ugly, so I decided to still
keep the margin, even if it not doing it would allow us to show an extra
letter.


GC> > ---------------------------------- >8 
--------------------------------------
GC> > diff --git a/wx_table_generator.cpp b/wx_table_generator.cpp
GC> > index 39715bca6..942fc369c 100644
GC> > --- a/wx_table_generator.cpp
GC> > +++ b/wx_table_generator.cpp
GC> > @@ -306,6 +306,14 @@ void 
wx_table_generator::do_compute_column_widths_if_necessary()
GC> >                      }
GC> >              }
GC> 
GC> That '}'       ^ wants to be indented four spaces. I hesitate to change
GC> code you're in the process of changing, so would you mind doing it as
GC> part of your forthcoming PR?

 Done.

[...snip the rationale...]
GC> So I think clipping should be applied based not on any abstract property
GC> like left-alignment, but rather on the exact unique property of representing
GC> the group-quote "Participant" field.

 I agree with this, but wx_table_generator was designed to be a generic
table-generating class and it doesn't seem right to impart to it any
special knowledge of this field. So there should be some way of telling it
to clip this column (and this column only) and group_quote_pdf_generator_wx
should do it. And which way could it be? All per-column information needed
by wx_table_generator is currently provided by its add_column() function,
and so it would make sense to add some parameter to configure column
clipping here as well.

 However this parameter would be totally redundant with the existing
"widest_text" one because clipping would only be enabled for the only
column for which "widest_text" is empty. This could be seen as just a
random coincidence, but I don't think this is the case and, instead, I
believe that any variable-width column would need to be clipped because
it must be variable-width in the first place exactly because the length of
its values is practically unlimited and so could overflow.

 Hence I think it's better to use "widest_text.empty()" as the indicator of
whether the column needs to be clipped and my PR does it like this. Please
let me know if you disagree.


GC> > And I also don't know what do you think from my experimental use
GC> > of braces for simple ctors (i.e. those just using the values given to them
GC> > to initialize member variables and not performing some complicated action)
GC> > instead of parentheses.
GC> 
GC> Here's how the alternatives speak to me:
GC> 
GC>   wxPoint(x, y_top)
GC> Call a function. Maybe it...draws a point on the screen? No, I
GC> happen to recognize the name, and I remember that it's a class,
GC> so this function must be a constructor. So I guess it probably
GC> constructs a temporary object.
GC> 
GC>   wxPoint{x, y_top}
GC> Only one possible meaning: construct a temporary. Much better.

 Yes, this is exactly why I'm (cautiously) warming up to using braces too.
Initially I avoided them as a plague because of the whole auto with
initializer_list disaster, but now, with C++17 in which "auto x{42}"
finally does the right (or, at least, not clearly wrong) thing (see
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3922.html), I do
start seeing advantages in using them.


GC> struct column_info:
GC>   is_centered_ is a member variable, initialized in the ctor
GC>   is_hidden() is a member function, whose return value is dynamic
GC> Should these really be implemented in those two different ways?

 Yes, the inconsistency is indeed jarring, now that I look at it. But there
is a reason for it, actually...

GC> Wouldn't it be better to treat is_hidden() the same as is_centered_?

... We could have is_hidden_ as a (const) member field. But I think it's
actually nicer to use functions like this if possible. The trouble is that
we can't have a similar is_centered() because the value of width_ is/will
be different from that of the initial width in the ctor, as width_ will be
set to the effective width of the column.

 So the choice is between introducing an unnecessary is_hidden_ variable or
this inconsistency and neither seems ideal. Instead, I chose to resolve
this inconsistency by making is_centered() a function too and renaming
is_centered_ variable to is_variable_width_ and reusing it for
needs_clipping() function. This looks like the best solution for me but,
again, please let me know if you don't think so.

GC> Is this a struct only because we want its members to be publicly
GC> accessible?

 Yes, there was no real encapsulation here, so it didn't seem useful to
pretend there was any. This has, however, changed now (admittedly, it has
started changing with is_hidden() addition, so maybe it should have been
changed to be a class back then -- but better late than never).

GC> But their values can also be changed by clients, and
GC> isn't that undesirable?

 width_ is/was supposed to be changed, but the other two should have been
const. They were not because it wasn't compatible with the requirements on
the standard container elements in C++98. Now that we use C++11 we
definitely can make them const though.

GC> Reading wx_table_generator::do_output_values() with its recent
GC> changes:
GC>   if(align_right_)
GC>   if(ci.is_centered_)
GC> it seems that right-alignment is a quasi-global, while
GC> center-alignment is a column_info data member.

 Yes, right-alignment is set for the entire table using align_right()
method and overrides the individual columns alignment.

GC> I understand how this evolved (right-alignment was recently added to
GC> support illustrations, while center-alignment was already used for
GC> group quotes), but when code is complex for "historical reasons", it's
GC> natural to ask whether it ought to be refactored for uniformity.

 I agree that something should be done here, but I'm not sure what. The
main problem I see is that currently nothing prevents the caller from
adding a variable-width and, hence, usually left-aligned, column to a
right-aligned table and the column would appear right-aligned in the
output, which is almost certainly not expected. Should such columns remain
left-aligned even in right-aligned tables? Or should they be forbidden in
them at all (because currently they're not used in them)?

 Perhaps we actually need 2 variants (and hence subclasses) of the table
generator: one for the tables with numeric contents only, such as used in
the illustrations, and another one supporting columns with textual
contents, such as the group quotes one? I initially decided against it
because having a single class with methods that can be called on it
dynamically seemed to be more flexible, but if we don't need this
flexibility, we could enforce it.

 For now I'm not doing anything about this because the problem is not
critical and I'm not sure what to do exactly, but, for the third time I
ask you (but there is no full moon tonight, so this is not binding) to let
me know if you think this should be changed soon and if you have any
preferences as to how exactly to do it.

 Thanks in advance!
VZ