[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] <AWT Dev> Review request for 8029339 Custom MultiResolution image support on Hi
From:       Jim Graham <james.graham () oracle ! com>
Date:       2015-08-28 18:16:42
Message-ID: 55E0A58A.6010107 () oracle ! com
[Download RAW message or body]

That looks good.  I'm not sure what the standard practice is these days 
for using @code vs @link in comments.  I think we used to have some sort 
of "first reference is @link, subsequent are @code" convention, but that 
may have changed, especially if you @see them as well and the 2 
paragraphs aren't that long so the list of @see references is nearby.

You don't @link or @see the Image class, you just @code reference it.

Don't forget to @see or mention the Toolkit.createImage(url/name) 
methods too which may also produce MRIs.  You might want to mention 
getImage and createImage in line without any arguments rather than the 2 
variants of getImage, then use the @see to point out the variants.

Is it worth @see'ing some or all of the drawImage variants?  Or at least 
one of them?  Is it too verbose to list them all?

			...jim

On 8/28/15 6:43 AM, Alexander Scherbatiy wrote:
>
>   Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/8029339/webrev.13
>
>     - The javadoc for MultiResolutionImage interface is updated.
>
> On 8/26/2015 1:45 AM, Phil Race wrote:
>> On 08/25/2015 02:26 PM, Jim Graham wrote:
>>> Is MRI intended to be implemented only by classes that extend
>>> java.awt.Image?  I think that's the only place we look for it.  If
>>> so, then we should state that. Perhaps:
>>>
>>> ---------
>>> This interface is designed to be an optional additional API supported
>>> by some implementations of {java.awt.Image} to allow them to provide
>>> alternate images for various rendering resolutions.  The various
>>> {Graphics.drawImage} variant methods will consult the methods of this
>>> interface if it is implemented on the argument {Image} object in
>>> order to choose the best representation to use for each rendering
>>> operation.
>>> <p>
>>> The {MRI} interface should be implemented by any subclass of
>>> {j.a.Image} that ...(existing text)... given image width and height.
>>> For convenience, toolkit images obtained from {...} will implement
>>> this interface on platforms that support naming conventions for
>>> resolution variants of stored image media and the {Abstract} and
>>> {Base} classes are provided to facilitate easy construction of custom
>>> multi-resolution images from a list of related images.
>>> ---------
>>>
>>> Should we list the naming conventions that we support (mainly just
>>> "@2x"?), or should that be listed in the documentation of the various
>>> getImage() and createImage() methods?
>>
>> Here I think so as to keep it all together.
>       I have filed and issue on it: JDK-8134670 Add documentation for
> @2x images loading on Mac OS X
> https://bugs.openjdk.java.net/browse/JDK-8134670
>>
>> The code itself seems fine so we are just discussing docs although I
>> also have a comment on the tests.
>>
>> Jim observed elswhere that the tests use internaI APi.
>> I wish the tests were not doing so at all but I notice sun.awt.OSInfo
>> used
>> in one file and then another file uses jdk.testlibrary.OSInfo which seems
>> to be a jtreg copy to avoid that internal dependency.
>> Really I don't like either of these. You can work out if its OSX with one
>> line of code rather than introducing a dependency.
>
>     I have updated the tests to use only jdk.testlibrary.OSInfo.
>
>    Thanks,
>    Alexandr.
>
>>
>> -phil.
>>
>>>
>>>
>>> On 8/19/15 4:51 AM, Alexander Scherbatiy wrote:
>>>>
>>>>    Hello,
>>>>
>>>>   Could you review the updated fix:
>>>> http://cr.openjdk.java.net/~alexsch/8029339/webrev.12
>>>>
>>>>    - Comment about nonempty return list is added for
>>>> MultiResolutionImage.getResolutionVariants() method
>>>
>>> That comment has a blank line.  If it is supposed to be 2 paragraphs
>>> then I think you need a <p> tag, otherwise the blank line should
>>> probably be removed (I don't need to see a webrev for that).
>>>
>>>
>>>
>>> The rest looks fine...
>>>
>>>             ...jim
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]