Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8011059/webrev.10/
- FLIP and TRANSLATE bit masks are also included for the SCALE
transform checking
- LWCToolkit checks an image in cache before requesting
multi-resolution image creation to prevent excessive string manipulation.
It needs to make imgCache accessible from the LWCToolkit or move
image2x staff calculation to the SunToolkit to use the right
synchronization.
imgCache is not created per Application context so there can be
issues to make it protected. The image2x staff does not relate to
SunToolkit.
That is why all staff related to ToolkitImage creation used to move
into ToolkitImageUtil in some previous fix. I just skipped the
synchronization for this particular case.
- The containsResolutionVariant method is removed from the
MultiResolutionImage
- The image observer is nullified in drawImage(Image,..ImageObserver)
method for the resolution variant.
The original image is always loaded because it needs to know the
image width and height for the resolution variant size calculation.
Only the original image sends notifications that an image is
loaded. The resolution variant is drawn silently so it does not break
existed image observer implementation.
On 11/23/2013 5:11 AM, Jim Graham wrote:
Hi Alexander,
If we are going to be advertising it as something that developers use
in production code then we would need to file this via CCC. With the
current implementation, any user that has their own ImageObservers
must use this API and so it becomes not only public, at a time when
there is a lockdown on new public API in 8.0, but it also means that
we are tied to its implementation and we can't rely on "it was
experimental and so we can change it at any point" - you can't say
that about an API that is necessary to correctly use a touted feature.
This issue has its own history. It has been already fixed for the
JDK 7u for the Java2D demo. It was decided to not bring new API for the
HiDPI support in the JDK 7.
It was suggested to using code like below to create a ScalableImage.
------------------------------
/**
* Class that loads and returns images according to
* scale factor of graphic device
*
* <pre> {@code
* Image image = new ScalableImage() {
*
* public Image createScaledImage(float scaleFactor) {
* return (scaleFactor <= 1) ? loadImage("image.png")
* : loadImage("im...@2x.png");
* }
*
* Image loadImage(String name){
* // load image
* }
* };}</pre>
*/
------------------------------
It was based on the display scale factor. The scale factor should be
manually retrieved from the Graphics2D and was not a part of the public API:
g2d.drawImage(scalbleImage.getScaledImage(scaleFactor),..).
From that time it was clear that there should be 2 basic operations
for the HiDPI images support:
- retrieve an image with necessary resolution
- retrieve images with all resolutions
The second one is useful when it is necessary to create one set of
images using the given one (for example, to draw a shape on all images).
The JDK 7 solution has been revisited for the JDK 8 and the neccesary
CCC request 8011059 has been created and approved.
Both the JDK-8011059 issue description and the approved CCC request says:
-----------------------
A user should have a unified way to provide high resolution images
that can be drawn on HIDPI displays according to the user provided
algorithm.
-----------------------
The 8011059 fix consists of two parts:
- Provide a way for a custom image creation that holds images with
different resolutions
- Load @2x images on Mac OS X
The first one of the fix can be used without the second. it is not
difficult to crate just a class which loads @2x images using the given
file path/url.
Now it is suggested to implement the given MultiResolutionImage
interface and override two methods:
- Image getResolutionVariant(int width, int height)
- List<Image> getResolutionVariants()
An image with necessary resolution should be drawn automatically in
SunGraphics2D.
What we need is to focus on the first part of the fix.
From this point of view it up to the user' code to load all or some
resolution variants by MediaTracker and using
Component.prepareImage/checkImage methods.
Thanks,
Alexandr.
I do not see any compatibility risks with the current fix. If a
user does not provide @2x images or explicitly overrides the
MultiResolutionImage nothing should be changed in his code.
If a developer uses a stock Java module in their app and they hand it
some @2x-enabled images then that code could fail.
If a developer creates an app and then later a graphic artist replaces
its media with a new set of media that includes @2x images then the
app could fail.
If an applet is deployed that uses stock imagery from its own web site
and someone on the web team for that same company/organization decides
to start introducing @2x media for a better web experience, then that
code could fail.
And, the changes introduced represent not "new functionality" but
existing behaviors that we've specified that are being violated - and
violated for reasons external to that code (i.e. whether or not a
particular file is found in the file system or on a web site).
Also, even if it could be attributed to something that was also fully
complicit amongst all parties involved, this feature can be deployed
without requiring code changes and we should do that. The current
issues with the image being returned in imageUpdate are avoidable,
inconvenient, and in violation of the existing drawImage/ImageObserver
contract.
The time testing of this API is the same as testing TollkitImages
that can hold @2x images.
We also will have the feedback about these API earlier. It is
better than introducing a public API in next release that will be
difficult to change.
Release cycles have vetting of new public API built in to their
scheduling in a responsible manner. An 11th hour sudden introduction
of a new API, especially one that is necessary for some developers to
use a new feature, is a lot more irresponsible.
All of the places that call containsResolutionVariant() are pointing
out a bug with this implementation. The resolution variants are
internal implementation details and should never be leaked through
any current interfaces. It looks like most of the cases involve
imageUpdate() methods that should be receiving a reference to the
original image, not the resolution variant. These pieces of could
should not be changing and the fact that you had to change them
points out that you've created a huge compatibility issue that
blocks this solution.
If someone uses the API with multi-resolution images he needs
also update his code according to this image usage.
First, their "use" of the new feature is not a code change, it is the
appearance of a new file in their deployment, or on a web site that
they tried to access. There isn't as much explicit intent there as
you are making it out to be.
It should be up to a user to preload all resolutions or only
part of them using the MediaTracker.
(And, btw, I didn't see MediaTracker on the list of places that were
changed to be aware of the new images. Also, the
prepareImage/checkImage methods that MT uses to trigger loading
weren't necessarily up to the task of preloading the necessary image
variants.)
The only place where an image is replaced to a resolution
variant and the image observer is invoked is the
drawImage(Image,..,ImageObserver).
Component.prepareImage() and Component.checkImage() are also intended
to trigger and track the loading of the required representation of an
image for the indicated Component.
There are 3 solutions how it can be handled:
- Preload an image with all resolution variants. The image
observer is not invoked in this case.
- Using the original image in the image observer for the
resolution variant (x,y,w,h should be rescaled).
- Using the resolution variant in the observer
The first one is not good solution for the ToolkitImage because
it is designed to load asynchronously.
Agreed.
The second one still can be surprising for a user because he has
notification that the original image has been already preloaded. Note
that a user can do something with this image so his actions will be
based on the image with another resolution.
It would be far less surprising than the third option which starts to
mention an image that he never interacted with.
It would also be similar to the behavior in 1.0 where we could
asynchronously reload the image in order to perform simple scaling on
it. In other words, this may be outdated behavior, but it is not new
behavior.
The third one provides the actual information to the user.
However, it requires that the user update his code in the image
observer. There are no compatibility problems. If multi-resolution
images are not used nothing should be changed. If they used, image
observers should be updated accordingly.
The fact that it requires the user to update his code to avoid bugs is
a non-starter. It means that they have work to do to use @2x images,
they can't just install the new files into their deployment. It means
that they will experience all sorts of "left hand didn't coordinate
with right hand" issues in their deployment and testing as often media
is managed by a different team than the code. It also means we are
exposing an internal implementation detail rather than making it "just
work". It also means that we are explicitly violating a long-standing
API contract in a way that directly impacts them.
The added information is not required for them to use the image and is
of questionable value given that the interactions they make with the
image are based on the design space of the unscaled variant -
information relative to a scaled variant then requires a bit of work
for them to use. We may want to provide this information at some
point, but for now we should not be introducing new behavior to a
really old API contract...
...jim
