<Swing Dev> [9] Review request for 7020860: BasicTreeUI contains getters/setters with unclear spec

[Semyon Sadetsky]

On 7/18/2016 7:29 PM, Phil Race wrote:

> /**
> -     * Sets the {@code TreeCellRenderer} to {@code tcr}. This invokes
> -     * {@code updateRenderer}.
> +     * Called when the {@code cellRenderer} property is changed in
> +     * the drawn tree component.
>       *
> -     * @param tcr the new value
> +     * @param tcr the new value of the {@code cellRenderer} property
>       */
>      protected void setCellRenderer(TreeCellRenderer tcr) {
>          completeEditing();
> @@ -468,10 +471,10 @@
>      }
>
> why did you remove the reference to updateRenderer() ? Was it wrong ?
I'm not sure that we want that level of the implementation details. Do 
you think the updateRenderer() call is more important than 
completeEditing() and updateSize() which called in the method as well?
> Also "Called when ... is changed"  occurs a few times in this webrev 
> .. but changed by what ?
Is it incorrect phrase in English? The source of the change may be any 
user or internal code. Those methods update UI upon the change notification.
>
>
>      /**
> -     * Returns the row height.
> +     * Returns the height of each row in the drawn tree component. If 
> the
> +     * returned value is less than or equal to 0 the height for each 
> row is
> +     * determined by the renderer.
>       *
> -     * @return the row height
> +     * @return the height of each row, in pixels
>       */
>      protected int getRowHeight() {
>          return (tree == null) ? -1 : tree.getRowHeight();
>      }
>
> Seems like it also returns -1 if there is no tree .. I don't see how the
> spec. words account for that.
It seems this check is redundant and it was added to avoid questions 
about NPE. The tree field is null only if UI is not installed yet or is 
uninstalled already. So, getRowHeight() method does not need to be 
called if tree == null, but if called the value it returns doesn't have 
any sense.
>
>
>      /**
> -     * Returns {@code true} if the tree root is visible.
> +     * Returns whether the root node of the drawn tree component is 
> displayable.
>       *
> -     * @return {@code true} if the tree root is visible
> +     * @return {@code true} if the root node of the tree is displayed
>       */
>      protected boolean isRootVisible() {
>          return (tree != null) ? tree.isRootVisible() : false;
>      }
>
> The API says "visible". Why change to "displyable", which usually is 
> reserved
> to mean whether something that has a native peer ?
> Maybe some clarification of what "visible" means is in order.
O.k. I would use "displayed" because it simply proxies 
tree.isRootVisible() which has "displayed" in its specs.

   Returns whether the root node of the drawn tree component *should be 
displayed*.

?

--Semyon

>
> -phil.
>
>
>
> On 06/21/2016 04:55 AM, Alexandr Scherbatiy wrote:
>> On 6/14/2016 5:37 PM, Semyon Sadetsky wrote:
>>> Hello,
>>>
>>> Please review fix for JDK9:
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-7020860
>>>
>>> webrev: http://cr.openjdk.java.net/~ssadetsky/7020860/webrev.00/
>>>
>>> The methods mentioned in JIRA are not getters/setters according to 
>>> JavaBeans specs, nevertheless some of them got wrong javadocs. The 
>>> proposed fix improves the situation.
>>
>>    I am not sure about the phrase "that is rendering each cell" in
>>
>>  474      * Returns the current instance of the {@link 
>> TreeCellRenderer} that is
>>  475      * rendering each cell.
>>
>>   Otherwise, the fix looks good to me.
>>
>>   Thanks,
>>   Alexandr.
>>
>>
>>>
>>> --Semyon
>>>
>>
>