DO NOT REPLY [Bug 39777] [PATCH] GSoC: floats implementation

[bugzilla]

https://issues.apache.org/bugzilla/show_bug.cgi?id=39777

Glenn Adams  changed:

   What|Removed |Added

 Status|ASSIGNED|NEW

--- Comment #15 from Glenn Adams  2012-04-11 06:17:36 UTC ---
change status from ASSIGNED to NEW for consistency

-- 
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-31 18:09 ---
(In reply to comment #12)
> Vincent, would you please add a testcase for the outstanding bug?
> "* bug when the whole normal content is typeset and there are both footnotes 
> and
> floats left to be placed;"

Done. I've also added a testcase for before-floats too large to even fit on one
page alone.

Vincent


-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-31 18:08 ---
Created an attachment (id=18666)
 --> (http://issues.apache.org/bugzilla/attachment.cgi?id=18666&action=view)
Disabled testcases for (known) non-working features; patch against the
Temp_Floats branch.


-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-31 10:25 ---
Vincent, would you please add a testcase for the outstanding bug?
"* bug when the whole normal content is typeset and there are both footnotes and
floats left to be placed;"

Whenever possible with reasonable effort, I create a testcase before I start
bugfixing. That documents the bug with a concrete and debug-friendly example and
lets you know when you've finished fixing the bug. Even if you don't fix the bug
right away, it is still helpful, because anyone else can start on the bugfix
right away if he/she has time. "Test first", as in XP. Only good experiences so
far. Note that adding a disabled testcase to the list automatically updates the
website with "known issues" (at least after next site deployment, see
http://xmlgraphics.apache.org/fop/knownissues.html).

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

Re: DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[Vincent Hennebert]

I managed to reenable one of the disabled test cases because you were fooled by
the default values for widows and orphans. Having only 3 lines in a block does
not allow any break possibilities with default widows and orphans. 4 lines
creates one break possibility in the middle.


Indeed, yes. Well, thanks!

Vincent

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-31 09:34 ---
Applied the latest two patches to a temporary branch ("Temp_Floats"):
http://svn.apache.org/viewvc?rev=427052&view=rev

I managed to reenable one of the disabled test cases because you were fooled by
the default values for widows and orphans. Having only 3 lines in a block does
not allow any break possibilities with default widows and orphans. 4 lines
creates one break possibility in the middle.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

Re: DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[Jeremias Maerki]

On 25.07.2006 18:59:00 Vincent Hennebert wrote:
> > DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
> > RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
> > .
> > ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
> > INSERTED IN THE BUG DATABASE.
> > 
> > http://issues.apache.org/bugzilla/show_bug.cgi?id=39777
> > 
> > 
> > 
> > 
> > 
> > --- Additional Comments From [EMAIL PROTECTED]  2006-07-23 19:47 ---
> > I finally had a chance to take a first look. What I've seen so far looks 
> > pretty
> > nice. A first simple test behaved as would be expected. Making a 
> > before-float
> > too big to fit on a page (although there are break points inside the 
> > content)
> > results in an OutOfMemoryError (probably due to an infinite loop).
> > 
> > It would be good if you would write a set of test cases for before-floats as
> > your next task. This is to document what works and what doesn't and to give 
> > you
> > and us more confidence when doing further chances in the code.
> 
> Testcases should be ready tomorrow.

Saw them, thanks a lot! I'll take a look tomorrow afternoon or Saturday.

> 
> > Finally, would you compile a list of classes you propose to move into the
> > "breaking" package? The idea itself is worth investigating since the 
> > layoutmgr
> > package has already grown rather big again.
> 
> On a general matter, I would put in the breaking subpackage all of the
> classes from layoutmgr and its subpackages which are related to the
> Knuth approach: the algorithm as well as the various Knuth elements. A
> quick look gave me the following classes:
> 
> AbstractBreaker
> BalancingColumnBreakingAlgorithm
> BreakingAlgorithm
> BlockKnuthSequence
> InlineKnuthSequence
> KnuthBlockBox
> KnuthBox
> KnuthElement
> KnuthGlue
> KnuthPenalty
> KnuthPossPosIter
> KnuthSequence
> PageBreakingAlgorithm
> inline.KnuthInlineBox

Ok, that's only strictly the pure Knuth stuff. There are a few other
classes we could talk about relocating, too. I'll try to categorize:

Element List Base class:
ListElement (superclass for KnuthElement and UnresolvedListElement)

"Unresolved" stuff:
SpaceResolver
UnresolvedListElement and subclasses

Utilities:
ElementList*.java
AreaAdditionUtil
LMiter
MinOptMaxUtil
TraitSetter

block-level LMs:
BlockLayoutManager
BlockContainerLayoutManager

"outer"-level LMs:
Flow-, StaticContent-, PageSequence-LM

LM-Infrastructure:
the rest

Maybe some of those classes could also be moved to some new subpackages
to clean the whole thing up a little more.

> This will probably create many access problems (with currently
> package-private members). But this will be an opportunity to clean up
> the whole thing a bit, I think.

Too much package-private access is bad anyway and is often done in order
to avoid writing accessor methods. So I guess this will be a good thing.

> In a second step there is also a number of inner classes which might be
> extracted and transformed into a top-level class of the new breaking
> subpackage. I'm mainly thinking of
> inline.LineLayoutManager.LineBreakingAlgorithm. I guess there are
> reasons why they currently are inner classes, but it may be conceptually
> better anyway to separate them from their surrounding class. This would
> have to be studied more deeply.

Yep, LineLayoutManager could probably do with some slimming down, too.

> As for when to apply the change, we may probably wait for the
> integration of Simon's work. I created the package just because I had a
> new class to put in it and thought that I might as well directly put it
> in the right package.

I agree. No good throwing unnecessary stones in Simon's path. But, Simon,
do you have an idea when you'll be ready?

> 
> So, WDYT?
> 
> 
> > What we need to decide now is whether to put the changes in a branch until 
> > they
> > stabilize or if we put it in Trunk. I'd prefer a branch for now so in case I
> > have time to finish the work on 0.93, we don't have any problems from this 
> > end.
> 
> A branch would be fine I think, as this would allow me to submit more
> gradual patches.

Good. I'll do that, then. ASAP. Damn busy week. And hot here. Tonight
was the first thunderstorm in many days of pure sunshine.

Sorry, I'm not more responsive this week.

Jeremias Maerki

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-26 16:06 ---
Created an attachment (id=18644)
 --> (http://issues.apache.org/bugzilla/attachment.cgi?id=18644&action=view)
Testcases for before-floats

Testcases for basic features of before-floats. There are also some disabled
testcases for features known to be broken.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

Re: DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[Vincent Hennebert]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-23 19:47 ---
I finally had a chance to take a first look. What I've seen so far looks pretty
nice. A first simple test behaved as would be expected. Making a before-float
too big to fit on a page (although there are break points inside the content)
results in an OutOfMemoryError (probably due to an infinite loop).

It would be good if you would write a set of test cases for before-floats as
your next task. This is to document what works and what doesn't and to give you
and us more confidence when doing further chances in the code.


Testcases should be ready tomorrow.



Finally, would you compile a list of classes you propose to move into the
"breaking" package? The idea itself is worth investigating since the layoutmgr
package has already grown rather big again.


On a general matter, I would put in the breaking subpackage all of the
classes from layoutmgr and its subpackages which are related to the
Knuth approach: the algorithm as well as the various Knuth elements. A
quick look gave me the following classes:

AbstractBreaker
BalancingColumnBreakingAlgorithm
BreakingAlgorithm
BlockKnuthSequence
InlineKnuthSequence
KnuthBlockBox
KnuthBox
KnuthElement
KnuthGlue
KnuthPenalty
KnuthPossPosIter
KnuthSequence
PageBreakingAlgorithm
inline.KnuthInlineBox

This will probably create many access problems (with currently
package-private members). But this will be an opportunity to clean up
the whole thing a bit, I think.

In a second step there is also a number of inner classes which might be
extracted and transformed into a top-level class of the new breaking
subpackage. I'm mainly thinking of
inline.LineLayoutManager.LineBreakingAlgorithm. I guess there are
reasons why they currently are inner classes, but it may be conceptually
better anyway to separate them from their surrounding class. This would
have to be studied more deeply.

As for when to apply the change, we may probably wait for the
integration of Simon's work. I created the package just because I had a
new class to put in it and thought that I might as well directly put it
in the right package.


So, WDYT?



What we need to decide now is whether to put the changes in a branch until they
stabilize or if we put it in Trunk. I'd prefer a branch for now so in case I
have time to finish the work on 0.93, we don't have any problems from this end.


A branch would be fine I think, as this would allow me to submit more
gradual patches.


Vincent

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777


[EMAIL PROTECTED] changed:

   What|Removed |Added

  Attachment #18617|0   |1
is obsolete||




--- Additional Comments From [EMAIL PROTECTED]  2006-07-25 08:32 ---
Created an attachment (id=18637)
 --> (http://issues.apache.org/bugzilla/attachment.cgi?id=18637&action=view)
The very same patch, with some english corrections as suggested by Simon
Pepping

* (OutOfLineRecord.)ProgressInfos renamed into ProgressInfo
* (OutOfLineRecord.)cumulatedLengths renamed into cumulativeLengths


-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-25 08:28 ---
(In reply to comment #7)

Thanks for your comments, Simon. A few answers:

> 2. Wiki page: The argument for an inner class in bullet 3 only refers
>to an object inner class. Therefore contradicts bullet 4, a static
>inner class, and is invalid.

Well, the private fields of any ProgressInfo instance are directly 
accessible within OutOfLineRecord. This avoids using getter and setter 
methods everywhere in OutOfLineRecord, for the progressInfo field as 
well as the prevProgress parameters of the get[Footnote|Float]Split 
methods. That's what I meant.


> 3. fo:float seems to be the only FO that can be placed in inline and
>block content. This causes a problem because FOP distinguishes
>between inline and block LMs rather much. I do not think that this
>problem needs to be solved in this project. It should be fine to
>make an implementation which works for inline fo:float FOs.

Ok, noted. On the tests I've done this seems to work, but I don't know 
if it will in all cases. I think the solving of this problem may be done 
together with the implementation of the keep-with-previous property for 
the anchor element. I put it on my TODO-list.


> 4. Infos is not correct english; better call it
>ProgressInfo. cumulatedLengths is better changed to
>cumulativeLengths.

Thanks, always glad to improve my english ;-)


> 5. The implementation does not use the max and min property of the BPD
>of a float. In a text with little stretch and shrink, e.g. an
>adapted version of the test file footnote_basic.xml, there is no
>possibility to place the floats with less than infinite badness,
>and they are moved towards the end of the page sequence.

Yes. If I'm correct the shrink/stretch capabilities of footnotes aren't 
considered either. More generally there is a problem with too short 
nodes (considered as such because no stretching is possible), which are 
handled separately from the other nodes. The algorithm may prefer a 
normal node with a deferred float instead of a "too short" node with the 
float on the same page.
I'm thinking about an improved algorithm which would handle such cases. 
I think I will give some ideas on the Wiki page soon, after I've 
submitted testcases. But before implementing it I'll perhaps work on 
side-floats, don't know yet.


> 6. The OutOfLineRecord.getFootnoteSplit and
>OutOfLineRecord.getFloatSplit methods suggest that there is room
>for two subclasses. It would be nice if the two conditional blocks
>in PageBreakingAlgorithm.computeDifference, if (floats.existing())
>and if (footnotes.existing()) could be merged, and the difference
>in the logic be moved to the footnotes and floats objects. But that
>is finishing touch.

I agree (that some further refactoring is possible). As this part of the 
code may change if an improved algorithm is to be implemented, I didn't 
put too much effort in it. This will belong to a second refactoring 
step.


Thanks,
Vincent


-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

Re: DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[Jeremias Maerki]

No need to worry about that one too much anymore. License headers are
going to change (again) and the copyright year vanishes. Cliff Schmidt
hasn't sent the info mail, yet, but the decision is official: 
http://www.apache.org/legal/src-headers.html

BTW, if anyone wants to volunteer for doing the header change, there are
nice ready-to-use scripts in the committers repo. :-)

On 24.07.2006 22:21:38 bugzilla wrote:
> 1. Copyright year on new classes should be only 2006.


Jeremias Maerki

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-24 20:21 ---
This is a good piece of work. A few remarks:

1. Copyright year on new classes should be only 2006.

2. Wiki page: The argument for an inner class in bullet 3 only refers
   to an object inner class. Therefore contradicts bullet 4, a static
   inner class, and is invalid.

3. fo:float seems to be the only FO that can be placed in inline and
   block content. This causes a problem because FOP distinguishes
   between inline and block LMs rather much. I do not think that this
   problem needs to be solved in this project. It should be fine to
   make an implementation which works for inline fo:float FOs.

4. Infos is not correct english; better call it
   ProgressInfo. cumulatedLengths is better changed to
   cumulativeLengths.

5. The implementation does not use the max and min property of the BPD
   of a float. In a text with little stretch and shrink, e.g. an
   adapted version of the test file footnote_basic.xml, there is no
   possibility to place the floats with less than infinite badness,
   and they are moved towards the end of the page sequence.

6. The OutOfLineRecord.getFootnoteSplit and
   OutOfLineRecord.getFloatSplit methods suggest that there is room
   for two subclasses. It would be nice if the two conditional blocks
   in PageBreakingAlgorithm.computeDifference, if (floats.existing())
   and if (footnotes.existing()) could be merged, and the difference
   in the logic be moved to the footnotes and floats objects. But that
   is finishing touch.



-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-23 19:47 ---
I finally had a chance to take a first look. What I've seen so far looks pretty
nice. A first simple test behaved as would be expected. Making a before-float
too big to fit on a page (although there are break points inside the content)
results in an OutOfMemoryError (probably due to an infinite loop).

It would be good if you would write a set of test cases for before-floats as
your next task. This is to document what works and what doesn't and to give you
and us more confidence when doing further chances in the code.

Finally, would you compile a list of classes you propose to move into the
"breaking" package? The idea itself is worth investigating since the layoutmgr
package has already grown rather big again.

What we need to decide now is whether to put the changes in a branch until they
stabilize or if we put it in Trunk. I'd prefer a branch for now so in case I
have time to finish the work on 0.93, we don't have any problems from this end.

Keep up the good work. This looks very promising.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-07-19 10:13 ---
Created an attachment (id=18617)
 --> (http://issues.apache.org/bugzilla/attachment.cgi?id=18617&action=view)
First patch for before-floats implementation

Basic implementation of before-floats. Known problems are:
* bug when the whole normal content is typeset and there are both footnotes and
floats left to be placed;
* floats too big to even fit on one page alone are not handled yet;
* the keep-with-previous property for the generated inline anchor-area is not
implemented;
* some clean-up in LayoutManagers should be possible;
* others that I may have missed...

Changelog:
* Created a breaking subpackage where, if this is agreed, all classes related
to the Knuth approach will eventually be placed.
* Created an OutOfLineRecord class for dealing with both before-floats and
footnotes. It is basically made of code extracted from PageBreakingAlgorithm
that was related to footnote handling. Most of it could also be used for
floats.

(pending: documentation and testcases)

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777


[EMAIL PROTECTED] changed:

   What|Removed |Added

 Status|NEW |ASSIGNED




--- Additional Comments From [EMAIL PROTECTED]  2006-06-14 07:49 ---
(In reply to comment #3)
> Created an attachment (id=18456)
 --> (http://issues.apache.org/bugzilla/attachment.cgi?id=18456&action=view) 
[edit]
> Updated patch, whith some additions and corrections
> 
> I think this patch may be applied. It contains new javadoc comments and adds
> some precisions to previous ones. Deprecated elements have been removed.

Patch applied. Thank you! Precious work for anyone who wants to dive in here.
http://svn.apache.org/viewvc?rev=414135&view=rev

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

Re: DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[Jeremias Maerki]

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

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777


[EMAIL PROTECTED] changed:

   What|Removed |Added

  Attachment #18447|0   |1
is obsolete||




--- Additional Comments From [EMAIL PROTECTED]  2006-06-13 16:03 ---
Created an attachment (id=18456)
 --> (http://issues.apache.org/bugzilla/attachment.cgi?id=18456&action=view)
Updated patch, whith some additions and corrections

I think this patch may be applied. It contains new javadoc comments and adds
some precisions to previous ones. Deprecated elements have been removed.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

Re: DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[Vincent Hennebert]

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?



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?
What does it mean that a box is auxiliary?



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?



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?



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






* some checkstyle fixes



HTH


Updated patch follows.
Thanks,
Vincent

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- 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).
pos and bAux are defined in ListElement/KnuthElement.

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

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

> * some checkstyle fixes

HTH

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.

DO NOT REPLY [Bug 39777] - [PATCH] GSoC: floats implementation

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=39777





--- Additional Comments From [EMAIL PROTECTED]  2006-06-11 16:00 ---
Created an attachment (id=18447)
 --> (http://issues.apache.org/bugzilla/attachment.cgi?id=18447&action=view)
proposed patch


-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the assignee for the bug, or are watching the assignee.