Re: [lmi] "Not enough room for even the first column" messagebox

[Greg Chicares]

On 2019-05-02 16:54, Vadim Zeitlin wrote:
> On Thu, 2 May 2019 01:20:24 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> The reason I was running the census described here:
> GC>   https://lists.nongnu.org/archive/html/lmi/2019-05/msg00002.html
> GC> is that the end user saw this messagebox:
> GC>   Not enough room for even the first column.
> GC>   [report_table.cpp : 132]
> GC> when trying to do
> GC>   Census | Print case to PDF
> 
>  Please see the bottom of this message for my hypothesis about why this
> happens and the suggestion for fixing it.

Let me describe my later research first, because it makes the questions
below easier to answer. I made a series of changes on a branch:

e5b9a61d (HEAD -> odd/zero_columns, origin/odd/zero_columns) Remove an 
unnecessary early-return statement
08a994b9 Comment early-return statement that are not in master
29925fd4 Explain a seeming anomaly
ced5a90c Suppress a misleading error message when no columns are requested
7c264373 Rewrite a couple of lines for clarity
8e2dd5d0 Don't circumvent an assertion that does not fail
b503329d Remove lines that served only to locate failures
f5a17905 Add a unit test for zero report columns
965d664e Investigate a reported defect
caf6f550 (origin/master, origin/HEAD, master) Guard against unit-test failures

so that the first branch commit:
  965d664e Investigate a reported defect
is littered with statements that show which lines were reached, in order
to localize the problem; and the last branch commit, e5b9a61d, removes
the clutter and tries to reduce differences vs. HEAD to a necessary
minimum, viz.:

* pdf_writer_wx.cpp: Relevant only to space-dog logos, for which I'll
  adopt your suggestion below.
* report_table_test.cpp: Added a new zero-column test that should
  probably be added to master, unless we decide that should be
  forbidden by an asserted constructor precondition.
* report_table.cpp: Split a test for an invariant
  - if one or more columns are requested, but even the first one
    doesn't fit, then that's an error indeed;
  - if zero columns are requested, then treat that as no error.
* wx_table_generator.cpp: Here are my real questions. To begin,
  here's a drastically simplified reproducible test case:

  File | New | Census
  Census | Edit cell
    "Reports" tab:
    check "Create supplemental report"
    OK
  Census | Add cell
  Census | Run case
  File | Print to PDF

As you had already figured that out, below, the problem is that a
supplemental report is requested, but with zero columns selected.
Whether such a request should be treated as an input problem far
upstream is a different issue that I'm not addressing yet.

With master, that test cases triggers assertions that prevent a
PDF from being produced. With odd/zero_columns HEAD, those
assertions are replaced by early function exits, so a PDF is
produced with a zero-column supplemental report. My question is
whether these early exits (1) are desirable, or (2) indicate
some problem where these functions are called. Specifically:

- is it okay to call
  wx_table_generator::output_horz_separator()
with arguments that violate 'begin_column < end_column'?

- is it okay to call
  wxRect wx_table_generator::text_rect()
with arguments that violate 'column < lmi::ssize(all_columns())'?

and, if the answer in either case is 'no', then what changes
should be made upstream to preclude those apparent precondition
violations?

> GC> I checked to see whether I could reproduce the problem more
> GC> easily by doing
> GC>   Census | Run cell
> GC>   File | Print to PDF
> GC> but that produced a different messagebox:
> GC>   Assertion 'height <= get_total_height() - y' failed.
> GC>   [pdf_writer_wx.cpp : 338]
> 
>  It would be really useful to have information about the current page when
> this assertion fails, but unfortunately it's inaccessible at this level. I
> wonder if we should consider adding try/catch statements around the calls
> to output_html() to add this information to the error messages. Or maybe we
> should pass the page name/identifier to pdf_writer::next_page() and use
> this in pdf_writer diagnostic messages?

That sounds like a good idea. I could have localized the zero-columns
issue faster if error messages had indicated which page was being
written.

I've tried passing context information to subroutines for use in
error reporting, and that has seemed cumbersome; besides, it's too
easy to forget to include that information when writing a diagnostic.
I tend to think the try-catch idea is better. It might be ideal to
wrap each logical (not necessarily physical) page in a try-block.

> GC> Because it's so much faster to run this case with x86_64-*-gcc-8.x,
> GC> I added some debugging code and rebuilt. The second messagebox
> GC> turned out to be a dogs-in-orbit issue:
> GC> 
> GC> [install_msw.sh]
> GC> for z in company_logo.png group_quote_banner.png ;
> GC>   do cp --archive /opt/lmi/src/lmi/gwc/$z /opt/lmi/data/ ;
> GC> done
> GC> 
> GC> That recently-added command unconditionally copies public PNG
> GC> files, to prevent problems with a brand-new installation. It
> GC> would be better to install proprietary files if available,
> GC> though, because the 'company_logo.png' with Ugolyok and
> GC> Veterok is larger. But any random person who builds lmi from
> GC> the savannah site alone should get a working system; would it
> GC> be possible to resize that file dynamically if it is larger
> GC> than the maximum size implicit in our PDF generation?
> 
>  Of course it would, but I wonder how/why is it preferable to just resizing
> (or cropping) these files statically? I assume we know the maximum size, as
> it's the size used by the proprietary logo, so it doesn't seem necessary to
> do it dynamically, does it? The only thing I'd consider doing in the code
> would be to check that the logo is not too big.

Yes, that's better. I'll do that. Thanks.

> GC> The original problem isn't solved by copying proprietary PNG
> GC> files from a production release. Here's the code I added
> GC> (for both issues):
> [...snip reasonably looking debugging code...]

[...which more or less became branch odd/zero_columns ...]

>  AFAICS this can only happen if standard_supplemental_report is used, but
> there are no columns in it.

Exactly. Hence, the simple test case above.

> And looking at the code, it doesn't seem to be
> impossible that this is what happens, i.e. I don't see what prevents
> SupplementalReport from being 1, but all SupplementalReportColumnsNamesN
> from being set to "[none]". If this is indeed what happens, we need to
> add some logic for setting SupplementalReport to 0 if no supplemental
> report columns are actually specified. Unfortunately I don't know what
> would be the best place to do it and while it could be done in
> pdf_command_wx.cpp (please let me know if you'd like me to do it), I'm
> almost sure that this is not the ideal place for this. Would you have a
> better idea?

Yes. I haven't yet worked out exactly how to do it, but I do know
where to do it, and it's upstream of any PDF code.