Re: [lmi] Another large census error with group quotes

[Greg Chicares]

On 2018-08-18 10:22, Vadim Zeitlin wrote:
> On Thu, 16 Aug 2018 23:48:43 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-08-16 19:26, Vadim Zeitlin wrote:
> GC> > On Thu, 9 Aug 2018 16:10:06 +0000 Greg Chicares <address@hidden> wrote:

[A problem was observed in production. With lmi-20180717T1419Z, the
symptom was
  Assertion 'current_page == total_pages' failed.
  [group_quote_pdf_gen_wx.cpp : 807]
and with a later version
  $git describe --always
  49d9408c
(and probably with HEAD today) it was
  Assertion 'height <= get_total_height() - y' failed.
  [pdf_writer_wx.cpp : 313]

The same assertion failure was seen for a simple illustration
(apparently on the cover page):

>> just load the file and Ctrl-I. It matters that I'm using the
>> "hero dogs in outer space" company logo. If I rename that
>> temporarily, then I successfully produce a logo-free PDF.

We exchanged personal email in order to share attachments; now let's
continue on the public mailing list.]

> GC> >  [...] Would it be possible for me to get the
> GC> > data required to reproduce the original problem with the group premium
> GC> > quotes?
> GC> 
> GC> I've constructed an input file (attached archive) that is free of any
> GC> personally-identifiable information and proprietary data; it does
> GC> reproduce the original problem when I try to produce a quote from it
> 
>  Thanks, I can reproduce the problem using it too. And I even have a fix:
> unfortunately, it was another instance of an off by 1 error, this time we
> output too many rows per page compared to compute_pages_for_table_rows()
> calculation. I've created https://github.com/vadz/lmi/pull/90 with the fix
> but it's basically just this:
[... group_quote_pdf_gen_wx.cpp ...]
> -        if(last_row_y <= pos_y)
> +        // If there is no space left for another row, start a new page.
> +        if(last_row_y <= pos_y + table_gen.row_height())
[...]
> i.e. it's too late to start another page after outputting a row that
> already ends below last_row_y, we need to check for it during the previous
> loop iteration.

Okay, thanks, that solves the group-quote problem for me, whether I use
the proprietary graphics in production or the nonproprietary dog-hero
graphics here:
  $cp -a gwc/company_logo.png gwc/group_quote_banner.png /opt/lmi/data
I'll push this change presently.

>  I guess computing a number of rows effectively output per page and
> asserting that it's equal to compute_pages_for_table_rows() result wouldn't
> be a bad idea

That sounds like a great idea.

> but I am not sure how would you prefer to do it, so I don't
> propose a patch for it right now. Personally, I'd create a class
> encapsulating the pagination logic, but you were against using objects for
> things like this in the past, so I hesitate to do it without your explicit
> agreement.

I don't understand. We already have compute_pages_for_table_rows(),
which is just a member function that accesses no members of its
containing class except as const arguments. (I assume that the
pdf_writer& argument is non-const only because of some accident in
the design of wxPdfDoc; it's used only to call
  pdf_writer.get_vert_margin()
which is presumably a simple getter that doesn't affect its object's
state.) Thus, it's trivial to rewrite this as a free function:
  int compute_pages_for_table_rows
      (int  vert_margin
      ,int& remaining_space
      ,int  header_height
      ,int  row_height
      ,int  last_row_y
      )
I guess we'd make this a class only if it needed to maintain state,
but I don't see what state you'd want to include.

> I also still don't know what can we do to systematically guard
> against such off by one errors in row-by-row output code :-(

A free function
  int compute_pages_for_table_rows(int, int&, int, int, int);
can be divorced from library dependencies and exhaustively unit-tested
in isolation. That should suffice to make off-by-one errors nearly
impossible, and subtle mistakes easy to correct with confidence.

Row-by-row output can be transformed to monolithic-block output by
buffering the rows and then outputting the monolith. BTW, if we did
that, then instead of formatting columns to a presumed width based
on a mask like "999,999,999.99", we could use the width of the widest
element: that wouldn't really matter for group quotes, but it would
make it easier to fit a large set of columns on an illustration.

>  Also, please note that we could change compute_pages_for_table_rows() to
> assume that we can fit one more row per page instead of the patch above as,
> in principle, it does fit on the page and you might prefer more compact
> appearance that we have now. But the current page is simpler and seems more
> obviously correct.

Actually, group quotes print
  System version: 20180719T1706Z   Page 1 of 15
at the very bottom of the page, and we'd like to move that up by
about the font height to leave a slight margin at the bottom. We'd
ideally have blank spacing of about the font height between that
footer and the last line of data, where today we seem to have
about twice that much space (though that may depend on which
graphics files are used).

The logo on an illustration cover page, while showing the same
assertion failure, has a different explanation (and the patch
above doesn't fix it when I use the 513x160 space-dogs logo):

> GC> >  In the meanwhile, I'll try to fix at least the problem at hand, but I
> GC> > don't see any really nice solution to it yet. It seems to be impossible 
> to
> GC> > ensure that the footnote appears at the bottom of the cover page from 
> the
> GC> > MST template as it doesn't know anything about the page size. But we 
> can't
> GC> > easily use the regular footer logic, allowing to position the footers
> GC> > correctly on all the other pages, on this one neither. I wonder if you 
> see
> GC> > some clever solution that I'm missing by chance...
> GC> 
> GC> Are you saying that the space-dogs graphic is gigantic, and it
> GC> can't be scaled on the cover page, but it can be scaled on the
> GC> other pages?
> 
>  No, not at all. It's just that I hardcode too much blank vertical space in
> cover.mst and with this graphic _and_ non-empty StateMarketingImprimatur,
> as is the case for NASD illustrations, this pushes the latter off the page.

Okay, then I think you're saying that the MST file is designed to work
with a 333x40 logo, so, even though the 513x160 canine graphic isn't
enormous, it's rather taller than 40 pixels--but MST doesn't even know
the page height, so it can't adapt to logos of different heights.

Oh, and BTW, some illustrations have a cover page with no logo,
probably where the old XSL templates didn't have one, but with this
modernization we should rectify that anomaly: every cover page should
have a logo.

>  As for scaling, the reason the graphic has different sizes on the cover
> page and the other ones is just that it uses different scaling factors:
> 5/3 for the former and 9/25 for the latter (and the reason these particular
> scaling factors were used is just to have the same dimensions as in the
> XSL-FO-based illustrations).

The XSL was written with a casual mindset: "we know we have this one
parameter, so let's twiddle it until everything seems to work". Let's
step back and consider things more carefully. Does MST pretty much
constrain us to using an approximately fixed logo height? If so, can
we (in MST, or perhaps in HTML or in wx) scale the logo dynamically
so that it always fits no matter what the PNG dimensions are?