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