[OpenJDK 2D-Dev] <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 19:49:37 UTC 2013
That last bullet item (or 2) is actually a bigger issue because I think
we've been ignoring it in coming up with the implementation, but it
could have a simple fix...
...jim
On 11/12/13 11:43 AM, Jim Graham wrote:
> 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?
>
> ...jim
>
> 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 2d-dev
mailing list