[OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hi Vadim,

Thanks for the review.
I have made suggested changes. Updated Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/

Please review.

Thanks,
Jay

-----Original Message-----
From: Vadim Pakhnushev 
Sent: Friday, October 16, 2015 8:15 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev at openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

What's the point of using Float.compare in the test?
Why not just check if
(horizontalNodeValue.equals(expectedHorizontalValue)) ?
Also please capitalize Attr in the declaration of horizontalattr and verticalattr.

Thanks,
Vadim

On 16.10.2015 17:36, Jayathirth D V wrote:
> Hello All,
>
> Can I get one more review please.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Thursday, October 15, 2015 6:05 PM
> To: Jayathirth D V; 2d-dev at openjdk.java.net; Philip Race
> Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-7182758: 
> BMPMetadata returns invalid PhysicalPixelSpacing
>
> The fix looks fine. The test can be improved a little bit to simplify the int->Integer boxing, but it is not necessary for now.
>
> Thanks.
>
> On 15.10.15 13:17, Jayathirth D V wrote:
>> Hi Sergey,
>>
>> I thought you suggested to check for tighter "true" condition instead of "false" condition.
>>
>> I have made changes to map horizontalNodeValue and verticalNodeValue to expected values. Please find updated Webrev link:
>>
>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/
>>
>> Please review.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Wednesday, October 14, 2015 7:34 PM
>> To: Jayathirth D V; 2d-dev at openjdk.java.net; Philip Race
>> Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-7182758:
>> BMPMetadata returns invalid PhysicalPixelSpacing
>>
>> Hi, Jay.
>>> I suggest to check correct/expected result in the test instead of previous incorrect zero.
>> Here, I suggested to check that the resulted horizontalNodeValue and verticalNodeValue are equal to some expected value. Because if it bigger than zero does not mean it is correct(note to use Float.compare(f1, f2) instead of "==").
>>
>>> Previously I forgot to mention, that it will be good to name the test by some useful name instead of some bug number(see examples in javax/imageio/plugins).
>>>
>>> On 13.10.15 11:12, Jayathirth D V wrote:
>>>> Hello All,
>>>>
>>>> Removed Trailing whitespace present in code.
>>>>
>>>> Please find updated webrev link:
>>>>
>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>> *From:* Jayathirth D V
>>>> *Sent:* Monday, October 12, 2015 2:15 PM
>>>> *To:* 2d-dev at openjdk.java.net; Philip Race; Sergey Bylokhov
>>>> *Subject:* [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for
>>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>>>
>>>> Hello All,
>>>>
>>>> Made small change on how we need to represent floating point 
>>>> constant in
>>>> Java(1000.0 -> 1000.0F).
>>>>
>>>> Please find updated Webrev link:
>>>>
>>>> Webrev : 
>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/
>>>>
>>>> Please review.
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>> *From:* Jayathirth D V
>>>> *Sent:* Thursday, October 08, 2015 2:20 PM
>>>> *To:* 2d-dev at openjdk.java.net <mailto:2d-dev at openjdk.java.net>; 
>>>> Philip Race; Sergey Bylokhov
>>>> *Subject:* [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for
>>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>>>
>>>> Hello All,
>>>>
>>>> Please review following fix in jdk9:
>>>>
>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7182758
>>>>
>>>> Webrev : 
>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/
>>>>
>>>> Bug : BMPMetadata returns invalid PhysicalPixelSpacing
>>>>
>>>> Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more 
>>>> than value 1 in BMP header. Horizontal & Vertical Physical pixel 
>>>> spacing were returned as zero.
>>>>
>>>>                               In getStandardDimensionNode() method 
>>>> of BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter.
>>>> When
>>>>
>>>>                               XPixelsPerMter/ YPixelsPerMter is 
>>>> more than 1. Resulted value is stored without decimal part, which resulted in zero.
>>>>
>>>> Solution : Made changes to how Horizontal & Vertical Physical pixel 
>>>> spacing is calculated so that decimal value is not truncated.
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>
>>> --
>>> Best regards, Sergey.
>>>
>>
>> --
>> Best regards, Sergey.
>>
>
> --
> Best regards, Sergey.