<Swing Dev> Review Request for 8039966: fix doclint block tag issues in swing filechooser and colorchooser classes

Thu May 15 21:45:49 UTC 2014

On 5/15/2014 9:02 AM, Alexander Scherbatiy wrote:
>
>   Hi Steve,
>
> src/share/classes/javax/swing/filechooser/FileView.java
> -------------------------
>      /**
>       * A human readable description of the file. For example,
>       * a file named <i>jag.jpg</i> might have a description that read:
>       * "A JPEG image file of James Gosling's face".
>
> +     * @return a {@code String} containing a description of the file or
> +     *         {@code null} if it is not available.
> +     *
>       */
>      public String getDescription(File f) {
>          return null;
>      }
> -------------------------
>
> The current javadoc for the getDescription(File f)  method does not 
> specify the returned value if the description is not available.
> It means that there can be applications that extend FileView class and 
> returns an empty string instead of null in this case.
>
> If you have the reason that the return value must be null if the 
> description is not available
> you need to create the  CCC request for the API change.
I don't think it "must" be null, but the default implementation in the 
abstract class is that it does return null.  The class description 
comment state,

So, I did not think I was revealing any implementation by stating that.
Are these @return tags supposed to describe what the method does or what 
it should do? Maybe I"m looking at this the wrong way. :)

Could I have left it as:
@return a {@code String} containing a description of the file

That would be easiest, but shouldn't something say unless overriden, 
this will return null?

-steve

>
> Make also sure that your changes does not break the backward 
> compatibility.
>
> Thanks,
> Alexandr.
>
>
> On 4/30/2014 1:09 PM, sergey malenkov wrote:
>> Now it looks OK for CCC request:
>> http://ccc.us.oracle.com/docs/process
>>
>> Thanks,
>> SAM
>>
>> On 30.04.2014 2:56, Steve Sides wrote:
>>> Review Request for 8039966: fix doclint block tag issues in swing 
>>> filechooser and colorchooser classes
>>>
>>> A few more changes, mostly just as noted.
>>> http://cr.openjdk.java.net/~ssides/8039966/8039966.5
>>>
>>> -steve
>>>
>>> On 4/25/2014 8:34 AM, sergey malenkov wrote:
>>>> Hi Steve,
>>>>
>>>> FileSystemView:
>>>>
>>>>      /**
>>>>       * Returns a File object constructed from the iven path string.
>>>> +     *
>>>> +     * @param path {@code String} representation of path
>>>> +     * @return a {@code File} object created from the given String
>>>>       */
>>>>      public File createFileObject(String path) {
>>>>
>>>>
>>>> I think, that "from the given {@code path}" is better than "from 
>>>> the given String".
>>>>
>>>>      /**
>>>>       * Gets the list of shown (i.e. not hidden) files.
>>>> +     *
>>>> +     * @param dir the root directory of files to be returned
>>>> +     * @param useFileHiding determine if hidden files are returned
>>>> +     * @return an array of {@code File} objects. Include hidden 
>>>> files if
>>>> +     *         {@code useFileHiding} is false.
>>>>       */
>>>>
>>>> I suggest to rewrite last sentence:
>>>> @ return an array of {@code File} objects representing files and 
>>>> directories in the given {@code dir}.
>>>> It includes hidden files if {@code useFileHiding} is {@code false}.
>>>>
>>>> FileView:
>>>>
>>>> Note, that all @return tags differ from each other.
>>>> +     * @return a String containing a description of the file
>>>> +     * @return a {@code String} containing a description of the 
>>>> type of the file
>>>> +     * @return an {@code Icon} which represents the specified 
>>>> {@code File}
>>>> I recommend to use something like this:
>>>> @return a {@code String} containing a description of the given file,
>>>>                 or {@code null} if it is not available.
>>>>
>>>>      /**
>>>>       * Whether the directory is traversable or not. This might be
>>>>       * useful, for example, if you want a directory to represent
>>>>       * a compound document and don't want the user to descend into 
>>>> it.
>>>> +     *
>>>> +     * @param f a {@code File} object representing a directory
>>>> +     * @return true if the directory is traversable
>>>>       */
>>>>      public Boolean isTraversable(File f) {
>>>>
>>>> Note that the method may return null. We should describe it 
>>>> somehow. For example,
>>>> @return true if the directory is traversable,
>>>>                 false if it is not traversable,
>>>>                 null if the file system should be asked.
>>>> @see FileSystemView#isTraversable
>>>>
>>>> Thanks,
>>>> SAM
>>>>
>>>>
>>>>
>>>> On 24.04.2014 22:00, Steve Sides wrote:
>>>>> Hi Sergey, et. all,
>>>>> changes as per comments: 
>>>>> http://sqeweb.us.oracle.com/coretools/ssides/webrevs/8039966/8039966.5/ 
>>>>>
>>>>>
>>>>> Some comments in line...
>>>>>
>>>>> On 4/24/2014 5:16 AM, sergey malenkov wrote:
>>>>>> Hello,
>>>>>>
>>>>>>      /**
>>>>>>       * The icon that represents this file in the 
>>>>>> <code>JFileChooser</code>.
>>>>>> +     *
>>>>>> +     * @param f a {@code File} object
>>>>>> +     * @return an {@code Icon} to be displayed by the file chooser
>>>>>>       */
>>>>>>      public Icon getIcon(File f) {
>>>>>>
>>>>>> What if the FileView class is used from another component, not 
>>>>>> the file chooser.
>>>>>> I prefer something like this: an {@code Icon} that represent the 
>>>>>> specified {@code File}.
>>>>> I see your point, although, this is in the 'filechooser' package 
>>>>> and is documented as
>>>>> "an abstract class that can be implemented to provide the 
>>>>> filechooser with UI information for a |File|."
>>>>> However, I used a return comment more akin to what you suggested.
>>>>>
>>>>>>
>>>>>>
>>>>>>      /**
>>>>>>       * Returns all root partitions on this system. For example, on
>>>>>>       * Windows, this would be the "Desktop" folder, while on DOS 
>>>>>> this
>>>>>>       * would be the A: through Z: drives.
>>>>>> +     *
>>>>>> +     * @return an array of {@code File} objects
>>>>>>       */
>>>>>>      public File[] getRoots() {
>>>>>>
>>>>>> It is obvious that this method returns an array of File objects.
>>>>>> It is better to specify these objects. For example:
>>>>>>
>>>>>> + * @return an array of File objects which represent root 
>>>>>> partitions of this file system.
>>>>> got it.
>>>>>>
>>>>>>      /**
>>>>>>       * Returns a File object constructed in dir from the given 
>>>>>> filename.
>>>>>> +     * If {@code dir} is {@code null}, then the new {@code File} 
>>>>>> instance is
>>>>>> +     * created as if by invoking the single-argument {@code 
>>>>>> File} constructor
>>>>>> +     * on the given {@code filename} string.
>>>>>> +     *
>>>>>> +     * @param dir an abstract pathname denoting a directory
>>>>>> +     * @param filename a {@code String} representation of a 
>>>>>> pathname
>>>>>> +     * @return a {@code File} object created from {@code dir} 
>>>>>> and {@code filename}
>>>>>>       */
>>>>>>      public File createFileObject(File dir, String filename) {
>>>>>>
>>>>>> Seems this javadoc describes an implementation details.
>>>>>> We should specify here that the created File object represents a 
>>>>>> file with specified filename, that is located in specified folder.
>>>>> I was looking into what this does and meandered on to the File doc 
>>>>> and kind of borrowed the javadoc idea from there, since this
>>>>> does the same.    I put it back to the original.
>>>>>
>>>>> -steve
>>>>>
>>>>>>
>>>>>> etc...
>>>>>>
>>>>>>
>>>>>> Note that all such changes in the API require an approval from CCC.
>>>>>> This is not a small typo. You really change the *public* API!
>>>>>>
>>>>>> Thanks
>>>>>> SAM
>>>>>>
>>>>>> On 18.04.2014 2:57, Steve Sides wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you please review the fix for the following bug:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8039966
>>>>>>>
>>>>>>> Webrev corresponding:
>>>>>>> http://cr.openjdk.java.net/~ssides/8039966/8039966.4/
>>>>>>>
>>>>>>> This addresses missing @parm and @return block tags in javadoc 
>>>>>>> for javax/swing/colorchooser and filechooser classes as noted by 
>>>>>>> doclint.
>>>>>>>
>>>>>>> It does not address methods which are missing javadoc comments 
>>>>>>> completely.
>>>>>>>
>>>>>>> You may want to check the wording of @param for 
>>>>>>> (un)installChooserPanel(). The original seemed a little 
>>>>>>> incorrect to me.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> -steve
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20140515/6ac330d9/attachment.html>