Bug 50786 - XSSFColor.getRgb() returns null for Excel 97-2003 indexed colors
Summary: XSSFColor.getRgb() returns null for Excel 97-2003 indexed colors
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.7-FINAL
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-15 11:23 UTC by andrei
Modified: 2011-03-04 08:22 UTC (History)
0 users



Attachments
Excel 97-2003 file (15.50 KB, application/vnd.ms-excel)
2011-02-15 11:26 UTC, andrei
Details

Note You need to log in before you can comment on or make changes to this bug.
Description andrei 2011-02-15 11:23:29 UTC
Steps:
1. Open old.xls Excel file (see in attachment).
2. Create new XLSX Excel file.
2. Copy cell with colored background from old.xls to new xlsx.
3. Process new xlsx doc with poi.
4. Get cell background color:
cell.getCellStyle().getFillForegroundXSSFColor().getRgb()

Actual: null
Expected: rgb byte array
Comment 1 andrei 2011-02-15 11:26:26 UTC
Created attachment 26661 [details]
Excel 97-2003 file
Comment 2 Nick Burch 2011-02-15 11:51:48 UTC
Can you unzip the file (.xlsx is a zip of xml files), and track down where your colour actually gets stored?
Comment 3 andrei 2011-02-16 05:56:04 UTC
styles.xml
<fgColor indexed="42"/>
Color is stored as indexed.

Possible solution:

XSSFColor color = ..;
short colorIndex = color.getIndexed();
HSSFColor indexedColor = HSSFColor.getIndexHash().get(colorIndex);
byte[] rgb = indexedColor.getTriplet();
Comment 4 Nick Burch 2011-02-18 12:42:14 UTC
Thanks for the investigating, sample file and proposed fix

I've added the HSSFColor lookup inside the getRGB method and added a unit test for it in r1072082.
Comment 5 andrei 2011-03-02 09:31:08 UTC
Noticed that indexed colors hashtable is not stored in HSSFColor. So getRGBOrARGB() method calls HSSFColor.getIndexHash() whitch every time (fill color, text color, border color) builds color hashtable. It's very slowly.

HSSFColor.getIndexHash() method has comment:
"this function returns all colors in a hastable.  Its not implemented as a
 static member/staticly initialized because that would be dirty in a
 server environment as it is intended.  This means you'll eat the time
 it takes to create it once per request but you will not hold onto it
 if you have none of those requests."
Comment 6 Nick Burch 2011-03-02 10:00:21 UTC
(In reply to comment #5)
> HSSFColor.getIndexHash() method has comment:
> "this function returns all colors in a hastable.  Its not implemented as a
>  static member/staticly initialized because that would be dirty in a
>  server environment as it is intended.  This means you'll eat the time
>  it takes to create it once per request but you will not hold onto it
>  if you have none of those requests."

I think we should probably replace this with a lazy initialized static cache on HSSFColor. That way your first call may be a little slow while it's built, but all other calls on the server will then be fast.

My plan would be to have it return an immutable hash which is statically cached, but also offer a 2nd method that will return a writable hash for anyone currently doing anything odd

I could swap that round with the new method name for the static cache version, and the old method name for the mutable one, if someone can think of a good reason why to do it that way!
Comment 7 andrei 2011-03-02 10:41:20 UTC
Thanks, it's a good idea
Comment 8 Nick Burch 2011-03-04 07:54:54 UTC
Change committed in r1077920.

Almost everyone seemed to just call getIndexedHash().get(..) so they will get the speedup with no need to change anything

In the very rare case that you were relying on editing the old hashtable, getModifiableIndexedHash will provide that.
Comment 9 andrei 2011-03-04 08:22:49 UTC
Excellent!