diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 2b12bdf79..eab4f6463 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -36,6 +36,7 @@ + 44609 - Handle leading spaces in formulas, such as '= 4' 44608 - Support for PercentPtg in the formula evaluator 44606 - Support calculated string values for evaluated formulas Add accessors to horizontal and vertical alignment in HSSFTextbox diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 57bc3fa34..a6467d516 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -33,6 +33,7 @@ + 44609 - Handle leading spaces in formulas, such as '= 4' 44608 - Support for PercentPtg in the formula evaluator 44606 - Support calculated string values for evaluated formulas Add accessors to horizontal and vertical alignment in HSSFTextbox diff --git a/src/java/org/apache/poi/hssf/model/FormulaParser.java b/src/java/org/apache/poi/hssf/model/FormulaParser.java index 7b89b90d8..07bb51483 100644 --- a/src/java/org/apache/poi/hssf/model/FormulaParser.java +++ b/src/java/org/apache/poi/hssf/model/FormulaParser.java @@ -943,23 +943,7 @@ end; } Stack stack = new Stack(); - // Excel allows to have AttrPtg at position 0 (such as Blanks) which - // do not have any operands. Skip them. - int i; - if(ptgs[0] instanceof AttrPtg) { - AttrPtg attrPtg0 = (AttrPtg) ptgs[0]; - if(attrPtg0.isSemiVolatile()) { - // no visible formula for semi-volatile - } else { - // TODO -this requirement is unclear and is not addressed by any junits - stack.push(ptgs[0].toFormulaString(book)); - } - i=1; - } else { - i=0; - } - - for ( ; i < ptgs.length; i++) { + for (int i=0 ; i < ptgs.length; i++) { Ptg ptg = ptgs[i]; // TODO - what about MemNoMemPtg? if(ptg instanceof MemAreaPtg || ptg instanceof MemFuncPtg || ptg instanceof MemErrPtg) { @@ -973,21 +957,30 @@ end; continue; } - if (ptg instanceof AttrPtg && ((AttrPtg) ptg).isOptimizedIf()) { - continue; + if (ptg instanceof AttrPtg) { + AttrPtg attrPtg = ((AttrPtg) ptg); + if (attrPtg.isOptimizedIf()) { + continue; + } + if (attrPtg.isSpace()) { + // POI currently doesn't render spaces in formulas + continue; + // but if it ever did, care must be taken: + // tAttrSpace comes *before* the operand it applies to, which may be consistent + // with how the formula text appears but is against the RPN ordering assumed here + } } final OperationPtg o = (OperationPtg) ptg; int nOperands = o.getNumberOfOperands(); final String[] operands = new String[nOperands]; - for (int j = nOperands-1; j >= 0; j--) { + for (int j = nOperands-1; j >= 0; j--) { // reverse iteration because args were pushed in-order if(stack.isEmpty()) { - //TODO: write junit to prove this works String msg = "Too few arguments suppled to operation token (" + o.getClass().getName() + "). Expected (" + nOperands - + " but got " + (nOperands - j + 1); - throw new FormulaParseException(msg); + + ") operands but got (" + (nOperands - j + 1) + ")"; + throw new IllegalStateException(msg); } operands[j] = (String) stack.pop(); } diff --git a/src/java/org/apache/poi/hssf/record/formula/AttrPtg.java b/src/java/org/apache/poi/hssf/record/formula/AttrPtg.java index 2ba472380..d355fbfa1 100644 --- a/src/java/org/apache/poi/hssf/record/formula/AttrPtg.java +++ b/src/java/org/apache/poi/hssf/record/formula/AttrPtg.java @@ -33,20 +33,42 @@ import org.apache.poi.util.BitFieldFactory; * @author Jason Height (jheight at chariot dot net dot au) */ -public class AttrPtg - extends OperationPtg -{ +public final class AttrPtg extends OperationPtg { public final static byte sid = 0x19; private final static int SIZE = 4; private byte field_1_options; private short field_2_data; - private BitField semiVolatile = BitFieldFactory.getInstance(0x01); - private BitField optiIf = BitFieldFactory.getInstance(0x02); - private BitField optiChoose = BitFieldFactory.getInstance(0x04); - private BitField optGoto = BitFieldFactory.getInstance(0x08); - private BitField sum = BitFieldFactory.getInstance(0x10); - private BitField baxcel = BitFieldFactory.getInstance(0x20); - private BitField space = BitFieldFactory.getInstance(0x40); + + // flags 'volatile' and 'space', can be combined. + // OOO spec says other combinations are theoretically possible but not likely to occur. + private static final BitField semiVolatile = BitFieldFactory.getInstance(0x01); + private static final BitField optiIf = BitFieldFactory.getInstance(0x02); + private static final BitField optiChoose = BitFieldFactory.getInstance(0x04); + private static final BitField optGoto = BitFieldFactory.getInstance(0x08); // skip + private static final BitField sum = BitFieldFactory.getInstance(0x10); + private static final BitField baxcel = BitFieldFactory.getInstance(0x20); // 'assignment-style formula in a macro sheet' + private static final BitField space = BitFieldFactory.getInstance(0x40); + + public static final class SpaceType { + private SpaceType() { + // no instances of this class + } + + /** 00H = Spaces before the next token (not allowed before tParen token) */ + public static final int SPACE_BEFORE = 0x00; + /** 01H = Carriage returns before the next token (not allowed before tParen token) */ + public static final int CR_BEFORE = 0x01; + /** 02H = Spaces before opening parenthesis (only allowed before tParen token) */ + public static final int SPACE_BEFORE_OPEN_PAREN = 0x02; + /** 03H = Carriage returns before opening parenthesis (only allowed before tParen token) */ + public static final int CR_BEFORE_OPEN_PAREN = 0x03; + /** 04H = Spaces before closing parenthesis (only allowed before tParen, tFunc, and tFuncVar tokens) */ + public static final int SPACE_BEFORE_CLOSE_PAERN = 0x04; + /** 05H = Carriage returns before closing parenthesis (only allowed before tParen, tFunc, and tFuncVar tokens) */ + public static final int CR_BEFORE_CLOSE_PAREN = 0x05; + /** 06H = Spaces following the equality sign (only in macro sheets) */ + public static final int SPACE_AFTER_EQUALITY = 0x06; + } public AttrPtg() { } @@ -56,6 +78,19 @@ public class AttrPtg field_1_options = in.readByte(); field_2_data = in.readShort(); } + private AttrPtg(int options, int data) { + field_1_options = (byte) options; + field_2_data = (short) data; + } + + /** + * @param type a constant from SpaceType + * @param count the number of space characters + */ + public static AttrPtg createSpace(int type, int count) { + int data = type & 0x00FF | (count << 8) & 0x00FFFF; + return new AttrPtg(space.set(0), data); + } public void setOptions(byte options) { @@ -131,21 +166,31 @@ public class AttrPtg return field_2_data; } - public String toString() - { - StringBuffer buffer = new StringBuffer(); + public String toString() { + StringBuffer sb = new StringBuffer(64); + sb.append(getClass().getName()).append(" ["); - buffer.append("AttrPtg\n"); - buffer.append("options=").append(field_1_options).append("\n"); - buffer.append("data =").append(field_2_data).append("\n"); - buffer.append("semi =").append(isSemiVolatile()).append("\n"); - buffer.append("optimif=").append(isOptimizedIf()).append("\n"); - buffer.append("optchos=").append(isOptimizedChoose()).append("\n"); - buffer.append("isGoto =").append(isGoto()).append("\n"); - buffer.append("isSum =").append(isSum()).append("\n"); - buffer.append("isBaxce=").append(isBaxcel()).append("\n"); - buffer.append("isSpace=").append(isSpace()).append("\n"); - return buffer.toString(); + if(isSemiVolatile()) { + sb.append("volatile "); + } + if(isSpace()) { + sb.append("space count=").append((field_2_data >> 8) & 0x00FF); + sb.append(" type=").append(field_2_data & 0x00FF).append(" "); + } + // the rest seem to be mutually exclusive + if(isOptimizedIf()) { + sb.append("if dist=").append(getData()); + } else if(isOptimizedChoose()) { + sb.append("choose dist=").append(getData()); + } else if(isGoto()) { + sb.append("skip dist=").append(getData()); + } else if(isSum()) { + sb.append("sum "); + } else if(isBaxcel()) { + sb.append("assign "); + } + sb.append("]"); + return sb.toString(); } public void writeBytes(byte [] array, int offset) diff --git a/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java b/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java index f922e75d8..b79236ff0 100644 --- a/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java +++ b/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java @@ -844,4 +844,48 @@ public final class TestFormulaParser extends TestCase { } assertEquals("SUM(A32769:A32770)", cell.getCellFormula()); } + + public void testSpaceAtStartOfFormula() { + // Simulating cell formula of "= 4" (note space) + // The same Ptg array can be observed if an excel file is saved with that exact formula + + AttrPtg spacePtg = AttrPtg.createSpace(AttrPtg.SpaceType.SPACE_BEFORE, 1); + Ptg[] ptgs = { spacePtg, new IntPtg(4), }; + String formulaString; + try { + formulaString = FormulaParser.toFormulaString(null, ptgs); + } catch (IllegalStateException e) { + if(e.getMessage().equalsIgnoreCase("too much stuff left on the stack")) { + throw new AssertionFailedError("Identified bug 44609"); + } + // else some unexpected error + throw e; + } + // FormulaParser strips spaces anyway + assertEquals("4", formulaString); + + ptgs = new Ptg[] { new IntPtg(3), spacePtg, new IntPtg(4), spacePtg, new AddPtg()}; + formulaString = FormulaParser.toFormulaString(null, ptgs); + assertEquals("3+4", formulaString); + } + + /** + * Checks some internal error detecting logic ('stack underflow error' in toFormulaString) + */ + public void testTooFewOperandArgs() { + // Simulating badly encoded cell formula of "=/1" + // Not sure if Excel could ever produce this + Ptg[] ptgs = { + // Excel would probably have put tMissArg here + new IntPtg(1), + new DividePtg(), + }; + try { + FormulaParser.toFormulaString(null, ptgs); + fail("Expected exception was not thrown"); + } catch (IllegalStateException e) { + // expected during successful test + assertTrue(e.getMessage().startsWith("Too few arguments suppled to operation token")); + } + } }