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

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Tue Mar 4 12:41:45 UTC 2014 

On 3/4/14 4:35 PM, Alexander Scherbatiy wrote:
>
>  Could you review the updated fix:
>     http://cr.openjdk.java.net/~alexsch/8033534/webrev.05/
>
>   - JNU_CHECK_EXCEPTION_RETURN(env, NULL) after the 
> SetObjectArrayElement call.
>     Hope that the exception will never be thrown in the production code.
Thanks. The fix looks good.

>
>   Thanks,
>   Alexandr.
>
> On 3/4/2014 4:12 PM, Sergey Bylokhov wrote:
>> On 3/4/14 3:53 PM, Petr Pchelko wrote:
>>> 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.
>> I agree. If call to nativeGetNSImageRepresentationSizes will fail, 
>> will mean that some error exists in our code and it will be better to 
>> fail fast in this case, just return and throw and exception on java 
>> side.
>>>
>>> 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.
>>
>>
>


-- 
Best regards, Sergey.

More information about the awt-dev mailing list