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

[Greg Chicares]

[replying to
  https://lists.nongnu.org/archive/html/lmi/2019-05/msg00011.html
for those reading the html archives]

On 2019-05-06 23:12, Vadim Zeitlin wrote:
> 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.

Resolved now. It was an upstream logic error. When combining cell
parameters to produce composite parameters, the rule had been
  SupplementalReport := true if it's true for any cell
  SupplementalReportColumn[0..12] := its value for the last cell
which, in the scenario above, calls for a supplemental report to
be produced with zero columns.

Commit f7ad61cb5b seems a lot cleaner than my original idea:

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/ledger.cpp b/opt/lmi/stash/ledger.cpp
index 79f4aaa5..f0c78dcd 100644
--- a/ledger.cpp
+++ b/opt/lmi/stash/ledger.cpp
@@ -245,6 +245,25 @@ Ledger& Ledger::PlusEq(Ledger const& a_Addend)
         ++this_i, ++addend_i;
         }
 
+    bool const are_any_columns_selected =
+           !ledger_invariant_->SupplementalReportColumn00.empty()
+        || !ledger_invariant_->SupplementalReportColumn01.empty()
+        || !ledger_invariant_->SupplementalReportColumn02.empty()
+        || !ledger_invariant_->SupplementalReportColumn03.empty()
+        || !ledger_invariant_->SupplementalReportColumn04.empty()
+        || !ledger_invariant_->SupplementalReportColumn05.empty()
+        || !ledger_invariant_->SupplementalReportColumn06.empty()
+        || !ledger_invariant_->SupplementalReportColumn07.empty()
+        || !ledger_invariant_->SupplementalReportColumn08.empty()
+        || !ledger_invariant_->SupplementalReportColumn09.empty()
+        || !ledger_invariant_->SupplementalReportColumn10.empty()
+        || !ledger_invariant_->SupplementalReportColumn11.empty()
+        ;
+    ledger_invariant_->SupplementalReport =
+           ledger_invariant_->SupplementalReport
+        && are_any_columns_selected
+        ;
+
     LMI_ASSERT(this_i == l_map_rep.end() && addend_i == lm_addend.end());
 
     return *this;
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

(which I post here only because I'm about to erase it, and if I do so
without copying it somewhere, I'm sure to wish I hadn't).

>  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.

Done in commit 0fcb08d5b5.

> 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?

Correct: it isn't. The desirable behavior in this case is to
inhibit the composite's supplemental report.

But there's an auxiliary goal: to produce diagnostics that make
it easier to find out where problems like this one arise. Here,
it would have been most helpful to know that the problem arose
while attempting to generate the supplemental-report pages, as
discussed last month:

> 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:

It's not too late.

> 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]

Okay, those are good points.

> 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.

Okay: if we happen to forget to emit the context information, that's a
minor shortcoming--which we can easily fix when we notice it later.

My only objection here is to the macro. If adding a pdf-specific macro
here is a good idea, then other sections of lmi should have their own
section-specific macros, and we'll have lots of macros. But I don't
think we want macros to proliferate. Couldn't the extra information be
passed by reusing the existing LMI_ASSERT_WITH_MSG macro, perhaps
enhancing it in a general way to take an optional "CONTEXT" parameter?

> 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.

Furthermore, adding try-catch is a structural change that makes it
harder to follow the control flow. Not necessarily much harder, but
at least a little harder. A related objection is that it introduces
another indentation level.

>  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

You've made persuasive arguments for the approach you favor.
Let's go with that.

> 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).

Would you like to give it a try when you have nothing more urgent to do?