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

[Vadim Zeitlin]

On Mon, 6 May 2019 12:45:08 +0000 Greg Chicares <address@hidden> wrote:

GC> * wx_table_generator.cpp: Here are my real questions. To begin,
GC>   here's a drastically simplified reproducible test case:
GC> 
GC>   File | New | Census
GC>   Census | Edit cell
GC>     "Reports" tab:
GC>     check "Create supplemental report"
GC>     OK
GC>   Census | Add cell
GC>   Census | Run case
GC>   File | Print to PDF
GC> 
GC> As you had already figured that out, below, the problem is that a
GC> supplemental report is requested, but with zero columns selected.
GC> Whether such a request should be treated as an input problem far
GC> upstream is a different issue that I'm not addressing yet.

 But this seems rather crucial to know because, as far as the PDF
generation code is concerned, the changes to it depend on the answer to
this question:

- If this is allowed, PDF code should handle this gracefully and skip
  generating the supplemental report page instead of generating a
  nonsensical table without any columns.

- If this is not allowed, then asserting in the PDF code doesn't seem to
  be a bad reaction to it actually happening, and just the assert error
  message would need to be improved.

GC> With master, that test cases triggers assertions that prevent a
GC> PDF from being produced. With odd/zero_columns HEAD, those
GC> assertions are replaced by early function exits, so a PDF is
GC> produced with a zero-column supplemental report.

 This still isn't the desirable outcome however, is it?

GC> My question is
GC> whether these early exits (1) are desirable, or (2) indicate
GC> some problem where these functions are called. Specifically:
GC> 
GC> - is it okay to call
GC>   wx_table_generator::output_horz_separator()
GC> with arguments that violate 'begin_column < end_column'?
GC> 
GC> - is it okay to call
GC>   wxRect wx_table_generator::text_rect()
GC> with arguments that violate 'column < lmi::ssize(all_columns())'?
GC> 
GC> and, if the answer in either case is 'no', then what changes
GC> should be made upstream to preclude those apparent precondition
GC> violations?

 I think the answer to both questions is indeed "no" because I can't
imagine any situation in which doing it would make sense. As for the
changes to be done, IMO it's clear that we should just skip over generating
the table entirely if it doesn't have any columns.

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

 The more I think about this, the more I like the idea with passing the
page name to pdf_writer_wx::next_page() and I'd like to address your
criticisms of it in case it's not too late yet to convince you of its
superiority:

GC> I've tried passing context information to subroutines for use in
GC> error reporting, and that has seemed cumbersome;

 Here it wouldn't because we have to do it in one place only, when calling
next_page(). [digression: this might require changing this method to
start_page() and calling it for the first page too, but this could actually
be a good thing on its own merit anyhow]

GC> besides, it's too easy to forget to include that information when
GC> writing a diagnostic.

 True, but if all the existing checks in pdf_writer_wx.cpp written in a way
which includes it (I think of some private LMI_PDF_WRITER_ASSERT macro), it
would take an effort to fail to notice this when adding another check, so
we could mitigate it.

GC> I tend to think the try-catch idea is better. It might be ideal to
GC> wrap each logical (not necessarily physical) page in a try-block.

 Also, it's much easier to add extra diagnostics to each function as you
write (or modify) it than to add try/catch adding the extra diagnostics to
every place from which some function could be called.

 To summarize, I think doing this inside pdf_writer_wx itself would be more
robust and at least as simple as doing it outside of it (and I suspect that
it actually will be quite a bit easier). But doing it at all is even more
important than doing it in the best way, so just please let me know how
would you prefer to do it and let's implement it in some way because
getting the diagnostics with incomplete information is quite aggravating
and I'd like to fix this (even if this is obviously not urgent).

 Regards,
VZ