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.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.
