Re: [lmi] do_compute_column_widths_if_necessary()

[Vadim Zeitlin]

On Sat, 21 Apr 2018 00:57:40 +0000 Greg Chicares <address@hidden> wrote:

GC> Elsewhere in 'wx_table_generator.cpp':
GC> 
GC>     wxDCFontChanger set_header_font(dc_);
GC>     if(use_bold_headers_)
GC>         {
GC>         set_header_font.Set(get_header_font());
GC>         }
GC> 
GC> I'm confused because
GC>   set_header_font.Set(get_header_font());
GC> sounds like a no-op:
GC>   set_parameter(get_parameter());
GC> How does that cause a bold font to be used?

 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? Just in case it still doesn't, this wxDCFontChanger
object changes the font used by the wxDC provided to it in ctor for the
duration of its lifetime, so using it is a simple way to guarantee that the
normal, not bold, font is restored on exiting the current scope (i.e.
returning from output_header() in this case).


GC> Earlier, we had spoken of "PDF pixels"; what exactly are those?

 They correspond to the device-independent coordinates used by PDF.

GC> AIUI, the basic PDF device-independent unit is a point = 1/72 inch; and
GC> one pixel is one point at 72 DPI,

 Yes, exactly.

GC> e.g.; but what DPI does the wxPdfDoc DC assume?

 wxPdfDoc allows to customize this in several ways (which I might not
myself understand fully), but lmi code uses 72 DPI throughout as it uses
wxMM_POINTS mapping mode for wxPdfDC it uses, see pdf_writer_wx.cpp.


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.

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()?

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?

GC> where I conjecture that (0 == i.width_) may never be true.

 If it were true, the group quote display would be broken as the "Name"
column wouldn't be shown as wide as it should be. The last time I tested
this this wasn't the case.


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, 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().


 Please let me know if you have any further questions,
VZ