Footnotes in the float branch

[Luca Furini <lf...@cs.unibo.it>]

Hi all

I recently had the time (and the pleasure) to look at before-float 
implementation branch, and I played a bit with it.

I focused on the handling of footnotes, as I noticed that sometimes they 
were placed on a page following their citations without a real necessity 
to do it; as I wrote some time ago (and I rememeber there was some 
consesuns on this) this behaviour is acceptable for before floats, but is 
probably not what a user would expect for footnotes.

I have tried to fix this in the PageBreakingAlgorithm, computing a 
"minimum required index" for footnotes, so that no page break will be 
considered that unnecessarily defers some old footnotes to the next page.

I'm attaching a diff file showing the changes (or maybe should I just 
apply it?); after applying the patch, there are 4 more passing testcases 
(foonote_footnote-separator, footnote_large, footnote_positioning_{4,5}) 
and no regressions. Testcases footnote_positioning_{2,3} still generate 
some run-time exception, and in the next days I'm going to see what's 
wrong with them.

I add just a few comments about the new classes: I must admit that it took 
me a while to see and understand the interaction between the 
PageBreakingAlgorithm and the Footnotes / BeforeFloats Record, together 
with their inner Footnotes / BeforeFloats Progress.

In particular, at the beginning I thought the *Progress classes were just 
convenience classes to get "pieces" of footnotes and floats without 
directly fiddling with element lists, and I found only later that their 
methods can actually create new active nodes.

Another thing that I find a bit strange is that the PageBreakingAlgorithm 
does not directly interact with the before floats, as the calls to 
BeforeFloatsProgress.consider() are "hidden" in the FootnotesProgress 
class.

So, I was wondering whether it wouldn't be more "clear" to have the 
PageBreakingAlgorit control all the node creation logic, after having 
accessed information about footnotes and floats that could be placed in 
the page via the helper classes.

WDYT?

Regards
     Luca

Re: Footnotes in the float branch

[Luca Furini <lf...@cs.unibo.it>]

On Mon, 26 Mar 2007, Luca Furini wrote:

> I'm attaching a diff file showing the changes ....

Well, *now* I'm attaching bla bla :-)

Regards
     Luca

Re: Footnotes in the float branch

[Vincent Hennebert <vi...@anyware-tech.com>]

Grmblmblm... and the attached fo file, of course...

Vincent

Vincent Hennebert a écrit :
> Hi Luca,
> 
> Luca Furini a écrit :
>> Hi all
>>
>> I recently had the time (and the pleasure) to look at before-float
>> implementation branch, and I played a bit with it.
>>
>> I focused on the handling of footnotes, as I noticed that sometimes they
>> were placed on a page following their citations without a real necessity
>> to do it; as I wrote some time ago (and I rememeber there was some
>> consesuns on this) this behaviour is acceptable for before floats, but
>> is probably not what a user would expect for footnotes.
>>
>> I have tried to fix this in the PageBreakingAlgorithm, computing a
>> "minimum required index" for footnotes, so that no page break will be
>> considered that unnecessarily defers some old footnotes to the next page.
>>
>> I'm attaching a diff file showing the changes (or maybe should I just
>> apply it?); after applying the patch, there are 4 more passing testcases
>> (foonote_footnote-separator, footnote_large, footnote_positioning_{4,5})
>> and no regressions. Testcases footnote_positioning_{2,3} still generate
>> some run-time exception, and in the next days I'm going to see what's
>> wrong with them.
>>
>> I add just a few comments about the new classes: I must admit that it
>> took me a while to see and understand the interaction between the
>> PageBreakingAlgorithm and the Footnotes / BeforeFloats Record, together
>> with their inner Footnotes / BeforeFloats Progress.
>>
>> In particular, at the beginning I thought the *Progress classes were
>> just convenience classes to get "pieces" of footnotes and floats without
>> directly fiddling with element lists, and I found only later that their
>> methods can actually create new active nodes.
> 
> Actually all the node creation logic lies in the ActiveNodeRecorder
> class (handleNode method). But it is true that the *Progress classes
> make calls to this method and that it may be confusing.
> 
> 
>> Another thing that I find a bit strange is that the
>> PageBreakingAlgorithm does not directly interact with the before floats,
>> as the calls to BeforeFloatsProgress.consider() are "hidden" in the
>> FootnotesProgress class.
> 
> Yes and that's unfortunate. I realized that only later on. The rationale
> was to extract the handling of footnotes and before-floats from the
> PageBreakingAlgorithm class, which is already large enough, and to have
> dedicated classes for that. Among other things PageBreakingAlgorithm
> wouldn't have to bother adding the footnote/before-float separator or
> not, when, etc.
> 
>> So, I was wondering whether it wouldn't be more "clear" to have the
>> PageBreakingAlgorit control all the node creation logic, after having
>> accessed information about footnotes and floats that could be placed in
>> the page via the helper classes.
> 
> Yes, something like that. Eventually we would have a layout engine
> taking elements from several flows (normal flow, footnotes,
> before-floats, side-floats...) and wiring all those flows in a proper
> way.
> 
> 
>> WDYT?
> 
> I had a look at your patch and have several comments:
> - I see you re-enabled the noBreakBetween method; I don't think it's
>   a good solution because it artificially prevents some nodes to be
>   created, which even if bad may be necessary for some complex
>   documents. See for example the attached fo file. I also documented
>   a similar problem on the wiki [1]. While it makes the testcases work
>   it actually creates some bad layout in other cases.
> - I'm a bit reluctant about the newFootnotesCount field as it
>   re-introduces footnotes-related code in the PageBreakingAlgorithm.
>   Likewise, the findMinimumFootnoteIndex shouldn't be in the
>   PageBreakingAlgorithm class, if any.
> - there are checkstyle issues in your patch (tab characters, lines too
>   long, missing javadocs)
> 
> My feeling is that the Knuth algorithm can nicely handle such problems
> already as is. It's just a matter of defining the right demerits for
> deferred footnotes, and give a chance to too-short nodes with
> non-deferred footnotes to be considered WRT normal nodes with deferred
> ones. I seem to remember that there was also a problem with flushing
> floats on the last page (footnotes were unnecessarily deferred). I'd
> have to dig deeper into that. I'll try to illustrate my ideas in a patch
> in the next days.
> 
> 
> Cheers,
> Vincent
> 
> [1]
> http://wiki.apache.org/xmlgraphics-fop/GoogleSummerOfCode2006/FloatsImplementationProgress/ImplementingBeforeFloats#head-40ade416f873071dac75bd80dbd57c592efa3277
> 

Re: Footnotes in the float branch

[Vincent Hennebert <vi...@anyware-tech.com>]

Hi Luca,

Luca Furini a écrit :
> Hi all
>
> I recently had the time (and the pleasure) to look at before-float
> implementation branch, and I played a bit with it.
>
> I focused on the handling of footnotes, as I noticed that sometimes they
> were placed on a page following their citations without a real necessity
> to do it; as I wrote some time ago (and I rememeber there was some
> consesuns on this) this behaviour is acceptable for before floats, but
> is probably not what a user would expect for footnotes.
>
> I have tried to fix this in the PageBreakingAlgorithm, computing a
> "minimum required index" for footnotes, so that no page break will be
> considered that unnecessarily defers some old footnotes to the next page.
>
> I'm attaching a diff file showing the changes (or maybe should I just
> apply it?); after applying the patch, there are 4 more passing testcases
> (foonote_footnote-separator, footnote_large, footnote_positioning_{4,5})
> and no regressions. Testcases footnote_positioning_{2,3} still generate
> some run-time exception, and in the next days I'm going to see what's
> wrong with them.
>
> I add just a few comments about the new classes: I must admit that it
> took me a while to see and understand the interaction between the
> PageBreakingAlgorithm and the Footnotes / BeforeFloats Record, together
> with their inner Footnotes / BeforeFloats Progress.
>
> In particular, at the beginning I thought the *Progress classes were
> just convenience classes to get "pieces" of footnotes and floats without
> directly fiddling with element lists, and I found only later that their
> methods can actually create new active nodes.

Actually all the node creation logic lies in the ActiveNodeRecorder
class (handleNode method). But it is true that the *Progress classes
make calls to this method and that it may be confusing.


> Another thing that I find a bit strange is that the
> PageBreakingAlgorithm does not directly interact with the before floats,
> as the calls to BeforeFloatsProgress.consider() are "hidden" in the
> FootnotesProgress class.

Yes and that's unfortunate. I realized that only later on. The rationale
was to extract the handling of footnotes and before-floats from the
PageBreakingAlgorithm class, which is already large enough, and to have
dedicated classes for that. Among other things PageBreakingAlgorithm
wouldn't have to bother adding the footnote/before-float separator or
not, when, etc.

> So, I was wondering whether it wouldn't be more "clear" to have the
> PageBreakingAlgorit control all the node creation logic, after having
> accessed information about footnotes and floats that could be placed in
> the page via the helper classes.

Yes, something like that. Eventually we would have a layout engine
taking elements from several flows (normal flow, footnotes,
before-floats, side-floats...) and wiring all those flows in a proper
way.


> WDYT?

I had a look at your patch and have several comments:
- I see you re-enabled the noBreakBetween method; I don't think it's
  a good solution because it artificially prevents some nodes to be
  created, which even if bad may be necessary for some complex
  documents. See for example the attached fo file. I also documented
  a similar problem on the wiki [1]. While it makes the testcases work
  it actually creates some bad layout in other cases.
- I'm a bit reluctant about the newFootnotesCount field as it
  re-introduces footnotes-related code in the PageBreakingAlgorithm.
  Likewise, the findMinimumFootnoteIndex shouldn't be in the
  PageBreakingAlgorithm class, if any.
- there are checkstyle issues in your patch (tab characters, lines too
  long, missing javadocs)

My feeling is that the Knuth algorithm can nicely handle such problems
already as is. It's just a matter of defining the right demerits for
deferred footnotes, and give a chance to too-short nodes with
non-deferred footnotes to be considered WRT normal nodes with deferred
ones. I seem to remember that there was also a problem with flushing
floats on the last page (footnotes were unnecessarily deferred). I'd
have to dig deeper into that. I'll try to illustrate my ideas in a patch
in the next days.


Cheers,
Vincent

[1]
http://wiki.apache.org/xmlgraphics-fop/GoogleSummerOfCode2006/FloatsImplementationProgress/ImplementingBeforeFloats#head-40ade416f873071dac75bd80dbd57c592efa3277