<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