Bug 60898 - XSSFColor's getARGB() method returns a wrong color value when a workbook has a custom indexed color
Summary: XSSFColor's getARGB() method returns a wrong color value when a workbook has ...
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.15-FINAL
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-21 18:18 UTC by samson
Modified: 2017-05-26 23:19 UTC (History)
0 users



Attachments
wrong_color.xlsx (19.08 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2017-03-21 18:18 UTC, samson
Details
styles.xml of the workbook (3.88 KB, text/xml)
2017-03-21 18:19 UTC, samson
Details
A failing unit test (added to TestXSSFColor.jaca) (1.19 KB, patch)
2017-03-21 18:21 UTC, samson
Details | Diff
possible solution1 (1.58 KB, patch)
2017-03-21 18:22 UTC, samson
Details | Diff
solution 2 (partial) (1.95 KB, patch)
2017-03-21 18:23 UTC, samson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description samson 2017-03-21 18:18:04 UTC
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)
Comment 1 samson 2017-03-21 18:19:54 UTC
Created attachment 34862 [details]
styles.xml of the workbook
Comment 2 samson 2017-03-21 18:21:25 UTC
Created attachment 34863 [details]
A failing unit test (added to TestXSSFColor.jaca)
Comment 3 samson 2017-03-21 18:22:53 UTC
Created attachment 34864 [details]
possible solution1
Comment 4 samson 2017-03-21 18:23:51 UTC
Created attachment 34865 [details]
solution 2 (partial)
Comment 5 samson 2017-03-22 17:08:28 UTC
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() {
Comment 6 Greg Woolsey 2017-05-25 21:32:51 UTC
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.
Comment 7 Greg Woolsey 2017-05-26 23:19:41 UTC
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.