<Swing Dev> [11][JDK-4842658] RFR: DefaultListModel and DefaultComboBoxModel should support addAll (Collection c)

[Phil Race]

Use the syntax {@code c} instead of <code>c</code> everywhere
it is applicable.

Use @throws instead of @exception.
It is preferred these days.
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#@exception

doesn't -> does not (we should be formal in the docs).

> 560      * from the index specified.

should become

"from the specified index."


  * @param index the index from which to start adding elements from

this reads awkardly .. as we have "from" twice. Can we instead say :

|* @param index the position at which to start adding elements Or, maybe 
even better, borrow the wording from java.util.List's docs on the same 
named method since they are very similar : |

|index| - index at which to insert the first element from the specified 
collection

-phil.

On 04/11/2018 04:25 AM, Krishna Addepalli wrote:
> Hi All,
>
> Here is the modified webrev: http://cr.openjdk.java.net/~kaddepalli/4842658/webrev03/  as per Joe Darcy's suggestion to modify the api to accept a collection object that captures objects of classes that extend type E, in line with underlying Vector API.
>
> Thanks,
> Krishna
>
> -----Original Message-----
> From: Krishna Addepalli
> Sent: Tuesday, April 10, 2018 6:14 PM
> To: Sergey Bylokhov <sergey.bylokhov at oracle.com>; Andrej Golovnin <andrej.golovnin at gmail.com>
> Cc: swing-dev at openjdk.java.net
> Subject: RE: <Swing Dev> [11][JDK-4842658] RFR: DefaultListModel and DefaultComboBoxModel should support addAll (Collection c)
>
> Hi Sergey,
>
>> Please update the test to catch the difference between v00 and v01.
> Done.
>
>> Some comments about javadoc:
>>   - "183      * <p>" is unnecessary
>>   - Do not use dots at the end of @param/@return tags
> Done.
>
>   > - Not sure that the text below is necessary(it is duplicate description of @exception tag):
>   >      "202      * Throws an <code>IllegalArgumentException</code>if the index was invalid."
> Removed.
>
>    > - It is unclear what this text means:
>    >    "207 if <code>c</code> doesn't fall in the range of the list."
>
>    >  - Specify that the new methods will throw NPE on null Done.
>
>    > - Why in one class you use addElements/addElementsAt names and in another addAll?
> In the DefaultComboBoxModel, the functions to add a single element are named as addElement, addElementAt.
> To keep with that convention, I named the apis appropriately.
>
>    > - In the exception message you use "list" for "DefaultListModel" and "combobox" for DefaultComboBoxModel i
>    > guess it should be "xxx model". Or you can simplify it to "Index out of range: + index"
> Done.
>
>> It is also unclear what exception should be thrown,
>> IllegalArgumentException is a good candidate but other methods in
>> these classes like DefaultComboBoxModel.insertElementAt/removeElementAt will throw "ArrayIndexOutOfBoundsException", any thoughts/suggestions?
> I was also a bit confused about this. Initially I referred to the function above (removeRange) and saw that it was throwing IllegalArgumentException. Now, I have changed the exception to ArrayIndexOutOfBoundsException as you suggested.
>
> Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/4842658/webrev02/
>
> Thanks,
> Krishna
>
> On 09/04/2018 04:29, Krishna Addepalli wrote:
>> Hi Andrej,
>>
>> Appreciate for the  quick review. Here is the updated webrev:
>> http://cr.openjdk.java.net/~kaddepalli/4842658/webrev01/
>>
>> Thanks,
>> Krishna
>>
>> -----Original Message-----
>> From: Andrej Golovnin <andrej.golovnin at gmail.com>
>> Sent: Monday, April 9, 2018 3:15 PM
>> To: Krishna Addepalli <krishna.addepalli at oracle.com>
>> Cc: swing-dev at openjdk.java.net
>> Subject: Re: <Swing Dev> [11][JDK-4842658] RFR: DefaultListModel and
>> DefaultComboBoxModel should support addAll (Collection c)
>>
>> Hi Krishna,
>>
>>> Please review a simple fix for enhancement:
>>>
>>> JDK-4842658: https://bugs.openjdk.java.net/browse/JDK-4842658
>>>
>>> Webrev: http://cr.openjdk.java.net/~kaddepalli/4842658/webrev00/
>>>
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8201289
>>
>> src/java.desktop/share/classes/javax/swing/DefaultComboBoxModel.java
>>
>> 195         fireIntervalAdded(this, startIndex, getSize());
>>
>> It should be:
>>
>> 195         fireIntervalAdded(this, startIndex, getSize() - 1);
>>
>>
>> 221         fireIntervalAdded(this, index, index + c.size());
>>
>> It should be:
>>
>> 221         fireIntervalAdded(this, index, index + c.size() - 1);
>>
>>
>> src/java.desktop/share/classes/javax/swing/DefaultListModel.java
>>
>> 574         if(c.isEmpty()) {
>>
>> There should be a space after 'if'.
>>
>> Best regards,
>> Andrej Golovnin
>>
>
> --
> Best regards, Sergey.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180411/d2eeb84c/attachment.html>