<AWT Dev> [9] Review request for 8033534 Get MultiResolution image from native system
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Tue Mar 4 12:35:58 UTC 2014
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,
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.
>
>
More information about the awt-dev
mailing list