Hi Phil,

Thank you for the review. 

Here is the updated webrev: 





From: Phil Race 
Sent: Wednesday, April 11, 2018 11:33 PM
To: Krishna Addepalli <krishna.addepa...@oracle.com>; Sergey Bylokhov 
<sergey.bylok...@oracle.com>; Andrej Golovnin <andrej.golov...@gmail.com>
Cc: swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> [11][JDK-4842658] RFR: DefaultListModel and 
DefaultComboBoxModel should support addAll (Collection c)


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

Use @throws instead of @exception.
It is preferred these days.

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


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.
-----Original Message-----
From: Krishna Addepalli 
Sent: Tuesday, April 10, 2018 6:14 PM
To: Sergey Bylokhov HYPERLINK 
"mailto:sergey.bylok...@oracle.com";<sergey.bylok...@oracle.com>; Andrej 
Golovnin HYPERLINK "mailto:andrej.golov...@gmail.com";<andrej.golov...@gmail.com>
Cc: HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@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.


Some comments about javadoc:
 - "183      * <p>" is unnecessary
 - Do not use dots at the end of @param/@return tags

 > - 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."
  > - 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 
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"

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: 
On 09/04/2018 04:29, Krishna Addepalli wrote:

Hi Andrej,
Appreciate for the  quick review. Here is the updated webrev: 
-----Original Message-----
From: Andrej Golovnin HYPERLINK 
Sent: Monday, April 9, 2018 3:15 PM
To: Krishna Addepalli HYPERLINK 
Cc: HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@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

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);
574         if(c.isEmpty()) {
There should be a space after 'if'.
Best regards,
Andrej Golovnin

Best regards, Sergey.


Reply via email to