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

Petr Pchelko petr.pchelko at oracle.com
Tue Mar 4 03:53:28 PST 2014 

I'm fine with the fix.

>> Hello, Alexander.
>> 
>> In CImage.m:430 - Do we really want to describe and clear the exception?
>> May be it's better to simply return NULL and let Java handle the exception?
>> This made sense when you were continuing the loop, but now doesn't.
>   The exception is cleared because it should not be thrown on the Java side.
>   For example the Toolkit.getDefaultToolkit().getImage("NSImage://NSApplicationIcon") call
>   should not throw an exception in case if nativeGetNSImageRepresentationSizes() call fails.
>   It should just return an Image without resolution variants.
>   The ExceptionDescribe is left just for debugging purposes.
In real life this will never happen, so I'm fine with any decision) 
Just CHECK_EXCEPTION_RETUEN looks nicer than these 3 lines.

With best regards. Petr.

On 04.03.2014, at 15:39, Alexander Scherbatiy <alexandr.scherbatiy at oracle.com> wrote:

> On 3/4/2014 3:30 PM, Petr Pchelko wrote:
>> Hello, Alexander.
>> 
>> In CImage.m:430 - Do we really want to describe and clear the exception?
>> May be it's better to simply return NULL and let Java handle the exception?
>> This made sense when you were continuing the loop, but now doesn't.
>   The exception is cleared because it should not be thrown on the Java side.
>   For example the Toolkit.getDefaultToolkit().getImage("NSImage://NSApplicationIcon") call
>   should not throw an exception in case if nativeGetNSImageRepresentationSizes() call fails.
>   It should just return an Image without resolution variants.
>   The ExceptionDescribe is left just for debugging purposes.
> 
>  Thanks,
>  Alexandr.
> 
>> 
>> With best regards. Petr.
>> 
>> On 04.03.2014, at 15:04, Alexander Scherbatiy <alexandr.scherbatiy at oracle.com> wrote:
>> 
>>>  Hello,
>>> 
>>> Could you review the updated fix:
>>>   http://cr.openjdk.java.net/~alexsch/8033534/webrev.04
>>> 
>>>  - NULL is returned for all cases from the nativeGetNSImageRepresentationsCount method
>>>  - long lines are split
>>>  - SetObjectArrayElement can throw ArrayIndexOutOfBoundsException and ArrayStoreException exceptions.
>>>    We do not expect neither of them because the same length and type is used for the array creation and element settings.
>>>    I updated the exception handling to return NULL if an exception occurs.
>>> 
>>>  Thanks,
>>>  Alexandr.
>>> 
>>> 
>>> On 3/3/2014 11:48 PM, Sergey Bylokhov wrote:
>>>> Hi, Alexander.
>>>> nativeGetNSImageRepresentationsCount three times return different values in case of error (0, NULL, nil).
>>>> What exception we expect from the SetObjectArrayElement and why we continue in this case?
>>>> Also please split soooooooooooooo long lines in these files.
>>>> 
>>>> On 2/26/14 6:40 PM, Alexander Scherbatiy wrote:
>>>>> 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.
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>> 
>>>> -- 
>>>> Best regards, Sergey.
>

More information about the macosx-port-dev mailing list