Bug 30311 - Conditional formatting not implemented
Summary: Conditional formatting not implemented
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: HSSF (show other bugs)
Version: 2.5-FINAL
Hardware: PC All
: P3 major with 3 votes (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-07-24 18:28 UTC by sam s
Modified: 2008-04-15 10:12 UTC (History)
1 user (show)



Attachments
New Files (34.56 KB, application/octet-stream)
2008-03-18 21:55 UTC, Dmitriy Kumshayev
Details
Patch for Conditional Formatting (11.71 KB, patch)
2008-03-18 21:56 UTC, Dmitriy Kumshayev
Details | Diff
HSSFConditionalFormatting javadoc patch (2.07 KB, patch)
2008-03-24 22:45 UTC, Dmitriy Kumshayev
Details | Diff
svn diff of 2 files (3.04 KB, text/plain)
2008-03-24 23:47 UTC, Josh Micich
Details
Further javadocs and other minor changes (to replace patch 21709) (6.75 KB, patch)
2008-03-25 06:23 UTC, Dmitriy Kumshayev
Details | Diff
unit test for HSSFConditionalFormatting and fixes (11.12 KB, patch)
2008-03-28 17:12 UTC, Dmitriy Kumshayev
Details | Diff
Better support for CF read/modify mode (20.94 KB, patch)
2008-04-06 19:44 UTC, Dmitriy Kumshayev
Details | Diff
Set some bits in FontFormatting record to match with Excel (4.25 KB, patch)
2008-04-07 20:45 UTC, Dmitriy Kumshayev
Details | Diff
Fixes and API improvements (24.76 KB, patch)
2008-04-11 22:52 UTC, Dmitriy Kumshayev
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sam s 2004-07-24 18:28:47 UTC
A cell's conditional formatting cannot be retrieved/set/copied-from-an-existing-
cell. Many of the spreadsheets I'm working with use this Excel feature to 
highlight cells with specific values, in most cases with coloured backgrounds.
Comment 1 Dmitriy Kumshayev 2008-03-18 21:55:24 UTC
Created attachment 21680 [details]
New Files
Comment 2 Dmitriy Kumshayev 2008-03-18 21:56:32 UTC
Created attachment 21681 [details]
Patch for Conditional Formatting
Comment 3 Nick Burch 2008-03-19 05:28:15 UTC
Thanks for this patch Dmitriy, it looks great.

I've applied it to svn trunk, after having made a few tweaks, to get all the existing tests to still pass. (There seemed to be some problems with the formulas on certain files in the test suite, so I had to convert the ptgs into a non shared format, so they could be serialised)
Comment 4 Dmitriy Kumshayev 2008-03-24 22:45:17 UTC
Created attachment 21708 [details]
HSSFConditionalFormatting javadoc patch

A javadoc patch with a Conditional Formatting usage example
Comment 5 Josh Micich 2008-03-24 23:45:24 UTC
(In reply to comment #4)

Thanks for the javadoc, Dmitriy. Applied in r640711.

BTW I was attempting to improve this class a little bit (make fields private final, etc) but when I checked the junits at the end I found that none of them actually hit this class.  

Could you please make a junit that exercises HSSFConditionalFormatting especially the new addConditionalFormatting() methods on HSSFSheet? 

I'll attach a diff of the suggested improvements (which might help you think of test cases, if any of the changes are wrong).
Comment 6 Josh Micich 2008-03-24 23:47:34 UTC
Created attachment 21709 [details]
svn diff of 2 files

Suggested improvements to HSSFConditionalFormatting.  Minor changes to HSSFSheet.
Comment 7 Dmitriy Kumshayev 2008-03-25 06:23:18 UTC
Created attachment 21711 [details]
Further javadocs and other minor changes (to replace patch 21709)

Josh, 
Your changes do make sense. I continued in the same direction and 
made some additional minor changes. 
Also renamed some methods and further improved javadocs.
If I get more time, I will try to create a junit.
For now just this.

My patch(svn3.diff) includes your changes as well, so you can apply it instead of yours.
Comment 8 Josh Micich 2008-03-25 22:31:25 UTC
(In reply to comment #7)
> Created an attachment (id=21711) [details]
> Further javadocs and other minor changes (to replace patch 21709)

Applied in svn r641157
Comment 9 Dmitriy Kumshayev 2008-03-28 17:12:57 UTC
Created attachment 21733 [details]
unit test for HSSFConditionalFormatting and fixes
Comment 10 Josh Micich 2008-03-30 22:27:12 UTC
(In reply to comment #9)
> unit test for HSSFConditionalFormatting and fixes

Added this patch in svn r642878.

I did some other clean-up (mostly for better encapsulation).  Please review, and let me know if there is anything wrong.

POI is mostly coded in a way that reading an XLS file and rewriting it will have no ill-effects.  It seems like the conditional formatting stuff mostly obeys this principle.  However, writing a new XLS file or adding conditional formatting to an existing XLS file might need a bit of work.

I found two bugs in CFRuleRecord (which have been fixed and have junits now):  The 'undocumented' bits of the options field need to be set to zero (as far as I can tell).  Setting them all to one causes Excel to report errors when reading the generated file.  Another problem was that the wrong formula length field was being set when formula2 was changed.

Another test I tried but gave up on, was to create some conditional formatting using a FontFormatting object.  It seems that POI doesn't generate proper XLS files when this feature is used.  The 118 byte data generated is quite different to the one generated by Excel (if you attempt to make the formatting the same).


Comment 11 Dmitriy Kumshayev 2008-04-06 19:44:12 UTC
Created attachment 21784 [details]
Better support for CF read/modify mode

added methods 
getFontFormatting()
getBorderFormatting()
getFontFormatting() 
to HSSFConditionalFormattingRule 
to allow access for modifications of existing worksheets.
Attempted to match flags FontFormatting with the ones created by Excel.
Disabled condense flags Excel does not support them and there is not way 
to verify them.
Comment 12 Josh Micich 2008-04-06 20:27:45 UTC
(In reply to comment #11)
> Created an attachment (id=21784) [details]

patch applied in svn r645352.

Dmitriy,
 
Could you please have a look at comment 10?  This most recent patch doesn't seem to address those issues.

Can you write a test which sets some conditional formatting with a new FontFormatting instance, and produces an XLS file?  Last time I tried, Excel would not read that file.
Comment 13 Dmitriy Kumshayev 2008-04-07 06:48:54 UTC
Josh,

I did try to create a file and it worked fine for me. I also tried to read an xls file created by Excel and found that FontFormatting 118 bytes array looked a little bit different than the program expected. I made some corrections for that.

It would be good to create a test which produces a file, saves it and then reads it. What are the POI's conventions for file creation? Should I do it in a standard  TEMP directory or POI uses some properties to define that place? Do you know other tests which do that?
Comment 14 Josh Micich 2008-04-07 16:15:34 UTC
Dmitriy,

Looks like everything is mostly OK now.  Perhaps we should consider this bugzilla closed since this is a good milestone - POI now seems to support quite a few simple Conditional Formatting use-cases.  Any future bugs related to Conditional Formatting should probably get their own specific bugzilla.

(In reply to comment #13)
> ... What are the POI's conventions for file creation? ...

See methods at the top of hssf.usermodel.TestBugs.  The least preferred action is to create an XLS file that requires a developer to open and inspect - mostly because that operation is expensive.  Usually the only person that can properly judge success is the one who originally wrote the junit.
So - try to write tests that unambiguously detect bug conditions with plain java code.  For example, if you find that setting CFHeaderRecord.field_1_numcf=-1 causes excel to crash, write a junit that confirms POI does not do this, and put a comment on it explaining why this is important.  
This approach requires does more work though, because it involves a little bit of reverse engineering: "What exactly does my bug-fix change about the output XLS file that causes it to open OK in Excel?"



---------------
> ... I did try to create a file and it worked fine for me...

I found the old test which gave me trouble and re-ran it, and things are much better now. I also re-ran as-of svn revision 642878, but the bold format seems to get lost.  So you must have fixed something regarding font formatting since then.
The original (more serious bug) was that Excel would complain when opening the file, and (un)fortunately I couldn't reproduce that.  I must have inadvertently fixed that while completing the work for 642878.  

I attached the sample code below for you to take a look at.  Note that it still shows a difference in the 118 byte font format data.  Is that a problem? Your call.
The input file (cfEx.xls) for this test is a file created by Excel with one conditional format entry.  The only format is font-type: bold, so the sample code should ideally change nothing

--

InputStream is = new FileInputStream("cfEx.xls");
HSSFWorkbook wb = new HSSFWorkbook(is);
HSSFSheet sheet = wb.getSheetAt(0);

HSSFConditionalFormatting cf = sheet.getConditionalFormattingAt(0);
Region[] regions = cf.getFormattingRegions();

sheet.removeConditionalFormatting(0);

HSSFFontFormatting fontFmt = new HSSFFontFormatting();
fontFmt.setFontStyle(false, true);
byte op = CFRuleRecord.ComparisonOperator.BETWEEN;
HSSFConditionalFormattingRule rule = sheet.createConditionalFormattingRule(op, "5", "10",  fontFmt, null, null);

HSSFConditionalFormattingRule rule1 = cf.getRule(0); 

byte[] rawRecord1 = rule1.getCfRuleRecord().getFontFormatting().getRawRecord();
for (int i = 0; i < rawRecord1.length; i++) {
	System.out.print(rawRecord1[i] + ",");
}
System.out.println();

byte[] rawRecord = fontFmt.getFontFormattingBlock().getRawRecord();
for (int i = 0; i < rawRecord.length; i++) {
	System.out.print(rawRecord[i]+ ",");
}
System.out.println();


sheet.addConditionalFormatting(regions, new HSSFConditionalFormattingRule[] { rule, });

OutputStream os = new FileOutputStream("cfEx-out.xls");
wb.write(os);
os.close();
Comment 15 Dmitriy Kumshayev 2008-04-07 20:45:24 UTC
Created attachment 21789 [details]
Set some bits in FontFormatting record to match with Excel

I verified the comparison between Excel's FontFormatting record and POI's.
Actually the first 64 bytes of 118 byte array according to OpenOffice documentation are reserved for the font name. My code initializes this area with 0s which works fine for Excel. Excel seem to live some random garbage  there after the bytes containing 0 length. (I found a substring of a printer name in that area). I believe the comparison of these first 64 bytes does not give a lot.

I reviewed the rest 54 bytes and found one difference (which somehow did not affect Excel's behavior). I noticed that when the file is created by excel, 4 bytes at offset 112 (documented by OpenOffice as "Not used") always contain 0x7FFFFFFF.
So I did the same in the constructor. Now they should match with Excel.
Comment 16 Josh Micich 2008-04-08 22:55:42 UTC
(In reply to comment #15)
> Created an attachment (id=21789) [details]
> Set some bits in FontFormatting record to match with Excel

applied in svn r646194
Comment 17 Dmitriy Kumshayev 2008-04-11 22:52:58 UTC
Created attachment 21818 [details]
Fixes and API improvements

Improved logic working with CFRuleRecord option flags.
Now it is possible to create correct (so that they work in Excel ) 
Conditional Formatting rules not only for FontFormatting, 
but for the other two structures: PatternFormatting and BorderFormatting.

I do not include a test working with files yet.

Improved high level API. Now it is more convenient  and consistent.
Example :
Previous approach:
-----------------------------------------
// This  patternFtmt object is detached from the rule
// and consequently cannot notify the rule when it changes.
HSSFPatternFormatting patternFmt = new HSSFPatternFormatting();


HSSFConditionalFormattingRule rule = sheet.createConditionalFormattingRule(formula, null, null, patternFmt);

// It is also possible to share this patternFmt between more than one rules
// which creates potential conflicts between the rules,
// since changes on one patternFmt will implicitly affect all related rules
HSSFConditionalFormattingRule rule2 = sheet.createConditionalFormattingRule(formula2, null, null, patternFmt);


-----------------------------------------

New approach:
-----------------------------------------
HSSFConditionalFormattingRule rule = sheet.createConditionalFormattingRule(formula);

// This patternFmt always exists in context of it's parent rule.
// No conflicts possible. 
// API does not allow to share the same instance of HSSFPatternFormatting 
// between many rules.
HSSFPatternFormatting patternFmt = rule.createPatternFormatting();

-----------------------------------------
Comment 18 Josh Micich 2008-04-15 10:12:49 UTC
(In reply to comment #17)
> Created an attachment (id=21818) [details]
> Fixes and API improvements

Applied in svn r648334

I separated all the conditional formatting stuff from HSSFSheet (which has many methods already).  This will give more freedom to add methods to the API, like the recent overloaded convenience methods in this patch.

I noticed you changed the definition of CFRuleRecord.modificationBits - it broke a junit (easy fix).  Could you add a comment as to why you took the MSB out of the mask?