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

[Petr Pchelko]

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