Re: [lmi] [lmi-commits] master 5f53cca 2/9: Appropriately treat font sizes as static data

[Vadim Zeitlin]

On Wed, 19 Sep 2018 09:19:33 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit 5f53cca8cec0953fe72b11abb794ff38b34e32fe
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Appropriately treat font sizes as static data
GC> ---
GC>  ledger_pdf_generator_wx.cpp | 12 ++++--------
GC>  1 file changed, 4 insertions(+), 8 deletions(-)
GC> 
GC> diff --git a/ledger_pdf_generator_wx.cpp b/ledger_pdf_generator_wx.cpp
GC> index fac2ecf..0b98353 100644
GC> --- a/ledger_pdf_generator_wx.cpp
GC> +++ b/ledger_pdf_generator_wx.cpp
GC> @@ -741,13 +741,7 @@ class pdf_illustration : protected html_interpolator, 
protected pdf_writer_wx
GC>    public:
GC>      explicit pdf_illustration(Ledger const& ledger, fs::path const& 
pdf_out_file)
GC>          :html_interpolator {ledger.make_evaluator()}
GC> -        // Use non-default font sizes that are used to make the new
GC> -        // illustrations more similar to the previously existing ones.
GC> -        ,pdf_writer_wx
GC> -            (pdf_out_file.string()
GC> -            ,wxPORTRAIT
GC> -            ,{8, 9, 10, 12, 14, 18, 20}
GC> -            )
GC> +        ,pdf_writer_wx     (pdf_out_file.string(), wxPORTRAIT, font_sizes_)
GC>          ,ledger_           {ledger}
GC>      {
GC>          init_variables();
GC> @@ -958,9 +952,11 @@ class pdf_illustration : protected html_interpolator, 
protected pdf_writer_wx
GC>              );
GC>      }
GC>  
GC> -    // Source of the data.
GC>      Ledger const& ledger_;
GC>  
GC> +    // These font sizes differ from wxHTML defaults.
GC> +    static inline html_font_sizes font_sizes_ {8, 9, 10, 12, 14, 18, 20};
GC> +

 I don't really understand why did the comment above turn into the one here
in this commit. IMO the current commit is not very useful (as anybody who
cares about this can check that the sizes are different from defaults) and
raises more questions than it answers, notably _why_ do these sizes differ.
Of course, maybe it's obvious that they were carefully chosen in order to
match the existing outputs as closely as possible to anybody else reading
this code, but it definitely won't be obvious to me when I'm going to
reread it in a couple of months, after having forgotten everything about
it.

 IMHO we should preserve the explanatory part of the comment or remove it
completely and I'd prefer to do the former.

 Regards,
VZ