[OpenJDK 2D-Dev] <AWT Dev> [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays

[Jim Graham]

Hi Alexander,

I just noticed that the new interface was created in com.sun.  I also note that you discuss a number of issues below that relate to a developer creating one of their own Multi-Resolution images.  We should not be exporting an interface at this point for a developer to do any of that.  Any use of these interfaces beyond our own internal use to support @2x images will be unsupported in JDK8 as we do not have time to properly test and expose an interface with the remaining time in the JDK8 release schedule.  This interface should be moved to the internal sun.* hierarchy until we have time to vet it.

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.

On 11/21/2013 6:15 AM, Alexander Scherbatiy wrote:
>>
>> To check if it is "identity-ish", I'd use:
>>     ((type & ~(TYPE_TRANSLATION | TYPE_FLIP)) == 0)
>> To check for scale, use:
>>     ((type & ~(TYPE_TRANSLATION | TYPE_FLIP | TYPE_SCALE_MASK)) == 0)
>> and then the distance/hypot equations are in the final else as they are now.
>      Updated.

The test at line 3152 is sub-optimal.  If the transform includes both a SCALE and a TRANSLATE then this optimization will fail to trigger and the code will fall through to the default case (which produces the correct output, but misses the optimization).  Note above that I include the FLIP and TRANSLATE flags in with the SCALE mask to take care of this in my pseudo-code above.

>>>       - Some related to the ToolkitImage staff is moved from SunToolkit to ToolkitImageUtil
>>
>> I'm not sure why this was done - it added a bunch of busy-work for approving the webrev that I don't have time for.  Hopefully someone else can verify the cut/paste and refactoring.
>      I have reverted it back.

Thanks, it is much easier to verify what was modified now.

>> I'm still not sure why it isn't just left as DEFAULT on all platforms.  The other platforms don't even create resolution images anyway so their default is to just use the existing image which is fine.  We should not have platform issues affecting the default values for hints.
>
>      A user can create custom MultiResolution image.  If our target is to show the resolution variant according to the current transform on all platforms then it is ok.

If and when we expose the ability to create these images then the developer would want them to be used on all platforms if they are going to the trouble of manually creating them.  Until then the @2x images are only created on MacOS anyway so the value of the hint is irrelevant on other platforms.  Arguably, we might want to start loading the @2x images on other platforms too, but I think we can go into the 8.0 release with just MacOS support and worry about using it on other platforms in an 8 update release.  In either case, the default hint value should be DEFAULT.

>     However, a user can create it's own MultiResolutionImage implementation. So I added the static containsResolutionVariant() method and updated
>     necessary observers in our code. Usually the code "image!= img" is changed to "MultiResolutionImage.containsResolutionVariant(image, img)"

We should not be catering to custom uses at this point until a later release.

If any of this code requires changes to ImageObservers then that is a bug as I mentioned above.

>     and x, y, width, and height values are recalculated.  The same should be done in a user's code.

We should not require changes to developer's ImageObserver code to support @2x.

>> I was going to ask about that new method.  Why does the developer need to manually intercede here?  They should be able to grab an image that has an @2x variant, use MediaTracker or some other mechanism to make sure it is loaded, then create the cursor and the code that creates the cursor should notice the resolution variations and deal with it on its own...
>       Mac OS X gets a necessary image based on the current transform. A user can create its own MultiResolutionImage that contains images for the scales, for example, 0.5, 0.7, 1, 3.2.
>       We need to know a list of all images to create the NSImage.

Any code that triggers an image to load and then looks for that same reference in its imageUpdate() method (which is the case in the MediaTracker code) should continue to work.  That was the point I was trying to make and I've reiterated this above.  MediaTracker (which should support @2x images without any code changes) simply makes a good test case that we've handled the compatibility issues here...

			...jim