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

[Vadim Zeitlin]

On Mon, 20 Aug 2018 14:02:17 +0000 Greg Chicares <address@hidden> wrote:

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

 Thanks! Just for the record, this fix was merged as commit
48f47f6a7dc0f5630826d550553e4b1faa13652e

GC> >  I guess computing a number of rows effectively output per page and
GC> > asserting that it's equal to compute_pages_for_table_rows() result 
wouldn't
GC> > be a bad idea
GC> 
GC> That sounds like a great idea.
GC> 
GC> > but I am not sure how would you prefer to do it, so I don't
GC> > propose a patch for it right now. Personally, I'd create a class
GC> > encapsulating the pagination logic, but you were against using objects for
GC> > things like this in the past, so I hesitate to do it without your explicit
GC> > agreement.
GC> 
GC> I don't understand.

 IMO the original problem arose because compute_pages_for_table_rows() and
the code modified by 48f47f6a7dc0f5630826d550553e4b1faa13652e mentioned
above used different logic for computing the number of rows per page.
IM-further-O the best way to prevent problems like this from occurring in
the first place is to use only a single way of doing this, in a single
place. I.e. have an object encapsulating this logic and delegating to the
derived classes for doing whatever else needs to be done for each row
(either nothing in compute_pages_for_table_rows() case or actually
outputting it). This is called "template method" pattern, but, big words
aside, this is the best and simplest way that I know to guarantee
consistency between estimating something and actually doing it.

GC> We already have compute_pages_for_table_rows(),
[...]

 Yes, we can refactor it to make it a global function, but I don't see what
will we gain from doing this. After all, the problem we're discussing
wasn't due to anything done by compute_pages_for_table_rows(), it was
already perfectly correct. It's the code using it elsewhere that wasn't.

GC> > I also still don't know what can we do to systematically guard
GC> > against such off by one errors in row-by-row output code :-(
GC> 
GC> A free function
GC>   int compute_pages_for_table_rows(int, int&, int, int, int);
GC> can be divorced from library dependencies and exhaustively unit-tested
GC> in isolation. That should suffice to make off-by-one errors nearly
GC> impossible, and subtle mistakes easy to correct with confidence.

 No, sorry, this is just not true. There were no off-by-one errors in this
function in the first place.

GC> Row-by-row output can be transformed to monolithic-block output by
GC> buffering the rows and then outputting the monolith.

 Sorry, I don't see what do you mean by this. You still have to insert page
breaks somewhere.

 I know we've already discussed this in the past, but I still have an
impression that you underestimate the benefit of centralising all the logic
in a single place with the "template method" approach. Yes, it does require
you to use either OO, with derived classes for each use of the base class,
or lambdas (preferable nowadays), but IMO it's absolutely not such a huge
disadvantage as you seem to believe. While the advantage is real, as we
keep seeing again and again, with bugs due to the differences in the "same
logic" implemented in different places.

 And if you're really, really averse to the inversion of control that comes
with using this pattern, we could also refactor it in a different way by
defining an iterator-like class (other languages call such classes
"generators"), that could be queried for the next row or page break. AFAICS
this would be much heavier and less clear, but it would preserve the
existing structure of code.

 But in any case, we do either need an object to store the state or keep
this state in a function, but then use callbacks. If you'd like to do, I
could show either pseudo-code or actual patches implementing either or both
of these approaches, so that you could check if you still dislike them as
much as you did before (and even if using them would have allowed to avoid
this bug...).


GC> >  Also, please note that we could change compute_pages_for_table_rows() to
GC> > assume that we can fit one more row per page instead of the patch above 
as,
GC> > in principle, it does fit on the page and you might prefer more compact
GC> > appearance that we have now. But the current page is simpler and seems 
more
GC> > obviously correct.
GC> 
GC> Actually, group quotes print
GC>   System version: 20180719T1706Z   Page 1 of 15
GC> at the very bottom of the page, and we'd like to move that up by
GC> about the font height to leave a slight margin at the bottom.

 This is not a problem at all, we just need to add more space to *pos_y
in group_quote_pdf_generator_wx::output_footer().

GC> We'd
GC> ideally have blank spacing of about the font height between that
GC> footer and the last line of data, where today we seem to have
GC> about twice that much space (though that may depend on which
GC> graphics files are used).

 Yes, it does. With the current code, we will have between 1 and 2 rows of
blank space between the data and the footer. We could change this to be
between 0 and 1.


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

 Yes, indeed.

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

 Yes.

GC> The XSL was written with a casual mindset: "we know we have this one
GC> parameter, so let's twiddle it until everything seems to work". Let's
GC> step back and consider things more carefully. Does MST pretty much
GC> constrain us to using an approximately fixed logo height?

 No, it imposes no such constraints.

GC> If so, can we (in MST, or perhaps in HTML or in wx) scale the logo
GC> dynamically so that it always fits no matter what the PNG dimensions
GC> are?

 Yes we could, but this will look quite bad unless the dimensions are not a
multiple of the size. For the files typically used as logos, which
typically contain text and other sharp lines, scaling by arbitrary factor
results in very noticeable blurriness and it's better to avoid it. If we
really want to impose a fixed size, it would almost certainly be better to
ask for an image file of this size and tell users to produce their logos in
an image editing program that can scale (or crop) the image better than the
relatively simple algorithms implemented in wxImage::Scale().

 Regards,
VZ