Re: Review Request for 8039966: fix doclint block tag issues in swing filechooser and colorchooser classes

[Alexander Scherbatiy]

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