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

[Greg Chicares]

On 2018-02-11 16:23, Vadim Zeitlin wrote:
> On Sun, 11 Feb 2018 16:04:06 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> Vadim--Can you help me understand the flow of control in
> GC> page_with_tabular_report::render()?
> 
>  In brief, this structure directly translates the following pseudo-code:
> 
>       while have more pages:
>               render fixed, non-tabular part
>               while have more rows:
>                       draw row
>               start next page
> 
> I.e. the inner loop is over the table rows on a single page while the outer
> loop is over the pages.

Thanks, that really helps me to understand what's going on here.

Let me explain what I'd like to do (or at least attempt). In light of:

http://lists.nongnu.org/archive/html/lmi/2018-02/msg00016.html
|  Second problem is that currently there are 2 functions dealing with page
| breaks: page_with_tabular_report::render() and get_extra_pages_needed().
| This gives me twice as many opportunities to introduce bugs in them

...and...

http://lists.nongnu.org/archive/html/lmi/2018-02/msg00026.html
|  I've extracted the logic of one of the two PDF pagination functions (the
| one which was easier to refactor) in a new get_needed_pages_count() in
| miscellany.hpp and added a simple unit test for it

...my dream is to factor out the page_with_tabular_report::render()
logic as well, putting it into an expanded ::get_needed_pages_count(),
and then using this one-and-only-one, fully-unit-tested logic to
drive pagination everywhere: in the two functions mentioned above,
and in future work that we may not even have thought of yet--because
the algorithm is general, widely applicable, and hard to get right.

Then I think the pseudocode could be:

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

Then, as long as the unit tests are comprehensive, the pagination
is necessarily correct.

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

>  To help with understanding this function code, it probably would be
> helpful to mention that each (physical) page is composed of a fixed page
> part, including the header and all the text before the beginning of the
> table, and the varying part containing the table rows. The former must be
> rendered outside of the inner loop, while the latter is rendered inside it.

Printing the top of the page, and printing the bottom, are different
operations from printing the rows in between. They're already separate
function calls. The present render() happens to handle the top outside
the loop, and the bottom inside it, but if I understand this correctly
there's nothing about the "bottom" handling that _requires_ it to be
done inside a loop over rows: IOW, nothing that invalidates my
proposal above. In yet other words, I think the "latter" part here:

| The former must be rendered outside of the inner loop,
| while the latter is rendered inside it

is just an implementation detail: the former "must" be treated that
way, but the latter could be treated otherwise (outside the loop).

> GC> and the double for-loop confuses me: is the intention just
> GC> to set 'pos_y' iff 0==year?
> 
>  No, it must be set at the start of each new page. Also, setting pos_y is
> just a side effect and we could do it only once because it doesn't vary
> from page to page. But calling render_or_measure_fixed_page_part() to
> actually render the fixed page part for each new physical page is, of
> course, indispensable.

Let me paraphrase just to be sure I understand. We have to render
the header at the top of each page, obviously. A side effect of
that rendering is that it gives us pos_y. It's crucial to set
pos_y at least once, so that we know where the header ends and
the rows can begin. It would be possible to set that pos_y only
on the first of several pages, because it just so happens that
all pages have the same "top", at least for now. However, setting
pos_y each time through costs about a nanosecond, and it does
the right thing even if a future change makes the height of the
"top" vary by page.

> GC> ...is this function recursive?
> 
>  No.

Here's my burning question:
  class B           {virtual foo();};
  class C: public B {};
  class D: public B {virtual foo() override;};
For an object of class D, why call C::foo()?

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

And, if so, such a change would go against this recommendation
from the 2001 boost coding guidelines:

| Override a virtual function at most once in any path up the
| inheritance hierarchy. No derived class should implement a
| virtual function that is also implemented in one of its
| ancestors, except in the class where that function is first
| declared. This helps to clarify designs by keeping the model
| simple: the function to be called is either the default
| implementation, or it is the (single) overrider.

Would you consider that advice inapplicable today?

> GC> This function
> GC>   page_with_tabular_report::render()
> GC> calls
> GC>   numbered_page::render()
> GC> but that class doesn't declare such a function, so...is it
> GC> the intention to call
> GC>   page_with_footer::render()
> GC> (e.g., to delegate some low-level work),
> 
>  Yes, page_with_footer::render() draw the footer of the new page. It's,
> admittedly, confusing that the footer is drawn before drawing anything
> else, and this could be changed by rearranging both the initial call to the
> base class render() and this call. It might make things more clear (or
> not?) but won't change anything else.

Does "the footer is drawn before drawing anything else" mean...

 - after drawing the header, and then the rows, the footer is
   drawn: after header and rows, but before any subsequent
   drawing; or

 - when a page is about to be drawn, the footer is drawn,
   first of all, before anything else: before even the header

? Both could make sense: the first thing we do with a new
page might be to start writing at the top, or it might be
to write the page number at the bottom before anything else.

>  I hope my explanations help you to understand this better but I still
> don't see any clear simplifications here.

If we lay the rows out dynamically, on the fly, without
lookahead, then one loop must both figure out the layout
and render the output accordingly, and that complexity
is inherent and cannot be simplified away.

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