Created attachment 34861 [details] wrong_color.xlsx If a workbook has a custom indexed colors defined instead of the fixed color array (see the styles.xml file attached), the XSSFColor's getARGB() mehtod calls the 'getRGBOrARGB()' defined in 'ExtendedColor.java'. This mehtod simply referrs to a constant color array. But if a workbook has custom(modified) indexed colors (i.e. if an <indexedColors> element is defined inside the styles.xml part of the workbook), the indexed colors should refer to the color array defined in the styles.xml file. I have attached a sample .xlsx file here, and added a test to TestXSSFColor.java to show the problem(patch attached). Possible solutions. This problem could be solved by overriding the getRGBOrARGB() in XSSFColor.java in a way that custom indexed colors (if any) are looked up from the CTcolors. Unfortunately, the XSSFColor dosen't have an access to the CTcolors, so either a CTcolors field should be added to XSSFColor (hence also a new constructor will be added) or the indexed color can be obtained from the Dom Elelement. Solution 1 (Probably this is not a clean solution and more error prone): Get the Dom element from the 'ctColor' field, get the styleSheet element, and then get the indexed color. (see the attached XSSFColor.java.patch): Solution 2: add a CTcolors field to XSSFColor, and a constructor that takes bothe CTcolors and CTcolor. Consequently, change (probably all or most of) the calls to the "XSSFColor(CTcolor)" constructor with a call to "XSSFColor(CTcolor, CTcolors)". (see attached XSSFColor1.java.patch)
Created attachment 34862 [details] styles.xml of the workbook
Created attachment 34863 [details] A failing unit test (added to TestXSSFColor.jaca)
Created attachment 34864 [details] possible solution1
Created attachment 34865 [details] solution 2 (partial)
Comment on attachment 34865 [details] solution 2 (partial) Index: XSSFColor.java =================================================================== --- XSSFColor.java (revision 293556) +++ XSSFColor.java (working copy) @@ -23,6 +23,9 @@ import org.apache.poi.ss.usermodel.IndexedColors; import org.apache.poi.util.Internal; import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTColor; +import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTColors; +import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTRgbColor; +import org.w3c.dom.DOMException; /** * Represents a color in SpreadsheetML @@ -29,6 +32,7 @@ */ public class XSSFColor extends ExtendedColor { private final CTColor ctColor; + private CTColors indexedColors; /** * Create an instance of XSSFColor from the supplied XML bean @@ -36,6 +40,11 @@ public XSSFColor(CTColor color) { this.ctColor = color; } + + public XSSFColor(CTColors indexedColors, CTColor color){ + this.indexedColors = indexedColors; + this.ctColor = color; + } /** * Create an new instance of XSSFColor @@ -179,6 +188,38 @@ return rgb; } } + + @Override + protected byte[] getRGBOrARGB() throws DOMException{ + if(isIndexed() && hasCustomIndexedColors()){ + CTRgbColor ctRgbColor = indexedColors.getIndexedColors(). + getRgbColorList().get(getIndexed()); + String hexString = ctRgbColor.getDomNode().getAttributes().getNamedItem("rgb").getNodeValue(); + return hexStringToByteArray(hexString); + } + return super.getRGBOrARGB(); + } + + private boolean hasCustomIndexedColors(){ + + if (indexedColors == null) { + return false; + } + if (indexedColors.getIndexedColors() == null) { + return false; + } + return true; + } + + private byte[] hexStringToByteArray(String s) { + byte[] b = new byte[s.length() / 2]; + for (int i = 0; i < b.length; i++) { + int index = i * 2; + int v = Integer.parseInt(s.substring(index, index + 2), 16); + b[i] = (byte) v; + } + return b; + } @Override protected byte[] getStoredRBG() {
Solution 2 sounds worse than it looks, to my initial take. Constructor calls of all sorts are almost entirely from contexts that would have access to the workbook styles, and could pass in what is needed. The remaining paths can be documented as lacking custom color index support. XSSFColor itself can defensively check as needed and fall back on the current behavior when the custom index info is missing. Further, we can add some more support to StylesTable to avoid repeated calls into the CT* classes, as that is always expensive, and color lookups can be done thousands of times (or a few more orders of magnitude) in processing formatting for a single workbook. I'll see if I have some time to dig in.
Fixed in r1796359. Thanks for the unit test! Everything creating XSSFColor instances that subsequently may be used to generate an RGB value now have the opportunity to pass any document custom index mappings. The result should be proper color mappings without the need for extra code for the consumer, unless the application uses custom built XSSFColor instances directly. StylesTable now has a method to return the current document index color mapping, getIndexedColors(), which defaults to the built-in values but knows about custom document colors if they are defined.