Bugfix: An IllegalArgumentException was thrown when break-before was used inside a list

git-svn-id: https://svn.apache.org/repos/asf/xmlgraphics/fop/trunk@1362203 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/fop/layoutmgr/AbstractBreaker.java

@@ -506,7 +506,17 @@ public abstract class AbstractBreaker {
                 ListElement lastBreakElement = effectiveList.getElement(endElementIndex);
                 if (lastBreakElement.isPenalty()) {
                     KnuthPenalty pen = (KnuthPenalty)lastBreakElement;
-                    lastBreakClass = pen.getBreakClass();
+                    if (pen.getPenalty() == KnuthPenalty.INFINITE) {
+                        /**
+                         * That means that there was a keep.within-page="always", but that
+                         * it's OK to break at a column. TODO The break class is being
+                         * abused to implement keep.within-column and keep.within-page.
+                         * This is very misleading and must be revised.
+                         */
+                        lastBreakClass = Constants.EN_COLUMN;
+                    } else {
+                        lastBreakClass = pen.getBreakClass();
+                    }
                 } else {
                     lastBreakClass = Constants.EN_COLUMN;
                 }

src/java/org/apache/fop/layoutmgr/BlockContainerLayoutManager.java

@@ -1044,10 +1044,6 @@ public class BlockContainerLayoutManager extends BlockStackingLayoutManager impl
         return true;
     }
 
-    public int getBreakBefore() {
-        return BreakOpportunityHelper.getBreakBefore(this);
-    }
-
 }
 
 

src/java/org/apache/fop/layoutmgr/BlockLayoutManager.java

@@ -504,8 +504,4 @@ public class BlockLayoutManager extends BlockStackingLayoutManager implements Co
         return true;
     }
 
-    public int getBreakBefore() {
-        return BreakOpportunityHelper.getBreakBefore(this);
-    }
-
 }

src/java/org/apache/fop/layoutmgr/BlockStackingLayoutManager.java

@@ -1035,7 +1035,7 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
      * @return true if an element has been added due to a break-before.
      */
     protected boolean addKnuthElementsForBreakBefore(List returnList, LayoutContext context) {
-        int breakBefore = BreakOpportunityHelper.getBreakBefore(this);
+        int breakBefore = getBreakBefore();
         if (breakBefore == EN_PAGE
                 || breakBefore == EN_COLUMN
                 || breakBefore == EN_EVEN_PAGE
@@ -1049,6 +1049,17 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
         }
     }
 
+    /**
+     * Returns the highest priority break-before value on this layout manager or its
+     * relevant descendants.
+     *
+     * @return the break-before value (Constants.EN_*)
+     * @see BreakOpportunity#getBreakBefore()
+     */
+    public int getBreakBefore() {
+        return BreakOpportunityHelper.getBreakBefore(this);
+    }
+
     /**
      * Creates Knuth elements for break-after and adds them to the return list.
      * @param returnList return list to add the additional elements to

src/java/org/apache/fop/layoutmgr/PageBreaker.java

@@ -521,7 +521,6 @@ public class PageBreaker extends AbstractBreaker {
             return;
         case Constants.EN_COLUMN:
         case Constants.EN_AUTO:
-        case Constants.EN_PAGE:
         case -1:
             PageViewport pv = curPage.getPageViewport();
 
@@ -545,6 +544,7 @@ public class PageBreaker extends AbstractBreaker {
                 /*curPage = */pslm.makeNewPage(false);
             }
             return;
+        case Constants.EN_PAGE:
         default:
             log.debug("handling break-before after page " + pslm.getCurrentPageNum()
                 + " breakVal=" + getBreakClassName(breakVal));
@@ -560,7 +560,7 @@ public class PageBreaker extends AbstractBreaker {
     }
 
     /**
-     * Check if a blank page is needed to accomodate
+     * Check if a blank page is needed to accommodate
      * desired even or odd page number.
      * @param breakVal - value of break-before or break-after trait.
      */

src/java/org/apache/fop/layoutmgr/list/ListItemContentLayoutManager.java

@@ -20,6 +20,7 @@
 package org.apache.fop.layoutmgr.list;
 
 import java.util.LinkedList;
+import java.util.List;
 
 import org.apache.fop.area.Area;
 import org.apache.fop.area.Block;
@@ -28,9 +29,11 @@ import org.apache.fop.fo.flow.ListItemBody;
 import org.apache.fop.fo.flow.ListItemLabel;
 import org.apache.fop.fo.properties.KeepProperty;
 import org.apache.fop.layoutmgr.BlockStackingLayoutManager;
+import org.apache.fop.layoutmgr.BreakOpportunity;
 import org.apache.fop.layoutmgr.Keep;
 import org.apache.fop.layoutmgr.LayoutContext;
 import org.apache.fop.layoutmgr.LayoutManager;
+import org.apache.fop.layoutmgr.ListElement;
 import org.apache.fop.layoutmgr.NonLeafPosition;
 import org.apache.fop.layoutmgr.Position;
 import org.apache.fop.layoutmgr.PositionIterator;
@@ -40,7 +43,7 @@ import org.apache.fop.layoutmgr.TraitSetter;
 /**
  * LayoutManager for a list-item-label or list-item-body FO.
  */
-public class ListItemContentLayoutManager extends BlockStackingLayoutManager {
+public class ListItemContentLayoutManager extends BlockStackingLayoutManager implements BreakOpportunity {
 
     private Block curBlockArea;
 
@@ -220,5 +223,16 @@ public class ListItemContentLayoutManager extends BlockStackingLayoutManager {
     public Keep getKeepWithPrevious() {
         return Keep.KEEP_AUTO;
     }
+
+    @SuppressWarnings("unchecked")
+    @Override
+    public List<ListElement> getNextKnuthElements(LayoutContext context, int alignment) {
+        List<ListElement> elements = new LinkedList<ListElement>();
+        do {
+            elements.addAll(super.getNextKnuthElements(context, alignment));
+        } while (!isFinished());
+        return elements;
+    }
+
 }
 

src/java/org/apache/fop/layoutmgr/list/ListItemLayoutManager.java

@@ -35,6 +35,8 @@ import org.apache.fop.fo.flow.ListItemLabel;
 import org.apache.fop.fo.properties.KeepProperty;
 import org.apache.fop.layoutmgr.BlockStackingLayoutManager;
 import org.apache.fop.layoutmgr.BreakElement;
+import org.apache.fop.layoutmgr.BreakOpportunity;
+import org.apache.fop.layoutmgr.BreakOpportunityHelper;
 import org.apache.fop.layoutmgr.ConditionalElementListener;
 import org.apache.fop.layoutmgr.ElementListObserver;
 import org.apache.fop.layoutmgr.ElementListUtils;
@@ -56,13 +58,14 @@ import org.apache.fop.layoutmgr.SpaceResolver;
 import org.apache.fop.layoutmgr.TraitSetter;
 import org.apache.fop.traits.MinOptMax;
 import org.apache.fop.traits.SpaceVal;
+import org.apache.fop.util.BreakUtil;
 
 /**
  * LayoutManager for a list-item FO.
  * The list item contains a list item label and a list item body.
  */
-public class ListItemLayoutManager extends BlockStackingLayoutManager
-                    implements ConditionalElementListener {
+public class ListItemLayoutManager extends BlockStackingLayoutManager implements ConditionalElementListener,
+        BreakOpportunity {
 
     /** logging instance */
     private static Log log = LogFactory.getLog(ListItemLayoutManager.class);
@@ -204,6 +207,7 @@ public class ListItemLayoutManager extends BlockStackingLayoutManager
 
         // label
         childLC = makeChildLayoutContext(context);
+        childLC.setFlags(LayoutContext.SUPPRESS_BREAK_BEFORE);
         label.initialize();
         labelList = label.getNextKnuthElements(childLC, alignment);
 
@@ -217,6 +221,7 @@ public class ListItemLayoutManager extends BlockStackingLayoutManager
 
         // body
         childLC = makeChildLayoutContext(context);
+        childLC.setFlags(LayoutContext.SUPPRESS_BREAK_BEFORE);
         body.initialize();
         bodyList = body.getNextKnuthElements(childLC, alignment);
 
@@ -296,16 +301,23 @@ public class ListItemLayoutManager extends BlockStackingLayoutManager
             //Additional penalty height from penalties in the source lists
             int additionalPenaltyHeight = 0;
             int stepPenalty = 0;
+            int breakClass = EN_AUTO;
             KnuthElement endEl = (KnuthElement)elementLists[0].get(end[0]);
             if (endEl instanceof KnuthPenalty) {
                 additionalPenaltyHeight = endEl.getWidth();
-                stepPenalty = Math.max(stepPenalty, endEl.getPenalty());
+                stepPenalty = endEl.getPenalty() == -KnuthElement.INFINITE ? -KnuthElement.INFINITE : Math
+                        .max(stepPenalty, endEl.getPenalty());
+                breakClass = BreakUtil.compareBreakClasses(breakClass,
+                        ((KnuthPenalty) endEl).getBreakClass());
             }
             endEl = (KnuthElement)elementLists[1].get(end[1]);
             if (endEl instanceof KnuthPenalty) {
                 additionalPenaltyHeight = Math.max(
                         additionalPenaltyHeight, endEl.getWidth());
-                stepPenalty = Math.max(stepPenalty, endEl.getPenalty());
+                stepPenalty = endEl.getPenalty() == -KnuthElement.INFINITE ? -KnuthElement.INFINITE : Math
+                        .max(stepPenalty, endEl.getPenalty());
+                breakClass = BreakUtil.compareBreakClasses(breakClass,
+                        ((KnuthPenalty) endEl).getBreakClass());
             }
 
             int boxHeight = step - addedBoxHeight - penaltyHeight;
@@ -343,9 +355,9 @@ public class ListItemLayoutManager extends BlockStackingLayoutManager
                 int p = stepPenalty;
                 if (p > -KnuthElement.INFINITE) {
                     p = Math.max(p, keep.getPenalty());
+                    breakClass = keep.getContext();
                 }
-                returnList.add(new BreakElement(stepPosition, penaltyHeight, p, keep.getContext(),
-                        context));
+                returnList.add(new BreakElement(stepPosition, penaltyHeight, p, breakClass, context));
             }
         }
 
@@ -693,6 +705,13 @@ public class ListItemLayoutManager extends BlockStackingLayoutManager
         body.reset();
     }
 
+    @Override
+    public int getBreakBefore() {
+        int breakBefore = BreakOpportunityHelper.getBreakBefore(this);
+        breakBefore = BreakUtil.compareBreakClasses(breakBefore, label.getBreakBefore());
+        breakBefore = BreakUtil.compareBreakClasses(breakBefore, body.getBreakBefore());
+        return breakBefore;
+    }
 
 }
 

status.xml

@@ -63,6 +63,9 @@
       documents. Example: the fix of marks layering will be such a case when it's done.
     -->
     <release version="FOP Trunk" date="TBD">
+      <action context="Layout" dev="VH" type="fix">
+        An IllegalArgumentException was thrown when break-before was used inside a list.
+      </action>
       <action context="Layout" dev="VH" type="fix">
         When restarting layout for the last page, discard glues and penalties at the beginning of 
         the restarted Knuth sequence.

test/layoutengine/standard-testcases/list-item-body_break-before.xml

@@ -0,0 +1,96 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one or more
+  contributor license agreements.  See the NOTICE file distributed with
+  this work for additional information regarding copyright ownership.
+  The ASF licenses this file to You under the Apache License, Version 2.0
+  (the "License"); you may not use this file except in compliance with
+  the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+<!-- $Id$ -->
+<testcase>
+  <info>
+    <p>This test checks basic breaks in list bodies.</p>
+  </info>
+  <fo>
+    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
+      <fo:layout-master-set>
+        <fo:simple-page-master margin="5pt" page-width="11cm" page-height="3cm"
+          master-name="page">
+          <fo:region-body />
+        </fo:simple-page-master>
+      </fo:layout-master-set>
+      <fo:page-sequence master-reference="page">
+        <fo:flow flow-name="xsl-region-body">
+          <fo:list-block provisional-distance-between-starts="20pt"
+            provisional-label-separation="5pt">
+            <fo:list-item>
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>(a)</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  This content starts on page 1 and overflows to page 2. This content starts on page 1 and
+                  overflows to page 2. This content starts on page 1 and overflows to page 2. This content
+                  starts on page 1 and overflows to page 2. This content starts on page 1 and overflows to
+                  page 2. This content starts on page 1 and overflows to page 2. This content starts on page 1
+                  and overflows to page 2.
+                  <fo:block break-before="page" />
+                  This content is on page 3.
+                  <fo:block break-before="page" />
+                  This content is on page 4.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+            <fo:list-item>
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>(b)</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  <fo:block break-before="page" />
+                  This content is on page 5.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+            <fo:list-item>
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>(c)</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block break-before="page">
+                  This content is on page 6.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+            <fo:list-item break-before="page">
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>(d)</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  This content is on page 7.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+          </fo:list-block>
+        </fo:flow>
+      </fo:page-sequence>
+    </fo:root>
+  </fo>
+  <checks>
+    <eval expected="3" xpath="//lineArea[starts-with(., 'This content is on page 3.')]/ancestor::pageViewport/@nr" />
+    <eval expected="4" xpath="//lineArea[starts-with(., 'This content is on page 4.')]/ancestor::pageViewport/@nr" />
+    <eval expected="5" xpath="//lineArea[starts-with(., '(b)')]/ancestor::pageViewport/@nr" />
+    <eval expected="6" xpath="//lineArea[starts-with(., '(c)')]/ancestor::pageViewport/@nr" />
+    <eval expected="7" xpath="//lineArea[starts-with(., '(d)')]/ancestor::pageViewport/@nr" />
+  </checks>
+</testcase>

test/layoutengine/standard-testcases/list-item-body_break-before_multicolumn.xml

@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one or more
+  contributor license agreements.  See the NOTICE file distributed with
+  this work for additional information regarding copyright ownership.
+  The ASF licenses this file to You under the Apache License, Version 2.0
+  (the "License"); you may not use this file except in compliance with
+  the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+<!-- $Id$ -->
+<testcase>
+  <info>
+    <p>This test checks basic breaks in list bodies.</p>
+  </info>
+  <fo>
+    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
+      <fo:layout-master-set>
+        <fo:simple-page-master margin="2cm" page-width="15.0cm" page-height="7.0cm"
+          master-name="page">
+          <fo:region-body column-count="2" />
+        </fo:simple-page-master>
+      </fo:layout-master-set>
+      <fo:page-sequence master-reference="page">
+        <fo:flow flow-name="xsl-region-body">
+          <fo:list-block provisional-distance-between-starts="20pt" provisional-label-separation="5pt">
+            <fo:list-item>
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>(a)</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  On first page, first column
+                  <fo:block id="block-3" break-before="page" />
+                  On second page, first column, NOT first page, first column NEITHER first page, second column.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+          </fo:list-block>
+        </fo:flow>
+      </fo:page-sequence>
+    </fo:root>
+  </fo>
+  <checks>
+    <eval expected="1" xpath="//lineArea[starts-with(., 'On first page')]/ancestor::pageViewport/@nr" />
+    <eval expected="2" xpath="//lineArea[starts-with(., 'On second page')]/ancestor::pageViewport/@nr" />
+    <eval expected="1" xpath="count(//block[@prod-id='block-3']/ancestor::flow)" />
+  </checks>
+</testcase>

test/layoutengine/standard-testcases/page-sequence_last-page_space-before.xml

@@ -0,0 +1,59 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one or more
+  contributor license agreements.  See the NOTICE file distributed with
+  this work for additional information regarding copyright ownership.
+  The ASF licenses this file to You under the Apache License, Version 2.0
+  (the "License"); you may not use this file except in compliance with
+  the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+<!-- $Id$ -->
+<testcase>
+  <info>
+    <p>When layout is re-started for the last page, the space-before on the first element to appear 
+      on that page must be discarded.</p>
+  </info>
+  <fo>
+    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
+      <fo:layout-master-set>
+        <fo:simple-page-master master-name="page"
+          page-height="40pt" page-width="110pt" margin="5pt">
+          <fo:region-body background-color="#F0F0F0"/>
+        </fo:simple-page-master>
+        <fo:page-sequence-master master-name="pages">
+          <fo:repeatable-page-master-alternatives>
+            <fo:conditional-page-master-reference master-reference="page" page-position="last"/>
+            <fo:conditional-page-master-reference master-reference="page" page-position="any"/>
+          </fo:repeatable-page-master-alternatives>
+        </fo:page-sequence-master>
+      </fo:layout-master-set>
+      <fo:page-sequence master-reference="pages" font-size="8pt" line-height="10pt">
+        <fo:flow flow-name="xsl-region-body">
+          <fo:block-container height="30pt">
+            <fo:block>Filler 1</fo:block>
+            <fo:block>Filler 2</fo:block>
+            <fo:block>Filler 3</fo:block>
+          </fo:block-container>
+          <fo:block-container height="20pt" keep-with-next.within-page="always">
+            <fo:block>Before line 1</fo:block>
+          </fo:block-container>
+          <fo:block>Before the page break</fo:block>
+          <fo:block space-before="10pt">After the page break</fo:block>
+          <fo:block>After line 1</fo:block>
+          <fo:block>After line 2</fo:block>
+        </fo:flow>
+      </fo:page-sequence>
+    </fo:root>
+  </fo>
+  <checks>
+    <eval expected="3" xpath="count(//pageViewport)"/>
+  </checks>
+</testcase>

test/layoutengine/standard-testcases/table-cell_break-before_multicolumn.xml

@@ -0,0 +1,68 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one or more
+  contributor license agreements.  See the NOTICE file distributed with
+  this work for additional information regarding copyright ownership.
+  The ASF licenses this file to You under the Apache License, Version 2.0
+  (the "License"); you may not use this file except in compliance with
+  the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+<!-- $Id$ -->
+<testcase>
+  <info>
+    <p>This test checks basic breaks in table cells.</p>
+  </info>
+  <fo>
+    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
+      <fo:layout-master-set>
+        <fo:simple-page-master margin="2cm" page-width="21.0cm" page-height="29.7cm"
+          master-name="A4-portrait">
+          <fo:region-body column-count="2" />
+        </fo:simple-page-master>
+      </fo:layout-master-set>
+      <fo:page-sequence master-reference="A4-portrait">
+        <fo:flow flow-name="xsl-region-body">
+          <fo:table>
+            <fo:table-body>
+              <fo:table-row>
+                <fo:table-cell>
+                  <fo:block>Page 1, Column 1, Cell 1</fo:block>
+                </fo:table-cell>
+                <fo:table-cell>
+                  <fo:block>Page 1, Column 1, Cell 2</fo:block>
+                </fo:table-cell>
+                <fo:table-cell>
+                  <fo:block>Page 1, Column 1, Cell 3</fo:block>
+                </fo:table-cell>
+              </fo:table-row>
+              <fo:table-row>
+                <fo:table-cell>
+                  <fo:block id="block-4">Page 2, Column 1, Cell 1</fo:block>
+                </fo:table-cell>
+                <fo:table-cell>
+                  <fo:block break-before="page">Page 2, Column 1, Cell 2</fo:block>
+                </fo:table-cell>
+                <fo:table-cell>
+                  <fo:block>Page 2, Column 1, Cell 3</fo:block>
+                </fo:table-cell>
+              </fo:table-row>
+            </fo:table-body>
+          </fo:table>
+        </fo:flow>
+      </fo:page-sequence>
+    </fo:root>
+  </fo>
+  <checks>
+    <eval expected="1" xpath="//lineArea[starts-with(., 'Page 1')]/ancestor::pageViewport/@nr" />
+    <eval expected="2" xpath="//lineArea[starts-with(., 'Page 2')]/ancestor::pageViewport/@nr" />
+    <eval expected="1" xpath="count(//block[@prod-id='block-4']/ancestor::flow)" />
+  </checks>
+</testcase>