[OpenJDK 2D-Dev] [9] Review request for 8151303 [macosx] [hidpi] JButton's low-res. icon is visible when clicking on it
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Mon Aug 15 15:47:01 UTC 2016
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8151303/webrev.04
The MRCI.sizes arrays is reused for for MultiResolutionCachedImage.
Thanks,
Alexandr.
On 11/08/16 23:10, Jim Graham wrote:
> Hi Alexandr,
>
> Should something be done to transfer the array of sizes to the new
> instance if the source is an MRCI? Perhaps a special case for MRCI as
> well that calls mrciInstance.map(mapper) instead of constructing a
> brand new object from scratch?
>
> ...jim
>
> On 08/11/2016 01:32 AM, Alexander Scherbatiy wrote:
>>
>> Could you review the updated fix:
>> http://cr.openjdk.java.net/~alexsch/8151303/webrev.03
>>
>> MultiResolutionToolkitImage handing is added to the
>> MultiResolutionCachedImage.map() method.
>>
>> Thanks,
>> Alexandr.
>>
>> On 11/08/16 01:46, Jim Graham wrote:
>>> Ah, yes, only ToolkitImages can be "not yet loaded" in that manner.
>>>
>>> A quick look suggests that a MRCI should not be an instance of MRTI,
>>> but MRCI.map() does not force its argument to be an instance of MRCI,
>>> just MRI, so it would be possible for someone to pass an MRTI to
>>> MRCI.map() and then it would have this problem.
>>>
>>> Should we change the argument of MRCI.map() to MRCI? (And then you
>>> don't need to cast to Image and use getWidth/Height(Observer) to get
>>> its dimensions.)
>>>
>>> If that is not feasible, we should have it do something different for
>>> a non-MRCI...
>>>
>>> ...jim
>>>
>>> On 8/10/16 5:35 AM, Alexander Scherbatiy wrote:
>>>> On 09/08/16 03:49, Jim Graham wrote:
>>>>> Does MultiResolutionCachedImage.map() work if the Image hasn't been
>>>>> loaded yet (where getWidth/Height(Observer) can
>>>>> return -1)? Can it ever be called in a case like that?
>>>>
>>>> Could we rely on the fact that getWidth/Height(Observer) returns -1
>>>> only for ToolkitImage?
>>>>
>>>> If so, the passed MultiResolutionToolkitImage is handled in
>>>> MultiResolutionToolkitImage.map() method.
>>>> If no, the fix should be updated to load the image size in some way.
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>
>>>>>
>>>>> ...jim
>>>>>
>>>>> On 08/08/2016 12:48 PM, Alexander Scherbatiy wrote:
>>>>>>
>>>>>> Just a friendly reminder.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>
>>>>>> On 27/06/16 22:17, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you review the updated fix:
>>>>>>> http://cr.openjdk.java.net/~alexsch/8151303/webrev.02
>>>>>>>
>>>>>>> The fix does not use a new public API to apply filters to
>>>>>>> multi-resolution images.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>>
>>>>>>> On 14/05/16 02:54, Jim Graham wrote:
>>>>>>>> Another reason to avoid new API is that we don't have to
>>>>>>>> involve the
>>>>>>>> CCC to get this "bug fix" in...
>>>>>>>>
>>>>>>>> ...jim
>>>>>>>>
>>>>>>>> On 5/13/16 3:50 PM, Jim Graham wrote:
>>>>>>>>> That looks very tight. The only issue I'd have is that it
>>>>>>>>> would be
>>>>>>>>> better if this could be done with non-public API for
>>>>>>>>> now - the map() methods could live on one of the sun.awt.image
>>>>>>>>> classes or even in a Swing implementation utility class
>>>>>>>>> and still work just fine. When we have more time to figure
>>>>>>>>> out how
>>>>>>>>> we're going to handle filtering of MRIs in general
>>>>>>>>> we can decide if this API should live on the base MRI interface.
>>>>>>>>>
>>>>>>>>> The only thing we'd lose is BaseMRI having an optimized
>>>>>>>>> implementation of the map() method, but I don't think it's
>>>>>>>>> implementation represents enough of an optimization to matter
>>>>>>>>> when
>>>>>>>>> we are creating and loading dozens of images...
>>>>>>>>>
>>>>>>>>> ...jim
>>>>>>>>>
>>>>>>>>> On 5/12/16 10:08 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Could you review the updated fix:
>>>>>>>>>> http://cr.openjdk.java.net/~alexsch/8151303/webrev.01
>>>>>>>>>>
>>>>>>>>>> There was a suggestion to postpone the fix for the issue 8152309
>>>>>>>>>> Seamless way of using image filters with
>>>>>>>>>> multi-resolution images
>>>>>>>>>> and continue with the current one:
>>>>>>>>>> http://mail.openjdk.java.net/pipermail/2d-dev/2016-April/006766.html
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The new version of the fix introduces a mapper method which
>>>>>>>>>> allows
>>>>>>>>>> to map all resolution variants of one
>>>>>>>>>> multi-resolution image to another:
>>>>>>>>>> ------------
>>>>>>>>>> Image image = // original image
>>>>>>>>>> Image filteredImage = MultiResolutionImage.map(image,
>>>>>>>>>> (img) ->
>>>>>>>>>> /* return filtered image */);
>>>>>>>>>> ------------
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Alexandr.
>>>>>>>>>>
>>>>>>>>>> On 21/03/16 22:31, Jim Graham wrote:
>>>>>>>>>>> We could do that for our own filters, but any random custom
>>>>>>>>>>> filter
>>>>>>>>>>> could run into the same issue, so it might not make
>>>>>>>>>>> sense to upgrade the existing filter mechanism to always
>>>>>>>>>>> attempt
>>>>>>>>>>> to do MR filtering. We could either create a
>>>>>>>>>>> parallel set of MR-aware filter mechanisms (such as the
>>>>>>>>>>> previously
>>>>>>>>>>> suggested new method on Toolkit, or a new MR-aware
>>>>>>>>>>> version of FilteredImageSource for instance) and leave the
>>>>>>>>>>> existing mechanism as clearly documented as MR-unaware.
>>>>>>>>>>> Another idea is to tag a filter with an interface that
>>>>>>>>>>> indicates
>>>>>>>>>>> that it is MR aware? In any case, some thought needs
>>>>>>>>>>> to be given to feeding an MR image to a filter that
>>>>>>>>>>> (potentially
>>>>>>>>>>> or demonstrably) cannot deal with MR images.
>>>>>>>>>>>
>>>>>>>>>>> Alternately, we could then recommend that the old image
>>>>>>>>>>> filtering
>>>>>>>>>>> code isn't combined with multi-resolution images.
>>>>>>>>>>> It seems to me that the programmer is mostly in control over
>>>>>>>>>>> this
>>>>>>>>>>> happening since they've either manually created the
>>>>>>>>>>> MR image using the custiom MR image mechanism or they've
>>>>>>>>>>> supplied
>>>>>>>>>>> media with multiple resolution files (i.e. "@2x").
>>>>>>>>>>> Is that really the case?
>>>>>>>>>>>
>>>>>>>>>>> Whether it is a new filtering mechanism that must be adopted or
>>>>>>>>>>> simply declaring the old filtering mechanism as
>>>>>>>>>>> "obsolete with respect to MR images"...
>>>>>>>>>>>
>>>>>>>>>>> That recommendation then "restricts forward" in that, for
>>>>>>>>>>> example,
>>>>>>>>>>> since Swing relies on the old mechanism, Swing then
>>>>>>>>>>> becomes "not recommended for use with MR images", or "not MR
>>>>>>>>>>> aware". That's probably not a restriction we want to
>>>>>>>>>>> promote so it should be viewed as a temporary restriction
>>>>>>>>>>> reality
>>>>>>>>>>> and a bug that we'll fix soon, whether by using some
>>>>>>>>>>> other mechanism to achieve the desired affects, or creating
>>>>>>>>>>> a new
>>>>>>>>>>> MR-aware filtering mechanism and using it in Swing.
>>>>>>>>>>>
>>>>>>>>>>> Similarly, other 3rd party libraries that accept images and do
>>>>>>>>>>> anything more than display them will have to be
>>>>>>>>>>> upgraded as well before they become "MR aware" or "MR
>>>>>>>>>>> accepting"
>>>>>>>>>>> or whatever term applies here...
>>>>>>>>>>>
>>>>>>>>>>> ...jim
>>>>>>>>>>>
>>>>>>>>>>> On 3/21/16 8:54 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> The one more thing is that image filters should also be
>>>>>>>>>>>> updated
>>>>>>>>>>>> to use
>>>>>>>>>>>> them with multi-resolution images.
>>>>>>>>>>>> For example, consider the case:
>>>>>>>>>>>> ----------
>>>>>>>>>>>> Image mrImage = getMultiResolutionImage();
>>>>>>>>>>>> ImageProducer mriProducer = new
>>>>>>>>>>>> FilteredImageSource(mrImage.getSource(), new
>>>>>>>>>>>> CropImageFilter(0,
>>>>>>>>>>>> 0, w, h));
>>>>>>>>>>>> Toolkit.getDefaultToolkit().createImage(mriProducer);
>>>>>>>>>>>> ----------
>>>>>>>>>>>>
>>>>>>>>>>>> The crop image filter applied to each resolution variant just
>>>>>>>>>>>> cuts
>>>>>>>>>>>> images with the same size.
>>>>>>>>>>>> It seems that there should be added API which allows to set a
>>>>>>>>>>>> scale for
>>>>>>>>>>>> a provided image filter to be properly used with the given
>>>>>>>>>>>> resolution
>>>>>>>>>>>> variant.
>>>>>>>>>>>>
>>>>>>>>>>>> I have created a separated issue for multi-resolution images
>>>>>>>>>>>> filtering
>>>>>>>>>>>> support:
>>>>>>>>>>>> JDK-8152309 Seamless way of using image filters with
>>>>>>>>>>>> multi-resolution
>>>>>>>>>>>> images
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8152309
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>
>>>>>>>>>>>> On 15/03/16 20:35, Alexander Scherbatiy wrote:
>>>>>>>>>>>>> On 15/03/16 18:06, Sergey Bylokhov wrote:
>>>>>>>>>>>>>> On 15.03.16 17:01, Alexander Scherbatiy wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> One update will be that FilteredImageSource should
>>>>>>>>>>>>>>> implement
>>>>>>>>>>>>>>> MultiResolutionImageProducer even it is used for non
>>>>>>>>>>>>>>> multi-resolution
>>>>>>>>>>>>>>> images.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The MRI can be created using two general ways: using
>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>> number of
>>>>>>>>>>>>>>> resolution variants or generating a resolution variant with
>>>>>>>>>>>>>>> necessary
>>>>>>>>>>>>>>> quality on demand.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The current implementation is rely on that MRToolkitImage
>>>>>>>>>>>>>>> contains a
>>>>>>>>>>>>>>> fixed number of resolution variants. In this case
>>>>>>>>>>>>>>> MediaTracker
>>>>>>>>>>>>>>> can
>>>>>>>>>>>>>>> iterate over resolution variants and load them all.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Using MultiResolutionImageProducer leads that
>>>>>>>>>>>>>>> MRToolkitImage
>>>>>>>>>>>>>>> will not
>>>>>>>>>>>>>>> know about number of resolution variants in case when they
>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>> generated
>>>>>>>>>>>>>>> on the fly and there will be no way to load all of them by
>>>>>>>>>>>>>>> MediaTracker.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Just an idea to thinking about, can we save this filter to
>>>>>>>>>>>>>> the MRI
>>>>>>>>>>>>>> itself and apply it after some resolution variant will be
>>>>>>>>>>>>>> created on
>>>>>>>>>>>>>> the fly?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Do you mean that it helps to leave the code in the AquaUtils
>>>>>>>>>>>>> class
>>>>>>>>>>>>> unchanged:
>>>>>>>>>>>>> ---------------
>>>>>>>>>>>>> 124 static Image generateLightenedImage(final Image
>>>>>>>>>>>>> image,
>>>>>>>>>>>>> final
>>>>>>>>>>>>> int percent) {
>>>>>>>>>>>>> 125 final GrayFilter filter = new GrayFilter(true,
>>>>>>>>>>>>> percent);
>>>>>>>>>>>>> 126 final ImageProducer prod = new
>>>>>>>>>>>>> FilteredImageSource(image.getSource(), filter);
>>>>>>>>>>>>> 127 return
>>>>>>>>>>>>> Toolkit.getDefaultToolkit().createImage(prod);
>>>>>>>>>>>>> 128 }
>>>>>>>>>>>>> ---------------
>>>>>>>>>>>>>
>>>>>>>>>>>>> or it is just an a better way for a filtered
>>>>>>>>>>>>> multi-resolution
>>>>>>>>>>>>> image
>>>>>>>>>>>>> generation?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> As I see, the way to solve it is only using
>>>>>>>>>>>>>>> MRI.getResolutionVariants()
>>>>>>>>>>>>>>> method for the MultiResolutionImageProducer creation. So
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> result of
>>>>>>>>>>>>>>> the call
>>>>>>>>>>>>>>> toolkit.createImage(new
>>>>>>>>>>>>>>> FilteredImageSource(mrImage.getSource(),
>>>>>>>>>>>>>>> filter));
>>>>>>>>>>>>>>> will be a MRToolkitImage which is based on fixed
>>>>>>>>>>>>>>> number of
>>>>>>>>>>>>>>> filtered
>>>>>>>>>>>>>>> resolution variants from the original MRI.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ...jim
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 3/11/16 5:42 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>>>>>>> On 09/03/16 16:58, Sergey Bylokhov wrote:
>>>>>>>>>>>>>>>>>> Probably we should enhance
>>>>>>>>>>>>>>>>>> ImageProducer/Tk.createImage/ImageFilter to
>>>>>>>>>>>>>>>>>> support this functionality? It seems that the number of
>>>>>>>>>>>>>>>>>> usage of
>>>>>>>>>>>>>>>>>> this
>>>>>>>>>>>>>>>>>> check "image instanceof MultiResolutionImage" will grow
>>>>>>>>>>>>>>>>>> over time.
>>>>>>>>>>>>>>>>> ImageProducer produces pixels for an image and is not
>>>>>>>>>>>>>>>>> able to
>>>>>>>>>>>>>>>>> take
>>>>>>>>>>>>>>>>> an information about the image resolution variants.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> May be we can add Toolkit.createImage(Image image,
>>>>>>>>>>>>>>>>> ImageFilter
>>>>>>>>>>>>>>>>> imageFilter) method which takes MultiResolutionImage into
>>>>>>>>>>>>>>>>> account to
>>>>>>>>>>>>>>>>> cover the common case where filtered image is created.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>> I think that at least our own API should support
>>>>>>>>>>>>>>>>>> MultiResolutionImage
>>>>>>>>>>>>>>>>>> w/o such checks, otherwise the user will need to do the
>>>>>>>>>>>>>>>>>> same.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> cc 2d-dev
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 09.03.16 15:30, Alexander Scherbatiy wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Could you review the fix:
>>>>>>>>>>>>>>>>>>> bug:
>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8151303
>>>>>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~alexsch/8151303/webrev.00
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> The AquaUtils does not take into account
>>>>>>>>>>>>>>>>>>> MultiResolutionImage
>>>>>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>>>>>> selected/disabled/lightened images generation.
>>>>>>>>>>>>>>>>>>> The fix also leaves the MultiResolutionCachedImage
>>>>>>>>>>>>>>>>>>> check because
>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>> base system icon size can be differ from the requested
>>>>>>>>>>>>>>>>>>> painted
>>>>>>>>>>>>>>>>>>> size.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
More information about the 2d-dev
mailing list