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

Fri Apr 25 16:22:30 UTC 2014

On 25.04.2014 20:03, sergey malenkov wrote:
> Hi Steve,
>
> We should not confuse the cause and the result.
>
> The getShadowOuterColor method is a property of this Border object.
> Actually this object does not know about the component.
> So, we can say here that the color will be derived from background.
!!! can't say
>
> The getShadowOuterColor(Component) method is an utility method,
> that provides all calculations to resolve the null value of the property.
> Here we specify that the *somehow modified* background color
> will be used if the shadowOuterColor property is not set.
>
> Thanks,
> SAM
>
> On 24.04.2014 20:41, Steve Sides wrote:
>> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>