[OpenJDK 2D-Dev] Review request for 8029339 Custom MultiResolution image support on HiDPI displays
Jim Graham
james.graham at oracle.com
Thu Mar 26 21:48:46 UTC 2015
Hi Alexander,
MRI.java:
Who is the intended audience for the recommendation in the interface to
cache image variants? Are we asking the developers who call the methods
on the interface to cache the answers? That would be unwise because the
list of variants may change over time for some MRIs. Are we speaking to
the implementer? If so, then I think that it is unnecessary to remind
implementers of basic implementation strategies like "cache complicated
objects".
How about this wording for the getRV() method? - "Gets a specific image
that is the best variant to represent this logical image at the
indicated size and screen resolution."
Perhaps we should clarify in the doc comments for the dimension
arguments in getRV() that the dimensions are measured in pixels?
line 56 - delete blank line between doc comment and method declaration
Also, it is probably overkill to document that the interface
getVariants() method returns an unmodifiable list. Responsible
implementations would probably use an unmodifiable list, but I don't
think it should be required by the interface. We do need to specify
that it isn't required to be modifiable so that a caller doesn't expect
to be able to modify it, but that is a looser spec. How about "Gets a
readable list of all resolution variants. Note that many
implementations might return an unmodifiable list."?
AbstractMIR.java:
"provides a default implementation for the MRI" or "provides default
implementations of several methods in the <MRI> interface and the
<Image> class"? Actually, I'll note that it provides none of the
implementations of the MRI methods so maybe it should be "provides
default implementations of several <Image> methods for classes that want
to implement the <MRI> interface"?
In the doc example - wrap the list to make it unmodifiable
The doc example could be made 2 or 3 lines shorter by simply assuming
the base image is in index 0 (it's just an example so I'm not sure the
flexibility needs to be a part of the example).
getGraphics is allowed by the Image interface to return null. Actually,
the exact wording is that it can only be called for "offscreen images"
and a MRI is technically not "an offscreen image". Perhaps we should
return null here since modifying the base image is unlikely to modify
the variants and arguably it would be required by the wording in the
method docs (even if the base image was an offscreen, the MRI produced
from it stops being an offscreen)?
Are all of the empty "@Inherit" comments needed? I thought inheritance
was the default?
BaseMRI.java:
"This class is [an] array-based implementation of the {AMRI} class"
(missing "an" and "the")
We should probably include the comment about the sorting of the images
in the class comments as well as document that the algorithm is a simple
scan from the beginning for the first image large enough (and default ==
last image). The algorithm also might not work very well if the images
are not monotonically both wider and taller. How about adding this to
the class comments:
"The implementation will return the first image variant in the array
that is large enough to satisfy the rendering request. For best effect
the array of images should be sorted with each image being both wider
and taller than the previous image. The base image need not be the
first image in the array."
getVariants() - you need to return an unmodifiable list. asList()
returns a list that "writes back" to the array. You need to use
Collections.unmodifiableList(Arrays.asList(array)).
RenderingHints.java:
Where do we process this hint? Don't we need to pass it to the
getVariant() method?
I don't understand the hint values other than the one that always uses
the base image. Default I guess gives us the ability to use the Mac
algorithm of "next larger size" on Mac and "based on Screen DPI" on
Windows, but the 3rd value "ON" is so vague as to not have any meaning.
Perhaps we should just delete it, but then do we just always do the
Mac method? Or do we vaguely have our internal images have a
platform-specific method and the Abstract/Base classes just do what they
want with no control from the user? In FX we are also still struggling
with this issue and we may likely just do what the Mac does in all
cases, but perhaps AWT needs to "behave like the platform" more? If we
are going to have actual values, then we need to have them do something,
which means:
- SG2D needs to track the hint just like we do the other hints that
affect our processing
- MRI.getVariant() needs to have the hint as an argument
- BaseMRI should probably do something with that hint
- hint values should include:
- ..._DEFAULT - implementation gets to decide
- ..._BASE - always use the base image
- ..._SIZE_FIT - Mac algorithm of smallest image that is big enough
- ..._DPI_FIT - choose based on DPI of the screen, ignoring transform
line 978 - missing blank line between fields
SG2D.java:
- The interface says that you will be passing in the "logical DPI" of
the display, but here you are actually passing in the screen's scale factor.
BaseMRITest.java:
- testBaseMRI also passes in a scale rather than a DPI to the MRI method.
- How does the modification test pass when the implementation doesn't
use unmodifiable lists?
MRITest.java:
- also uses scale rather than DPI in several places
...jim
On 3/13/15 6:34 AM, Alexander Scherbatiy wrote:
>
> Hello,
>
> Could you review the proposed API based on MultiresolutionImage
> interface:
> http://cr.openjdk.java.net/~alexsch/8029339/webrev.06/
>
> - return unmodifiable list comment is added to the
> getResolutionVariants() method javadoc in MultiresolutionImage interface
> - base image size arguments are removed form the
> getResolutionVariant(...) method in MultiresolutionImage interface
> - BaseMultiResolutionImage class that allows to create a
> multi-resolution image based on resolution variant array is added
> - the test for the BaseMultiResolutionImage is added
>
> Thanks,
> Alexandr.
>
> On 2/14/2015 3:23 AM, Jim Graham wrote:
>> The second solution looks good. I'd make it standard practice
>> (perhaps even mentioned in the documentation) to return unmodifiable
>> lists from the getVariants() method. The Collections class provides
>> easy methods to create these lists, and it sends a clear message to
>> the caller that the list was provided for them to read, but not write
>> to. Otherwise they may add a new image to the list you provided them
>> and wonder why it wasn't showing up. Also, an unmodifiable list can
>> be cached and reused for subsequent calls without having to create a
>> new list every time.
>>
>> In getResolutionVariant() was there a reason why the base dimensions
>> were provided as float? The destination dimensions make sense as
>> float since they could be the result of a scale, but the source
>> dimensions are typically getWidth/getHeight() on the base image. A
>> related question would be if they are needed at all, since the
>> implementation should probably already be aware of what the base image
>> is and what its dimensions are. I'm guessing they are provided
>> because the implementation in SG2D already knows them and it makes it
>> easier to forward the implementation on to a shared (static?) method?
>>
>> With respect to default implementations, I take it that the BaseMRI is
>> along the pattern that we see in Swing for Base classes. Would it be
>> helpful to provide an implementation (in addition or instead) that
>> allows a developer to take a bunch of images and quickly make an MRI
>> without having to override anything? The implementations of
>> getBaseImage() and getResolutionVariants() are pretty straightforward
>> and a fairly reasonable default algorithm can be provided for
>> getRV(dimensions). This question is more of an idle question for my
>> own curiosity than a stumbling block...
>>
>> ...jim
>>
>> On 1/22/2015 6:49 AM, Alexander Scherbatiy wrote:
>>>
>>> Hi Phil,
>>>
>>> I have prepared two versions of the proposed API:
>>>
>>> I) Resolution variants are added directly to the Image:
>>> http://cr.openjdk.java.net/~alexsch/8029339/list/webrev.00
>>>
>>> II) MultiResolutionImage interface is used:
>>> http://cr.openjdk.java.net/~alexsch/8029339/webrev.05
>>>
>>> It could help to decide which way should be chosen for the the
>>> multi-resolution image support.
>>>
>>> Below are some comments:
>>>
>>> 1. High level goal:
>>> Introduce an API that allows to create and handle an image with
>>> resolution variants.
>>>
>>> 2. What is not subject of the provided API
>>> - Scale naming convention for high-resolution images
>>> - Providing pixel scale factor for the screen/window
>>>
>>> 3. Use cases
>>> 3.1 Loading and drawing high-resolution icons in IntelliJ IDEA
>>> A high-resolution image is loaded from resources and stored in
>>> JBHiDPIScaledImage class which is a subclass of the buffered image.
>>> The high-resolution image is used to create a disabled icon in the
>>> IconLoader.getDisabledIcon(icon) method.
>>> https://github.com/JetBrains/intellij-community/blob/master/platform/util/src/com/intellij/openapi/util/IconLoader.java
>>>
>>>
>>>
>>> 3.2 Loading and drawing high-resolution icons in NetBeans
>>> NetBeans does not have support for the high-resolution icons
>>> loading.
>>> It loads an icon from the file system using
>>> Toolkit.getDefaultToolkit().getImage(url) method or from resources
>>> by ImageReader and store it in ToolTipImage class which is
>>> subclass of the buffered image.
>>> ImageUtilities.createDisabledIcon(icon) method creates a disabled
>>> icon by applying RGBImageFilter to the icon.
>>> http://hg.netbeans.org/main/file/97dcf49eb4a7/openide.util/src/org/openide/util/ImageUtilities.java
>>>
>>>
>>>
>>> 3.3 Loading system icons in JDK 1.8
>>> JDK requests icons from the native system for system L&Fs and
>>> applies filters for them.
>>> See for example AquaUtils.generateLightenedImage() method:
>>> http://hg.openjdk.java.net/jdk9/client/jdk/file/e6f48c4fad38/src/java.desktop/macosx/classes/com/apple/laf/AquaUtils.java
>>>
>>>
>>>
>>> 4. HiDPI support for Images on different OSes
>>>
>>> 4.1 Mac OS X
>>> Cocoa API contains NSImage that allows to work with image
>>> representations: add/remove/get all representations.
>>> It picks up an image with necessary resolution based on the
>>> screen backing store pixel scale factor and applied transforms.
>>> https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSImage_Class/Reference/Reference.html
>>>
>>>
>>>
>>> 4.2 Linux
>>> GTK+ 3 API has gtkcssimagescaled lib (it seems that it is not
>>> public/stable)
>>> that parses the -gtk-scaled css property and draws a GtkCssImage
>>> according to the given scale factor.
>>>
>>> I have not found information about the HiDPI support in Xlib.
>>>
>>> 4.3 Windows
>>> I have only found the tutorial that suggests to select and draw a
>>> bitmap using the queried DPI
>>> and scale the coordinates for drawing a rectangular frame
>>> http://msdn.microsoft.com/en-us/library/dd464659%28v=vs.85%29.aspx
>>>
>>> Windows also provides the horizontal and vertical DPI of the
>>> desktop
>>> http://msdn.microsoft.com/en-us/library/windows/apps/dd371316
>>>
>>> 5. Pseudo API
>>> Below are some ways which illustrates how multi-resolution images
>>> can be created and used.
>>>
>>> 5.1 Resolution variants are stored directly in Image class.
>>> To query a resolution variant it needs to compare the resolution
>>> variant width/height
>>> with the requested high-resolution size.
>>> ------------
>>> public abstract class Image {
>>>
>>> public void addResolutionVariant(Image image) {...}
>>> public List<Image> getResolutionVariants() {...}
>>> }
>>> ------------
>>> // create a disabled image with resolution variants
>>>
>>> Image disabledImage = getDisabledImage(image);
>>>
>>> for (Image rv : image.getResolutionVariants()) {
>>> disabledImage.addResolutionVariant(getDisabledImage(rv));
>>> }
>>> ------------
>>> This approach requires that all resolution variants have been
>>> created even not of them are really used.
>>>
>>> 5.2 Resolution variants are stored in a separate object that
>>> allows to create them by demand.
>>> To query a resolution variant it needs to compare the resolution
>>> variant scale factor
>>> with the requested scale (that can include both screen DPI scale
>>> and applied transforms).
>>> ------------
>>> public abstract class Image {
>>>
>>> public static interface ResolutionVariant {
>>> Image getImage();
>>> float getScaleFactor();
>>> }
>>>
>>> public void addResolutionVariant(ResolutionVariant
>>> resolutionVariant) {...}
>>> public List<ResolutionVariant> getResolutionVariants() {...}
>>> }
>>> ------------
>>> // create a disabled image with resolution variants
>>> Image disabledImage = getDisabledImage(image);
>>>
>>> for (Image.ResolutionVariant rv : image.getResolutionVariants()) {
>>> disabledImage.addResolutionVariant(new
>>> Image.ResolutionVariant() {
>>>
>>> public Image getImage() {
>>> return getDisabledImage(rv.getImage());
>>> }
>>>
>>> public float getScaleFactor() {
>>> return rv.getScaleFactor();
>>> }
>>> });
>>> }
>>> ------------
>>>
>>> It does not have problem if a predefined set of images is provided
>>> (like image.png and image at 2x.png on the file system).
>>> This does not cover cases where a resolution variant can be created
>>> using the exact requested size (like loading icons from the native
>>> system).
>>> A resolution variant can be queried based on a scale factor and
>>> applied transforms.
>>>
>>> 5.3 The provided example allows to create a resolution variant
>>> using the requested high-resolution image size.
>>> ------------
>>> public interface MultiResolutionImage {
>>> Image getResolutionVariant(float width, float height);
>>> }
>>> ------------
>>> // create a multi-resolution image
>>> Image mrImage = new AbstractMultiResolutionImage() {
>>>
>>> public Image getResolutionVariant(float width, float
>>> height) {
>>> // create and return a resolution variant with exact
>>> requested width/height size
>>> }
>>>
>>> protected Image getBaseImage() {
>>> return baseImage;
>>> }
>>> };
>>> ------------
>>> // create a disabled image with resolution variants
>>> Image disabledImage = null;
>>> if (image instanceof MultiResolutionImage) {
>>> final MultiResolutionImage mrImage = (MultiResolutionImage)
>>> image;
>>> disabledImage = new AbstractMultiResolutionImage(){
>>>
>>> public Image getResolutionVariant(float width, float
>>> height) {
>>> return
>>> getDisabledImage(mrImage.getResolutionVariant(width, height));
>>> }
>>>
>>> protected Image getBaseImage() {
>>> return getDisabledImage(mrImage);
>>> }
>>> };
>>> } else {
>>> disabledImage = getDisabledImage(image);
>>> }
>>> ------------
>>>
>>> Thanks,
>>> Alexandr.
>
More information about the 2d-dev
mailing list