Hi, Krishna.
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
- Why in one class you use addElements/addElementsAt names and in another addAll? - 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?

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 <andrej.golov...@gmail.com>
Sent: Monday, April 9, 2018 3:15 PM
To: Krishna Addepalli <krishna.addepa...@oracle.com>
Cc: 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