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.scherba...@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.scherba...@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.scherba...@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.scherba...@oracle.com
>>>>>>>> <mailto:alexandr.scherba...@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.
>
