<Swing Dev> Review Request for 8039966: fix doclint block tag issues in swing filechooser and colorchooser classes
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Fri May 16 13:07:11 UTC 2014
On 5/16/2014 1:45 AM, Steve Sides wrote:
>
> 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,
>
> "If |FileView| returns |null| for any method, |JFileChooser| then uses
> the L&F specific view to get the information. So, for example, if you
> provide a |FileView| class that returns an |Icon| for JPG files, and
> returns |null| icons for all other files, the UI's |FileView| will
> provide default icons for all other files."
>
> 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?
Yes. This is ok.
Thanks,
Alexandr.
>
> -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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the swing-dev
mailing list