Re: [lmi] PATCH: simplify attachment page handling

[Vadim Zeitlin]

On Sat, 26 May 2018 23:54:00 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-05-26 09:26, Vadim Zeitlin wrote:
GC> > 
GC> >  I'm a bit afraid of having conflicts in PDF generation code in the 
future,
GC> > as you're making sweeping changes to it while I'm trying to add pagination
GC> > functionality to it at the same time, so I'd like to start submitting 
parts
GC> > of my work even although it's not complete yet.
GC> 
GC> That's great news.

 To give more details about this, pagination basically works now, but there
is one big problem: the headers. Currently they're not repeated on the
subsequent pages and I need to do something about it, but I still don't
know what yet.

GC> While you've been busy elsewhere, I have been making some broad
GC> changes, but I'm just about done with this round of them. My remaining
GC> goals for the short term are:
GC> 
GC> (1) to rework wx_table_generator::compute_column_widths(), perhaps as a
GC> free function that can be unit-tested, much as page_count() is extensively
GC> tested by test_page_count(); and

 Yes, seeing how much more complex it has become, it looks like a good
idea.

GC> (2) to enable '-Wconversion', which we last discussed here:
GC>   https://lists.nongnu.org/archive/html/lmi/2018-03/msg00023.html

 This would be nice to have too, especially if it doesn't require too much
effort.

GC> Maybe I should focus on (1), because (2) is likely to lead to sweeping
GC> changes.

 I'm not that worried about these changes conflicting with the changes in
{ledger_pdf_generator,pdf_writer}_wx.cpp, where there are few such warnings
(just a single one in the former), but this will almost certainly result in
some conflicts in my use-std-fs branch, as you will have to deal with
-Wconversion warnings in Boost.Filesystem headers, probably by wrapping
them inside the GCC diagnostic push/pop pragmas, while I have changed the
same lines in that branch. But these conflicts should be relatively simple
to resolve... 

 BTW, I'll also need to correct the (tons of) remaining -Wconversion
warnings in wxWidgets headers if you want to do this for all lmi files as
there are still plenty of them left, I don't know why did the message
linked to above only mentions the couple of warnings about double-to-int
casts when g++ 7 gives a couple of dozens more of them for casts from long
to int in the same header.


GC> >  The first one is rather trivial but I'm not sure what will you think 
about
GC> > it. On one hand, you might love it because it gets rid of a template and
GC> > makes the code simpler. OTOH you might disagree with it because it
GC> > sacrifices conceptual purity (it pretends that attachment page IS-A
GC> > numbered page, which is not really true) at the altar of convenience.
GC> 
GC> Cherry-picked and pushed.

 Thanks!


GC> Not long ago, I looked for examples of inheritance in lmi, and found
GC> that I've hardly used it except for a few mix-ins and some auxiliary
GC> facilities that are essentially independent libraries. Inheritance is
GC> seductive, but it's not plastic: it's like carving stone rather than
GC> molding clay. If we start out with a perfect, ageless hierarchy that
GC> can never change (e.g., real numbers <-- complex numbers), inheritance
GC> can be an elegant way of embodying it in code. But it's difficult to
GC> refactor when the vision is imperfect or impermanent, say, when a
GC> business requirement changes. I mention this now only to explain why
GC> I don't think the conceptual purity of an inheritance graph like this
GC> was ever going to be conserved.

 I think the current (well, not even so current any more, which probably
means that it's going to ebb soon) whiplash against inheritance goes too
far. It's definitely not the cure of all ills, but it still can be the best
way to abstract the differences between a family of related objects while
allowing to reuse the code between them. In ledger_pdf_generator_wx.cpp
code I think it's the simplest solution allowing to handle the pages mostly
generically while still accounting for the differences between them.

 And I don't think any considerations of conceptual purity are really
relevant here, there should be no problem with adding virtual methods or,
maybe, changing the existing ones (although IME this should happen much
more rarely) as well as adding classes or whole branches to the inheritance
trees if needed. It's normal for the code to change and OO allows to handle
these changes better than purely procedural code.


GC> >  The patch is at https://github.com/vadz/lmi/pull/84 and the commit 
message
GC> > there should, hopefully, describe it clearly. One thing I don't really say
GC> > in it is that the real motivation is that numbered_page::get_extra_pages()
GC> > will take place of pagination in the upcoming commits
GC> 
GC> As mentioned above, page_count() is unit-tested by test_page_count(),
GC> and I believe such unit testing is extremely valuable. Will the new
GC> approach be similarly amenable to unit testing?

 Not really. The method itself is pretty trivial:

---------------------------------- >8 --------------------------------------
    int get_extra_pages_needed
        (Ledger const&              ledger
        ,pdf_writer_wx&             writer
        ,html_interpolator const&   interpolate_html
        ) override
    {
        page_break_positions_ = writer.paginate_html
            (writer.get_page_width()
            ,get_footer_top() - writer.get_vert_margin()
            ,interpolate_html.expand_template(page_template_name_)
            );

        // The cast is safe, we're never going to have more than INT_MAX
        // pages and if we, somehow, do, the caller checks that this function
        // returns a positive value.
        return static_cast<int>(page_break_positions_.size()) - 1;
    }
---------------------------------- >8 --------------------------------------

and while you could think that the real logic is inside paginate_html(),
it's quite simple as well as it uses wxHTML classes to do real pagination.
These classes do have some unit tests (which I've added while streamlining
them a little), but these tests are, of course, not lmi-specific.

 It's not obvious if we can/should insert some unit tests between wxHTML
and lmi code here. I think it would be relatively complicated (how to check
that we get the correct results from paginate_html()?), without being
commensurately useful. I'd rather concentrate on integration tests and
check that running known illustrations produces the (previously checked)
known outputs or something like this.


GC> > and so, if we want to be able to paginate the attachment page as well
GC> > (and I think we do; please correct me if I'm wrong)
GC> 
GC> I think we'd better assume that pagination must work for "attachment"
GC> pages as for any other type of page.

 This is what I did, thanks for confirming it.

GC> I just ran a 'sample2_naic' illustration, and on the last page (the page
GC> with "Certification Statements"), the "AGENT OR AUTHORIZED REPRESENTATIVE"
GC> text almost touches the bluish line that separates it from the footer. If
GC> evolving business requirements require more to be printed on this virtual
GC> page, it will need to become two physical pages. We can't rule that out
GC> because business requirements can be capricious, even though the motivating
GC> idea was to produce one single sheet of paper that a customer would sign
GC> with a pen.

 It would also presumably be needed to avoid page breaks in the middle of
this section, wouldn't it? This is not implemented yet (but hopefully will
be soon), but I plan to allow using CSS page-break-inside property in
wxHTML for this, e.g. you'd be able to use

        <div style="page-break-inside: avoid">
                <p>
                AGENT / AUTHORIZED REPRESENTATIVE<br>
                ...
        </div>

to prevent any page breaks inside this section (unless its height is
greater than the page size...). I hope this should be enough to achieve the
wanted layout in all cases, but please let me know if you see any cases
that would not be covered by this.
                
 Thanks in advance,
VZ