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

[Vadim Zeitlin]

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.

 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.

GC> Filtering out everything
GC> that doesn't seem to affect control flow:
GC> 
GC>         // The table may need several pages, loop over them.
GC>         for(int year = 0; year < year_max; )
GC>             for(;;)
GC>                 {
GC>                 ++year;
GC>                 if(year == year_max)
GC>                     {
GC>                     // We will also leave the outer loop.
GC>                     break;
GC>                     }
GC>                 if(year % rows_per_group == 0)
GC>                     {
GC>                     if(pos_y > page_bottom - rows_in_next_group*row_height)
GC>                         {
GC>                         [base class's render() called here]

 It's important to notice that next_page() is also called here.

GC>                         break;
GC>                         }
GC>                     }
GC>                 }
GC>             }
GC> 
GC> What does the middle 'for' (i.e., "for(;;)" accomplish?

 IMHO it makes the code more clear.

GC> This comment
GC>                     // We will also leave the outer loop.
GC> apparently means that 'break;' exits the inner "for(;;)" loop,
GC> and, because "if(year == year_max)" is true, then the outer
GC> loop's "; year < year_max;" condition is false, so it's also
GC> exited.

 Yes, exactly.

GC> But why can't this all be done more clearly in a single
GC> 'for' loop?
GC> 
GC> I looked back at this change:
GC>   git show 74872e0
GC> where "for(;;)" was introduced. It was originally:
GC>     for(; year < year_max; ++year)
GC> but even there the logic was:
GC> 
GC>         for(int year = 0; year < year_max; ++year)
GC>             {
GC>             int pos_y = render_or_measure_fixed_page_part ...
GC>             for(; year < year_max; ++year)
GC>                 if...
GC>                     if...
GC>                         break;
GC> 
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.

GC> Can this be replaced by simpler logic? What do you think of
GC> the tentative attempt presented in the patch below? I'm not
GC> satisfied with it because I'd like to get rid of all 'break;'
GC> statements, but...is this function recursive?

 No.

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.

GC> or to call
GC>   page_with_tabular_report::render()
GC> recursively?

 No, definitely not.

GC> I'm hoping that, if I can comprehend this function deeply,
GC> I'll be able either to implement it in terms of something
GC> akin to get_needed_pages_count(), or at least to add
GC> assertions based on such unit-tested code.
GC> 
GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> diff --git a/ledger_pdf_generator_wx.cpp b/ledger_pdf_generator_wx.cpp
GC> index f9ac78ec6..e6987f705 100644
GC> --- a/ledger_pdf_generator_wx.cpp
GC> +++ b/ledger_pdf_generator_wx.cpp
GC> @@ -1610,60 +1610,65 @@ class page_with_tabular_report
GC>  
GC>          // The table may need several pages, loop over them.
GC>          int const year_max = ledger.GetMaxLength();
GC> -        for(int year = 0; year < year_max; )
GC> +        for(int year = 0; year < year_max; ++year)
GC>              {
GC> -            int pos_y = render_or_measure_fixed_page_part
GC> -                (table
GC> -                ,writer
GC> -                ,interpolate_html
GC> -                ,oe_render
GC> -                );
GC> +            int pos_y = 0;
GC> +            if(0 == year)
GC> +                {
GC> +                pos_y = render_or_measure_fixed_page_part
GC> +                    (table
GC> +                    ,writer
GC> +                    ,interpolate_html
GC> +                    ,oe_render
GC> +                    );
GC> +                }

 No, this is definitely wrong because render_or_measure_fixed_page_part()
must be called for each page and not only the first one.

 I hope my explanations help you to understand this better but I still
don't see any clear simplifications here. We could use a single loop and
make calling render_or_measure_fixed_page_part() conditional inside it, but
this would seem to make things less, not more, clear to me...

 Regards,
VZ