Re: [lmi] Pagination anomaly

[Vadim Zeitlin]

On Thu, 1 Feb 2018 23:47:12 +0000 Greg Chicares <address@hidden> wrote:

GC> Vadim--It's probably easier to reproduce and recognize this anomaly
GC> than to describe it,

 It was indeed easy to reproduce this, thank you.

GC> Adding or subtracting one to the year in the "Date of birth" field
GC> and regenerating the PDF suppresses the anomaly, producing either a
GC> shorter illustration that lacks the "empty" page, or one that has a
GC> single line of data toward the top of that page in addition to the
GC> footer. Because the original PDF (generated without manipulating the
GC> date of birth) has a full block of five years as its last quinquennial
GC> block, I can't resist speculating that the spurious "empty" page might
GC> invisibly show a blank line that would separate that from any following
GC> block (of which there is none). Or something like that.

 The really sad thing is that I had thought about exactly this case and
handled it correctly (I think?), in the following code:

        // Each group actually takes rows_per_group+1 rows because of the
        // separator row between groups, hence the second +1, but there is no
        // need for the separator after the last group, hence the first +1.
        int const groups_per_page = (rows_per_page + 1) / (rows_per_group + 1);

However I managed to make a much more stupid mistake in almost the next
line where I just divided the total number of years by the number of years
per page, instead of rounding it up properly, so the function returning the
number of extra pages was off by 1 when the number of years was exactly
divisible by the number of years per page.

 And, to top if off, I also made a matching mistake in the function
actually generating the output, which created an extra page in the same
case.

 This PR: https://github.com/vadz/lmi/pull/61 fixed both of these problems
and generates correct output, without blank page, for me.


GC> In a "distribution" generated by 'make fardel', running this case
GC> elicited these messageboxes:
GC> 
GC>   Logic error: 1 missing extra pages.
GC>   [file /opt/lmi/src/lmi/ledger_pdf_generator_wx.cpp, line 1203]
GC> 
GC>   Assertion 'extra_pages_ > 0' failed.
GC>   [file /opt/lmi/src/lmi/ledger_pdf_generator_wx.cpp, line 1213]
GC> 
GC> which seem like they might be helpful for tracking down the anomaly.

 Yes, indeed, but not the same one: the confusing warning about both having
too many (> 0) extra pages and not enough of them is an anomaly on its own
and while it's not supposed to happen normally, it's still worth fixing it
and I did this in https://github.com/vadz/lmi/pull/62

GC> However, if we just 'make install' and run the case in the directory
GC> where that command installs lmi, no messageboxes are seen. All I can
GC> guess is that we've done something wrong in the 'fardel', but the
GC> messageboxes seem to fit the anomaly observed in the PDF so well
GC> that I thought I should report them, even though I don't think you
GC> will see them yourself.

 I indeed don't see them (I did something different to see the problem
above and to fix it in PR 62) and this worries me because even though the
old code was wrong, it was still self-consistent, and should have always
generated as many pages as it had estimated, AFAICS. So there must be some
other mistake somewhere, but I don't know where is it. If you can create a
reproducible test case for it, it would be very helpful, otherwise I'll
just keep starring at this code.


 To return to the fixes above: both of them should, IMO, be applied, in
whichever order you prefer. However I should warn that I didn't test them
with MinGW because I have some troubles with building the latest wxWidgets
(as I also wanted to test lmi with it...) using install_wx.make at the
moment. I'll resolve this later, but I just didn't have the time to do it
yet. And the patches do work under MSW with MSVS (2017, as 2015 is not
recent enough to compile the latest lmi code any longer) and Linux, so it
should be safe enough to apply them.

 Thanks in advance,
VZ