Re: [lmi] Pagination anomaly

[Vadim Zeitlin]

On Sun, 4 Feb 2018 13:59:43 +0000 Greg Chicares <address@hidden> wrote:

GC> I've applied and pushed both changesets, and rebuilt lmi with MinGW
GC> (without rebuilding wx). The previously-reported anomaly in the
GC> seventh cell is no longer seen (thanks), but now there seems to be
GC> another problem: I load the input census shared the other day,
GC> press Ctrl-Shift-I [Census | Print case to PDF], and all expected
GC> PDF files are written, but a messagebox warns:
GC>   Logic error: 1 missing extra page(s)
GC> on line 1207 of 'ledger_pdf_generator_wx.cpp'.

 Sorry, it took me a long(er than expected) time to find this problem
because I couldn't see it initially -- because I completely forgot that I
had intentionally modified my sample.policy to insert a "FIXME-VZ
NonGuaranteedFootnote" string into the corrseponding datum because I wasn't
sure where this string was supposed to appear. And this, of course, changed
the page layout just so slightly, but enough to hide the problem. After
undoing this change to sample.policy, I could see the bug immediately and,
examining the PDF generated for the composite ledger more closely, I saw
that there was an even worse problem in it and all the other ones: the
first year on the non-first page of each tabular report was skipped because
of a bug I introduced in the last fix. The fix is trivial and I've created
https://github.com/vadz/lmi/pull/63 for it (which also contains another
small change) or you can just do this:

---------------------------------- >8 --------------------------------------
diff --git a/ledger_pdf_generator_wx.cpp b/ledger_pdf_generator_wx.cpp
index 1e23f6a..111eb0d 100644
--- a/ledger_pdf_generator_wx.cpp
+++ b/ledger_pdf_generator_wx.cpp
@@ -1610,7 +1610,7 @@ class page_with_tabular_report
 
         // The table may need several pages, loop over them.
         int const year_max = ledger.GetMaxLength();
-        for(int year = 0; year < year_max; ++year)
+        for(int year = 0; year < year_max; )
             {
             int pos_y = render_or_measure_fixed_page_part
                 (table
---------------------------------- >8 --------------------------------------
        
but I still have a few more problems/questions with/about this code.

 First one is that it's painfully obvious that I can't avoid bugs in it, so
it would be great to have tests verifying that this works correctly.
Unfortunately this code is not easy to test in isolation, so I need to
either refactor it in some non-trivial and not really natural way to allow
it, or we need to implement the text output idea from the other thread, but
do it in a way allowing to test the pagination logic. The latter would
probably be more useful, wouldn't it?

 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 (as if
I needed it), so I wonder if I should remove the latter and just run the
code from render() twice, once with output disabled, to just compute the
number of pages needed, and second time for real. One disadvantage of doing
it like this is that get_extra_pages_needed() is currently more efficient
than render(), but I'm not sure if it's really noticeable. Another possible
advantage of the current approach is that get_extra_pages_needed() result
is used as a check for render() and that if (exactly) one of them is buggy,
chances are that we notice it due to the warning which was triggered. Does
this justify keeping both functions?

 Third and final problem is that I think that the current code is still
wrong, although at least now it's consistently wrong in both functions. The
case when it is possibly wrong is when the page can fit 5*n+k rows with
k = 2, 3 or 4 and the total number of rows is 5*n+l with l<k. In this case,
all but the last page contain exactly n groups, but the last but one page
could contain n+1 groups, with the last one being incomplete. Instead,
currently it still contains n groups and a new page is used for the last
incomplete group, so we could have a page with just one row on it, even if
there was space for up to 4 rows on the previous page. This seems wrong to
me because it doesn't use the pages optimally, but I'm not completely sure
whether this should really be changed because it has a potential advantage
of having exactly the same number of years on all the previous pages. E.g.,
to take the concrete example I've been testing with, there are 74 years in
the composite PDF and the page could fit 39 of them. As this is only enough
for 7 complete groups, the first page shows the years 1..35 and the second
one currently (i.e. with the fix above applied) shows the years 36..70,
with the years 71..74 overflowing to the last page. Would it be better to
show 1..35 on the first page, but 36..74 on the second one, even if it
would make it longer than the first one? I think it would, but I'd be
grateful if you could please confirm it.

 Thanks in advance and sorry again for the delay with fixing yet another
bug in this code!
VZ