<AWT Dev> [9] Review request for 8035069 [macosx] Loading resolution variants by demand

[Sergey Bylokhov]

Hello, Alexander.
The fix looks good to me too. Thanks!

On 3/14/14 10:38 PM, Petr Pchelko wrote:
> Hello, Alexander.
>
> The updated version of the fix looks good to me.
>
> With best regards. Petr.
>
> 14 марта 2014 г., в 7:53 после полудня, Alexander Scherbatiy <alexandr.scherbatiy at oracle.com> написал(а):
>
>> Hello,
>>
>> Could you review the updated fix:
>>   http://cr.openjdk.java.net/~alexsch/8035069/webrev.01
>>
>> On 3/13/2014 12:21 PM, Petr Pchelko wrote:
>>> Hello, Alexander.
>>>
>>> 1. As Sergey always says, could you please split the long lines.
>>> 2. Instead of the MultiResolutionImageMapper you could use a BiFunction<Image, Integer, Integer>
>>> 3. About the ImageCache. As it's uses an AppContext, could you please mention in the JavaDoc that is must be used from the thread with an AppContext only?
>>      1. 2. and 3 are updated.
>>> 4. I don't really like that you are duplicating the RecyclableSingleton class. May be it's better to make also move it out from com.apple.laf and reuse?
>>       I have added the getSoftReferenceValue(Object key, Supplier<T> supplier) method to the AppContext class. It should reduce the code duplication.
>>> 5. Looks like the old ImageCache contained the following lines:
>>>
>>>   116 if (state.is(JRSUIConstants.Animating.YES)) {
>>>   117     return false;
>>>   118 }
>>> I agree that these are probably not needed, but could you please verify that? Also after these were removed the ImageCache.setImage never returns false, so it could be made void.
>>      Thank you for pointing it out. This is the necessary check for the animated images in the Aqua L&F. I just forgot to move it to the AquaPainter.
>>
>>      I have found one more issue that ImageIcon preloads images by calling image.getProperty("comment", imageObserver) where the imageObserver
>>      is usually null. The MultiResolutionBufferedImage created non-preloaded resolution variants and they were not shown because JMenuItem as an image observer
>>      returns false for the image loading. This is described in the issue 8037405 JMenuItem should check L&F icons in the image observer
>>         https://bugs.openjdk.java.net/browse/JDK-8037405
>>
>>       It seems as a common problem so I added resolution variants preloading to the MultiResolutionBufferedImage.
>>
>>    Thanks,
>>    Alexandr.
>>> With best regards. Petr.
>>>
>>> On 12.03.2014, at 19:03, Alexander Scherbatiy <alexandr.scherbatiy at oracle.com> wrote:
>>>
>>>> Hello,
>>>>
>>>> Could you review the fix:
>>>>   bug: https://bugs.openjdk.java.net/browse/JDK-8035069
>>>>   webrev: http://cr.openjdk.java.net/~alexsch/8035069/webrev.00
>>>>
>>>>   Image resolution variants are generated by request and cached in the ImageCache.
>>>>
>>>>   - ImageCache is refactored to store different type of images and moved to sun.awt.image package.
>>>>   - An object is used as the cache key instead of hash code to prevent inetsection of hash codes for
>>>>     different type of images.
>>>>   - The base image for MultiResolutionBufferedImage is not cached and used for the hash code calculation
>>>>     in the getResolutionVariant method.
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>


-- 
Best regards, Sergey.