lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] First version of the illustrations pagination patch


From: Vadim Zeitlin
Subject: [lmi] First version of the illustrations pagination patch
Date: Sat, 23 Jun 2018 23:35:27 +0200

 Hello,

 Here is the long-promised version of the pagination patch:

                https://github.com/vadz/lmi/pull/86

 As previously mentioned, these changes require updating both wxWidgets and
wxPdfDocument libraries, so the first 2 commits do just that. The following
ones do some simplifications/improvements/refactorings before implementing
the pagination logic in

https://github.com/vadz/lmi/pull/86/commits/9e2b2ba14662a49805e5bf114e669bdea3171f86

and further improving it in

https://github.com/vadz/lmi/pull/86/commits/8f011a168bfe8a79395e0bfe90a03a8169095d95

 The diff is not very clear in respect to the changes to the .mst files but
all that happened here was

https://github.com/vadz/lmi/pull/86/commits/d04ce1e50924e9153c238f8ccef6870a0ab1a3ed

which merged the previously distinct reg_d_indiv_notes{1,2}.mst as they
were only separate files before because of the lack of proper pagination
support. But after merging these two, the old reg_d_indiv_notes3.mst, which
does need to begin on its own new page, and so shouldn't be merged with the
other ones, was renamed to reg_d_indiv_notes2.mst to keep the numbering
consecutive, and this, unfortunately, has made the diff less readable.
BTW, I thought about merging nast_notes{1,2}.mst too, but it seems like
those also really need to be on separate pages, so I finally didn't do this
neither -- please let me know if I was wrong.


 As the PR description mentions, there is one known problem: the header is
not repeated on the continuation pages so we don't get the logo nor the
"Explanatory Notes" title on the second (or subsequent) pages currently.
This will be fixed soon by implementing support for the <header> tag, but I
haven't completely finished this just yet.


 Another thing that we discussed before was having tests for the pagination
logic but this PR doesn't include any. The reason for this is that the real
implementation of pagination is inside wxHtmlDCRenderer and wxHTML core
(i.e. wxHtmlCell-derived classes) itself and it isn't really simple to test
them from lmi code. I did add some unit tests for this API to wxWidgets
itself, even though they don't really exercise all of this code neither. I
did try to find some way of adding useful tests to lmi itself, but couldn't
really think of anything, IMO the best (and the only really useful) test is
just generating the illustrations.


 Finally, I'm also somewhat bothered by the inefficiency of the new code,
as it parses the same HTML text multiple times, instead of doing it only
once and then reusing its internal representation. Unfortunately, the
latter is not easy (or perhaps even possible) to do right now due to wxHTML
API limitations and I'd need to make more changes to wxWidgets in order to
allow doing this in a nice way and, again, I just didn't have time to do it
yet, especially because this was lower priority than <header> support. And
perhaps you don't consider this a[n important] problem at all.


 After mentioning all the problems with this PR, I probably should also say
that, on the positive side, it does seem to work fine for me: I've tested
both the group quotes generation (which should be unchanged, and seems to
be) and the illustration generation, including using the sample2ipp
product which previously resulted in truncated "PolicyYearFootnote", but
now just creates an extra page instead.

 Please let me know if you have any comments on the new code or find any
problems with it.

 Thanks in advance,
VZ


reply via email to

[Prev in Thread] Current Thread [Next in Thread]