<AWT Dev> [9] Review request for 8033534 Get MultiResolution image from native system

[Petr Pchelko]

Hello, Alexander.

The fix look good to me.

With best regards. Petr.

26 февр. 2014 г., в 6:40 после полудня, Alexander Scherbatiy <alexandr.scherbatiy at oracle.com> написал(а):

> 
> Hello,
> 
>  Could you review the updated fix:
>     http://cr.openjdk.java.net/~alexsch/8033534/webrev.03/
> 
> On 2/26/2014 4:54 PM, Petr Pchelko wrote:
>> Hello, Alexander.
>> 
>> I have a couple of comments:
>> 
>> 1. You could replace the first loop with indexOfObjectPassingTest method.. Not sure if this would look cleaner, up to you.
>    Updated. There is one more way to use one loop instead of two. All of them does not look simple.
> 
>>  2. I suppose JNFNewObjectArray could throw the OOM and we would get a parfait warning, could you please add CHECK_NULL_RETURN after it.
>    CHECK_NULL_RETURN is added.
>> 3. In CImage.java you are setting the currentImageIndex to the biggest image representation smaller that the one requested and this representation
>> would be used as a base one in the MultiResolutionBufferedImage. However in MultiResolutionBufferedImage getResolutionVariant you are returning
>> the smallest variant bigger than the requested one. Is this correct?
>    I think that it is correct. Assume that an image with size 300x300 is requested but there are only resolution variants with sizes [250x250] and [350x350].
>    The resolution variant with  [350x350] size is used as the base image.  Now we need to draw the image to region [300x300]. The resolution variant
>    with size [350x350] will be used from the MultiResolution image.
> 
>  Thanks,
>  Alexandr.
> 
> 
>> 
>> Thank you.
>> With best regards. Petr.
>> 
>> On 26.02.2014, at 16:08, Alexander Scherbatiy <alexandr.scherbatiy at oracle.com> wrote:
>> 
>>>  Hello,
>>> 
>>>  Could you review the updated fix:
>>>      http://cr.openjdk.java.net/~alexsch/8033534/webrev.02/
>>> 
>>>  This is the same fix. The only difference is that the MultiResolutionBufferedImage class is used from the fix JDK-8035069.
>>> 
>>>  Thanks,
>>>  Alexandr.
>>> 
>>> 
>>> On 2/10/2014 7:05 PM, Scott Palmer wrote:
>>>> Just to be clear, "the image representations are chosen to be closest to the requested size" is not accurate.
>>>> This change returns the smallest image representation that is greater than or equal to the requested size.  (Which I believe is the correct thing to do.)
>>>> A smaller image representation may be closer to the requested size, but it makes more sense to return a larger image since scaling down to size should produce better results than scaling up.
>>>> 
>>>> Scott
>>>> 
>>>> 
>>>> On Mon, Feb 10, 2014 at 8:53 AM, Alexander Scherbatiy <alexandr.scherbatiy at oracle.com <mailto:alexandr.scherbatiy at oracle.com>> wrote:
>>>> 
>>>> 
>>>>    Could you review the updated fix:
>>>>    http://cr.openjdk.java.net/~alexsch/8033534/webrev.01
>>>>    <http://cr.openjdk.java.net/%7Ealexsch/8033534/webrev.01>
>>>> 
>>>>     - The image representations are chosen to be closest to the
>>>>    requested size.
>>>> 
>>>>      Thanks,
>>>>      Alexandr.
>>>> 
>>>> 
>>>>    On 2/4/2014 5:00 PM, Sergey Bylokhov wrote:
>>>> 
>>>>        Hi, Alexander.
>>>>        I think that getResolutionVariant should return an image which
>>>>        is close as much as possible to the requested size.
>>>> 
>>>>        On 04.02.2014 16:42, Alexander Scherbatiy wrote:
>>>> 
>>>> 
>>>>            Hello,
>>>> 
>>>>            Could you review the fix:
>>>>              bug: https://bugs.openjdk.java.net/browse/JDK-8033534
>>>>              webrev:
>>>>            http://cr.openjdk.java.net/~alexsch/8033534/webrev.00
>>>>            <http://cr.openjdk.java.net/%7Ealexsch/8033534/webrev.00>
>>>> 
>>>>            - The method that gets a sorted array of NSImage
>>>>            representaion pixel sizes for the given image size is added
>>>>            - A MultiResolution image is created if an NSImage has
>>>>            several representations for the given image size
>>>> 
>>>>            Thanks,
>>>>            Alexandr.
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>