Re: <Sound Dev> [10] Review Request: 8181566 JavaSound javadoc clarification

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: <Sound Dev> [10] Review Request: 8181566 JavaSound javadoc clarification

Sergey Bylokhov
The DRAFT version of CSR was created:
https://bugs.openjdk.java.net/browse/JDK-8185020

----- [hidden email] wrote:
>
> The fix is updated:
> http://cr.openjdk.java.net/~serb/8181566/webrev.01
>

> > I don't see the point of prettying up the docs on the un-used, commented out constants.

> > Can't we just delete them ? Seems like the decision was made years ago not to include them in the API

>
It could be removed, but I still consider them as kind of todo, I hope to check them one by one at some point.
Reply | Threaded
Open this post in threaded view
|

Re: <Sound Dev> [10] Review Request: 8181566 JavaSound javadoc clarification

Alex Menkov-2
MidiSystem.java:

@@ -1096,22 +1113,26 @@
                      return device;
                  }
              }
          }

- /* Provider class not specified or cannot be found, or
- provider class specified, and no appropriate device available or
- provider class and instance specified and instance cannot be found or
is not appropriate */
+ /*
+ * Provider class not specified or cannot be found, or provider class
+ * specified, and no appropriate device available or provider class and
+ * instance specified and instance cannot be found or is not appropriate
+ */

Old comment looks better (each "or" condition in a separate line)

- /* No default are specified, or if something is specified, everything
- failed. */
+ /*
+ * No default are specified, or if something is specified, everything
+ * failed.
+ */

"No default is specified" or "No defaults are specified"


AudioFileFormat.java:

-         * Finalizes the equals method.
+         * Indicates whether the specified object is equal to this file
type,
+         * returning {@code true} if the objects are the same.
+         *
+         * @param  obj the reference object with which to compare
+         * @return {@code true} if this file type is the same as the
{@code obj}
+         *         argument; {@code false} otherwise
           */
          @Override
          public final boolean equals(final Object obj) {

The implementation checks if the objects are equal, not that the objects
are the same

The same for AudioFormat.equals()

(Note that in most cases equals() methods of JavaSound classes just call
super.equals(), i.e. they really test is the objects are the same)

--alex


On 07/20/2017 18:46, Sergey Bylokhov wrote:

> The DRAFT version of CSR was created:
> https://bugs.openjdk.java.net/browse/JDK-8185020
>
> ----- [hidden email] wrote:
>>
>> The fix is updated:
>> http://cr.openjdk.java.net/~serb/8181566/webrev.01
>>
>> > I don't see the point of prettying up the docs on the un-used, commented out constants.
>> > Can't we just delete them ? Seems like the decision was made years ago not to include them in the API
>>
>
> It could be removed, but I still consider them as kind of todo, I hope
> to check them one by one at some point.
>
Reply | Threaded
Open this post in threaded view
|

Re: <Sound Dev> [10] Review Request: 8181566 JavaSound javadoc clarification

Sergey Bylokhov
In reply to this post by Sergey Bylokhov
Hi, Alex.
Thank you for review!
I would like to propose and discuss other text:

If the "equals()" method is overridden to not use "==":

    /**
     * Indicates whether the specified object is equal to this info object,
     * returning {@code true} if the objects are equal.
     *
     * @param  obj the reference object with which to compare
     * @return {@code true} the specified object is equal to this info
     *         object; {@code false} otherwise
     */
    public final boolean equals(Object obj) {


And two versions if the "==" from Object is used:

    /**
     * Indicates whether the specified object is equal to this info object,
     * returning {@code true} if the objects are identical.
     *
     * @param  obj the reference object with which to compare
     * @return {@code true} the specified object is equal to this info
     *         object; {@code false} otherwise
     */
    public final boolean equals(Object obj) {

or:

    /**
     * Indicates whether the specified object is equal to this info object,
     * returning {@code true} if the objects are the same.
     *
     * @param  obj the reference object with which to compare
     * @return {@code true} the specified object is equal to this info
     *         object; {@code false} otherwise
     */
    public final boolean equals(Object obj) {


In all versions the text in "@return " tag is short and the same. But description have different words about the objects: "identical"/"equal"/"the same".

ps: The text for the current fix was copied from the Integer.java which I usually use when in doubts.

----- [hidden email] wrote:

> MidiSystem.java:
>
> @@ -1096,22 +1113,26 @@
>                       return device;
>                   }
>               }
>           }
>
> - /* Provider class not specified or cannot be found, or
> - provider class specified, and no appropriate device available or
> - provider class and instance specified and instance cannot be found
> or
> is not appropriate */
> + /*
> + * Provider class not specified or cannot be found, or provider
> class
> + * specified, and no appropriate device available or provider class
> and
> + * instance specified and instance cannot be found or is not
> appropriate
> + */
>
> Old comment looks better (each "or" condition in a separate line)
>
> - /* No default are specified, or if something is specified,
> everything
> - failed. */
> + /*
> + * No default are specified, or if something is specified,
> everything
> + * failed.
> + */
>
> "No default is specified" or "No defaults are specified"
>
>
> AudioFileFormat.java:
>
> -         * Finalizes the equals method.
> +         * Indicates whether the specified object is equal to this
> file
> type,
> +         * returning {@code true} if the objects are the same.
> +         *
> +         * @param  obj the reference object with which to compare
> +         * @return {@code true} if this file type is the same as the
>
> {@code obj}
> +         *         argument; {@code false} otherwise
>            */
>           @Override
>           public final boolean equals(final Object obj) {
>
> The implementation checks if the objects are equal, not that the
> objects
> are the same
>
> The same for AudioFormat.equals()
>
> (Note that in most cases equals() methods of JavaSound classes just
> call
> super.equals(), i.e. they really test is the objects are the same)
>
> --alex
>
>
> On 07/20/2017 18:46, Sergey Bylokhov wrote:
> > The DRAFT version of CSR was created:
> > https://bugs.openjdk.java.net/browse/JDK-8185020
> >
> > ----- [hidden email] wrote:
> >>
> >> The fix is updated:
> >> http://cr.openjdk.java.net/~serb/8181566/webrev.01
> >>
> >> > I don't see the point of prettying up the docs on the un-used,
> commented out constants.
> >> > Can't we just delete them ? Seems like the decision was made
> years ago not to include them in the API
> >>
> >
> > It could be removed, but I still consider them as kind of todo, I
> hope
> > to check them one by one at some point.
> >
Reply | Threaded
Open this post in threaded view
|

Re: <Sound Dev> [10] Review Request: 8181566 JavaSound javadoc clarification

Alex Menkov-2
Hi Sergey,

I'm fine with this ("the same" (for 2nd case) looks more clear to me,
but "identical" is also ok)

--alex

On 07/30/2017 16:07, Sergey Bylokhov wrote:

> Hi, Alex.
> Thank you for review!
> I would like to propose and discuss other text:
>
> If the "equals()" method is overridden to not use "==":
>
>     /**
>      * Indicates whether the specified object is equal to this info object,
>      * returning {@code true} if the objects are equal.
>      *
>      * @param  obj the reference object with which to compare
>      * @return {@code true} the specified object is equal to this info
>      *         object; {@code false} otherwise
>      */
>     public final boolean equals(Object obj) {
>
>
> And two versions if the "==" from Object is used:
>
>     /**
>      * Indicates whether the specified object is equal to this info object,
>      * returning {@code true} if the objects are identical.
>      *
>      * @param  obj the reference object with which to compare
>      * @return {@code true} the specified object is equal to this info
>      *         object; {@code false} otherwise
>      */
>     public final boolean equals(Object obj) {
>
> or:
>
>     /**
>      * Indicates whether the specified object is equal to this info object,
>      * returning {@code true} if the objects are the same.
>      *
>      * @param  obj the reference object with which to compare
>      * @return {@code true} the specified object is equal to this info
>      *         object; {@code false} otherwise
>      */
>     public final boolean equals(Object obj) {
>
>
> In all versions the text in "@return " tag is short and the same. But description have different words about the objects: "identical"/"equal"/"the same".
>
> ps: The text for the current fix was copied from the Integer.java which I usually use when in doubts.
>
> ----- [hidden email] wrote:
>
>> MidiSystem.java:
>>
>> @@ -1096,22 +1113,26 @@
>>                       return device;
>>                   }
>>               }
>>           }
>>
>> - /* Provider class not specified or cannot be found, or
>> - provider class specified, and no appropriate device available or
>> - provider class and instance specified and instance cannot be found
>> or
>> is not appropriate */
>> + /*
>> + * Provider class not specified or cannot be found, or provider
>> class
>> + * specified, and no appropriate device available or provider class
>> and
>> + * instance specified and instance cannot be found or is not
>> appropriate
>> + */
>>
>> Old comment looks better (each "or" condition in a separate line)
>>
>> - /* No default are specified, or if something is specified,
>> everything
>> - failed. */
>> + /*
>> + * No default are specified, or if something is specified,
>> everything
>> + * failed.
>> + */
>>
>> "No default is specified" or "No defaults are specified"
>>
>>
>> AudioFileFormat.java:
>>
>> -         * Finalizes the equals method.
>> +         * Indicates whether the specified object is equal to this
>> file
>> type,
>> +         * returning {@code true} if the objects are the same.
>> +         *
>> +         * @param  obj the reference object with which to compare
>> +         * @return {@code true} if this file type is the same as the
>>
>> {@code obj}
>> +         *         argument; {@code false} otherwise
>>            */
>>           @Override
>>           public final boolean equals(final Object obj) {
>>
>> The implementation checks if the objects are equal, not that the
>> objects
>> are the same
>>
>> The same for AudioFormat.equals()
>>
>> (Note that in most cases equals() methods of JavaSound classes just
>> call
>> super.equals(), i.e. they really test is the objects are the same)
>>
>> --alex
>>
>>
>> On 07/20/2017 18:46, Sergey Bylokhov wrote:
>>> The DRAFT version of CSR was created:
>>> https://bugs.openjdk.java.net/browse/JDK-8185020
>>>
>>> ----- [hidden email] wrote:
>>>>
>>>> The fix is updated:
>>>> http://cr.openjdk.java.net/~serb/8181566/webrev.01
>>>>
>>>>> I don't see the point of prettying up the docs on the un-used,
>> commented out constants.
>>>>> Can't we just delete them ? Seems like the decision was made
>> years ago not to include them in the API
>>>>
>>>
>>> It could be removed, but I still consider them as kind of todo, I
>> hope
>>> to check them one by one at some point.
>>>
Reply | Threaded
Open this post in threaded view
|

Re: <Sound Dev> [10] Review Request: 8181566 JavaSound javadoc clarification

Sergey Bylokhov
Hi, Alex.
Here is an updated webrev:
http://cr.openjdk.java.net/~serb/8181566/webrev.03

The new CSR:
https://bugs.openjdk.java.net/browse/JDK-8185020

On 17.08.2017 9:48, Alex Menkov wrote:

> Hi Sergey,
>
> I'm fine with this ("the same" (for 2nd case) looks more clear to me,
> but "identical" is also ok)
>
> --alex
>
> On 07/30/2017 16:07, Sergey Bylokhov wrote:
>> Hi, Alex.
>> Thank you for review!
>> I would like to propose and discuss other text:
>>
>> If the "equals()" method is overridden to not use "==":
>>
>>     /**
>>      * Indicates whether the specified object is equal to this info
>> object,
>>      * returning {@code true} if the objects are equal.
>>      *
>>      * @param  obj the reference object with which to compare
>>      * @return {@code true} the specified object is equal to this info
>>      *         object; {@code false} otherwise
>>      */
>>     public final boolean equals(Object obj) {
>>
>>
>> And two versions if the "==" from Object is used:
>>
>>     /**
>>      * Indicates whether the specified object is equal to this info
>> object,
>>      * returning {@code true} if the objects are identical.
>>      *
>>      * @param  obj the reference object with which to compare
>>      * @return {@code true} the specified object is equal to this info
>>      *         object; {@code false} otherwise
>>      */
>>     public final boolean equals(Object obj) {
>>
>> or:
>>
>>     /**
>>      * Indicates whether the specified object is equal to this info
>> object,
>>      * returning {@code true} if the objects are the same.
>>      *
>>      * @param  obj the reference object with which to compare
>>      * @return {@code true} the specified object is equal to this info
>>      *         object; {@code false} otherwise
>>      */
>>     public final boolean equals(Object obj) {
>>
>>
>> In all versions the text in "@return " tag is short and the same. But
>> description have different words about the objects:
>> "identical"/"equal"/"the same".
>>
>> ps: The text for the current fix was copied from the Integer.java
>> which I usually use when in doubts.
>>
>> ----- [hidden email] wrote:
>>
>>> MidiSystem.java:
>>>
>>> @@ -1096,22 +1113,26 @@
>>>                       return device;
>>>                   }
>>>               }
>>>           }
>>>
>>> - /* Provider class not specified or cannot be found, or
>>> - provider class specified, and no appropriate device available or
>>> - provider class and instance specified and instance cannot be found
>>> or
>>> is not appropriate */
>>> + /*
>>> + * Provider class not specified or cannot be found, or provider
>>> class
>>> + * specified, and no appropriate device available or provider class
>>> and
>>> + * instance specified and instance cannot be found or is not
>>> appropriate
>>> + */
>>>
>>> Old comment looks better (each "or" condition in a separate line)
>>>
>>> - /* No default are specified, or if something is specified,
>>> everything
>>> - failed. */
>>> + /*
>>> + * No default are specified, or if something is specified,
>>> everything
>>> + * failed.
>>> + */
>>>
>>> "No default is specified" or "No defaults are specified"
>>>
>>>
>>> AudioFileFormat.java:
>>>
>>> -         * Finalizes the equals method.
>>> +         * Indicates whether the specified object is equal to this
>>> file
>>> type,
>>> +         * returning {@code true} if the objects are the same.
>>> +         *
>>> +         * @param  obj the reference object with which to compare
>>> +         * @return {@code true} if this file type is the same as the
>>>
>>> {@code obj}
>>> +         *         argument; {@code false} otherwise
>>>            */
>>>           @Override
>>>           public final boolean equals(final Object obj) {
>>>
>>> The implementation checks if the objects are equal, not that the
>>> objects
>>> are the same
>>>
>>> The same for AudioFormat.equals()
>>>
>>> (Note that in most cases equals() methods of JavaSound classes just
>>> call
>>> super.equals(), i.e. they really test is the objects are the same)
>>>
>>> --alex
>>>
>>>
>>> On 07/20/2017 18:46, Sergey Bylokhov wrote:
>>>> The DRAFT version of CSR was created:
>>>> https://bugs.openjdk.java.net/browse/JDK-8185020
>>>>
>>>> ----- [hidden email] wrote:
>>>>>
>>>>> The fix is updated:
>>>>> http://cr.openjdk.java.net/~serb/8181566/webrev.01
>>>>>
>>>>>> I don't see the point of prettying up the docs on the un-used,
>>> commented out constants.
>>>>>> Can't we just delete them ? Seems like the decision was made
>>> years ago not to include them in the API
>>>>>
>>>>
>>>> It could be removed, but I still consider them as kind of todo, I
>>> hope
>>>> to check them one by one at some point.
>>>>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Sound Dev> [10] Review Request: 8181566 JavaSound javadoc clarification

Alex Menkov-2
The fix looks good.
I've added myself as a reviewer at the CSR.

--alex

On 08/21/2017 16:02, Sergey Bylokhov wrote:

> Hi, Alex.
> Here is an updated webrev:
> http://cr.openjdk.java.net/~serb/8181566/webrev.03
>
> The new CSR:
> https://bugs.openjdk.java.net/browse/JDK-8185020
>
> On 17.08.2017 9:48, Alex Menkov wrote:
>> Hi Sergey,
>>
>> I'm fine with this ("the same" (for 2nd case) looks more clear to me,
>> but "identical" is also ok)
>>
>> --alex
>>
>> On 07/30/2017 16:07, Sergey Bylokhov wrote:
>>> Hi, Alex.
>>> Thank you for review!
>>> I would like to propose and discuss other text:
>>>
>>> If the "equals()" method is overridden to not use "==":
>>>
>>>     /**
>>>      * Indicates whether the specified object is equal to this info
>>> object,
>>>      * returning {@code true} if the objects are equal.
>>>      *
>>>      * @param  obj the reference object with which to compare
>>>      * @return {@code true} the specified object is equal to this info
>>>      *         object; {@code false} otherwise
>>>      */
>>>     public final boolean equals(Object obj) {
>>>
>>>
>>> And two versions if the "==" from Object is used:
>>>
>>>     /**
>>>      * Indicates whether the specified object is equal to this info
>>> object,
>>>      * returning {@code true} if the objects are identical.
>>>      *
>>>      * @param  obj the reference object with which to compare
>>>      * @return {@code true} the specified object is equal to this info
>>>      *         object; {@code false} otherwise
>>>      */
>>>     public final boolean equals(Object obj) {
>>>
>>> or:
>>>
>>>     /**
>>>      * Indicates whether the specified object is equal to this info
>>> object,
>>>      * returning {@code true} if the objects are the same.
>>>      *
>>>      * @param  obj the reference object with which to compare
>>>      * @return {@code true} the specified object is equal to this info
>>>      *         object; {@code false} otherwise
>>>      */
>>>     public final boolean equals(Object obj) {
>>>
>>>
>>> In all versions the text in "@return " tag is short and the same. But
>>> description have different words about the objects:
>>> "identical"/"equal"/"the same".
>>>
>>> ps: The text for the current fix was copied from the Integer.java
>>> which I usually use when in doubts.
>>>
>>> ----- [hidden email] wrote:
>>>
>>>> MidiSystem.java:
>>>>
>>>> @@ -1096,22 +1113,26 @@
>>>>                       return device;
>>>>                   }
>>>>               }
>>>>           }
>>>>
>>>> - /* Provider class not specified or cannot be found, or
>>>> - provider class specified, and no appropriate device available or
>>>> - provider class and instance specified and instance cannot be found
>>>> or
>>>> is not appropriate */
>>>> + /*
>>>> + * Provider class not specified or cannot be found, or provider
>>>> class
>>>> + * specified, and no appropriate device available or provider class
>>>> and
>>>> + * instance specified and instance cannot be found or is not
>>>> appropriate
>>>> + */
>>>>
>>>> Old comment looks better (each "or" condition in a separate line)
>>>>
>>>> - /* No default are specified, or if something is specified,
>>>> everything
>>>> - failed. */
>>>> + /*
>>>> + * No default are specified, or if something is specified,
>>>> everything
>>>> + * failed.
>>>> + */
>>>>
>>>> "No default is specified" or "No defaults are specified"
>>>>
>>>>
>>>> AudioFileFormat.java:
>>>>
>>>> -         * Finalizes the equals method.
>>>> +         * Indicates whether the specified object is equal to this
>>>> file
>>>> type,
>>>> +         * returning {@code true} if the objects are the same.
>>>> +         *
>>>> +         * @param  obj the reference object with which to compare
>>>> +         * @return {@code true} if this file type is the same as the
>>>>
>>>> {@code obj}
>>>> +         *         argument; {@code false} otherwise
>>>>            */
>>>>           @Override
>>>>           public final boolean equals(final Object obj) {
>>>>
>>>> The implementation checks if the objects are equal, not that the
>>>> objects
>>>> are the same
>>>>
>>>> The same for AudioFormat.equals()
>>>>
>>>> (Note that in most cases equals() methods of JavaSound classes just
>>>> call
>>>> super.equals(), i.e. they really test is the objects are the same)
>>>>
>>>> --alex
>>>>
>>>>
>>>> On 07/20/2017 18:46, Sergey Bylokhov wrote:
>>>>> The DRAFT version of CSR was created:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8185020
>>>>>
>>>>> ----- [hidden email] wrote:
>>>>>>
>>>>>> The fix is updated:
>>>>>> http://cr.openjdk.java.net/~serb/8181566/webrev.01
>>>>>>
>>>>>>> I don't see the point of prettying up the docs on the un-used,
>>>> commented out constants.
>>>>>>> Can't we just delete them ? Seems like the decision was made
>>>> years ago not to include them in the API
>>>>>>
>>>>>
>>>>> It could be removed, but I still consider them as kind of todo, I
>>>> hope
>>>>> to check them one by one at some point.
>>>>>
>
>