<AWT Dev> [10] Review request for 8148619: Select the closest resolution variant in BaseMultiResolutionImage

[Phil Race]

This class which is concrete - not abstract - would have been better 
called "BasicMultiResolutionImage"

I do think I have issues with the documentation on this class and some 
aspects of
its behaviour that should be documented and implemented, but I don't 
think we should
try to make it something it was not intended to be.

I'm not going to  try to solve it all in this email but here are some of 
my thoughts.

- There is nothing inherently wrong with the idea that the application 
needs to provide a
sorted list of images in increasing size so long as it is documented as 
such.
If a smarter or different algorithm is required, the developer can write one
subclassing AbstractMultiResolutionImage

- I think these words need examination and clarification :
"For best effect the array of images should be sorted with each image being
both wider and taller than the previous image."

What does "for best effect mean" ? Perhaps it means it won't throw any
exceptions and will still paint, but the image selection algorithm is 
designed
with that assumption.

Let's instead say
- The algorithm used by this class is a simple linear search of the 
array to find
the first image that meets its criteria

- The first image that is both at least as tall and at least as wide as 
the target area
is selected.

- In the event that no image meets this, the final image will be selected

- It is the application's responsibility to ensure they are sorted 
accordingly.

-  The criteria are based on the assumption that in most cases, scaling
    down an image will produce better results than scaling up an image.

- If the provided images do not share the same aspect ratio,
   as might be the case if the application expects them to be used under
   non-uniform transforms, this selection algorithm may not be appropriate.

- There is no notion of selection by closeness.
   If the application does not want a very large image scaled down,
   by a large factor, it should provide additional images, or not use 
this class.


Since I don't think you can adapt this class to solve all these problems 
without
too many changes, it is better to stick to what it claims.

-phil.


On 09/08/2017 11:48 AM, Semyon Sadetsky wrote:
> On 09/08/2017 11:29 AM, Sergey Bylokhov wrote:
>
>> This is not a mistake, it was done intentionally because this is a 
>> small/basic/simple/fast implementation.
> This is important criteria but it is not enough for public API. In 
> internal classes the internal implementation logic could be shared 
> between the provider class and its client, but this obviously violates 
> the encapsulation concept.  But this is not suitable for a public API. 
> Look at the way how this API is designed: To have 
> BaseMultiResolutionImage woking correctly the client must provide 
> BaseMultiResolutionImage internal data structure prepared by a certain 
> rule in constructor argument. This API design looks inappropriate to 
> common standard without any significant reason.
>
> --Semyon
>>
>> On 9/8/17 11:06, Semyon Sadetsky wrote:
>>> On 09/07/2017 01:41 PM, Sergey Bylokhov wrote:
>>>> Hi, Semyon.
>>>>
>>>> The responsibility for sorting of an array was intentionally moved 
>>>> to the user, because the getResolutionVariant method () is called 
>>>> in each draw of the image. For this purpose in documentation for a 
>>>> class and in a documentation for constructors it was specified that 
>>>> the array shall be sorted. It is the reason why the bug of 
>>>> JDK-8147849 was closed.
>>> This seems to me a mistake in the API design which this update is 
>>> trying to fix. Since we opened this API for everybody's use the base 
>>> implementation of the MultiResolutionImage  should be covering its 
>>> most frequent and general usage. And in this general usage the 
>>> appropriate resolution among the provided image variants should be 
>>> chosen at the time the MR image is drawn. This is the logic we use 
>>> internally in JDK to draw icons and other UI stuff on the HiDPI 
>>> display and it is in demand for user applications in most cases.
>>> When a predefined order of the image variant selection is required 
>>> then getResolutionVariant() may be overridden to achieve that. But 
>>> obviously this is a rare case and it shouldn't be a basic 
>>> implementation.
>>>
>>> --Semyon
>>>>
>>>> RFE which you try to fix cover another use-case:
>>>> If the user will use the sorted array  [@1 , at 20] then we should 
>>>> select @1 if "-Dsun.java2d.uiScale=2" is used but not @20
>>>>
>>>> On 9/6/17 19:31, Semyon Sadetsky wrote:
>>>>> Hello,
>>>>>
>>>>> Please review fix for JDK10:
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8148619
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8148619/webrev.00/
>>>>>
>>>>> The algorithm selecting the best resolution variant of the 
>>>>> BaseMultiResolutionImage was updated to be insensitive to the 
>>>>> order of image variants in the initially provided array.
>>>>>
>>>>> The BaseMultiResolutionImage specification was updated 
>>>>> correspondingly.
>>>>>
>>>>> --Semyon
>>>>>
>>>>
>>>>
>>>
>>
>>
>