Re: [lmi] do_compute_column_widths_if_necessary()

[Greg Chicares]

On 2018-04-21 13:08, Vadim Zeitlin wrote:
> On Sat, 21 Apr 2018 00:57:40 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC>     wxDCFontChanger set_header_font(dc_);
[...]
>  I think the confusion is due to the not very clear local variable name.
> Perhaps it should have been called header_font_setter instead, would this
> clarify its purpose?

Yes. Done. I didn't realize that wxDCFontChanger is like wxBusyCursor.

> GC> I'm also not sure what the name do_compute_column_widths_if_necessary()
> GC> means--specifically the "if necessary" part. Is it unnecessary to call
> GC> this function at all in some contexts? Or OTOH is it always necessary
> GC> to call this function, but unnecessary to call it again after it has
> GC> been called once? (I'm guessing it's the latter, and that the flag
> GC> 'has_column_widths_' just means that the function has been called once.)
> 
>  Yes, this guess is completely correct.

Okay, then this flag is used for optimization: once this function has been
called, it ought to be unnecessary to call it again.

But how do we know that's actually unnecessary? AFAICT, it needs to be
called after the last call to add_column()...so wouldn't it make sense
for add_column() to assert that this flag is false, because any later
call to add_column() would require calculating the column widths again?

And how do we know where to call the function that sets column widths?
It's called in
  do_get_cell_x()
  output_horz_separator()
  output_header()
all of which access elements of 'columns_', but not in these other
functions
  cell_rect()
  do_output_values()
that also access columns similarly.

How can we be sure it's called exactly when necessary? Clearly it can't
be called in the ctor, before add_columns() is ever called. Would it
make sense to add an accessor like

    std::vector<column_info> get_columns()
    {
    do_compute_column_widths(); // Only called in this one place.
    return columns_;
    }

and use it everywhere do_compute_column_widths() is now explicitly
called?

> GC> Here:
> GC>     if(0 == i.width_)
> GC>         {
> GC>         num_expand++;
> GC>         }
> GC> do you really mean to compare width_ to zero, or did you intend
> GC>     if(i.is_variable_width_)
> GC> instead?
> 
>  It's the same thing, but using is_variable_width_ might indeed be better.
> This would require making it public, however. Maybe it we should rather add
> another accessor to column_info(), e.g. has_fixed_width()?

Yes, I think I'll go ahead and do that, because I'm not sure it's
really the same thing. I'm sure that 'is_variable_width_' or
has_fixed_width() express the actual intention, but I'm not sure
'0 == i.width_' is guaranteed to mean the same thing here--maybe
it is guaranteed, but for reasons that are not immediately obvious
to me.

> GC> AIUI, "variable-width" columns have width zero when constructed,
> GC> and 'is_variable_width_' captures that status.
> 
>  Yes.
> 
> GC> However, it seems that width_ is increased to the width of the column
> GC> header, before the snippet above is reached,
> 
>  I don't think this is the case, where do you see this happen?

I haven't seen it happen. I just don't see how it's guaranteed
never to happen. So I think I'll rework this in terms of the
accessor you suggested above, because that will certainly dispel
my uncertainty.

> GC> Why not remove "hidden" columns before formatting? This would simplify
> GC> the code in 'wx_table_generator.cpp', which explicitly skips hidden
> GC> columns in multiple places. AFAICT, we could just forbear to add hidden
> GC> columns where indicated below.
> GC> 
> GC> ...in 'ledger_pdf_generator_wx.cpp':
> GC>             if(should_show_column(ledger, column++))
> GC>                 {
> GC>                 label = i.label;
> GC>                 }
> GC>             //else: Leave the label empty to avoid showing the column.
> GC> 
> GC>             table.add_column(label, i.widest_text);
> GC> 
> GC> ...in 'group_quote_pdf_gen_wx.cpp':
> GC>     if(!/* various conditions */)
> GC>         {
> GC>         // Leave the header empty to hide this column.
> GC>         break;
> GC>         }
> GC>     ...
> GC>     table_gen.add_column(header, cd.widest_text_);
> GC> 
> GC> The add_column() calls appear right after the only code that seems to
> GC> hide columns, so why call add_column() at all for hidden columns?
> 
>  Because, as explained in the add_column() comment,

    // Providing an empty header suppresses the column display, while still
    // taking it into account in output_row(), providing a convenient way to
    // hide a single column without changing the data representation.

> it is very convenient
> to be able to pass the same values to output_row() whether all of them are
> supposed to be shown or not. This is not especially relevant for the
> illustrations generation currently because everything is static (i.e. no
> columns are hidden depending on some run-time conditions), but it allows
> group_quote_pdf_generator_wx code to be simpler and more natural than it
> would have otherwise been. I'd very much prefer to leave it like this
> rather than having to carefully construct the proper vector, matching the
> currently shown columns, to pass to output_row().

Can you provide more context? I'm still having trouble understanding this.
I understand that some group-quote columns are hidden (there's one large
set of columns that are potentially shown, and some are suppressed at
run time, e.g. if their contents are all zero). But AIUI either a column
is hidden (and all its rows are hidden), or shown (and all its rows are
shown). I'm supposing, perhaps incorrectly, that there comes a moment
when we know which columns are hidden (at which point we might simply
remove them from the vector of columns), and that that moment comes
before we render the columns (so rows would render correctly). What am
I missing here?