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 ,@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