Hello, Alexander. The updated version of the fix looks good to me.
With best regards. Petr. 14 марта 2014 г., в 7:53 после полудня, Alexander Scherbatiy <alexandr.scherba...@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.scherba...@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. >>> >
