<Swing Dev> Review Request for 8040893: fix doclint block tag issues in swing border classes
Steve Sides
steve.sides at oracle.com
Tue Apr 29 22:54:48 UTC 2014
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