Re: [lmi] Control flow in page_with_tabular_report::render()

[Vadim Zeitlin]

On Sun, 11 Feb 2018 18:12:03 +0000 Greg Chicares <address@hidden> wrote:

GC> Let me explain what I'd like to do (or at least attempt). In light of:
GC> 
GC> http://lists.nongnu.org/archive/html/lmi/2018-02/msg00016.html
GC> |  Second problem is that currently there are 2 functions dealing with page
GC> | breaks: page_with_tabular_report::render() and get_extra_pages_needed().
GC> | This gives me twice as many opportunities to introduce bugs in them
GC> 
GC> ...and...
GC> 
GC> http://lists.nongnu.org/archive/html/lmi/2018-02/msg00026.html
GC> |  I've extracted the logic of one of the two PDF pagination functions (the
GC> | one which was easier to refactor) in a new get_needed_pages_count() in
GC> | miscellany.hpp and added a simple unit test for it
GC> 
GC> ...my dream is to factor out the page_with_tabular_report::render()
GC> logic as well, putting it into an expanded ::get_needed_pages_count(),
GC> and then using this one-and-only-one, fully-unit-tested logic to
GC> drive pagination everywhere: in the two functions mentioned above,
GC> and in future work that we may not even have thought of yet--because
GC> the algorithm is general, widely applicable, and hard to get right.

 Yes, I understand and agree that it's a good idea. I didn't think it was
worth extracting this logic in a separate table pagination class because I
thought it was so simple as to be not worth it, but this was experimentally
shown to be far from being true.

GC> Then I think the pseudocode could be:
GC> 
GC>   pages = magic_class.pages();
GC>   for(p: pages)
GC>     {
GC>     print header;
GC>     rows = magic_class.rows(p);
GC>     for(r: rows)
GC>       {
GC>       print row;
GC>       // use magic_class somehow to print blank rows as needed
GC>       // (many ways to do that--no need to choose one now)
GC>       }
GC>     print footer with page number;
GC>     }

 Yes, but it still has 2 loops and, IMHO, rewriting it as one (while
certainly possible) wouldn't help with code clarity at all.

GC> I understand that render() doesn't work that way today: instead, it
GC> waits until it's about to print something, and decides at that point
GC> whether to print a row, print a blank line, or start a new page.
GC> But in principle all those decisions can be made in one unit-testable
GC> class and embodied in some data structures that it can provide.

 Yes.

GC> >  To help with understanding this function code, it probably would be
GC> > helpful to mention that each (physical) page is composed of a fixed page
GC> > part, including the header and all the text before the beginning of the
GC> > table, and the varying part containing the table rows. The former must be
GC> > rendered outside of the inner loop, while the latter is rendered inside 
it.
GC> 
GC> Printing the top of the page, and printing the bottom, are different
GC> operations from printing the rows in between. They're already separate
GC> function calls. The present render() happens to handle the top outside
GC> the loop, and the bottom inside it, but if I understand this correctly
GC> there's nothing about the "bottom" handling that requires it to be
GC> done inside a loop over rows: IOW, nothing that invalidates my
GC> proposal above. In yet other words, I think the "latter" part here:
GC> 
GC> | The former must be rendered outside of the inner loop,
GC> | while the latter is rendered inside it
GC> 
GC> is just an implementation detail: the former "must" be treated that
GC> way, but the latter could be treated otherwise (outside the loop).

 I'm sorry, I find this very confusing because it directly contradicts the
pseudo-code above: the "latter" here is the part containing the actual
table rows and (I'm tempted to say "of course") it must be done in a loop
over rows, mustn't it?

GC> > GC> and the double for-loop confuses me: is the intention just
GC> > GC> to set 'pos_y' iff 0==year?
GC> > 
GC> >  No, it must be set at the start of each new page. Also, setting pos_y is
GC> > just a side effect and we could do it only once because it doesn't vary
GC> > from page to page. But calling render_or_measure_fixed_page_part() to
GC> > actually render the fixed page part for each new physical page is, of
GC> > course, indispensable.
GC> 
GC> Let me paraphrase just to be sure I understand. We have to render
GC> the header at the top of each page, obviously. A side effect of
GC> that rendering is that it gives us pos_y. It's crucial to set
GC> pos_y at least once, so that we know where the header ends and
GC> the rows can begin. It would be possible to set that pos_y only
GC> on the first of several pages, because it just so happens that
GC> all pages have the same "top", at least for now. However, setting
GC> pos_y each time through costs about a nanosecond, and it does
GC> the right thing even if a future change makes the height of the
GC> "top" vary by page.

 Yes, this is absolutely correct.

GC> > GC> ...is this function recursive?
GC> > 
GC> >  No.
GC> 
GC> Here's my burning question:
GC>   class B           {virtual foo();};
GC>   class C: public B {};
GC>   class D: public B {virtual foo() override;};
GC> For an object of class D, why call C::foo()?

 Generally speaking, each object must call its base class version of the
method until it explicitly wants to replace its implementation, which is
not the goal here.

GC> Is it that someday, maybe C will override B::foo(), and then
GC> that new C::foo() will be called automatically without any
GC> need to change D's implementation?

 Yes. Alternatively, a completely new class Z might be added to the
hierarchy between B and C and its foo() will still be called, as expected.

GC> And, if so, such a change would go against this recommendation
GC> from the 2001 boost coding guidelines:
GC> 
GC> | Override a virtual function at most once in any path up the
GC> | inheritance hierarchy. No derived class should implement a
GC> | virtual function that is also implemented in one of its
GC> | ancestors, except in the class where that function is first
GC> | declared. This helps to clarify designs by keeping the model
GC> | simple: the function to be called is either the default
GC> | implementation, or it is the (single) overrider.
GC> 
GC> Would you consider that advice inapplicable today?

 It's (still) a good advice, and I try to follow it when possible, but
sometimes it's just not worth it. I.e. in this particular case we would
need to make render() non-virtual in the base class and call some virtual
do_render_page_decorations(), which would be overridden in numbered_page,
and do_render_page_body(), which would be overridden in
page_with_tabular_report. This is certainly doable, but I doubt it would be
simpler and I don't like the idea of having to add new virtual methods each
time it makes sense to abstract drawing a new page region in a common base
class.

GC> Does "the footer is drawn before drawing anything else" mean...
GC> 
GC>  - after drawing the header, and then the rows, the footer is
GC>    drawn: after header and rows, but before any subsequent
GC>    drawing; or
GC> 
GC>  - when a page is about to be drawn, the footer is drawn,
GC>    first of all, before anything else: before even the header

 Currently it's the latter, just because the footer is drawn by the base
class method and I prefer to always call the base class version first,
before doing all the rest in the derived classes.

GC> If we precalculate the number of pages, and the number of
GC> data rows to show on each (and their spacing), then we've
GC> split the work into two separate pieces, each of which can
GC> be simpler than their combination. And the first can be
GC> unit-tested exhaustively. That's my dream.

 We can indeed do it like this. Would you like me to change the code in
this way or do you prefer to do it yourself?

 Thanks,
VZ