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

[Greg Chicares]

On 2018-02-09 02:25, Vadim Zeitlin wrote:
> On Thu, 8 Feb 2018 23:46:48 +0000 Greg Chicares <address@hidden> wrote:

[...when many numeric columns are shown, a long "Participant" name can
overflow, scribbling over the numbers in columns to its right...]

> GC> >  It should be pretty simple: we just need to set the clipping rectangle
> GC> > before calling DrawText() in wx_table_generator::do_output_values(). The
> GC> > only potential problem is that I haven't used clipping with wxPdfDC yet,
> GC> > but it's definitely implemented in its code and so I think it has all
> GC> > chances to work.
> GC> > 
> GC> >  Please let me know if I should do this,
> GC> Yes, please.
> 
>  I'll make a properly tested (including compiling it with MinGW, which I
> didn't do yet) PR tomorrow, but I can already say that this trivial change:

Works for me.

> ---------------------------------- >8 --------------------------------------
> diff --git a/wx_table_generator.cpp b/wx_table_generator.cpp
> index 39715bca6..942fc369c 100644
> --- a/wx_table_generator.cpp
> +++ b/wx_table_generator.cpp
> @@ -306,6 +306,14 @@ void 
> wx_table_generator::do_compute_column_widths_if_necessary()
>                      }
>              }

That '}'       ^ wants to be indented four spaces. I hesitate to change
code you're in the process of changing, so would you mind doing it as
part of your forthcoming PR?

> +            wxDCClipper clip
> +                (dc_
> +                ,wxRect
> +                    {wxPoint{x, y_top}
> +                    ,wxSize{width, row_height_}
> +                    }
> +                );
> +
>              dc_.DrawText(s, x_text, y_text);
>              }
> ---------------------------------- >8 --------------------------------------
> 
> does work and clips the name as expected. I don't know if we should
> restrict setting the clipping region only to the left-aligned columns
> (because the others would become unreadable or, worse, wrongly readable, if
> clipped).

I suppose this function, originally written for group quotes, is now used for
illustrations as well, so we have to be sure it's appropriate for both.

For illustrations, columns may rarely contain short enumerative strings, like
{Male, Female}, whose values are static const and cannot be changed by users
(so their lengths are fixed, known, and short); but they most commonly contain
numeric data, and Ledger::AutoScale() guarantees that their formatted widths
are strictly within known limits (even for amounts up to the US national debt).
Therefore, by design, illustration columns can never overflow, and it would be
wrong to clip them. (In theory, clipping can have no effect, but in practice,
it might do actual harm, because there could be a defect in the clipping
implementation.)

For group quotes, we've hard-coded column widths that we believe are highly
likely to be adequate--but we can't really rule out the possibility that
numeric values will overflow. Input the national debt, and they'll overflow.
It's undesirable to clip them because we'd rather observe the overflow on
the output, except for the unique case of the "Participant" column. That
unique column just copies an arbitrary input string of practically unbounded
length. You'd have to enter implausibly large input values to get the numeric
columns to overflow, and they probably wouldn't overflow much (one trillion
could be a silly mistake, but entering a googol would require too much work);
but "Participant" could much more easily be fifty characters, or a hundred,
or more, as it's likely to be pasted from a spreadsheet rather than typed in.

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

> And I also don't know what do you think from my experimental use
> of braces for simple ctors (i.e. those just using the values given to them
> to initialize member variables and not performing some complicated action)
> instead of parentheses.

Here's how the alternatives speak to me:

  wxPoint(x, y_top)
Call a function. Maybe it...draws a point on the screen? No, I
happen to recognize the name, and I remember that it's a class,
so this function must be a constructor. So I guess it probably
constructs a temporary object.

  wxPoint{x, y_top}
Only one possible meaning: construct a temporary. Much better.

> If you have any thoughts on either or both of these
> subjects, please let me know.

Let me share some thoughts on other, related subjects, which occurred to
me while reading this code.

struct column_info:
  is_centered_ is a member variable, initialized in the ctor
  is_hidden() is a member function, whose return value is dynamic
Should these really be implemented in those two different ways?
Wouldn't it be better to treat is_hidden() the same as is_centered_?

Is this a struct only because we want its members to be publicly
accessible? But their values can also be changed by clients, and
isn't that undesirable?

Reading wx_table_generator::do_output_values() with its recent
changes:
  if(align_right_)
  if(ci.is_centered_)
it seems that right-alignment is a quasi-global, while
center-alignment is a column_info data member. I understand how
this evolved (right-alignment was recently added to support
illustrations, while center-alignment was already used for group
quotes), but when code is complex for "historical reasons", it's
natural to ask whether it ought to be refactored for uniformity.