Re: [lmi] Header pagination PR

[Vadim Zeitlin]

On Tue, 18 Sep 2018 18:14:22 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-09-18 14:26, Vadim Zeitlin wrote:
GC> [...]
GC> >   https://github.com/vadz/lmi/pull/96
GC> 
GC> * First step: reviewing only the commit messages, not the code...
GC> 
GC> 1c398a86 (HEAD -> master, origin/master, origin/HEAD) \
GC>          Implement support for headers for normal PDF illustration pages
GC> 
GC>     Show this tag in action in the "Explanatory Notes" page of reg_d
GC>     individual illustration: with sample2ipp its second page correctly
GC>     repeats the logo and the title now, while it didn't have them before.
GC> 
GC> After testing it with 'sample2ipp', I believe I should use the
GC> new <header> tag in all other '*.mst' files that have a logo, right?

 Yes, absolutely, sorry for forgetting to say this. I only used it in a
single file for testing, but if you don't find any problems with its use
here, it should be used everywhere else.

 Moreover, I also think that we might do something similar for the footers
now: they have less variability than the headers, which allowed me to
handle them in the code without too much trouble, but I think it would be
better to define the footers entirely in the .mst files too, rather than
doing it partially there and partially in the headers. Please let me know
if you agree and I'll make the necessary changes (it is not urgent, of
course).

GC> 71dc467e Pass the illustration and interpolator to the page ctor
GC> 
GC> In uncommitted work that I've put aside for the moment, I believe
GC> I can demonstrate that the 'illustration_' member can provide
GC> everything we've been passing as arguments to so many functions:
GC>   {Ledger, interpolator, pdf_writer}

 It could, but I'd like to keep the separation between the illustration and
the interpolator as I feel it could be useful to not use the same object
for both in the future (e.g. we could write some tests verifying that the
interpolation works correctly without using any illustration objects). And
I think it's generally preferable to reduce dependencies between objects by
passing the object U (user) the object D (dependency) that it needs
directly instead of passing the object U the object I (intermediate) which
it can use to get D from, as it reduces long-distance coupling between the
different components. Here the difference is not very big and the entire
subsystem is sufficiently small that it might not be a concern at all
(although small things have tendency to grow with time), but I don't think
it's really that burdensome to pass these objects as arguments neither and
I'd prefer not to change that.

GC> 'ledger_pdf_generator_wx.cpp': 92 hunks of changes, so this will take a
GC> little longer.

 Maybe reviewing the changes commit-by-commit will be simpler than looking
at all of them at once. E.g. 71dc467eb0a3ff0b83b81f3ded4e1c21934604a8
contains a lot of changes, but they're trivial as this commit is a pure
refactoring and doesn't actually change anything. OTOH the following
commits b2ec7acde109641d06fd0c7ee774a32bb9219fc4 and
1c398a864f52103b63276f2ad13ad89e20f03bc1 contain fewer, but less trivial,
changes.

GC> It all seems fine at a superficial glance; the only question I have is
GC> here:
GC> 
GC>   class reg_d_indiv_curr : public page_with_tabular_report
GC>   {
GC>     public:
GC>       using page_with_tabular_report::page_with_tabular_report;
GC> 
GC> and in the various similar hunks preceding. I suppose this is a modern
GC> C++ idiom that I can't recall seeing previously. Is it really just a
GC> C++98 using declaration? Why does a derived class need to declare its
GC> base class's ctor in such a way?

 In order to have it. Without this using declaration (it's new in C++11,
the name of the corresponding feature is "inheriting constructors"), we
would need to redeclare the constructor taking the same arguments in this
class. With it, we just bring the base class constructor in scope of this
class.

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