#64241 - shade and tint calculation are based now on scRGB (opposed to HSL) colorspace

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1875522 13f79535-47bb-0310-9956-ffa450edef68

10 files changed, 156 insertions, 142 deletions

diff --git a/src/java/org/apache/poi/sl/draw/DrawPaint.java b/src/java/org/apache/poi/sl/draw/DrawPaint.java
index eb18e10a4b..157c4103e9 100644
--- a/src/java/org/apache/poi/sl/draw/DrawPaint.java
+++ b/src/java/org/apache/poi/sl/draw/DrawPaint.java
@@ -417,14 +417,18 @@ public class DrawPaint {
 
         Color result = color.getColor();
 
-        double alpha = getAlpha(result, color);
+        final double alpha = getAlpha(result, color);
+
+        final double[] scRGB = RGB2SCRGB(result);
+        applyShade(scRGB, color);
+        applyTint(scRGB, color);
+        result = SCRGB2RGB(scRGB);
+
         // values are in the range [0..100] (usually ...)
         double[] hsl = RGB2HSL(result);
         applyHslModOff(hsl, 0, color.getHueMod(), color.getHueOff());
         applyHslModOff(hsl, 1, color.getSatMod(), color.getSatOff());
         applyHslModOff(hsl, 2, color.getLumMod(), color.getLumOff());
-        applyShade(hsl, color);
-        applyTint(hsl, color);
 
         result = HSL2RGB(hsl[0], hsl[1], hsl[2], alpha);
 
@@ -477,23 +481,22 @@ public class DrawPaint {
      *
      * For a shade, the equation is luminance * %tint.
      */
-    private static void applyShade(double[] hsl, ColorStyle fc) {
+    private static void applyShade(double[] scRGB, ColorStyle fc) {
         int shade = fc.getShade();
         if (shade == -1) {
             return;
         }
 
-        double shadePct = shade / 100_000.;
-        hsl[2] *= shadePct;
+        final double shadePct = shade / 100_000.;
+        for (int i=0; i<3; i++) {
+            scRGB[i] = Math.max(0, Math.min(1, scRGB[i]*shadePct));
+        }
     }
 
     /**
      * Apply the tint
-     *
-     * For a tint, the equation is luminance * %tint + (1-%tint).
-     * (Note that 1-%tint is equal to the lumOff value in DrawingML.)
      */
-    private static void applyTint(double[] hsl, ColorStyle fc) {
+    private static void applyTint(double[] scRGB, ColorStyle fc) {
         int tint = fc.getTint();
         if (tint == -1 || tint == 0) {
             return;
@@ -502,15 +505,8 @@ public class DrawPaint {
         // see 18.8.19 fgColor (Foreground Color)
         double tintPct = tint / 100_000.;
 
-
-        // The tint value is stored as a double from -1.0 .. 1.0, where -1.0 means 100% darken
-        // and 1.0 means 100% lighten. Also, 0.0 means no change.
-        if (tintPct < 0) {
-            // Lum’ = Lum * (1.0 + tint)
-            hsl[2] *= (1 + tintPct);
-        } else {
-            // Lum‘ = Lum * (1.0-tint) + (HLSMAX – HLSMAX * (1.0-tint))
-            hsl[2] = hsl[2]*(1-tintPct) + (100-100*(1-tintPct));
+        for (int i=0; i<3; i++) {
+            scRGB[i] =  1 - (1 - scRGB[i]) * tintPct;
         }
     }
 
@@ -749,36 +745,50 @@ public class DrawPaint {
     }
 
     /**
-     * Convert sRGB float component [0..1] from sRGB to linear RGB [0..100000]
+     * Convert sRGB Color to scRGB [0..1] (0:red,1:green,2:blue).
+     * Alpha needs to be handled separately.
      *
-     * @see Color#getRGBColorComponents(float[])
+     * @see <a href="https://referencesource.microsoft.com/#PresentationCore/Core/CSharp/System/Windows/Media/Color.cs,1048">.Net implementation sRgbToScRgb</a>
      */
-    public static int srgb2lin(float sRGB) {
-        // scRGB has a linear gamma of 1.0, scale the AWT-Color which is in sRGB to linear RGB
-        // see https://en.wikipedia.org/wiki/SRGB (the reverse transformation)
-        if (sRGB <= 0.04045d) {
-            return (int)Math.rint(100000d * sRGB / 12.92d);
-        } else {
-            return (int)Math.rint(100000d * Math.pow((sRGB + 0.055d) / 1.055d, 2.4d));
+    public static double[] RGB2SCRGB(Color color) {
+        float[] rgb = color.getColorComponents(null);
+        double[] scRGB = new double[3];
+        for (int i=0; i<3; i++) {
+            if (rgb[i] < 0) {
+                scRGB[i] = 0;
+            } else if (rgb[i] <= 0.04045) {
+                scRGB[i] = rgb[i] / 12.92;
+            } else if (rgb[i] <= 1) {
+                scRGB[i] = Math.pow((rgb[i] + 0.055) / 1.055, 2.4);
+            } else {
+                scRGB[i] = 1;
+            }
         }
+        return scRGB;
     }
 
     /**
-     * Convert linear RGB [0..100000] to sRGB float component [0..1]
+     * Convert scRGB [0..1] components (0:red,1:green,2:blue) to sRGB Color.
+     * Alpha needs to be handled separately.
      *
-     * @see Color#getRGBColorComponents(float[])
+     * @see <a href="https://referencesource.microsoft.com/#PresentationCore/Core/CSharp/System/Windows/Media/Color.cs,1075">.Net implementation ScRgbTosRgb</a>
      */
-    public static float lin2srgb(int linRGB) {
-        // color in percentage is in linear RGB color space, i.e. needs to be gamma corrected for AWT color
-        // see https://en.wikipedia.org/wiki/SRGB (The forward transformation)
-        if (linRGB <= 0.0031308d) {
-            return (float)(linRGB / 100000d * 12.92d);
-        } else {
-            return (float)(1.055d * Math.pow(linRGB / 100000d, 1.0d/2.4d) - 0.055d);
+    public static Color SCRGB2RGB(double... scRGB) {
+        final double[] rgb = new double[3];
+        for (int i=0; i<3; i++) {
+            if (scRGB[i] < 0) {
+                rgb[i] = 0;
+            } else if (scRGB[i] <= 0.0031308) {
+                rgb[i] = scRGB[i] * 12.92;
+            } else if (scRGB[i] < 1) {
+                rgb[i] = 1.055 * Math.pow(scRGB[i], 1.0 / 2.4) - 0.055;
+            } else {
+                rgb[i] = 1;
+            }
         }
+        return new Color((float)rgb[0],(float)rgb[1],(float)rgb[2]);
     }
 
-
     static void fillPaintWorkaround(Graphics2D graphics, Shape shape) {
         // the ibm jdk has a rendering/JIT bug, which throws an AIOOBE in
         // TexturePaintContext$Int.setRaster(TexturePaintContext.java:476)
diff --git a/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFColor.java b/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFColor.java
index 4818015b34..63daaf6395 100644
--- a/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFColor.java
+++ b/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFColor.java
@@ -111,8 +111,8 @@ public class XSLFColor {
     }
 
     private Color toColor(CTScRgbColor scrgb) {
-        // color in percentage is in linear RGB color space, i.e. needs to be gamma corrected for AWT color
-        return new Color(DrawPaint.lin2srgb(scrgb.getR()), DrawPaint.lin2srgb(scrgb.getG()), DrawPaint.lin2srgb(scrgb.getB()));
+        // percental [0..100000] scRGB color space  needs to be gamma corrected for AWT/sRGB colorspace
+        return DrawPaint.SCRGB2RGB(scrgb.getR()/100_000d,scrgb.getG()/100_000d,scrgb.getB()/100_000d);
     }
 
     private Color toColor(CTSRgbColor srgb) {
@@ -225,15 +225,16 @@ public class XSLFColor {
             alphaPct = (addAlpha) ? rgb.addNewAlpha() : null;
         } else {
             CTScRgbColor rgb = fill.addNewScrgbClr();
-            rgb.setR(DrawPaint.srgb2lin(rgbaf[0]));
-            rgb.setG(DrawPaint.srgb2lin(rgbaf[1]));
-            rgb.setB(DrawPaint.srgb2lin(rgbaf[2]));
+            double[] scRGB = DrawPaint.RGB2SCRGB(color);
+            rgb.setR((int)Math.rint(scRGB[0]*100_000d));
+            rgb.setG((int)Math.rint(scRGB[1]*100_000d));
+            rgb.setB((int)Math.rint(scRGB[2]*100_000d));
             alphaPct = (addAlpha) ? rgb.addNewAlpha() : null;
         }
 
         // alpha (%)
         if (alphaPct != null) {
-            alphaPct.setVal((int)(100000 * rgbaf[3]));
+            alphaPct.setVal((int)Math.rint(rgbaf[3]*100_000));
         }
     }
 
diff --git a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFSlide.java b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFSlide.java
index b9cda3fd21..cdb3dc4182 100644
--- a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFSlide.java
+++ b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFSlide.java
@@ -16,7 +16,7 @@
 ==================================================================== */
 package org.apache.poi.xslf.usermodel;
 
-import static org.apache.poi.sl.TestCommonSL.sameColor;
+import static org.apache.poi.sl.TestCommonSL.getColor;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -139,7 +139,7 @@ public class TestXSLFSlide {
         assertEquals(40.0, r1.getFontSize(), 0);
         assertTrue(r1.isBold());
         assertTrue(r1.isItalic());
-        assertTrue(sameColor(new Color(148, 198, 0), r1.getFontColor()));
+        assertEquals(new Color(148, 198, 0), getColor(r1.getFontColor()));
         assertNull(sh1.getFillColor());
         assertNull(sh1.getLineColor());
 
@@ -152,7 +152,7 @@ public class TestXSLFSlide {
         assertEquals(18.0, r2.getFontSize(), 0);
         assertFalse(r2.isBold());
         assertFalse(r2.isItalic());
-        assertTrue(sameColor(Color.white, r2.getFontColor()));
+        assertEquals(Color.white, getColor(r2.getFontColor()));
         assertEquals(new Color(148, 198, 0), sh2.getFillColor());
         assertEquals(new Color(148, 198, 0), sh2.getLineColor()); // slightly different from PowerPoint!
 
@@ -168,7 +168,7 @@ public class TestXSLFSlide {
         //assertEquals(32.4.0, r3.getFontSize());
         assertTrue(r3.isBold());
         assertTrue(r3.isItalic());
-        assertTrue(sameColor(new Color(148, 198, 0), r3.getFontColor()));
+        assertEquals(new Color(148, 198, 0), getColor(r3.getFontColor()));
         assertNull(sh3.getFillColor());
         assertNull(sh3.getLineColor());
 
diff --git a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java
index 2cb7c412ce..72cf8cc388 100644
--- a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java
+++ b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java
@@ -16,7 +16,7 @@
 ==================================================================== */
 package org.apache.poi.xslf.usermodel;
 
-import static org.apache.poi.sl.TestCommonSL.sameColor;
+import static org.apache.poi.sl.TestCommonSL.getColor;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
@@ -285,7 +285,7 @@ public class TestXSLFTextParagraph {
 
         assertNull(p.getBulletFontColor());
         p.setBulletFontColor(Color.red);
-        assertTrue(sameColor(Color.red, p.getBulletFontColor()));
+        assertEquals(Color.red, getColor(p.getBulletFontColor()));
 
         assertNull(p.getBulletFontSize());
         p.setBulletFontSize(200.);
diff --git a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextRun.java b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextRun.java
index 90f07e29c3..a3158b234d 100644
--- a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextRun.java
+++ b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextRun.java
@@ -18,7 +18,7 @@
  */
 package org.apache.poi.xslf.usermodel;
 
-import static org.apache.poi.sl.TestCommonSL.sameColor;
+import static org.apache.poi.sl.TestCommonSL.getColor;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -55,9 +55,9 @@ public class TestXSLFTextRun {
         assertEquals(0., r.getCharacterSpacing(), 0);
         assertFalse(r.getRPr(true).isSetSpc());
 
-        assertTrue(sameColor(Color.black, r.getFontColor()));
+        assertEquals(Color.black, getColor(r.getFontColor()));
         r.setFontColor(Color.red);
-        assertTrue(sameColor(Color.red, r.getFontColor()));
+        assertEquals(Color.red, getColor(r.getFontColor()));
 
         assertEquals("Calibri", r.getFontFamily());
         r.setFontFamily("Arial");
@@ -78,7 +78,7 @@ public class TestXSLFTextRun {
         assertTrue(r.isSubscript());
         r.setSubscript(false);
         assertFalse(r.isSubscript());
-        
+
         ppt.close();
     }
 
diff --git a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextShape.java b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextShape.java
index 0d2c94999b..21f609f227 100644
--- a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextShape.java
+++ b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextShape.java
@@ -16,7 +16,7 @@
 ==================================================================== */
 package org.apache.poi.xslf.usermodel;
 
-import static org.apache.poi.sl.TestCommonSL.sameColor;
+import static org.apache.poi.sl.TestCommonSL.getColor;
 import static org.apache.poi.xslf.usermodel.TestXSLFSimpleShape.getSpPr;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -41,6 +41,7 @@ import org.apache.poi.sl.usermodel.TextParagraph.TextAlign;
 import org.apache.poi.sl.usermodel.VerticalAlignment;
 import org.apache.poi.xddf.usermodel.text.XDDFBodyProperties;
 import org.apache.poi.xddf.usermodel.text.XDDFTextBody;
+import org.apache.poi.xddf.usermodel.text.XDDFTextParagraph;
 import org.apache.poi.xslf.XSLFTestDataSamples;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -76,7 +77,7 @@ public class TestXSLFTextShape {
         verifySlide7(slide.get(6));
         verifySlide8(slide.get(7));
         verifySlide10(slide.get(9));
-        
+
         ppt.close();
     }
 
@@ -118,11 +119,11 @@ public class TestXSLFTextShape {
         // now check text properties
         assertEquals("Centered Title", shape1.getText());
         assertEquals("Centered Title",
-            tb1.getParagraphs().stream().map(p -> p.getText()).collect(Collectors.joining("\n")));
+            tb1.getParagraphs().stream().map(XDDFTextParagraph::getText).collect(Collectors.joining("\n")));
         XSLFTextRun r1 = shape1.getTextParagraphs().get(0).getTextRuns().get(0);
         assertEquals("Calibri", r1.getFontFamily());
         assertEquals(44.0, r1.getFontSize(), 0);
-        assertTrue(sameColor(Color.black, r1.getFontColor()));
+        assertEquals(Color.black, getColor(r1.getFontColor()));
 
         XSLFTextShape shape2 = (XSLFTextShape)shapes.get(1);
         XDDFTextBody tb2 = shape2.getTextBody();
@@ -155,7 +156,7 @@ public class TestXSLFTextShape {
         assertNull(tbp2.getAnchoring());
 
         assertEquals("subtitle", shape2.getText());
-        assertEquals("subtitle", tb2.getParagraphs().stream().map(p -> p.getText()).collect(Collectors.joining("\n")));
+        assertEquals("subtitle", tb2.getParagraphs().stream().map(XDDFTextParagraph::getText).collect(Collectors.joining("\n")));
         XSLFTextRun r2 = shape2.getTextParagraphs().get(0).getTextRuns().get(0);
         assertEquals("Calibri", r2.getFontFamily());
         assertEquals(32.0, r2.getFontSize(), 0);
@@ -199,7 +200,7 @@ public class TestXSLFTextShape {
         XSLFTextRun r1 = shape1.getTextParagraphs().get(0).getTextRuns().get(0);
         assertEquals("Calibri", r1.getFontFamily());
         assertEquals(44.0, r1.getFontSize(), 0);
-        assertTrue(sameColor(Color.black, r1.getFontColor()));
+        assertEquals(Color.black, getColor(r1.getFontColor()));
 
         XSLFTextShape shape2 = (XSLFTextShape)shapes.get(1);
         CTPlaceholder ph2 = shape2.getPlaceholderDetails().getCTPlaceholder(false);
@@ -234,8 +235,8 @@ public class TestXSLFTextShape {
         assertEquals("Content", pr1.getRawText());
         assertEquals("Calibri", pr1.getFontFamily());
         assertEquals(32.0, pr1.getFontSize(), 0);
-        assertEquals(27.0, pr1.getParentParagraph().getLeftMargin(), 0); 
-        assertEquals("\u2022", pr1.getParentParagraph().getBulletCharacter()); 
+        assertEquals(27.0, pr1.getParentParagraph().getLeftMargin(), 0);
+        assertEquals("\u2022", pr1.getParentParagraph().getBulletCharacter());
         assertEquals("Arial", pr1.getParentParagraph().getBulletFont());
 
         XSLFTextRun pr2 = shape2.getTextParagraphs().get(1).getTextRuns().get(0);
@@ -310,7 +311,7 @@ public class TestXSLFTextShape {
         assertEquals(TextAlign.LEFT, r1.getParentParagraph().getTextAlign());
         assertEquals("Calibri", r1.getFontFamily());
         assertEquals(40.0, r1.getFontSize(), 0);
-        assertTrue(sameColor(Color.black, r1.getFontColor()));
+        assertEquals(Color.black, getColor(r1.getFontColor()));
         assertTrue(r1.isBold());
         assertFalse(r1.isItalic());
         assertFalse(r1.isUnderlined());
@@ -384,7 +385,7 @@ public class TestXSLFTextShape {
         assertEquals(TextAlign.CENTER, r1.getParentParagraph().getTextAlign());
         assertEquals("Calibri", r1.getFontFamily());
         assertEquals(44.0, r1.getFontSize(), 0);
-        assertTrue(sameColor(Color.black, r1.getFontColor()));
+        assertEquals(Color.black, getColor(r1.getFontColor()));
 
         XSLFTextShape shape2 = (XSLFTextShape)shapes.get(1);
         CTPlaceholder ph2 = shape2.getPlaceholderDetails().getCTPlaceholder(false);
@@ -452,7 +453,7 @@ public class TestXSLFTextShape {
         assertEquals(0, pr5.getParentParagraph().getIndentLevel());
         assertEquals("Right", pr5.getRawText());
         assertEquals("Calibri", pr5.getFontFamily());
-        assertTrue(sameColor(Color.black, pr5.getFontColor()));
+        assertEquals(Color.black, getColor(pr5.getFontColor()));
     }
 
     @SuppressWarnings("unused")
@@ -460,7 +461,7 @@ public class TestXSLFTextShape {
         XSLFSlideLayout layout = slide.getSlideLayout();
         List<XSLFShape> shapes = slide.getShapes();
         // TODO
-    }    
+    }
 
     void verifySlide7(XSLFSlide slide){
         XSLFSlideLayout layout = slide.getSlideLayout();
@@ -492,7 +493,7 @@ public class TestXSLFTextShape {
         assertEquals(TextAlign.CENTER, r1.getParentParagraph().getTextAlign());
         assertEquals("Calibri", r1.getFontFamily());
         assertEquals(44.0, r1.getFontSize(), 0);
-        assertTrue(sameColor(Color.black, r1.getFontColor()));
+        assertEquals(Color.black, getColor(r1.getFontColor()));
         assertFalse(r1.isBold());
 
         XSLFTextShape shape2 = (XSLFTextShape)shapes.get(1);
@@ -565,7 +566,7 @@ public class TestXSLFTextShape {
         assertEquals(TextAlign.LEFT, r1.getParentParagraph().getTextAlign());
         assertEquals("Calibri", r1.getFontFamily());
         assertEquals(20.0, r1.getFontSize(), 0);
-        assertTrue(sameColor(Color.black, r1.getFontColor()));
+        assertEquals(Color.black, getColor(r1.getFontColor()));
         assertTrue(r1.isBold());
 
         XSLFTextShape shape2 = (XSLFTextShape)shapes.get(1);
@@ -652,8 +653,7 @@ public class TestXSLFTextShape {
         assertEquals(TextAlign.CENTER, r1.getParentParagraph().getTextAlign());
         assertEquals("Calibri", r1.getFontFamily());
         assertEquals(12.0, r1.getFontSize(), 0);
-        // TODO calculation of tint might be incorrect
-        assertTrue(sameColor(new Color(191,191,191), r1.getFontColor()));
+        assertEquals(new Color(0x898989), getColor(r1.getFontColor()));
 
         XSLFTextShape dt = (XSLFTextShape)slide.getPlaceholderByType(STPlaceholderType.INT_DT);
         assertEquals("Friday, October 21, 2011", dt.getText());
@@ -741,7 +741,7 @@ public class TestXSLFTextShape {
         assertEquals("Calibri", textRun.getFontFamily());
         lv5PPr.setAlgn(STTextAlignType.CTR);
         assertEquals(TextAlign.CENTER, paragraph.getTextAlign());
-        
+
         ppt.close();
     }
 
@@ -945,10 +945,10 @@ public class TestXSLFTextShape {
         assertEquals("Calibri", r3.getFontFamily());
         lv3PPr.setAlgn(STTextAlignType.CTR);
         assertEquals(TextAlign.CENTER, p3.getTextAlign());
-        
+
         ppt.close();
     }
-    
+
     @Test
     public void metroBlob() throws IOException, ReflectiveOperationException {
         assumeFalse(xslfOnly);
diff --git a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTheme.java b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTheme.java
index 1a504e7950..4a90fbc769 100644
--- a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTheme.java
+++ b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTheme.java
@@ -16,7 +16,7 @@
 ==================================================================== */
 package org.apache.poi.xslf.usermodel;
 
-import static org.apache.poi.sl.TestCommonSL.sameColor;
+import static org.apache.poi.sl.TestCommonSL.getColor;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
@@ -72,7 +72,7 @@ public class TestXSLFTheme {
 
         XSLFTextShape sh1 = (XSLFTextShape)getShape(slide, "Rectangle 3");
         XSLFTextRun run1 = sh1.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(Color.white, run1.getFontColor()));
+        assertEquals(Color.white, getColor(run1.getFontColor()));
         assertEquals(new Color(79, 129, 189), sh1.getFillColor());
         assertTrue(sh1.getFillStyle().getPaint() instanceof SolidPaint) ;   // solid fill
 
@@ -95,13 +95,13 @@ public class TestXSLFTheme {
 
         XSLFTextShape sh1 = (XSLFTextShape)getShape(slide, "Rectangle 4");
         XSLFTextRun run1 = sh1.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(Color.white, run1.getFontColor()));
+        assertEquals(Color.white, getColor(run1.getFontColor()));
         assertEquals(new Color(148, 198, 0), sh1.getFillColor());
         assertTrue(sh1.getFillStyle().getPaint() instanceof SolidPaint) ;   // solid fill
 
         XSLFTextShape sh2 = (XSLFTextShape)getShape(slide, "Title 3");
         XSLFTextRun run2 = sh2.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(new Color(148, 198, 0), run2.getFontColor()));
+        assertEquals(new Color(148, 198, 0), getColor(run2.getFontColor()));
         assertNull(sh2.getFillColor());  // no fill
 
         assertTrue(slide.getSlideLayout().getFollowMasterGraphics());
@@ -113,7 +113,7 @@ public class TestXSLFTheme {
 
         XSLFTextShape sh2 = (XSLFTextShape)getShape(slide, "Title 1");
         XSLFTextRun run2 = sh2.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(new Color(148, 198, 0), run2.getFontColor()));
+        assertEquals(new Color(148, 198, 0), getColor(run2.getFontColor()));
         assertNull(sh2.getFillColor());  // no fill
         // font size is 40pt and scale factor is 90%
         assertEquals(36.0, run2.getFontSize(), 0);
@@ -125,12 +125,12 @@ public class TestXSLFTheme {
 
         XSLFTextShape sh1 = (XSLFTextShape)getShape(slide, "Subtitle 3");
         XSLFTextRun run1 = sh1.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(new Color(66, 66, 66), run1.getFontColor()));
+        assertEquals(new Color(66, 66, 66), getColor(run1.getFontColor()));
         assertNull(sh1.getFillColor());  // no fill
 
         XSLFTextShape sh2 = (XSLFTextShape)getShape(slide, "Title 2");
         XSLFTextRun run2 = sh2.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(new Color(148, 198, 0), run2.getFontColor()));
+        assertEquals(new Color(148, 198, 0), getColor(run2.getFontColor()));
         assertNull(sh2.getFillColor());  // no fill
 
         assertFalse(slide.getSlideLayout().getFollowMasterGraphics());
@@ -160,12 +160,12 @@ public class TestXSLFTheme {
 
         XSLFTextShape sh1 = (XSLFTextShape)getShape(slide, "Title 3");
         XSLFTextRun run1 = sh1.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(Color.white, run1.getFontColor()));
+        assertEquals(Color.white, getColor(run1.getFontColor()));
         assertNull(sh1.getFillColor());  // no fill
 
         XSLFTextShape sh2 = (XSLFTextShape)getShape(slide, "Subtitle 4");
         XSLFTextRun run2 = sh2.getTextParagraphs().get(0).getTextRuns().get(0);
-        assertTrue(sameColor(Color.white, run2.getFontColor()));
+        assertEquals(Color.white, getColor(run2.getFontColor()));
         assertNull(sh2.getFillColor());  // no fill
     }
 }
diff --git a/src/scratchpad/testcases/org/apache/poi/hslf/model/TestShapes.java b/src/scratchpad/testcases/org/apache/poi/hslf/model/TestShapes.java
index 34de4fb252..fa0b4ea71b 100644
--- a/src/scratchpad/testcases/org/apache/poi/hslf/model/TestShapes.java
+++ b/src/scratchpad/testcases/org/apache/poi/hslf/model/TestShapes.java
@@ -17,7 +17,7 @@
 
 package org.apache.poi.hslf.model;
 
-import static org.apache.poi.sl.TestCommonSL.sameColor;
+import static org.apache.poi.sl.TestCommonSL.getColor;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -70,15 +70,10 @@ public final class TestShapes {
 
     @Before
     public void setUp() throws Exception {
-        InputStream is1 = null, is2 = null;
-        try {
-            is1 = _slTests.openResourceAsStream("empty.ppt");
+        try (InputStream is1 = _slTests.openResourceAsStream("empty.ppt");
+             InputStream is2 = _slTests.openResourceAsStream("empty_textbox.ppt")) {
             ppt = new HSLFSlideShow(is1);
-            is2 = _slTests.openResourceAsStream("empty_textbox.ppt");
             pptB = new HSLFSlideShow(is2);
-        } finally {
-            is1.close();
-            is2.close();
         }
     }
 
@@ -121,13 +116,12 @@ public final class TestShapes {
 
         assertTrue(shape.get(1) instanceof HSLFAutoShape); //group shape
         assertEquals(ellipseAnchor, shape.get(1).getAnchor()); //group shape
-        
+
         ppt2.close();
     }
 
     /**
      * Verify that we can read TextBox shapes
-     * @throws Exception
      */
     @Test
     public void textBoxRead() throws IOException {
@@ -142,20 +136,29 @@ public final class TestShapes {
             assertEquals(txtbox.getTextParagraphs().get(0).getTextRuns().size(), 1);
             HSLFTextRun rt = txtbox.getTextParagraphs().get(0).getTextRuns().get(0);
 
-            if (text.equals("Hello, World!!!")){
-                assertEquals(32, rt.getFontSize(), 0);
-                assertTrue(rt.isBold());
-                assertTrue(rt.isItalic());
-            } else if (text.equals("I am just a poor boy")){
-                assertEquals(44, rt.getFontSize(), 0);
-                assertTrue(rt.isBold());
-            } else if (text.equals("This is Times New Roman")){
-                assertEquals(16, rt.getFontSize(), 0);
-                assertTrue(rt.isBold());
-                assertTrue(rt.isItalic());
-                assertTrue(rt.isUnderlined());
-            } else if (text.equals("Plain Text")){
-                assertEquals(18, rt.getFontSize(), 0);
+            switch (text) {
+                case "Hello, World!!!":
+                    assertNotNull(rt.getFontSize());
+                    assertEquals(32, rt.getFontSize(), 0);
+                    assertTrue(rt.isBold());
+                    assertTrue(rt.isItalic());
+                    break;
+                case "I am just a poor boy":
+                    assertNotNull(rt.getFontSize());
+                    assertEquals(44, rt.getFontSize(), 0);
+                    assertTrue(rt.isBold());
+                    break;
+                case "This is Times New Roman":
+                    assertNotNull(rt.getFontSize());
+                    assertEquals(16, rt.getFontSize(), 0);
+                    assertTrue(rt.isBold());
+                    assertTrue(rt.isItalic());
+                    assertTrue(rt.isUnderlined());
+                    break;
+                case "Plain Text":
+                    assertNotNull(rt.getFontSize());
+                    assertEquals(18, rt.getFontSize(), 0);
+                    break;
             }
         }
     }
@@ -179,30 +182,30 @@ public final class TestShapes {
         shape.setAnchor(new Rectangle2D.Double(100,100,100,10));
         slide.addShape(shape);
         shape.resizeToFitText();
-        
+
         ByteArrayOutputStream bos = new ByteArrayOutputStream();
         ss.write(bos);
-        
+
         ss = new HSLFSlideShow(new ByteArrayInputStream(bos.toByteArray()));
         slide = ss.getSlides().get(0);
         HSLFTextBox tb = (HSLFTextBox)slide.getShapes().get(0);
         List<HSLFTextParagraph> para = tb.getTextParagraphs();
         HSLFTextRun tr = para.get(0).getTextRuns().get(0);
         assertEquals("para 1 run 1. ", tr.getRawText());
-        assertTrue(sameColor(Color.black, tr.getFontColor()));
+        assertEquals(Color.black, getColor(tr.getFontColor()));
         tr = para.get(0).getTextRuns().get(1);
         assertEquals("para 1 run 2.\r",  tr.getRawText());
-        assertTrue(sameColor(Color.red, tr.getFontColor()));
+        assertEquals(Color.red, getColor(tr.getFontColor()));
         tr = para.get(1).getTextRuns().get(0);
         assertEquals("para 2 run 1. ", tr.getRawText());
-        assertTrue(sameColor(Color.yellow, tr.getFontColor()));
+        assertEquals(Color.yellow, getColor(tr.getFontColor()));
         tr = para.get(1).getTextRuns().get(1);
         assertEquals("para 2 run 2. para 2 run 3.", tr.getRawText());
-        assertTrue(sameColor(Color.black, tr.getFontColor()));
+        assertEquals(Color.black, getColor(tr.getFontColor()));
         assertTrue(tr.isStrikethrough());
     }
-        
-    
+
+
     /**
      * Verify that we can add TextBox shapes to a slide
      * and set some of the style attributes
@@ -230,12 +233,13 @@ public final class TestShapes {
         // Check it before save
         rt = txtbox.getTextParagraphs().get(0).getTextRuns().get(0);
         assertEquals(val, rt.getRawText());
+        assertNotNull(rt.getFontSize());
         assertEquals(42, rt.getFontSize(), 0);
         assertTrue(rt.isBold());
         assertTrue(rt.isItalic());
         assertFalse(rt.isUnderlined());
         assertEquals("Arial", rt.getFontFamily());
-        assertTrue(sameColor(Color.red, rt.getFontColor()));
+        assertEquals(Color.red, getColor(rt.getFontColor()));
 
         // Serialize and read again
         ByteArrayOutputStream out = new ByteArrayOutputStream();
@@ -250,13 +254,14 @@ public final class TestShapes {
 
         // Check after save
         assertEquals(val, rt.getRawText());
+        assertNotNull(rt.getFontSize());
         assertEquals(42, rt.getFontSize(), 0);
         assertTrue(rt.isBold());
         assertTrue(rt.isItalic());
         assertFalse(rt.isUnderlined());
         assertEquals("Arial", rt.getFontFamily());
-        assertTrue(sameColor(Color.red, rt.getFontColor()));
-        
+        assertEquals(Color.red, getColor(rt.getFontColor()));
+
         ppt2.close();
     }
 
@@ -368,7 +373,7 @@ public final class TestShapes {
 
         line = (HSLFLine)grshape.get(1);
         assertEquals(new Rectangle2D.Double(300, 300, 500, 0), line.getAnchor());
-        
+
         ss.close();
     }
 
@@ -421,7 +426,6 @@ public final class TestShapes {
     public void shapeId() throws IOException {
         HSLFSlideShow ss = new HSLFSlideShow();
         HSLFSlide slide = ss.createSlide();
-        HSLFShape shape = null;
 
         //EscherDgg is a document-level record which keeps track of the drawing groups
         EscherDggRecord dgg = ss.getDocumentRecord().getPPDrawingGroup().getEscherDggRecord();
@@ -434,7 +438,7 @@ public final class TestShapes {
         int dgShapesUsed = dg.getNumShapes();          // number of shapes in the slide
         //insert 3 shapes and make sure the Ids are properly incremented
         for (int i = 0; i < 3; i++) {
-            shape = new HSLFLine();
+            HSLFShape shape = new HSLFLine();
             assertEquals(0, shape.getShapeId());
             slide.addShape(shape);
             assertTrue(shape.getShapeId() > 0);
@@ -461,7 +465,7 @@ public final class TestShapes {
         //make sure it is so
         int numClusters = dgg.getNumIdClusters();
         for (int i = 0; i < 1025; i++) {
-            shape = new HSLFLine();
+            HSLFShape shape = new HSLFLine();
             slide.addShape(shape);
         }
         assertEquals(numClusters + 1, dgg.getNumIdClusters());
@@ -492,7 +496,7 @@ public final class TestShapes {
         assertEquals("Border width is 5 pt", sh4.getText());
         assertEquals(Color.black, sh4.getLineColor());
         assertEquals(5.0, sh4.getLineWidth(), 0);
-        
+
         ss.close();
     }
 }
diff --git a/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestTextRun.java b/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestTextRun.java
index 327720cb85..6bae49681a 100644
--- a/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestTextRun.java
+++ b/src/scratchpad/testcases/org/apache/poi/hslf/usermodel/TestTextRun.java
@@ -17,7 +17,7 @@
 
 package org.apache.poi.hslf.usermodel;
 
-import static org.apache.poi.sl.TestCommonSL.sameColor;
+import static org.apache.poi.sl.TestCommonSL.getColor;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
@@ -31,7 +31,6 @@ import java.util.List;
 
 import org.apache.poi.hslf.HSLFTestDataSamples;
 import org.apache.poi.hslf.model.textproperties.TextPropCollection;
-import org.apache.poi.hslf.record.Record;
 import org.apache.poi.hslf.record.TextBytesAtom;
 import org.apache.poi.hslf.record.TextCharsAtom;
 import org.apache.poi.hslf.record.TextHeaderAtom;
@@ -42,6 +41,7 @@ import org.junit.Test;
 /**
  * Tests for TextRuns
  */
+@SuppressWarnings("UnusedAssignment")
 public final class TestTextRun {
 	// SlideShow primed on the test data
 	private HSLFSlideShow ss;
@@ -61,7 +61,7 @@ public final class TestTextRun {
 	    ssRich.close();
 	    ss.close();
 	}
-	
+
 	/**
 	 * Test to ensure that getting the text works correctly
 	 */
@@ -125,7 +125,7 @@ public final class TestTextRun {
 		HSLFSlide slideOne = ss.getSlides().get(0);
 		List<HSLFTextParagraph> paras = slideOne.getTextParagraphs().get(0);
 		HSLFTextParagraph para = paras.get(0);
-		
+
         TextHeaderAtom tha = null;
         TextBytesAtom tba = null;
         TextCharsAtom tca = null;
@@ -150,14 +150,12 @@ public final class TestTextRun {
             else if (r instanceof TextBytesAtom) tba = (TextBytesAtom)r;
             else if (r instanceof TextCharsAtom) tca = (TextCharsAtom)r;
         }
-		
+
 		assertEquals(changeBytesOnly, HSLFTextParagraph.getRawText(paras));
 		assertNull(tca);
 		assertNotNull(tba);
 
 		// Bytes -> Chars
-        assertNull(tca);
-        assertNotNull(tba);
 		assertEquals(changeBytesOnly, HSLFTextParagraph.getRawText(paras));
 
 		String changeByteChar = "This is a test title with a '\u0121' g with a dot";
@@ -168,14 +166,13 @@ public final class TestTextRun {
             if (r instanceof TextHeaderAtom) tha = (TextHeaderAtom)r;
             else if (r instanceof TextBytesAtom) tba = (TextBytesAtom)r;
             else if (r instanceof TextCharsAtom) tca = (TextCharsAtom)r;
-        }		
+        }
 
 		assertEquals(changeByteChar, HSLFTextParagraph.getRawText(paras));
 		assertNotNull(tca);
 		assertNull(tba);
 
 		// Chars -> Chars
-		assertNull(tba);
 		assertNotNull(tca);
 		assertEquals(changeByteChar, HSLFTextParagraph.getRawText(paras));
 
@@ -187,7 +184,7 @@ public final class TestTextRun {
             if (r instanceof TextHeaderAtom) tha = (TextHeaderAtom)r;
             else if (r instanceof TextBytesAtom) tba = (TextBytesAtom)r;
             else if (r instanceof TextCharsAtom) tca = (TextCharsAtom)r;
-        }       
+        }
 
 		assertEquals(changeCharChar, HSLFTextParagraph.getRawText(paras));
 		assertNotNull(tca);
@@ -445,7 +442,7 @@ public final class TestTextRun {
 		HSLFSlide sl = ppt.getSlides().get(0);
         List<List<HSLFTextParagraph>> textParass = sl.getTextParagraphs();
 		assertEquals(2, textParass.size());
-		
+
 		List<HSLFTextParagraph> textParas = textParass.get(0);
 		rt = textParass.get(0).get(0).getTextRuns();
 		assertEquals(1, rt.size());
@@ -541,7 +538,7 @@ public final class TestTextRun {
                 // tx.storeText();
             }
         }
-        
+
         HSLFSlideShow ppt2 = HSLFTestDataSamples.writeOutAndReadBack(ppt1);
         for(HSLFSlide slide : ppt2.getSlides()){
             for(HSLFShape sh : slide.getShapes()){
@@ -550,7 +547,7 @@ public final class TestTextRun {
                     List<HSLFTextParagraph> run = tx.getTextParagraphs();
                     HSLFTextRun rt = run.get(0).getTextRuns().get(0);
                     assertTrue(rt.isBold());
-                    assertTrue(sameColor(Color.RED, rt.getFontColor()));
+                    assertEquals(Color.RED, getColor(rt.getFontColor()));
                 }
             }
         }
@@ -564,11 +561,13 @@ public final class TestTextRun {
         HSLFSlide slide = ppt.getSlides().get(0);
 
         int[] sizes = {36, 24, 12, 32, 12, 12};
-        
+
         int i=0;
         for (List<HSLFTextParagraph> textParas : slide.getTextParagraphs()) {
-            assertEquals("Arial", textParas.get(0).getTextRuns().get(0).getFontFamily());
-            assertEquals(sizes[i++], textParas.get(0).getTextRuns().get(0).getFontSize().intValue());
+			HSLFTextRun first = textParas.get(0).getTextRuns().get(0);
+            assertEquals("Arial", first.getFontFamily());
+            assertNotNull(first.getFontSize());
+            assertEquals(sizes[i++], first.getFontSize().intValue());
         }
         ppt.close();
     }
diff --git a/src/testcases/org/apache/poi/sl/TestCommonSL.java b/src/testcases/org/apache/poi/sl/TestCommonSL.java
index bb04ad71cb..2911b12e42 100644
--- a/src/testcases/org/apache/poi/sl/TestCommonSL.java
+++ b/src/testcases/org/apache/poi/sl/TestCommonSL.java
@@ -29,10 +29,10 @@ import org.junit.Ignore;
 @Ignore
 public class TestCommonSL {
 
-    public static boolean sameColor(Color colorExpected, PaintStyle paintActual) {
-        if (!(paintActual instanceof SolidPaint)) return false;
-        Color thisC = DrawPaint.applyColorTransform(((SolidPaint)paintActual).getSolidColor());
-        return thisC.equals(colorExpected);
+    public static Color getColor(PaintStyle paintActual) {
+        return (paintActual instanceof SolidPaint)
+            ? DrawPaint.applyColorTransform(((SolidPaint)paintActual).getSolidColor())
+            : null;
     }
 
 }