You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-commits@xmlgraphics.apache.org by ga...@apache.org on 2013/10/31 20:44:26 UTC
svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./
src/java/org/apache/fop/fo/ src/java/org/apache/fop/fo/flow/
src/java/org/apache/fop/fo/flow/table/
src/java/org/apache/fop/fo/pagination/ src/java/org/apache/fop/layoutmgr/
src/java/org/apache/fop/l...
Author: gadams
Date: Thu Oct 31 19:44:25 2013
New Revision: 1537600
URL: http://svn.apache.org/r1537600
Log:
FOP-2310: Fix misplaced table cell border in WM RTL context.
Modified:
xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/FObj.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/BlockContainer.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/InlineContainer.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/table/Table.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/BlockLayoutManager.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListBlockLayoutManager.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemContentLayoutManager.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemLayoutManager.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableAndCaptionLayoutManager.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCaptionLayoutManager.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCellLayoutManager.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/RTFPlaceHolderHelper.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/rtflib/tools/BuilderContext.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingMode.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraits.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsGetter.java
xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsSetter.java
xmlgraphics/fop/trunk/status.xml
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/FObj.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/FObj.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/FObj.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/FObj.java Thu Oct 31 19:44:25 2013
@@ -611,7 +611,7 @@ public abstract class FObj extends FONod
if (bidiLevel >= 0) {
if ((this.bidiLevel < 0) || (bidiLevel < this.bidiLevel)) {
this.bidiLevel = bidiLevel;
- if (parent != null) {
+ if ((parent != null) && !isBidiPropagationBoundary()) {
FObj foParent = (FObj) parent;
int parentBidiLevel = foParent.getBidiLevel();
if ((parentBidiLevel < 0) || (bidiLevel < parentBidiLevel)) {
@@ -646,10 +646,25 @@ public abstract class FObj extends FONod
return level;
}
}
+ if (isBidiInheritanceBoundary()) {
+ break;
+ }
}
return -1;
}
+ protected boolean isBidiBoundary(boolean propagate) {
+ return false;
+ }
+
+ private boolean isBidiInheritanceBoundary() {
+ return isBidiBoundary(false);
+ }
+
+ private boolean isBidiPropagationBoundary() {
+ return isBidiBoundary(true);
+ }
+
/**
* Add a new extension attachment to this FObj.
* (see org.apache.fop.fo.FONode for details)
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/BlockContainer.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/BlockContainer.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/BlockContainer.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/BlockContainer.java Thu Oct 31 19:44:25 2013
@@ -98,7 +98,8 @@ public class BlockContainer extends FObj
referenceOrientation = pList.get(PR_REFERENCE_ORIENTATION).getNumeric();
span = pList.get(PR_SPAN).getEnum();
writingModeTraits = new WritingModeTraits(
- WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()));
+ WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()),
+ pList.getExplicit(PR_WRITING_MODE) != null);
disableColumnBalancing = pList.get(PR_X_DISABLE_COLUMN_BALANCING).getEnum();
}
@@ -280,6 +281,14 @@ public class BlockContainer extends FObj
return writingModeTraits.getWritingMode();
}
+ /**
+ * Obtain writing mode explicit indicator.
+ * @return the writing mode explicit indicator
+ */
+ public boolean getExplicitWritingMode() {
+ return writingModeTraits.getExplicitWritingMode();
+ }
+
/** {@inheritDoc} */
public String getLocalName() {
return "block-container";
@@ -292,5 +301,10 @@ public class BlockContainer extends FObj
public int getNameId() {
return FO_BLOCK_CONTAINER;
}
-}
+ @Override
+ protected boolean isBidiBoundary(boolean propagate) {
+ return getExplicitWritingMode();
+ }
+
+}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/InlineContainer.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/InlineContainer.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/InlineContainer.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/InlineContainer.java Thu Oct 31 19:44:25 2013
@@ -96,7 +96,8 @@ public class InlineContainer extends FOb
overflow = pList.get(PR_OVERFLOW).getEnum();
referenceOrientation = pList.get(PR_REFERENCE_ORIENTATION).getNumeric();
writingModeTraits = new WritingModeTraits(
- WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()));
+ WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()),
+ pList.getExplicit(PR_WRITING_MODE) != null);
}
/**
@@ -238,6 +239,14 @@ public class InlineContainer extends FOb
return writingModeTraits.getWritingMode();
}
+ /**
+ * Obtain writing mode explicit indicator.
+ * @return the writing mode explicit indicator
+ */
+ public boolean getExplicitWritingMode() {
+ return writingModeTraits.getExplicitWritingMode();
+ }
+
/** {@inheritDoc} */
public String getLocalName() {
return "inline-container";
@@ -256,4 +265,9 @@ public class InlineContainer extends FOb
return false;
}
+ @Override
+ protected boolean isBidiBoundary(boolean propagate) {
+ return getExplicitWritingMode();
+ }
+
}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/table/Table.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/table/Table.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/table/Table.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/table/Table.java Thu Oct 31 19:44:25 2013
@@ -139,7 +139,8 @@ public class Table extends TableFObj imp
tableOmitFooterAtBreak = pList.get(PR_TABLE_OMIT_FOOTER_AT_BREAK).getEnum();
tableOmitHeaderAtBreak = pList.get(PR_TABLE_OMIT_HEADER_AT_BREAK).getEnum();
writingModeTraits = new WritingModeTraits(
- WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()));
+ WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()),
+ pList.getExplicit(PR_WRITING_MODE) != null);
//Bind extension properties
widowContentLimit = pList.get(PR_X_WIDOW_CONTENT_LIMIT).getLength();
@@ -554,6 +555,11 @@ public class Table extends TableFObj imp
return writingModeTraits.getWritingMode();
}
+ /** {@inheritDoc} */
+ public boolean getExplicitWritingMode() {
+ return writingModeTraits.getExplicitWritingMode();
+ }
+
/** @return the "fox:widow-content-limit" extension FO trait */
public Length getWidowContentLimit() {
return widowContentLimit;
@@ -620,4 +626,9 @@ public class Table extends TableFObj imp
return ranges;
}
+ @Override
+ protected boolean isBidiBoundary(boolean propagate) {
+ return getExplicitWritingMode();
+ }
+
}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java Thu Oct 31 19:44:25 2013
@@ -96,7 +96,8 @@ public class PageSequence extends Abstra
masterReference = pList.get(PR_MASTER_REFERENCE).getString();
referenceOrientation = pList.get(PR_REFERENCE_ORIENTATION).getNumeric();
writingModeTraits = new WritingModeTraits(
- WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()));
+ WritingMode.valueOf(pList.get(PR_WRITING_MODE).getEnum()),
+ pList.getExplicit(PR_WRITING_MODE) != null);
if (masterReference == null || masterReference.equals("")) {
missingPropertyError("master-reference");
}
@@ -403,6 +404,16 @@ public class PageSequence extends Abstra
}
}
+ /**
+ * {@inheritDoc}
+ */
+ public boolean getExplicitWritingMode() {
+ if (writingModeTraits != null) {
+ return writingModeTraits.getExplicitWritingMode();
+ } else {
+ return false;
+ }
+ }
@Override
protected Stack collectDelimitedTextRanges(Stack ranges, DelimitedTextRange currentRange) {
@@ -423,6 +434,11 @@ public class PageSequence extends Abstra
return ranges;
}
+ @Override
+ protected boolean isBidiBoundary(boolean propagate) {
+ return true;
+ }
+
/**
* Releases a page-sequence's children after the page-sequence has been fully processed.
*/
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/BlockLayoutManager.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/BlockLayoutManager.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/BlockLayoutManager.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/BlockLayoutManager.java Thu Oct 31 19:44:25 2013
@@ -360,7 +360,7 @@ public class BlockLayoutManager extends
curBlockArea.setIPD(super.getContentAreaIPD());
- curBlockArea.setBidiLevel(getBlockFO().getBidiLevel());
+ curBlockArea.setBidiLevel(getBlockFO().getBidiLevelRecursive());
TraitSetter.addBreaks(curBlockArea,
getBlockFO().getBreakBefore(), getBlockFO().getBreakAfter());
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListBlockLayoutManager.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListBlockLayoutManager.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListBlockLayoutManager.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListBlockLayoutManager.java Thu Oct 31 19:44:25 2013
@@ -242,6 +242,8 @@ public class ListBlockLayoutManager exte
int contentIPD = referenceIPD - getIPIndents();
curBlockArea.setIPD(contentIPD);
+ curBlockArea.setBidiLevel(getListBlockFO().getBidiLevel());
+
setCurrentArea(curBlockArea);
}
return curBlockArea;
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemContentLayoutManager.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemContentLayoutManager.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemContentLayoutManager.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemContentLayoutManager.java Thu Oct 31 19:44:25 2013
@@ -181,6 +181,7 @@ public class ListItemContentLayoutManage
//TODO: Check - itemIPD never set?
curBlockArea.setIPD(itemIPD);
//curBlockArea.setHeight();
+ curBlockArea.setBidiLevel(getPartFO().getBidiLevel());
TraitSetter.setProducerID(curBlockArea, getPartFO().getId());
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemLayoutManager.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemLayoutManager.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemLayoutManager.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/list/ListItemLayoutManager.java Thu Oct 31 19:44:25 2013
@@ -615,6 +615,8 @@ public class ListItemLayoutManager exten
int contentIPD = referenceIPD - getIPIndents();
curBlockArea.setIPD(contentIPD);
+ curBlockArea.setBidiLevel(fo.getBidiLevel());
+
setCurrentArea(curBlockArea);
}
return curBlockArea;
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableAndCaptionLayoutManager.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableAndCaptionLayoutManager.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableAndCaptionLayoutManager.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableAndCaptionLayoutManager.java Thu Oct 31 19:44:25 2013
@@ -184,6 +184,7 @@ public class TableAndCaptionLayoutManage
Area parentArea = parentLayoutManager.getParentArea(curBlockArea);
int referenceIPD = parentArea.getIPD();
curBlockArea.setIPD(referenceIPD);
+ curBlockArea.setBidiLevel(getTableAndCaptionFO().getBidiLevel());
// Get reference IPD from parentArea
setCurrentArea(curBlockArea); // ??? for generic operations
}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCaptionLayoutManager.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCaptionLayoutManager.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCaptionLayoutManager.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCaptionLayoutManager.java Thu Oct 31 19:44:25 2013
@@ -180,6 +180,7 @@ public class TableCaptionLayoutManager e
Area parentArea = parentLayoutManager.getParentArea(curBlockArea);
int referenceIPD = parentArea.getIPD();
curBlockArea.setIPD(referenceIPD);
+ curBlockArea.setBidiLevel(getTableCaptionFO().getBidiLevel());
// Get reference IPD from parentArea
setCurrentArea(curBlockArea); // ??? for generic operations
}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCellLayoutManager.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCellLayoutManager.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCellLayoutManager.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/layoutmgr/table/TableCellLayoutManager.java Thu Oct 31 19:44:25 2013
@@ -441,12 +441,14 @@ public class TableCellLayoutManager exte
Block[][] blocks = new Block[getTableCell().getNumberRowsSpanned()][getTableCell()
.getNumberColumnsSpanned()];
GridUnit[] gridUnits = (GridUnit[]) primaryGridUnit.getRows().get(startRow);
+ int level = getTableCell().getBidiLevelRecursive();
for (int x = 0; x < getTableCell().getNumberColumnsSpanned(); x++) {
GridUnit gu = gridUnits[x];
BorderInfo border = gu.getBorderBefore(borderBeforeWhich);
int borderWidth = border.getRetainedWidth() / 2;
if (borderWidth > 0) {
- addBorder(blocks, startRow, x, Trait.BORDER_BEFORE, border, firstOnPage);
+ addBorder(blocks, startRow, x, Trait.BORDER_BEFORE, border,
+ firstOnPage, level);
adjustYOffset(blocks[startRow][x], -borderWidth);
adjustBPD(blocks[startRow][x], -borderWidth);
}
@@ -457,7 +459,8 @@ public class TableCellLayoutManager exte
BorderInfo border = gu.getBorderAfter(borderAfterWhich);
int borderWidth = border.getRetainedWidth() / 2;
if (borderWidth > 0) {
- addBorder(blocks, endRow, x, Trait.BORDER_AFTER, border, lastOnPage);
+ addBorder(blocks, endRow, x, Trait.BORDER_AFTER, border,
+ lastOnPage, level);
adjustBPD(blocks[endRow][x], -borderWidth);
}
}
@@ -466,7 +469,8 @@ public class TableCellLayoutManager exte
BorderInfo border = gridUnits[0].getBorderStart();
int borderWidth = border.getRetainedWidth() / 2;
if (borderWidth > 0) {
- addBorder(blocks, y, 0, Trait.BORDER_START, border, inFirstColumn);
+ addBorder(blocks, y, 0, Trait.BORDER_START, border,
+ inFirstColumn, level);
adjustXOffset(blocks[y][0], borderWidth);
adjustIPD(blocks[y][0], -borderWidth);
}
@@ -474,7 +478,7 @@ public class TableCellLayoutManager exte
borderWidth = border.getRetainedWidth() / 2;
if (borderWidth > 0) {
addBorder(blocks, y, gridUnits.length - 1, Trait.BORDER_END, border,
- inLastColumn);
+ inLastColumn, level);
adjustIPD(blocks[y][gridUnits.length - 1], -borderWidth);
}
}
@@ -511,10 +515,12 @@ public class TableCellLayoutManager exte
if (getTableCell().getDisplayAlign() == EN_CENTER) {
Block space = new Block();
space.setBPD((cellBPD - usedBPD) / 2);
+ space.setBidiLevel(getTableCell().getBidiLevelRecursive());
curBlockArea.addBlock(space);
} else if (getTableCell().getDisplayAlign() == EN_AFTER) {
Block space = new Block();
space.setBPD(cellBPD - usedBPD);
+ space.setBidiLevel(getTableCell().getBidiLevelRecursive());
curBlockArea.addBlock(space);
}
}
@@ -590,11 +596,12 @@ public class TableCellLayoutManager exte
}
private void addBorder(Block[][] blocks, int i, int j, Integer side, BorderInfo border,
- boolean outer) {
+ boolean outer, int level) {
if (blocks[i][j] == null) {
blocks[i][j] = new Block();
blocks[i][j].addTrait(Trait.IS_REFERENCE_AREA, Boolean.TRUE);
blocks[i][j].setPositioning(Block.ABSOLUTE);
+ blocks[i][j].setBidiLevel(level);
}
blocks[i][j].addTrait(side, BorderProps.makeRectangular(border.getStyle(),
border.getRetainedWidth(), border.getColor(),
@@ -629,6 +636,7 @@ public class TableCellLayoutManager exte
block.setBPD(bpd);
block.setXOffset(xoffset + startIndent - paddingStart);
block.setYOffset(yoffset + borderBeforeWidth);
+ block.setBidiLevel(getTableCell().getBidiLevelRecursive());
return block;
}
@@ -654,6 +662,7 @@ public class TableCellLayoutManager exte
curBlockArea.setXOffset(xoffset + startIndent);
curBlockArea.setYOffset(yoffset);
curBlockArea.setIPD(cellIPD);
+ curBlockArea.setBidiLevel(getTableCell().getBidiLevelRecursive());
/*Area parentArea =*/ parentLayoutManager.getParentArea(curBlockArea);
// Get reference IPD from parentArea
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java Thu Oct 31 19:44:25 2013
@@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements
}
public StructureTreeElement startNode(String name, Attributes attributes, StructureTreeElement parent) {
- PDFStructElem parentElem = parent == null ? ancestors.getFirst() : (PDFStructElem) parent;
+ PDFStructElem parentElem;
+ if ((parent != null) && (parent instanceof PDFStructElem)) {
+ parentElem = (PDFStructElem) parent;
+ } else {
+ parentElem = ancestors.getFirst();
+ }
PDFStructElem structElem = createStructureElement(name, parentElem, attributes,
pdfFactory, eventBroadcaster);
ancestors.addFirst(structElem);
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/RTFPlaceHolderHelper.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/RTFPlaceHolderHelper.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/RTFPlaceHolderHelper.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/RTFPlaceHolderHelper.java Thu Oct 31 19:44:25 2013
@@ -65,8 +65,10 @@ public class RTFPlaceHolderHelper {
builderContext.pushContainer(newRow);
builderContext.getTableContext().selectFirstColumn();
}
- } catch (Exception ex) {
- throw new RtfException(ex.getMessage());
+ } catch (org.apache.fop.apps.FOPException e) {
+ throw new RtfException(e.getMessage());
+ } catch (java.io.IOException e) {
+ throw new RtfException(e.getMessage());
}
}
}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/rtflib/tools/BuilderContext.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/rtflib/tools/BuilderContext.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/rtflib/tools/BuilderContext.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/rtf/rtflib/tools/BuilderContext.java Thu Oct 31 19:44:25 2013
@@ -196,8 +196,8 @@ public class BuilderContext {
} else {
/* This should never happen unless a placeholder is not catered for
* in the RTFHandler.endContainer method. */
- LOG.warn("Unhandled RTF structure tag mismatch detected between " +
- aClass.getSimpleName() + " and "+object.getClass().getSimpleName());
+ LOG.warn("Unhandled RTF structure tag mismatch detected between "
+ + aClass.getSimpleName() + " and " + object.getClass().getSimpleName());
}
}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingMode.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingMode.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingMode.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingMode.java Thu Oct 31 19:44:25 2013
@@ -54,8 +54,9 @@ public final class WritingMode extends T
* Assign writing mode traits from this trait to the specified
* writing mode traits setter.
* @param wms a writing mode traits setter
+ * @param explicit true if writing mode property explicitly specified
*/
- public void assignWritingModeTraits(WritingModeTraitsSetter wms) {
+ public void assignWritingModeTraits(WritingModeTraitsSetter wms, boolean explicit) {
Direction inlineProgressionDirection;
Direction blockProgressionDirection;
Direction columnProgressionDirection;
@@ -97,7 +98,7 @@ public final class WritingMode extends T
wms.setColumnProgressionDirection(columnProgressionDirection);
wms.setRowProgressionDirection(rowProgressionDirection);
wms.setShiftDirection(shiftDirection);
- wms.setWritingMode(this);
+ wms.setWritingMode(this, explicit);
}
/**
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraits.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraits.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraits.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraits.java Thu Oct 31 19:44:25 2013
@@ -31,20 +31,21 @@ public class WritingModeTraits implement
private Direction rowProgressionDirection;
private Direction shiftDirection;
private WritingMode writingMode;
+ private boolean explicit;
/**
* Default writing mode traits constructor.
*/
public WritingModeTraits() {
- this (WritingMode.LR_TB);
+ this (WritingMode.LR_TB, false);
}
/**
* Construct writing mode traits using the specified writing mode.
* @param writingMode a writing mode traits object
*/
- public WritingModeTraits(WritingMode writingMode) {
- assignWritingModeTraits(writingMode);
+ public WritingModeTraits(WritingMode writingMode, boolean explicit) {
+ assignWritingModeTraits(writingMode, explicit);
}
/**
@@ -125,17 +126,25 @@ public class WritingModeTraits implement
}
/**
+ * @return the "explicit-writing-mode" trait.
+ */
+ public boolean getExplicitWritingMode() {
+ return explicit;
+ }
+
+ /**
* @param writingMode the "writing-mode" trait.
*/
- public void setWritingMode(WritingMode writingMode) {
+ public void setWritingMode(WritingMode writingMode, boolean explicit) {
this.writingMode = writingMode;
+ this.explicit = explicit;
}
/**
* @param writingMode the "writing-mode" trait.
*/
- public void assignWritingModeTraits(WritingMode writingMode) {
- writingMode.assignWritingModeTraits(this);
+ public void assignWritingModeTraits(WritingMode writingMode, boolean explicit) {
+ writingMode.assignWritingModeTraits(this, explicit);
}
/**
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsGetter.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsGetter.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsGetter.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsGetter.java Thu Oct 31 19:44:25 2013
@@ -55,4 +55,9 @@ public interface WritingModeTraitsGetter
*/
WritingMode getWritingMode();
+ /**
+ * @return the "explicit-writing-mode" trait
+ */
+ boolean getExplicitWritingMode();
+
}
Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsSetter.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsSetter.java?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsSetter.java (original)
+++ xmlgraphics/fop/trunk/src/java/org/apache/fop/traits/WritingModeTraitsSetter.java Thu Oct 31 19:44:25 2013
@@ -58,13 +58,14 @@ public interface WritingModeTraitsSetter
* Set value of writing-mode trait.
* @param writingMode the "writing-mode" trait
*/
- void setWritingMode(WritingMode writingMode);
+ void setWritingMode(WritingMode writingMode, boolean explicit);
/**
* Collectivelly assign values to all writing mode traits based upon a specific
* writing mode.
* @param writingMode the "writing-mode" trait
+ * @param explicit true if writing mode explicitly specified
*/
- void assignWritingModeTraits(WritingMode writingMode);
+ void assignWritingModeTraits(WritingMode writingMode, boolean explicit);
}
Modified: xmlgraphics/fop/trunk/status.xml
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/status.xml?rev=1537600&r1=1537599&r2=1537600&view=diff
==============================================================================
--- xmlgraphics/fop/trunk/status.xml (original)
+++ xmlgraphics/fop/trunk/status.xml Thu Oct 31 19:44:25 2013
@@ -59,6 +59,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="GA" type="fix" fixes-bug="FOP-2310">
+ Fix misplaced table cell border in WM RTL context.
+ </action>
<action context="Renderers" dev="GA" type="add" fixes-bug="FOP-2298">
Enable support for PDF page transitions.
</action>
---------------------------------------------------------------------
To unsubscribe, e-mail: fop-commits-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: fop-commits-help@xmlgraphics.apache.org
Re: svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./
src/java/org/apache/fop/fo/ src/java/org/apache/fop/fo/flow/
src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/pagination/
src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/l...
Posted by Glenn Adams <gl...@skynav.com>.
On Fri, Nov 1, 2013 at 10:50 AM, Vincent Hennebert <vh...@gmail.com>wrote:
> On 01/11/13 18:21, Glenn Adams wrote:
>
>> On Fri, Nov 1, 2013 at 9:11 AM, Vincent Hennebert <vhennebert@gmail.com
>> >wrote:
>>
>> On 01/11/13 16:52, Glenn Adams wrote:
>>>
>>> On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <gl...@skynav.com> wrote:
>>>>
>>>>
>>>> On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <
>>>>> vhennebert@gmail.com
>>>>>
>>>>>> wrote:
>>>>>>
>>>>>
>>>>> We have here a typical example of how code is being made worse due to
>>>>>
>>>>>> the rigidity of a code checking tool:
>>>>>>
>>>>> <snip/>
>
>
>>>>>> I suggest you revert this change.
>>>>>>
>>>>>>
>>>>>> No thank you. The original code is broken since it depends on use of
>>>>> a
>>>>> ClassCastException when it shouldn't. I only fixed this because
>>>>> findbugs
>>>>> reported this problem and whomever checked in in the first place didn't
>>>>> fix
>>>>> the warning correctly.
>>>>>
>>>>> I have recently pointed out on a couple of occasions that people are
>>>>> leaving checkstyle or findbugs warnings in the tree and not fixing
>>>>> them.
>>>>> If
>>>>> they don't fix them in the first place, then I'll do it when I do a
>>>>> commit
>>>>> in order to get a clean tree. If they want to go back and revise my
>>>>> fix,
>>>>> that's fine with me as long as they don't leave warnings lying around.
>>>>>
>>>>>
>>>>> To elaborate to make sure I'm understood:
>>>>
>>>> If anyone commits a change that introduces a new checkbug or findbug
>>>> warning, or breaks an existing junit test, then that person is
>>>> responsible
>>>> for fixing the problem ASAP. If they fail to do so, then whatever
>>>> fix-ups
>>>> get applied by other committers is acceptable provided they don't
>>>> introduce
>>>> other new warnings or break existing tests.
>>>>
>>>> This is just SOP.
>>>>
>>>>
>>> We do have a no Checkstyle warning policy after the revised Checkstyle
>>> configuration file we agreed on some time ago.
>>>
>>> However, we have never made a similar agreement on FindBugs, precisely
>>> because people couldn’t agree on the interest of this tool due to the
>>> high ratio of noise (at least in its standard configuration).
>>>
>>> If you want to use FindBugs on your local copy, then you’re absolutely
>>> free to do so, but please don’t impose it on other people without first
>>> reaching an agreement about it.
>>>
>>>
>> I beg to differ. We have been operating on a no warnings policy for
>> findbugs as long as I've been with the project.
>>
>
> No we haven’t. We had a discussion a few years ago about introducing
> such a policy, but people were not excited about it and no vote
> occurred:
> http://markmail.org/message/**taztvj2ms7x6tryb<http://markmail.org/message/taztvj2ms7x6tryb>
>
>
>
> The fact that folks are
>> keeping it up to date (without warnings) in general proves that is the
>> case.
>>
>
> It just shows that more than one developers informally use FindBugs,
> that’s all.
>
> As far as I’m concerned, I find the amount of false positives (in its
> stock configuration) an annoyance rather than a help. And, in the
> present case, counter-productive.
There's an easy fix here Vincent. Just start running and fixing findbugs
warnings like the rest of us. Then you won't have to complain about me
fixing your findbugs warnings in a way you don't like.
Yes, it is a bit annoying, but I find it occasionally catches real bugs.
>
>
>
> IMO your change is not acceptable because it makes the code less
>>> future-proof (less robust to future changes). Which to me is at least as
>>> important as introducing warnings or breaking tests.
>>>
>>
>>
>> So fix it in the way you like that doesn't introduce a findbugs warning.
>>
>
>
> Vincent
>
Re: svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./ src/java/org/apache/fop/fo/
src/java/org/apache/fop/fo/flow/ src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/pagination/
src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/l...
Posted by Vincent Hennebert <vh...@gmail.com>.
On 01/11/13 18:21, Glenn Adams wrote:
> On Fri, Nov 1, 2013 at 9:11 AM, Vincent Hennebert <vh...@gmail.com>wrote:
>
>> On 01/11/13 16:52, Glenn Adams wrote:
>>
>>> On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <gl...@skynav.com> wrote:
>>>
>>>
>>>> On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <vhennebert@gmail.com
>>>>> wrote:
>>>>
>>>> We have here a typical example of how code is being made worse due to
>>>>> the rigidity of a code checking tool:
<snip/>
>>>>>
>>>>> I suggest you revert this change.
>>>>>
>>>>>
>>>> No thank you. The original code is broken since it depends on use of a
>>>> ClassCastException when it shouldn't. I only fixed this because findbugs
>>>> reported this problem and whomever checked in in the first place didn't
>>>> fix
>>>> the warning correctly.
>>>>
>>>> I have recently pointed out on a couple of occasions that people are
>>>> leaving checkstyle or findbugs warnings in the tree and not fixing them.
>>>> If
>>>> they don't fix them in the first place, then I'll do it when I do a
>>>> commit
>>>> in order to get a clean tree. If they want to go back and revise my fix,
>>>> that's fine with me as long as they don't leave warnings lying around.
>>>>
>>>>
>>> To elaborate to make sure I'm understood:
>>>
>>> If anyone commits a change that introduces a new checkbug or findbug
>>> warning, or breaks an existing junit test, then that person is responsible
>>> for fixing the problem ASAP. If they fail to do so, then whatever fix-ups
>>> get applied by other committers is acceptable provided they don't
>>> introduce
>>> other new warnings or break existing tests.
>>>
>>> This is just SOP.
>>>
>>
>> We do have a no Checkstyle warning policy after the revised Checkstyle
>> configuration file we agreed on some time ago.
>>
>> However, we have never made a similar agreement on FindBugs, precisely
>> because people couldn’t agree on the interest of this tool due to the
>> high ratio of noise (at least in its standard configuration).
>>
>> If you want to use FindBugs on your local copy, then you’re absolutely
>> free to do so, but please don’t impose it on other people without first
>> reaching an agreement about it.
>>
>
> I beg to differ. We have been operating on a no warnings policy for
> findbugs as long as I've been with the project.
No we haven’t. We had a discussion a few years ago about introducing
such a policy, but people were not excited about it and no vote
occurred:
http://markmail.org/message/taztvj2ms7x6tryb
> The fact that folks are
> keeping it up to date (without warnings) in general proves that is the case.
It just shows that more than one developers informally use FindBugs,
that’s all.
As far as I’m concerned, I find the amount of false positives (in its
stock configuration) an annoyance rather than a help. And, in the
present case, counter-productive.
>> IMO your change is not acceptable because it makes the code less
>> future-proof (less robust to future changes). Which to me is at least as
>> important as introducing warnings or breaking tests.
>
>
> So fix it in the way you like that doesn't introduce a findbugs warning.
Vincent
Re: svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./
src/java/org/apache/fop/fo/ src/java/org/apache/fop/fo/flow/
src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/pagination/
src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/l...
Posted by Glenn Adams <gl...@skynav.com>.
On Fri, Nov 1, 2013 at 9:11 AM, Vincent Hennebert <vh...@gmail.com>wrote:
> On 01/11/13 16:52, Glenn Adams wrote:
>
>> On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <gl...@skynav.com> wrote:
>>
>>
>>> On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <vhennebert@gmail.com
>>> >wrote:
>>>
>>> We have here a typical example of how code is being made worse due to
>>>> the rigidity of a code checking tool:
>>>>
>>>> Author: gadams
>>>>
>>>>> Date: Thu Oct 31 19:44:25 2013
>>>>> New Revision: 1537600
>>>>>
>>>>> Modified:
>>>>> xmlgraphics/fop/trunk/src/****java/org/apache/fop/render/**
>>>>> pdf/PDFStructureTreeBuilder.****java
>>>>> URL: http://svn.apache.org/viewvc/****xmlgraphics/fop/trunk/src/**<http://svn.apache.org/viewvc/**xmlgraphics/fop/trunk/src/**>
>>>>> java/org/apache/fop/render/****pdf/PDFStructureTreeBuilder.**
>>>>> java?rev=1537600&r1=1537599&****r2=1537600&view=diff<http://**
>>>>> svn.apache.org/viewvc/**xmlgraphics/fop/trunk/src/**
>>>>> java/org/apache/fop/render/**pdf/PDFStructureTreeBuilder.**
>>>>> java?rev=1537600&r1=1537599&**r2=1537600&view=diff<http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff>
>>>>> >
>>>>> ==============================****============================**==**
>>>>> ==================
>>>>> --- xmlgraphics/fop/trunk/src/****java/org/apache/fop/render/**
>>>>> pdf/PDFStructureTreeBuilder.****java (original)
>>>>> +++ xmlgraphics/fop/trunk/src/****java/org/apache/fop/render/**
>>>>> pdf/PDFStructureTreeBuilder.****java Thu Oct 31 19:44:25 2013
>>>>>
>>>>> @@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements
>>>>> }
>>>>>
>>>>> public StructureTreeElement startNode(String name, Attributes
>>>>> attributes, StructureTreeElement parent) {
>>>>> - PDFStructElem parentElem = parent == null ?
>>>>> ancestors.getFirst() : (PDFStructElem) parent;
>>>>> + PDFStructElem parentElem;
>>>>> + if ((parent != null) && (parent instanceof PDFStructElem)) {
>>>>> + parentElem = (PDFStructElem) parent;
>>>>> + } else {
>>>>> + parentElem = ancestors.getFirst();
>>>>> + }
>>>>> PDFStructElem structElem = createStructureElement(name,
>>>>> parentElem, attributes,
>>>>> pdfFactory, eventBroadcaster);
>>>>> ancestors.addFirst(structElem)****;
>>>>>
>>>>>
>>>> The modified code doesn’t behave the same as the original. In the
>>>> original code, if parent is not null but is not an instance of
>>>> PDFStructElem, then a ClassCastException will occur.
>>>>
>>>> In the new code, parentElem will silently be set to
>>>> ancestors.getFirst().
>>>>
>>>> The original code is fail-fast: if a mistake is introduced in a later
>>>> modification, such that it’s no longer an instance of PDFStructElem that
>>>> is being passed, then it will throw an exception. Forcing the developer
>>>> to reconsider the change, or adapt this method appropriately.
>>>>
>>>> With the new version, no exception will be thrown, and depending on the
>>>> modification, it may or may not be correct. More likely not. And this
>>>> will go unnoticed. And if it’s noticed, then a debugging session will
>>>> have to be launched, and the code studied, and after a while the
>>>> developer may arrive in this method, and then wonder why there is this
>>>> instanceof check, what does it mean, what should be done for
>>>> a non-PDFStructElem, etc. And after some (long) time being passed they
>>>> might figure out that there was a mistake in their modification.
>>>>
>>>> I suggest you revert this change.
>>>>
>>>>
>>> No thank you. The original code is broken since it depends on use of a
>>> ClassCastException when it shouldn't. I only fixed this because findbugs
>>> reported this problem and whomever checked in in the first place didn't
>>> fix
>>> the warning correctly.
>>>
>>> I have recently pointed out on a couple of occasions that people are
>>> leaving checkstyle or findbugs warnings in the tree and not fixing them.
>>> If
>>> they don't fix them in the first place, then I'll do it when I do a
>>> commit
>>> in order to get a clean tree. If they want to go back and revise my fix,
>>> that's fine with me as long as they don't leave warnings lying around.
>>>
>>>
>> To elaborate to make sure I'm understood:
>>
>> If anyone commits a change that introduces a new checkbug or findbug
>> warning, or breaks an existing junit test, then that person is responsible
>> for fixing the problem ASAP. If they fail to do so, then whatever fix-ups
>> get applied by other committers is acceptable provided they don't
>> introduce
>> other new warnings or break existing tests.
>>
>> This is just SOP.
>>
>
> We do have a no Checkstyle warning policy after the revised Checkstyle
> configuration file we agreed on some time ago.
>
> However, we have never made a similar agreement on FindBugs, precisely
> because people couldn’t agree on the interest of this tool due to the
> high ratio of noise (at least in its standard configuration).
>
> If you want to use FindBugs on your local copy, then you’re absolutely
> free to do so, but please don’t impose it on other people without first
> reaching an agreement about it.
>
I beg to differ. We have been operating on a no warnings policy for
findbugs as long as I've been with the project. The fact that folks are
keeping it up to date (without warnings) in general proves that is the case.
>
> IMO your change is not acceptable because it makes the code less
> future-proof (less robust to future changes). Which to me is at least as
> important as introducing warnings or breaking tests.
So fix it in the way you like that doesn't introduce a findbugs warning.
>
>
>
> Vincent
>
Re: svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./ src/java/org/apache/fop/fo/
src/java/org/apache/fop/fo/flow/ src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/pagination/
src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/l...
Posted by Vincent Hennebert <vh...@gmail.com>.
On 01/11/13 16:52, Glenn Adams wrote:
> On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <gl...@skynav.com> wrote:
>
>>
>> On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <vh...@gmail.com>wrote:
>>
>>> We have here a typical example of how code is being made worse due to
>>> the rigidity of a code checking tool:
>>>
>>> Author: gadams
>>>> Date: Thu Oct 31 19:44:25 2013
>>>> New Revision: 1537600
>>>>
>>>> Modified:
>>>> xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>>>> pdf/PDFStructureTreeBuilder.**java
>>>> URL: http://svn.apache.org/viewvc/**xmlgraphics/fop/trunk/src/**
>>>> java/org/apache/fop/render/**pdf/PDFStructureTreeBuilder.**
>>>> java?rev=1537600&r1=1537599&**r2=1537600&view=diff<http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff>
>>>> ==============================**==============================**
>>>> ==================
>>>> --- xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>>>> pdf/PDFStructureTreeBuilder.**java (original)
>>>> +++ xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>>>> pdf/PDFStructureTreeBuilder.**java Thu Oct 31 19:44:25 2013
>>>> @@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements
>>>> }
>>>>
>>>> public StructureTreeElement startNode(String name, Attributes
>>>> attributes, StructureTreeElement parent) {
>>>> - PDFStructElem parentElem = parent == null ?
>>>> ancestors.getFirst() : (PDFStructElem) parent;
>>>> + PDFStructElem parentElem;
>>>> + if ((parent != null) && (parent instanceof PDFStructElem)) {
>>>> + parentElem = (PDFStructElem) parent;
>>>> + } else {
>>>> + parentElem = ancestors.getFirst();
>>>> + }
>>>> PDFStructElem structElem = createStructureElement(name,
>>>> parentElem, attributes,
>>>> pdfFactory, eventBroadcaster);
>>>> ancestors.addFirst(structElem)**;
>>>>
>>>
>>> The modified code doesn’t behave the same as the original. In the
>>> original code, if parent is not null but is not an instance of
>>> PDFStructElem, then a ClassCastException will occur.
>>>
>>> In the new code, parentElem will silently be set to
>>> ancestors.getFirst().
>>>
>>> The original code is fail-fast: if a mistake is introduced in a later
>>> modification, such that it’s no longer an instance of PDFStructElem that
>>> is being passed, then it will throw an exception. Forcing the developer
>>> to reconsider the change, or adapt this method appropriately.
>>>
>>> With the new version, no exception will be thrown, and depending on the
>>> modification, it may or may not be correct. More likely not. And this
>>> will go unnoticed. And if it’s noticed, then a debugging session will
>>> have to be launched, and the code studied, and after a while the
>>> developer may arrive in this method, and then wonder why there is this
>>> instanceof check, what does it mean, what should be done for
>>> a non-PDFStructElem, etc. And after some (long) time being passed they
>>> might figure out that there was a mistake in their modification.
>>>
>>> I suggest you revert this change.
>>>
>>
>> No thank you. The original code is broken since it depends on use of a
>> ClassCastException when it shouldn't. I only fixed this because findbugs
>> reported this problem and whomever checked in in the first place didn't fix
>> the warning correctly.
>>
>> I have recently pointed out on a couple of occasions that people are
>> leaving checkstyle or findbugs warnings in the tree and not fixing them. If
>> they don't fix them in the first place, then I'll do it when I do a commit
>> in order to get a clean tree. If they want to go back and revise my fix,
>> that's fine with me as long as they don't leave warnings lying around.
>>
>
> To elaborate to make sure I'm understood:
>
> If anyone commits a change that introduces a new checkbug or findbug
> warning, or breaks an existing junit test, then that person is responsible
> for fixing the problem ASAP. If they fail to do so, then whatever fix-ups
> get applied by other committers is acceptable provided they don't introduce
> other new warnings or break existing tests.
>
> This is just SOP.
We do have a no Checkstyle warning policy after the revised Checkstyle
configuration file we agreed on some time ago.
However, we have never made a similar agreement on FindBugs, precisely
because people couldn’t agree on the interest of this tool due to the
high ratio of noise (at least in its standard configuration).
If you want to use FindBugs on your local copy, then you’re absolutely
free to do so, but please don’t impose it on other people without first
reaching an agreement about it.
IMO your change is not acceptable because it makes the code less
future-proof (less robust to future changes). Which to me is at least as
important as introducing warnings or breaking tests.
Vincent
Re: svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./
src/java/org/apache/fop/fo/ src/java/org/apache/fop/fo/flow/
src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/pagination/
src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/l...
Posted by Glenn Adams <gl...@skynav.com>.
On Fri, Nov 1, 2013 at 7:39 AM, Glenn Adams <gl...@skynav.com> wrote:
>
> On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <vh...@gmail.com>wrote:
>
>> We have here a typical example of how code is being made worse due to
>> the rigidity of a code checking tool:
>>
>> Author: gadams
>>> Date: Thu Oct 31 19:44:25 2013
>>> New Revision: 1537600
>>>
>>> Modified:
>>> xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>>> pdf/PDFStructureTreeBuilder.**java
>>> URL: http://svn.apache.org/viewvc/**xmlgraphics/fop/trunk/src/**
>>> java/org/apache/fop/render/**pdf/PDFStructureTreeBuilder.**
>>> java?rev=1537600&r1=1537599&**r2=1537600&view=diff<http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff>
>>> ==============================**==============================**
>>> ==================
>>> --- xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>>> pdf/PDFStructureTreeBuilder.**java (original)
>>> +++ xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>>> pdf/PDFStructureTreeBuilder.**java Thu Oct 31 19:44:25 2013
>>> @@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements
>>> }
>>>
>>> public StructureTreeElement startNode(String name, Attributes
>>> attributes, StructureTreeElement parent) {
>>> - PDFStructElem parentElem = parent == null ?
>>> ancestors.getFirst() : (PDFStructElem) parent;
>>> + PDFStructElem parentElem;
>>> + if ((parent != null) && (parent instanceof PDFStructElem)) {
>>> + parentElem = (PDFStructElem) parent;
>>> + } else {
>>> + parentElem = ancestors.getFirst();
>>> + }
>>> PDFStructElem structElem = createStructureElement(name,
>>> parentElem, attributes,
>>> pdfFactory, eventBroadcaster);
>>> ancestors.addFirst(structElem)**;
>>>
>>
>> The modified code doesn’t behave the same as the original. In the
>> original code, if parent is not null but is not an instance of
>> PDFStructElem, then a ClassCastException will occur.
>>
>> In the new code, parentElem will silently be set to
>> ancestors.getFirst().
>>
>> The original code is fail-fast: if a mistake is introduced in a later
>> modification, such that it’s no longer an instance of PDFStructElem that
>> is being passed, then it will throw an exception. Forcing the developer
>> to reconsider the change, or adapt this method appropriately.
>>
>> With the new version, no exception will be thrown, and depending on the
>> modification, it may or may not be correct. More likely not. And this
>> will go unnoticed. And if it’s noticed, then a debugging session will
>> have to be launched, and the code studied, and after a while the
>> developer may arrive in this method, and then wonder why there is this
>> instanceof check, what does it mean, what should be done for
>> a non-PDFStructElem, etc. And after some (long) time being passed they
>> might figure out that there was a mistake in their modification.
>>
>> I suggest you revert this change.
>>
>
> No thank you. The original code is broken since it depends on use of a
> ClassCastException when it shouldn't. I only fixed this because findbugs
> reported this problem and whomever checked in in the first place didn't fix
> the warning correctly.
>
> I have recently pointed out on a couple of occasions that people are
> leaving checkstyle or findbugs warnings in the tree and not fixing them. If
> they don't fix them in the first place, then I'll do it when I do a commit
> in order to get a clean tree. If they want to go back and revise my fix,
> that's fine with me as long as they don't leave warnings lying around.
>
To elaborate to make sure I'm understood:
If anyone commits a change that introduces a new checkbug or findbug
warning, or breaks an existing junit test, then that person is responsible
for fixing the problem ASAP. If they fail to do so, then whatever fix-ups
get applied by other committers is acceptable provided they don't introduce
other new warnings or break existing tests.
This is just SOP.
>
>
>>
>> Thanks,
>> Vincent
>>
>
>
Re: svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./
src/java/org/apache/fop/fo/ src/java/org/apache/fop/fo/flow/
src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/pagination/
src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/l...
Posted by Glenn Adams <gl...@skynav.com>.
On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <vh...@gmail.com>wrote:
> We have here a typical example of how code is being made worse due to
> the rigidity of a code checking tool:
>
> Author: gadams
>> Date: Thu Oct 31 19:44:25 2013
>> New Revision: 1537600
>>
>> Modified:
>> xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>> pdf/PDFStructureTreeBuilder.**java
>> URL: http://svn.apache.org/viewvc/**xmlgraphics/fop/trunk/src/**
>> java/org/apache/fop/render/**pdf/PDFStructureTreeBuilder.**
>> java?rev=1537600&r1=1537599&**r2=1537600&view=diff<http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff>
>> ==============================**==============================**
>> ==================
>> --- xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>> pdf/PDFStructureTreeBuilder.**java (original)
>> +++ xmlgraphics/fop/trunk/src/**java/org/apache/fop/render/**
>> pdf/PDFStructureTreeBuilder.**java Thu Oct 31 19:44:25 2013
>> @@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements
>> }
>>
>> public StructureTreeElement startNode(String name, Attributes
>> attributes, StructureTreeElement parent) {
>> - PDFStructElem parentElem = parent == null ? ancestors.getFirst()
>> : (PDFStructElem) parent;
>> + PDFStructElem parentElem;
>> + if ((parent != null) && (parent instanceof PDFStructElem)) {
>> + parentElem = (PDFStructElem) parent;
>> + } else {
>> + parentElem = ancestors.getFirst();
>> + }
>> PDFStructElem structElem = createStructureElement(name,
>> parentElem, attributes,
>> pdfFactory, eventBroadcaster);
>> ancestors.addFirst(structElem)**;
>>
>
> The modified code doesn’t behave the same as the original. In the
> original code, if parent is not null but is not an instance of
> PDFStructElem, then a ClassCastException will occur.
>
> In the new code, parentElem will silently be set to
> ancestors.getFirst().
>
> The original code is fail-fast: if a mistake is introduced in a later
> modification, such that it’s no longer an instance of PDFStructElem that
> is being passed, then it will throw an exception. Forcing the developer
> to reconsider the change, or adapt this method appropriately.
>
> With the new version, no exception will be thrown, and depending on the
> modification, it may or may not be correct. More likely not. And this
> will go unnoticed. And if it’s noticed, then a debugging session will
> have to be launched, and the code studied, and after a while the
> developer may arrive in this method, and then wonder why there is this
> instanceof check, what does it mean, what should be done for
> a non-PDFStructElem, etc. And after some (long) time being passed they
> might figure out that there was a mistake in their modification.
>
> I suggest you revert this change.
>
No thank you. The original code is broken since it depends on use of a
ClassCastException when it shouldn't. I only fixed this because findbugs
reported this problem and whomever checked in in the first place didn't fix
the warning correctly.
I have recently pointed out on a couple of occasions that people are
leaving checkstyle or findbugs warnings in the tree and not fixing them. If
they don't fix them in the first place, then I'll do it when I do a commit
in order to get a clean tree. If they want to go back and revise my fix,
that's fine with me as long as they don't leave warnings lying around.
>
> Thanks,
> Vincent
>
Re: svn commit: r1537600 - in /xmlgraphics/fop/trunk: ./ src/java/org/apache/fop/fo/
src/java/org/apache/fop/fo/flow/ src/java/org/apache/fop/fo/flow/table/ src/java/org/apache/fop/fo/pagination/
src/java/org/apache/fop/layoutmgr/ src/java/org/apache/fop/l...
Posted by Vincent Hennebert <vh...@gmail.com>.
We have here a typical example of how code is being made worse due to
the rigidity of a code checking tool:
> Author: gadams
> Date: Thu Oct 31 19:44:25 2013
> New Revision: 1537600
>
> Modified:
> xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java
> URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff
> ==============================================================================
> --- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java (original)
> +++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java Thu Oct 31 19:44:25 2013
> @@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements
> }
>
> public StructureTreeElement startNode(String name, Attributes attributes, StructureTreeElement parent) {
> - PDFStructElem parentElem = parent == null ? ancestors.getFirst() : (PDFStructElem) parent;
> + PDFStructElem parentElem;
> + if ((parent != null) && (parent instanceof PDFStructElem)) {
> + parentElem = (PDFStructElem) parent;
> + } else {
> + parentElem = ancestors.getFirst();
> + }
> PDFStructElem structElem = createStructureElement(name, parentElem, attributes,
> pdfFactory, eventBroadcaster);
> ancestors.addFirst(structElem);
The modified code doesn’t behave the same as the original. In the
original code, if parent is not null but is not an instance of
PDFStructElem, then a ClassCastException will occur.
In the new code, parentElem will silently be set to
ancestors.getFirst().
The original code is fail-fast: if a mistake is introduced in a later
modification, such that it’s no longer an instance of PDFStructElem that
is being passed, then it will throw an exception. Forcing the developer
to reconsider the change, or adapt this method appropriately.
With the new version, no exception will be thrown, and depending on the
modification, it may or may not be correct. More likely not. And this
will go unnoticed. And if it’s noticed, then a debugging session will
have to be launched, and the code studied, and after a while the
developer may arrive in this method, and then wonder why there is this
instanceof check, what does it mean, what should be done for
a non-PDFStructElem, etc. And after some (long) time being passed they
might figure out that there was a mistake in their modification.
I suggest you revert this change.
Thanks,
Vincent