]> source.dussan.org Git - poi.git/commitdiff
Fix for bug 45234 - Removed incorrect shared formula conversion in CFRuleRecord
authorJosh Micich <josh@apache.org>
Thu, 19 Jun 2008 19:07:20 +0000 (19:07 +0000)
committerJosh Micich <josh@apache.org>
Thu, 19 Jun 2008 19:07:20 +0000 (19:07 +0000)
git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@669658 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/changes.xml
src/documentation/content/xdocs/status.xml
src/java/org/apache/poi/hssf/record/CFRuleRecord.java
src/java/org/apache/poi/hssf/usermodel/HSSFSheetConditionalFormatting.java
src/testcases/org/apache/poi/hssf/record/TestCFRuleRecord.java

index 169eb16fc196a822c175dfdb95366020843395c6..ad6552e6dcb185ac98742f89c71c9ee5f3a8ba8d 100644 (file)
@@ -37,6 +37,7 @@
 
                <!-- Don't forget to update status.xml too! -->
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="fix">45234 - Removed incorrect shared formula conversion in CFRuleRecord</action>
            <action dev="POI-DEVELOPERS" type="fix">45001 - Improved HWPF Range.replaceText()</action>
            <action dev="POI-DEVELOPERS" type="fix">44692 - Fixed HSSFPicture.resize() to properly resize pictures if the underlying columns/rows have modified size</action>
            <action dev="POI-DEVELOPERS" type="add">Support custom image renderers in HSLF</action>
index 47f042d4312871a4ee53e5a8b3482a129e7a0998..82ee8cee80af5023bc0bcd4856bfa44a872f6a5b 100644 (file)
@@ -34,6 +34,7 @@
        <!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="fix">45234 - Removed incorrect shared formula conversion in CFRuleRecord</action>
            <action dev="POI-DEVELOPERS" type="fix">45001 - Improved HWPF Range.replaceText()</action>
            <action dev="POI-DEVELOPERS" type="fix">44692 - Fixed HSSFPicture.resize() to properly resize pictures if the underlying columns/rows have modified size</action>
            <action dev="POI-DEVELOPERS" type="add">Support custom image renderers in HSLF</action>
index 2b1705abe2d382e20f2034192e7cff04d6169ec8..d000b5311dce906f228338a83da0cf51f8a788b6 100644 (file)
    See the License for the specific language governing permissions and
    limitations under the License.
 ==================================================================== */
-        
 
 package org.apache.poi.hssf.record;
 
-import java.util.Stack;
-
 import org.apache.poi.hssf.model.FormulaParser;
-import org.apache.poi.hssf.model.Workbook;
 import org.apache.poi.hssf.record.cf.BorderFormatting;
 import org.apache.poi.hssf.record.cf.FontFormatting;
 import org.apache.poi.hssf.record.cf.PatternFormatting;
@@ -30,7 +26,6 @@ import org.apache.poi.hssf.usermodel.HSSFWorkbook;
 import org.apache.poi.util.BitField;
 import org.apache.poi.util.BitFieldFactory;
 import org.apache.poi.util.LittleEndian;
-import org.apache.poi.util.StringUtil;
 
 /**
  * Conditional Formatting Rule Record.
@@ -59,9 +54,6 @@ public final class CFRuleRecord extends Record
 
        private byte  field_2_comparison_operator;
 
-       private short field_3_formula1_len;
-       private short field_4_formula2_len;
-
        private int   field_5_options;
 
        private static final BitField modificationBits = bf(0x003FFFFF); // Bits: font,align,bord,patt,prot
@@ -121,8 +113,6 @@ public final class CFRuleRecord extends Record
        {
                field_1_condition_type=conditionType;
                field_2_comparison_operator=comparisonOperation;
-               field_3_formula1_len = (short)0;
-               field_4_formula2_len = (short)0;
 
                // Set modification flags to 1: by default options are not modified
                field_5_options = modificationBits.setValue(field_5_options, -1);
@@ -147,8 +137,8 @@ public final class CFRuleRecord extends Record
                this(conditionType, comparisonOperation); 
                field_1_condition_type = CONDITION_TYPE_CELL_VALUE_IS;
                field_2_comparison_operator = comparisonOperation;
-               setParsedExpression1(formula1);
-               setParsedExpression2(formula2);
+               field_17_formula1 = formula1;
+               field_18_formula2 = formula2;
        }
 
        /**
@@ -167,63 +157,38 @@ public final class CFRuleRecord extends Record
                Ptg[] formula1 = parseFormula(formulaText1, workbook);
                Ptg[] formula2 = parseFormula(formulaText2, workbook);
                return new CFRuleRecord(CONDITION_TYPE_CELL_VALUE_IS, comparisonOperation, formula1, formula2);
-               
        }
 
-       /**
-        * Constructs a Formula record and sets its fields appropriately.
-        * Note - id must be 0x06 (NOT 0x406 see MSKB #Q184647 for an 
-        * "explanation of this bug in the documentation) or an exception
-        *  will be throw upon validation
-        *
-        * @param in the RecordInputstream to read the record from
-        */
-
-       public CFRuleRecord(RecordInputStream in)
-       {
+       public CFRuleRecord(RecordInputStream in) {
                super(in);
        }
 
+       protected void fillFields(RecordInputStream in) {
+               field_1_condition_type = in.readByte();
+               field_2_comparison_operator = in.readByte();
+               int field_3_formula1_len = in.readUShort();
+               int field_4_formula2_len = in.readUShort();
+               field_5_options = in.readInt();
+               field_6_not_used = in.readShort();
 
+               if (containsFontFormattingBlock()) {
+                       fontFormatting = new FontFormatting(in);
+               }
 
-       protected void fillFields(RecordInputStream in) {
-               try {
-                       field_1_condition_type = in.readByte();
-                       field_2_comparison_operator = in.readByte();
-                       field_3_formula1_len = in.readShort();
-                       field_4_formula2_len = in.readShort();
-                       field_5_options = in.readInt();
-                       field_6_not_used = in.readShort();
-
-                       if (containsFontFormattingBlock()) {
-                               fontFormatting = new FontFormatting(in);
-                       }
-
-                       if (containsBorderFormattingBlock()) {
-                               borderFormatting = new BorderFormatting(in);
-                       }
-
-                       if (containsPatternFormattingBlock()) {
-                               patternFormatting = new PatternFormatting(in);
-                       }
-
-                       if (field_3_formula1_len > 0) {
-                               Stack ptgs = Ptg.createParsedExpressionTokens(field_3_formula1_len, in);
-                               // Now convert any fields as required
-                               ptgs = SharedFormulaRecord.convertSharedFormulas(ptgs, 0, 0);
-                               field_17_formula1 = toArray(ptgs);
-                       }
-                       if (field_4_formula2_len > 0) {
-                               Stack ptgs = Ptg.createParsedExpressionTokens(field_4_formula2_len, in);
-
-                               // Now convert any fields as required
-                               ptgs = SharedFormulaRecord.convertSharedFormulas(ptgs, 0, 0);
-                               field_18_formula2 = toArray(ptgs);
-                       }
-               } catch (java.lang.UnsupportedOperationException uoe) {
-                       throw new RecordFormatException(uoe);
+               if (containsBorderFormattingBlock()) {
+                       borderFormatting = new BorderFormatting(in);
+               }
+
+               if (containsPatternFormattingBlock()) {
+                       patternFormatting = new PatternFormatting(in);
                }
 
+               if (field_3_formula1_len > 0) {
+                       field_17_formula1 = Ptg.readTokens(field_3_formula1_len, in);
+               }
+               if (field_4_formula2_len > 0) {
+                       field_18_formula2 = Ptg.readTokens(field_4_formula2_len, in);
+               }
        }
 
        public byte getConditionType()
@@ -323,24 +288,6 @@ public final class CFRuleRecord extends Record
        }
        
 
-       /**
-        * get the length (in number of tokens) of the expression 1
-        * @return  expression length
-        */
-       private short getExpression1Length()
-       {
-               return field_3_formula1_len;
-       }
-       
-
-       /**
-        * get the length (in number of tokens) of the expression 2
-        * @return  expression length
-        */
-       private short getExpression2Length()
-       {
-               return field_4_formula2_len;
-       }
        /**
         * get the option flags
         *
@@ -489,16 +436,6 @@ public final class CFRuleRecord extends Record
                return field_18_formula2;
        }
        
-       private void setParsedExpression1(Ptg[] ptgs) {
-               short len = getTotalPtgSize(field_17_formula1 = ptgs);
-               field_3_formula1_len = len;
-       }
-
-       private void setParsedExpression2(Ptg[] ptgs) {
-               short len = getTotalPtgSize(field_18_formula2 = ptgs);
-               field_4_formula2_len = len;
-       }
-       
        /**
         * called by constructor, should throw runtime exception in the event of a
         * record passed with a differing ID.
@@ -519,6 +456,17 @@ public final class CFRuleRecord extends Record
                return sid;
        }
 
+       /**
+        * @param ptgs may be <code>null</code>
+        * @return encoded size of the formula
+        */
+       private static int getFormulaSize(Ptg[] ptgs) {
+               if (ptgs == null) {
+                       return 0;
+               }
+               return Ptg.getEncodedSize(ptgs);
+       }
+       
        /**
         * called by the class that is responsible for writing this sucker.
         * Subclasses should implement this so that their data is passed back in a
@@ -528,18 +476,20 @@ public final class CFRuleRecord extends Record
         * @param data byte array containing instance data
         * @return number of bytes written
         */
-
        public int serialize(int pOffset, byte [] data)
        {
                
+               int formula1Len=getFormulaSize(field_17_formula1);
+               int formula2Len=getFormulaSize(field_18_formula2);
+               
                int offset = pOffset;
                int recordsize = getRecordSize();
                LittleEndian.putShort(data, 0 + offset, sid);
                LittleEndian.putShort(data, 2 + offset, (short)(recordsize-4));
                data[4 + offset] = field_1_condition_type;
                data[5 + offset] = field_2_comparison_operator;
-               LittleEndian.putShort(data, 6 + offset, field_3_formula1_len);
-               LittleEndian.putShort(data, 8 + offset, field_4_formula2_len);
+               LittleEndian.putUShort(data, 6 + offset, formula1Len);
+               LittleEndian.putUShort(data, 8 + offset, formula2Len);
                LittleEndian.putInt(data,  10 + offset, field_5_options);
                LittleEndian.putShort(data,14 + offset, field_6_not_used);
                
@@ -562,16 +512,12 @@ public final class CFRuleRecord extends Record
                        offset += patternFormatting.serialize(offset, data);
                }
                
-               if (getExpression1Length()>0)
-               {
-                       Ptg.serializePtgStack(convertToTokenStack(field_17_formula1), data, offset);
-                       offset += getExpression1Length();
+               if (field_17_formula1 != null) {
+                       offset += Ptg.serializePtgs(field_17_formula1, data, offset);
                }
 
-               if (getExpression2Length()>0)
-               {
-                       Ptg.serializePtgStack(convertToTokenStack(field_18_formula2), data, offset);
-                       offset += getExpression2Length();
+               if (field_18_formula2 != null) {
+                       offset += Ptg.serializePtgs(field_18_formula2, data, offset);
                }
                if(offset - pOffset != recordsize) {
                        throw new IllegalStateException("write mismatch (" + (offset - pOffset) + "!=" + recordsize + ")");
@@ -586,24 +532,12 @@ public final class CFRuleRecord extends Record
                                        (containsFontFormattingBlock()?fontFormatting.getRawRecord().length:0)+
                                        (containsBorderFormattingBlock()?8:0)+
                                        (containsPatternFormattingBlock()?4:0)+
-                                       getExpression1Length()+
-                                       getExpression2Length()
+                                       getFormulaSize(field_17_formula1)+
+                                       getFormulaSize(field_18_formula2)
                                        ;
                return retval;
        }
 
-       private short getTotalPtgSize(Ptg[] ptgs)
-       {
-               if( ptgs == null) {
-                       return 0;
-               }
-               short  retval = 0;
-               for (int i = 0; i < ptgs.length; i++)
-               {
-                       retval += ptgs[i].getSize();
-               }
-               return retval;
-       }
 
        public String toString()
        {
@@ -629,8 +563,6 @@ public final class CFRuleRecord extends Record
        
        public Object clone() {
                CFRuleRecord rec = new CFRuleRecord(field_1_condition_type, field_2_comparison_operator);
-               rec.field_3_formula1_len = field_3_formula1_len;
-               rec.field_4_formula2_len = field_4_formula2_len;
                rec.field_5_options = field_5_options;
                rec.field_6_not_used = field_6_not_used;
                if (containsFontFormattingBlock()) {
@@ -642,10 +574,10 @@ public final class CFRuleRecord extends Record
                if (containsPatternFormattingBlock()) {
                        rec.patternFormatting = (PatternFormatting) patternFormatting.clone();
                }
-               if (field_3_formula1_len > 0) {
+               if (field_17_formula1 != null) {
                        rec.field_17_formula1 = (Ptg[]) field_17_formula1.clone();
                }
-               if (field_4_formula2_len > 0) {
+               if (field_18_formula2 != null) {
                        rec.field_18_formula2 = (Ptg[]) field_18_formula2.clone();
                }
 
@@ -653,30 +585,17 @@ public final class CFRuleRecord extends Record
        }
 
        /**
+        * TODO - parse conditional format formulas properly i.e. produce tRefN and tAreaN instead of tRef and tArea
+        * this call will produce the wrong results if the formula contains any cell references
+        * One approach might be to apply the inverse of SharedFormulaRecord.convertSharedFormulas(Stack, int, int)
+        * Note - two extra parameters (rowIx & colIx) will be required. They probably come from one of the Region objects.
+        * 
         * @return <code>null</code> if <tt>formula</tt> was null.
         */
-       private static Ptg[] parseFormula(String formula, HSSFWorkbook workbook)
-       {
+       private static Ptg[] parseFormula(String formula, HSSFWorkbook workbook) {
                if(formula == null) {
                        return null;
                }
                return FormulaParser.parse(formula, workbook);
        }
-
-       // TODO - treat formulas as token arrays instead of Stacks throughout the rest of POI
-       private static Stack convertToTokenStack(Ptg[] ptgs)
-       {
-               Stack parsedExpression = new Stack();
-               // fill the Ptg Stack with Ptgs of new formula
-               for (int k = 0; k < ptgs.length; k++) 
-               {
-                       parsedExpression.push(ptgs[ k ]);
-               }
-               return parsedExpression;
-       }
-       private static Ptg[] toArray(Stack ptgs) {
-               Ptg[] result = new Ptg[ptgs.size()];
-               ptgs.toArray(result);
-               return result;
-       }
 }
index 6862a87ea36e835de402de668e30572eb63e40df..4df94e4b527f0c5c06db568d2fe13ca1717ab54a 100644 (file)
@@ -39,7 +39,8 @@ public final class HSSFSheetConditionalFormatting {
 \r
        /**\r
         * A factory method allowing to create a conditional formatting rule\r
-        * with a cell comparison operator \r
+        * with a cell comparison operator<p/>\r
+        * TODO - formulas containing cell references are currently not parsed properly \r
         *\r
         * @param comparisonOperation - a constant value from\r
         *               <tt>{@link HSSFConditionalFormattingRule.ComparisonOperator}</tt>: <p>\r
@@ -72,8 +73,8 @@ public final class HSSFSheetConditionalFormatting {
        /**\r
         * A factory method allowing to create a conditional formatting rule with a formula.<br>\r
         *\r
-        * The formatting rules are applied by Excel when the value of the formula not equal to 0.\r
-        *\r
+        * The formatting rules are applied by Excel when the value of the formula not equal to 0.<p/>\r
+        * TODO - formulas containing cell references are currently not parsed properly\r
         * @param formula - formula for the valued, compared with the cell\r
         */\r
        public HSSFConditionalFormattingRule createConditionalFormattingRule(String formula) {\r
index afc44e7043e0d1e8a9e00b041fa8d1d4f153cc68..1eb052bec7aef725bf71716dd9862ff1edac1c86 100644 (file)
 
 package org.apache.poi.hssf.record;
 
+import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
 import org.apache.poi.hssf.record.CFRuleRecord.ComparisonOperator;
 import org.apache.poi.hssf.record.cf.BorderFormatting;
 import org.apache.poi.hssf.record.cf.FontFormatting;
 import org.apache.poi.hssf.record.cf.PatternFormatting;
+import org.apache.poi.hssf.record.formula.Ptg;
+import org.apache.poi.hssf.record.formula.RefNPtg;
+import org.apache.poi.hssf.record.formula.RefPtg;
 import org.apache.poi.hssf.usermodel.HSSFWorkbook;
 import org.apache.poi.hssf.util.HSSFColor;
 import org.apache.poi.util.LittleEndian;
@@ -296,7 +300,57 @@ public final class TestCFRuleRecord extends TestCase
                // check all remaining flag bits (some are not well understood yet)
                assertEquals(0x203FFFFF, flags);
        }
+       
+       private static final byte[] DATA_REFN = {
+               // formula extracted from bugzilla 45234 att 22141
+               1, 3, 
+               9, // formula 1 length 
+               0, 0, 0, -1, -1, 63, 32, 2, -128, 0, 0, 0, 5,
+               // formula 1: "=B3=1" (formula is relative to B4)
+               76, -1, -1, 0, -64, // tRefN(B1)
+               30, 1, 0,       
+               11,     
+       };
+
+       /**
+        * tRefN and tAreaN tokens must be preserved when re-serializing conditional format formulas
+        */
+       public void testReserializeRefNTokens() {
+               
+               RecordInputStream is = new TestcaseRecordInputStream(CFRuleRecord.sid, DATA_REFN);
+               CFRuleRecord rr = new CFRuleRecord(is);
+               Ptg[] ptgs = rr.getParsedExpression1();
+               assertEquals(3, ptgs.length);
+               if (ptgs[0] instanceof RefPtg) {
+                       throw new AssertionFailedError("Identified bug 45234");
+               }
+               assertEquals(RefNPtg.class, ptgs[0].getClass());
+               RefNPtg refNPtg = (RefNPtg) ptgs[0];
+               assertTrue(refNPtg.isColRelative());
+               assertTrue(refNPtg.isRowRelative());
+               
+               byte[] data = rr.serialize();
+               
+               if (!compareArrays(DATA_REFN, 0, data, 4, DATA_REFN.length)) {
+                       fail("Did not re-serialize correctly");
+               }
+       }
 
+       private static boolean compareArrays(byte[] arrayA, int offsetA, byte[] arrayB, int offsetB, int length) {
+               
+               if (offsetA + length > arrayA.length) {
+                       return false;
+               }
+               if (offsetB + length > arrayB.length) {
+                       return false;
+               }
+               for (int i = 0; i < length; i++) {
+                       if (arrayA[i+offsetA] != arrayB[i+offsetB]) {
+                               return false;
+                       }
+               }
+               return true;
+       }
 
        public static void main(String[] ignored_args)
        {