<Swing Dev> Review Request for 8040893: fix doclint block tag issues in swing border classes

[Steve Sides]

Hi Sergey,
I see what you mean. I have a question on some of the BevelBorder method 
docs.
Regarding the getHighlight(or Shadow)Inner/Outer)Color() methods (no 
parameter)
http://cr.openjdk.java.net/~ssides/8040893/8040893.3/src/share/classes/javax/swing/border/BevelBorder.java.sdiff.html

For the methods which take a Component parameter, it is commented the 
color is derived from the Component's background, so these always return 
some Color.
For the methods which take no Component parameter, they return null if  
the color is derived from the background(ie., not specified at 
instantiation).

My question is, are comments like

  201      * @return the outer highlight {@code Color} or {@code null} if highlight
  202      *         colors are derived from background

seen as referring too much to the implementation or clarifying statements?
If it simply says "...or {@code null} is none was specified", it leaves it unclear if there is a highlight or shadow color.

It seems that the javadoc comment could have said

* Returns the outer highlight color of the bevel border.
* If none was specified at instantiation, the highlight
* color is derived from the background.

Then the return could have said, "or null  if none was specified" and 
there would be more clarity.

Or, do we not want that kind of implementation detail revealed at all?

-steve


On 4/24/2014 4:39 AM, sergey malenkov wrote:
> Hello,
>
> I don't think that it is a good idea to specify current implementation.
> For example,
>
>       * Returns whether or not the border is opaque.
> +     *
> +     * @return true if {@code color} is not {@code null}, false if 
> {@code color}
> +     *         is {@code null}
>
> Once it is specified we never can fix it.
> For example, the MatteBorder.isBorderOpaque method, specified above,
> can be fixed in the following way:
> +    * @return true if the internal color is not null *and uses 
> alpha-channel*.
>
> So I suggest to rewrite it's documentation in the following way:
>       * Returns whether or not the border is opaque.
> +    *
> +    * @return {@code true} if this border is opaque,
> +    *                 {@code false} otherwise
>
> The same issue with other similar methods.
>
> Thanks,
> SAM
>
> On 23.04.2014 21:49, Steve Sides wrote:
>> this has the noted change for getEtchType():
>>
>> http://cr.openjdk.java.net/~ssides/8040893/8040893.3/
>>
>> -steve
>>
>> On 4/22/2014 2:03 AM, Alexander Scherbatiy wrote:
>>> On 4/21/2014 11:54 PM, Steve Sides wrote:
>>>>
>>>> On 4/21/2014 2:45 AM, Alexander Scherbatiy wrote:
>>>>>
>>>>> Hello Steve,
>>>>>
>>>>> ------------------------
>>>>>      /**
>>>>>       * Returns the type of the bevel border.
>>>>> +     *
>>>>> +     * @return 0 if the bevel type is {@code RAISED}, 1 if {@code 
>>>>> LOWERED}
>>>>>       */
>>>>> ------------------------
>>>>>
>>>>> I think such description reveals unnecessary level of code 
>>>>> abstraction.
>>>>> Developers always need to use BevelBorder.RAISED and 
>>>>> BevelBorder.LOWERED constant and never their numerical 
>>>>> representation.
>>>>> It would better to say that the method return the bevel type: 
>>>>> RAISED or  LOWERED.
>>>> I thought about that, but the method signature is
>>>> public *int*getBevelType()
>>>> and I wasn't sure if saying it returns "RAISED or LOWERED" clearly 
>>>> matched the signature. I will make it so.
>>>
>>>    This looks good.
>>>    There is the javadoc for the 
>>> BorderLayout.addLayoutComponent(Component comp, Object constraints) 
>>> method that says:
>>>    "For border layouts, the constraint must be one of the following 
>>> constants: NORTH, SOUTH, EAST, WEST, or CENTER."
>>>    and does not mention that NORTH constant is actually the "North" 
>>> String.
>>> http://docs.oracle.com/javase/7/docs/api/java/awt/BorderLayout.html
>>>
>>>   Could you also update the javadoc for the 
>>> EtchedBorder.getEtchType() method in the same way?
>>>
>>>>
>>>>>
>>>>> ------------------------
>>>>>      /**
>>>>>       * Returns whether or not the border is opaque.
>>>>> +     *
>>>>> +     * @return true
>>>>>       */
>>>>>      public boolean isBorderOpaque() { return true; }
>>>>> ------------------------
>>>>> It would better to also add the similar comment from the parent 
>>>>> AbstractBorder class like: "This implementation returns true".
>>>>> It seems that the "code" tag can be used with boolean values: 
>>>>> {@code true}
>>>>
>>>> okay, http://cr.openjdk.java.net/~ssides/8040893/8040893.2/ has 
>>>> changes for above
>>>
>>>    This version looks good for me.
>>>
>>>   Thanks,
>>>   Alexandr.
>>>>
>>>> thanks for the quick feedback,
>>>>
>>>> -steve
>>>>
>>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>> On 4/18/2014 2:48 AM, Steve Sides wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Could you please review the fix for the following bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8040893
>>>>>>
>>>>>> Webrev corresponding:
>>>>>> http://cr.openjdk.java.net/~ssides/8040893/
>>>>>>
>>>>>> webrevComment.txt:
>>>>>> This addresses missing @parm and @return block tags in javadoc for
>>>>>> javax/swing/border classes as noted by doclint.
>>>>>>
>>>>>> It does not address methods which are missing javadoc comment 
>>>>>> altogether,
>>>>>> of which there are many.
>>>>>>
>>>>>> It also a adds @return to isBorderOpaque() methods which were 
>>>>>> incorrectly
>>>>>> inheriting the default, @return false, from AbstractBorder.
>>>>>>
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> -steve
>>>>>>
>>>>>
>>>>
>>>
>>
>