Re: [lmi] do_compute_column_widths_if_necessary()

[Greg Chicares]

On 2018-04-21 20:42, Vadim Zeitlin wrote:
> On Sat, 21 Apr 2018 19:54:27 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-04-21 13:08, Vadim Zeitlin wrote:
[...]
> GC> > it is very convenient to be able to pass the same values to
> GC> > output_row() whether all of them are supposed to be shown or not.
> GC> > This is not especially relevant for the illustrations generation
> GC> > currently because everything is static (i.e. no columns are hidden
> GC> > depending on some run-time conditions),

Actually, should_show_column() does hide certain columns on illustrations.
IssueAge is never shown on composites. NASD illustrations show one set of
columns if minimum premium is "split", and another set otherwise.

> GC> > but it allows
> GC> > group_quote_pdf_generator_wx code to be simpler and more natural than
> GC> > it would have otherwise been. I'd very much prefer to leave it like
> GC> > this rather than having to carefully construct the proper vector,
> GC> > matching the currently shown columns, to pass to output_row().
> GC> 
> GC> Can you provide more context? I'm still having trouble understanding this.
> GC> I understand that some group-quote columns are hidden (there's one large
> GC> set of columns that are potentially shown, and some are suppressed at
> GC> run time, e.g. if their contents are all zero). But AIUI either a column
> GC> is hidden (and all its rows are hidden), or shown (and all its rows are
> GC> shown). I'm supposing, perhaps incorrectly, that there comes a moment
> GC> when we know which columns are hidden (at which point we might simply
> GC> remove them from the vector of columns),
> 
>  There is no vector of columns.

On a literal level, there would seem to be, here:

    std::vector<column_info> columns_;

and here:

    if(should_show_column(ledger, column++))
        {
        label = i.label;
        }
    //else: Leave the label empty to avoid showing the column.
    table.add_column(label, i.widest_text);

Here's the model I was assuming. You have a spreadsheet with a hundred
columns. You want to copy and paste some of them to create a report with
ten columns. For each of the ten columns desired, you copy it from the
source, and paste it into the destination. Otherwise, for the ninety
columns you don't desire, you do nothing at all. It wouldn't make sense
to copy some of those ninety along with metadata to hide them.

But add_column() doesn't copy a column in that sense. It indicates an
intention to use a column, and accordingly adds certain metadata to
the columns_ vector:
    columns_.push_back(column_info(header, width));
but it does not pass the data that the column contains.

Thus, data is passed not by column, but by row...

> There is a vector of rows, where each row
> is an array of values for the given row. We could, in principle, go through
> this vector and filter out the values that need to be hidden from each row
> (which would need to become a vector and not a simple array instead). But

I really don't like working with C-style arrays. And in fact there
was only one of them, so I replaced it with a vector of strings.
I also changed the API that took "std::string const*", because it was
needed only for that one C-style array--and all the vectors passed to
it had call data() first, which feels like casting to void.

> this is clearly less convenient than just always putting all the values in
> the row values in group_quote_pdf_generator_wx::add_ledger() and then
> deciding which of the columns do we want to show or not when calling
> output_row() in save() later (notice that we don't know which columns
> should be shown and which one shouldn't in add_ledger() itself yet).

And there's output_header(), too, in addition to output_row().

>  Alternatively, think of it in this way: we have some UI-independent data
> and we have UI layer (PDF writer in this case). It is usually better to
> keep data in its natural form (which means including all the columns here)
> and then configure the UI to show it in the appropriate way (by hiding some
> of the columns) rather than massaging the data into some special format
> suitable for UI.

That's a feasible approach. I imagined at first that you had taken a
different approach, but now I understand. It's a fundamental choice
that permeates the whole design, and we aren't going to change it
because no other approach is clearly better.

>  Of course, it could be done in either way, but I see no reason at all to
> change the current code and even if I were writing it again, I'd still do
> it like this because it seems much more natural to me. Could you please
> explain what problem do you have with doing it like this?

I initially thought it might be simple to change, as in my spreadsheet
example above, which passes only the data that are wanted, instead of
passing extra data along with metadata indicating which data to ignore.
In that case, we wouldn't have to write code to skip "hidden" columns
in various places, or worry about whether we've added such code in
every place that needs it. But filtering data before it's passed
rather than after would introduce its own complications.