On 13.06.2006 17:58:11 Vincent Hennebert wrote:
> Hi,
> 
> > ------- Additional Comments From [EMAIL PROTECTED]  2006-06-12 12:45 -------
> > (In reply to comment #0)
> > 
> >>This patch isn't really meant to be applied... Rather to be reviewed by
> >>interested parties to check if I'm not wrong. Changelog:
> >>* javadocs for the Knuth line- and page-breaking algorithms. Some items are
> >>marked with double question marks because I haven't found out yet what is 
> >>their
> >>purpose. I will probably find eventually, but if anybody has immediate hints
> >>they will be welcome.
> > 
> > 
> > KnuthBlockBox:
> > bpdim seems to be used in concert with the proprietary display-align="fill"
> > value Luca implemented. See AbstractBreaker.optimizeLineLength(). If I
> > understand it right it is somehow used to make sure all the pages have more 
> > or
> > less the same amount of content (in bpd).
> 
> OK. Actually this is the natural width (without stretching nor
> shrinking) of the line represented by this box. This field apparently
> exists because it isn't possible to get the min/opt/max values stored in
> a MinOptMax object. Otherwise it could be retrieved from the opt of the
> ipdRange field. It may perhaps be useful to add such methods to
> MinOptMax?

Aha. But then we'd better rename the variable to a more speaking name.
MinOptMax should only be used if we also use the min and max values
because simple types are generally favored over objects (speed/memory).

> > pos and bAux are defined in ListElement/KnuthElement.
> 
> Hmmm. Does a Position object represent the index of the Knuth element
> (here KnuthBlockBox) in the sequence managed by the corresponding LM?

Yes. The Position is used so the LM knows later what part of the FO to
paint in addAreas().

> What does it mean that a box is auxiliary?

It's just a special marker that is used in certain areas. See:
http://www.nabble.com/Re%3A-Knuth-algorithm-problem-p1045573.html

> 
> > BreakingAlgorithm:
> > alignment: EN_BEFORE is not used. EN_START is used instead, since the class 
> > is
> > used in both ipd and bpd. EN_BEFORE is mapped into EN_START. Actually, 
> > alignment
> > uses a slightly different set of value than the FO properties, so reusing 
> > the
> > integer constants may not be the best thing, but we're not under Java 5, 
> > yet,
> > where we could use enums.
> > bFirst is used for the text-indent property so only the first paragraph of a
> > block is indented. See block_text-indent.xml.
> 
> You probably mean the first /line/ of a block?

No, I mean paragraph. You can have multiple paragraphs in a block if
line breaks survive or you have nested block-level elements.

> 
> > partOverflowRecovery is used in page breaking to defer an element which 
> > would
> > overflow the available BPD to the next page if it's the only element in a 
> > part
> > (=line or page).
> 
> I'm still a bit unsure of what 'part' means in the javadoc of
> BreakingAlgorithm.isPartOverflowRecoveryActivated. A line/page? A
> word/block? A Knuth box?

A line or page depending on the direction the breaking algorithm works.
I just needed to find a term to match "line" and "page". Maybe there's a
better one. A part is a subsequence of Knuth elements formed by breaking
decisions of the breaking algorithm.

> 
> >>* some methods have been marked deprecated because AFAICT they are not 
> >>called
> >>anywhere. If this is agreed I'll remove them in my next patch
> > 
> > 
> > +1
> > 
> > 
> >>* bugfix? In the last for loop of the method
> >>layoutmgr.PageBreakingAlgorithm.noBreakBetween I think the exit condition 
> >>should
> >>be a strict comparison ('<' instead of '<='). Confirmation?
> > 
> > 
> > not sure. :-(
> 
> The code in the for loop checks if the element pointed to by index is a
> legal breakpoint. If the exit comparison isn't strict index may reach
> the value breakIndex, which by definition is a legal break. I guess the
> purpose of the noBreakBetween method is to check that there is no legal
> break between the two given breakpoints, /excluded/. The line
>      storedValue = (index == breakIndex)
> would confirm that.
> 
> 
> > 
> > 
> >>* the javadoc comments for some methods have been removed because they will
> >>inherit them from their super-class
> > 
> > 
> > I think Checkstyle will bark about that. If you do Ctrl-J in Eclipse, you 
> > get an
> > automatic @see entry which satisfies Checkstyle. @inheritDoc does not work 
> > in
> > every Java version.
> 
> In fact checkstyle doesn't complain. It seems to be smart enough to
> detect that there is a javadoc for the original version of a redefined
> method. In such cases javadoc copies the definition from the
> super-class, and that's also what Eclipse does in the tooltip. I may put
> @see statements, but I think it doesn't really make sense.

Ah ok, then maybe an older Checkstyle version complained about that. At
least, I had something like that in my mind.

> 
> > 
> > 
> >>* some checkstyle fixes
> > 
> > 
> > HTH
> 
> Updated patch follows.
> Thanks,
> Vincent

Thanks, Vincent. I'll look at it right away.

Jeremias Maerki

Reply via email to