RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v7]

[Alexey Ivanov]

On Wed, 3 Jan 2024 03:45:06 GMT, Renjith Kannath Pariyangad <rkannathpari at openjdk.org> wrote:

>> Hi Reviewers, 
>> There was a typo for color conversion instead of dstColorSpace function srcColorSpace was used. Please review and let me know your suggestions if any. 
>> 
>> Renjith.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Copyright year updated
>  - Review suggestions implemented

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java line 1:

> 1: /*

Please update the copyright year.

test/jdk/java/awt/color/NonICCFilterTest.java line 38:

> 36:  * @test
> 37:  * @bug 8316497
> 38:  * @summary Verifies Color filter on Non ICC profile

Suggestion:

 * @summary Verifies Color filter on non-ICC profile

test/jdk/java/awt/color/NonICCFilterTest.java line 51:

> 49:             csRGB = ColorSpace.getInstance(bSrc ? ColorSpace.CS_LINEAR_RGB :
> 50:                     ColorSpace.CS_sRGB);
> 51:         }

I propose to create a better wrapper:


        protected TestColorSpace(ColorSpace csRGB) {
            super(csRGB.getType(), csRGB.getNumComponents());
            this.csRGB = csRGB;
        }


You'll create the wrapper in a cleaner way:



    private static ColorSpace createColorSpace(boolean isSrc) {
        return new TestColorSpace(ColorSpace.getInstance(isSrc
                                                         ? ColorSpace.CS_LINEAR_RGB
                                                         : ColorSpace.CS_sRGB));
    }

test/jdk/java/awt/color/NonICCFilterTest.java line 53:

> 51:         }
> 52: 
> 53:         public float[] toRGB(float[] colorvalue) {

Please add `@Override` annotations to the methods: `toRGB`, `fromRGB`, `toCIEXYZ`, `fromCIEXYZ`.

test/jdk/java/awt/color/NonICCFilterTest.java line 55:

> 53:         public float[] toRGB(float[] colorvalue) {
> 54:             return colorvalue;
> 55:         }

Suggestion:

        public float[] toRGB(float[] colorvalue) {
            return csRGB.toRGB(colorvalue);
        }


This is what Sergey means. The comment applies to `fromRGB` method too.

test/jdk/java/awt/color/NonICCFilterTest.java line 101:

> 99:         BufferedImage src, dest;
> 100:         src = createTestImage(true);
> 101:         dest = createTestImage(false);

Declare variables at the same line where you initialise them:

Suggestion:

        BufferedImage src = createTestImage(true);
        BufferedImage dest = createTestImage(false);


Use `dst` as a more common abbreviation?

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

PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1802942699
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440885046
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440842711
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440879982
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440886545
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440881611
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440883343