[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-swing-dev
Subject: Re: <Swing Dev> [OpenJDK 2D-Dev] [9] Review request for 8151303 [macosx] [hidpi] JButton's low-res.
From: Jim Graham <james.graham () oracle ! com>
Date: 2016-08-15 18:19:23
Message-ID: 97ffae8e-24b6-563a-32b3-66f773a23726 () oracle ! com
[Download RAW message or body]
Looks good. +1
...jim
On 08/15/2016 08:47 AM, Alexander Scherbatiy wrote:
>
> 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.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic