You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Vincent Hennebert <vi...@anyware-tech.com> on 2008/06/19 12:36:30 UTC
Re: svn commit: r669118 - in /xmlgraphics/fop/branches/fop-0_95:
./ src/java/org/apache/fop/fo/flow/ src/java/org/apache/fop/fo/flow/table/
src/java/org/apache/fop/fo/properties/ src/java/org/apache/fop/layoutmgr/
src/java/org/apache/fop/layoutmgr/list/ sr...
Hi,
> Author: jeremias
> Date: Wed Jun 18 02:02:45 2008
> New Revision: 669118
>
> URL: http://svn.apache.org/viewvc?rev=669118&view=rev
> Log:
> Bugzilla #44412:
> Regression fix for empty pages caused by multiple collapsible breaks.
> No more empty block areas if a break-before occurs on the first child of an FO to match the behaviour of tables and other FO implementations (clarification by XSL WG pending).
> Added an accessor interface for break-before/-after to avoid long if..else lists in BlockStackingLayoutManager.
I’m afraid I’m not happy with this change. There are a couple of issues
that should be looked at IMO.
Simple things first:
- BlockStackingLM.getBreakBefore(boolean) is always given true as its
parameter. Why specifying a parameter then? Plus this method is called
only within BlockStackingLM, so it should be made private (at least
for now).
- BlockStackingLM.getBreakBefore(): same here, this method should be
made private. And if fobj doesn’t implement BreakPropertySet, why
should EN_AUTO be returned? If this method is called on an object that
doesn’t support it, I guess it should rather throw an exception than
silently return a default value. Otherwise that makes it difficult to
track down errors.
- why is childLMForBreakSkip a weak reference?? What will happen if the
object has been released at the moment where it’s retrieved? Will the
code still behave correctly? Furthermore, this childLMForBreakSkip
object will by definition always be the first child LM of the current
LM. So this field might not be needed at all in the end.
- I was hoping that the way I implemented breaks in table could be
generalized to the other LMs. Maybe that approach has its flaws that
I haven’t noticed but so far this works for tables.
Basically the idea is that instead of creating break elements, a LM
sets a flag on the LayoutContext it was given through the
getNextKnuthElements call. Then it’s the ancestor block stacking LM
that decides whether a break element must be produced (when the child
LM is in the middle of the sequence) or if the break property must be
passed on to the parent LM (when the child LM is the first child
of the block stacking LM).
See TableContentLM.getKnuthElementsForRowIterator,
RowGroupLM.getNextKnuthElements,
LayoutContext.get/setBreakBefore/After
I think it would be very unfortunate to have two different mechanisms
for handling breaks. The thing is, I’ll be offline for the next 10 days
so I’ll have no time to do/review anything, and I realise that we
wouldn’t want to delay the release of 0.95 too much. We could imagine to
leave this change as is in the 0.95 branch, but it should not be merged
back into the Trunk.
Vincent
--
Vincent Hennebert Anyware Technologies
http://people.apache.org/~vhennebert http://www.anyware-tech.com
Apache FOP Committer FOP Development/Consulting
Re: svn commit: r669118 - in /xmlgraphics/fop/branches/fop-0_95: ./ src/java/org/apache/fop/fo/flow/ src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/properties/ src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/layoutmgr/list/
sr...
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Thanks for your feedback!
On 19.06.2008 12:36:30 Vincent Hennebert wrote:
> Hi,
>
> > Author: jeremias
> > Date: Wed Jun 18 02:02:45 2008
> > New Revision: 669118
> >
> > URL: http://svn.apache.org/viewvc?rev=669118&view=rev
> > Log:
> > Bugzilla #44412:
> > Regression fix for empty pages caused by multiple collapsible breaks.
> > No more empty block areas if a break-before occurs on the first child of an FO to match the behaviour of tables and other FO implementations (clarification by XSL WG pending).
> > Added an accessor interface for break-before/-after to avoid long if..else lists in BlockStackingLayoutManager.
>
> I’m afraid I’m not happy with this change. There are a couple of issues
> that should be looked at IMO.
>
> Simple things first:
> - BlockStackingLM.getBreakBefore(boolean) is always given true as its
> parameter. Why specifying a parameter then? Plus this method is called
> only within BlockStackingLM, so it should be made private (at least
> for now).
Same reason as usual. Refactoring, planning for the "future", then at
the end not noticing that some people like it more cleaned up. ;-)
> - BlockStackingLM.getBreakBefore(): same here, this method should be
> made private. And if fobj doesn’t implement BreakPropertySet, why
> should EN_AUTO be returned? If this method is called on an object that
> doesn’t support it, I guess it should rather throw an exception than
> silently return a default value. Otherwise that makes it difficult to
> track down errors.
BlockStackingLM is base class for FOs not supporting breaks (ex.
footnote-body or static-content). addKnuthElementsForBreak*() is called
anyway. EN_AUTO means no break condition and no action. So I think this
is not problematic.
> - why is childLMForBreakSkip a weak reference?? What will happen if the
> object has been released at the moment where it’s retrieved? Will the
> code still behave correctly? Furthermore, this childLMForBreakSkip
> object will by definition always be the first child LM of the current
> LM. So this field might not be needed at all in the end.
That was a mistake. I didn't remember that there is a childLMs field.
That's improved now.
> - I was hoping that the way I implemented breaks in table could be
> generalized to the other LMs. Maybe that approach has its flaws that
> I haven’t noticed but so far this works for tables.
> Basically the idea is that instead of creating break elements, a LM
> sets a flag on the LayoutContext it was given through the
> getNextKnuthElements call. Then it’s the ancestor block stacking LM
> that decides whether a break element must be produced (when the child
> LM is in the middle of the sequence) or if the break property must be
> passed on to the parent LM (when the child LM is the first child
> of the block stacking LM).
> See TableContentLM.getKnuthElementsForRowIterator,
> RowGroupLM.getNextKnuthElements,
> LayoutContext.get/setBreakBefore/After
>
> I think it would be very unfortunate to have two different mechanisms
> for handling breaks.
Please note that the break handling in tables has always been different
than all other FOs. And table came last. When I first implemented table
layout I took a shortcut by not supporting reentry to
getNextKnuthElements() after a forced break. After all, the method is
called getNextKnuthElements(), not getKnuthElements().
The problem with your approach is that you have to create the full
element list for the table (to get the right values back through the
LayoutContext) and only then can you do add the break element. In
the other LMs, break-before is handled first (and the method returns
early if a break-before happens). I assumed this gets revisited with the
new page breaking approach which is why I didn't invest more time in
this.
> The thing is, I’ll be offline for the next 10 days
> so I’ll have no time to do/review anything, and I realise that we
> wouldn’t want to delay the release of 0.95 too much. We could imagine to
> leave this change as is in the 0.95 branch, but it should not be merged
> back into the Trunk.
>
> Vincent
>
>
> --
> Vincent Hennebert Anyware Technologies
> http://people.apache.org/~vhennebert http://www.anyware-tech.com
> Apache FOP Committer FOP Development/Consulting
Jeremias Maerki
Re: svn commit: r669118 - in /xmlgraphics/fop/branches/fop-0_95: ./ src/java/org/apache/fop/fo/flow/ src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/properties/ src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/layoutmgr/list/
sr...
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
I'm going to be away most of this week so I don't have much time for FOP,
either. And next week isn't much better. I guess it's going to be July
then.
On 19.06.2008 12:36:30 Vincent Hennebert wrote:
<snip/>
> I think it would be very unfortunate to have two different mechanisms
> for handling breaks. The thing is, I’ll be offline for the next 10 days
> so I’ll have no time to do/review anything, and I realise that we
> wouldn’t want to delay the release of 0.95 too much. We could imagine to
> leave this change as is in the 0.95 branch, but it should not be merged
> back into the Trunk.
Jeremias Maerki