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

Jim Graham james.graham at oracle.com
Tue Nov 12 11:43:59 PST 2013

Hi Alexander,

Some minor issues with this fix:

- All RenderingHints have a default setting where the system can choose 
for you.  The primary thing that the default settings allow is for the 
answer to be based off of another hint.  Often the QUALITY hint provides 
the swing vote if an individual hint is left "DEFAULT".  That should 
probably also be the setting used for SG2D, and would hinge off of the 
devScale, for example, as the deciding factor.

- Descriptions for "on" and "off" should be something like "Use 
resolution variants of images" and "Use only default resolution variants 
of images" (and "Default resolution variant usage").  Most of the other 
descriptions on hints are statements of what is going to happen or what 
is going to be used rather than a simple 'the X state is Y'.

- It looks like the transform is used in SG2D to decide if the hiDPI 
image should be used.  I'm not familiar with the Mac's native use of 
@2x, but I thought that they hinged the decision off of the "retina 
scale" of the display, not off of the current transform.  That way, if 
you scale down an icon it doesn't change its look when it reaches .5x 
scale (or .75x depending on the cutoff).  Personally, I think it is 
better to not use the transform to keep animations smooth, but I'm not 
sure what Apple did.

- The logic in using the transform is also a bit murky.  I think if you 
set the scale on a retina display to exactly 1/2 it would use the HiDPI 
version even though the scale was 1:1.  Since I support not examining 
the transform there, I'm going to present this as another reason why we 
should just base it on devScale, but the logic could be fixed if we 
really plan to use the transform here.

- The new logic in "isHiDPIImage()" is confusing because you line up 
logic operations from 2 different levels of parentheses.  I believe that 
one version of our style guidelines included a rule that allowed 
"indenting to parentheses level" and I would think that would be a good 
rule to apply here.  Or do something else to make the logic flow there 
less tricky to read.

- Eventually we are going to need this support in more pipelines.  I 
believe that Win8 already has parameters that affect choices of images, 
but they are only currently deployed on the Surface tablets (i.e. there 
are no supported high DPI displays for desktop that I'm aware of, but 
some of the Surface tablets ship with high DPI screens).  What would the 
impact be if we moved this into a more general class in src/share?  I 
suppose we might spend extra time looking for variants of images that we 
don't need?

- Has any thought been given to the issues that someone raised with cursors?

- Has any thought been given to my comments about MediaTracker and image 
observer states for multi-res images?  I don't see any attempt here to 
preload the @2x image.  The problem would probably only be seen on 
multi-headed systems where one display is retina and one is not - you 
would find the images suddenly reloading when you move the window to the 
other screen and the application might not expect that to happen.  Which 
image is the Observer registered on?  Since the image is handed to the 
Observer, will an application be surprised when their observer gets a 
handle to an image they've never seen?  Is it an issue if one of the 
"alternate resolution variant" images leaks into an application's 
"hands" via the observer callbacks?


On 11/11/13 7:59 AM, Alexander Scherbatiy wrote:
>   Could you review the updated fix:
>      http://cr.openjdk.java.net/~alexsch/8011059/webrev.06/
>    Only internal API is exposed:
>    - MultiResolutionImage interface with method
> "getResolutionVariant(int width, int height)" is added to the
> com.sun.awt package
>    - Hints to switch on/off the resolution variant usage are added to
> SunHints class
>    - Test is updated to use the  MultiResolutionImage interface
>   Thanks,
>   Alexandr.
> On 11/5/2013 3:16 PM, Alexander Scherbatiy wrote:
>>    Thank you for the review.
>>   Could you look at the updated fix:
>>      http://cr.openjdk.java.net/~alexsch/8011059/webrev.05/
>>   - URL is parsed to protocol, host, port, and path parts in the
>> LWCToolkit class.
>>      I checked that URL(protocol, host, port, file) constructor
>> correctly handles -1 port value.
>>   - Only last file name after last '/' in the URL path  is converted
>> to @2x name
>>   - Tests that check correct URL and path translation to @2x names are
>> added to the ScalableImageTest
>>   Thanks,
>>   Alexandr.
>> On 11/1/2013 12:46 AM, Peter Levart wrote:
>>> On 10/29/2013 05:45 PM, Alexander Scherbatiy wrote:
>>>>> 2. I'm not sure that the proposed getScaledImageName()
>>>>> implementation in ScalableToolkitImage works perfectly for URLs
>>>>> like this:
>>>>> http://www.exampmle.com/dir/image
>>>>> In this case it will try to find 2x image here:
>>>>> http://www.example@2x.com/dir/image
>>>>> which doesn't look correct.
>>>>        Fixed. Only path part of a URL is converted to path2x.
>>> Hi Alexander,
>>> URLs like this:
>>> http://www.example.com/dir.ext/image
>>> will still be translated to:
>>> http://www.example.com/dir@2x.ext/image
>>> I think you need to search for last '.' after the last  '/' in the
>>> getScaledImageName();
>>> Also the following code has some additional bugs:
>>>   853         static Image toScalableImage(Image image, URL url) {
>>>   854
>>>   855             if (url != null && !url.toString().contains("@2x")
>>>   856                     && !(image instanceof ScalableToolkitImage)) {
>>>   857                 String protocol = url.getProtocol();
>>>   858                 String host = url.getHost();
>>>   859                 String file = url.getPath();
>>>   860                 String file2x =*host +*getScaledImageName(file);
>>>   861                 try {
>>>   862                     URL url2x = new URL(protocol, host, file2x);
>>>   863                     url2x.openStream();
>>>   864                     return new ScalableToolkitImage(image,
>>> getDefaultToolkit().getImage(url2x));
>>>   865                 } catch (Exception ignore) {
>>>   866                 }
>>>   867             }
>>>   868             return image;
>>>   869         }
>>> Why are you prepending *host* to getScaledImageName(file) in line
>>> 860? Let's take the following URL for example:
>>> http://www.example.com/dir/image.jpg
>>> protocol = "http"
>>> host = "www.example.com"
>>> file = "/dir/image.jpg"
>>> file2x = "*www.example.com*/dir/image at 2x.jpg"
>>> url2x = URL("http://www.example.com*www.example.com*/dir/image@2x.jpg")
>>> You are missing a part in URL (de)construction - the optional port!
>>> For example in the following URL:
>>> http://www.example.com:8080/dir/image.jpg
>>> You should extract the port from original URL and use it in new URL
>>> construction if present (!= -1).
>>> I would also close the stream explicitly after probing for existence
>>> of resource rather than delegating to GC which might not be promptly
>>> and can cause resource exhaustion (think of MAX. # of open file
>>> descriptors):
>>>         try (InputStream probe = url.openStream()) {}
>>> Regards, Peter

More information about the awt-dev mailing list