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:
http://cr.openjdk.java.net/~kaddepalli/4842658/webrev01/
Thanks,
Krishna
-----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
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.
