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

sergey malenkov sergey.malenkov at oracle.com
Wed Apr 30 09:09:17 UTC 2014


Now it looks OK for CCC request:
http://ccc.us.oracle.com/docs/process

Thanks,
SAM

On 30.04.2014 2:54, Steve Sides wrote:
> Hopefully 4th time's a charm.
>
> Review Request for 8040893: fix doclint block tag issues in swing 
> border classes, rev 4:
>
> http://cr.openjdk.java.net/~ssides/8040893/8040893.4/
>
> -steve
>
> On 4/25/2014 9:22 AM, sergey malenkov wrote:
>>
>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>




More information about the swing-dev mailing list