Re: [lmi] Header pagination PR

[Greg Chicares]

On 2018-09-18 20:07, Vadim Zeitlin wrote:
> 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.

I'll make a note of that, as an inline comment.

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

I do agree, and it's not urgent. Can you do this entirely in '*.mst',
or are code changes needed? If it's all '*.mst' work, then it shouldn't
conflict with anything I plan to do this week. Or, if it needs only a
few code changes, it might not be too difficult to merge soon. Or we
can put it off until later.

> 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.

Let me give that some more thought. I do agree that U and D should be
kept separate. What I'm thinking is that
  U(D d, I i) : d_(d), i_(i) {...}
  U::foo() {d_.bar();}
is one way of doing it, and
  U(D d) : d_(d) {...}
  U::foo() {d_.get_i().bar();} // U has a d_ but no i_
is another way that has similar advantages. It depends on how many
foo's and bar's there are, and whether any of them call other functions
  quux(d_, i_);
in a way that requires passing the arguments.

But I'll take a fresh look tomorrow and compare HEAD with the work
I had put off to the side. We've been working toward the same goal,
and it might turn out that you've already completely achieved it.

> 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.

Simpler, yes, but OTOH that would be cheating. If I understand your reason
for a change, and then look at the code modifications, I'll of course see
that the modifications look all right, which makes it harder to perceive
the details. If I look at the result of applying half a dozen commits
without any prior expectation of what they should do, then I'll discern
your intention from the code, and along the way I'll naturally question
everything. IOW, you're describing an easier top-down path, but I learn
more and review better if I work from the bottom up--and the extra cost
is worth it if I find even one detail that might be improved.

In this case, I've gone through everything and have little to say.

> 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?
[...]
>  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.

Oh. I remember seeing that term in my slowly-progressing studies of
modern C++, but I didn't take much notice because I didn't understand
its motivating purpose. It would probably be better to read books than
the standard, but the standard is in an always-open PDF, whereas canines
can get upset if I push my chair back in order to access the bookshelf.