Hi Phil,
Thank you for the review. Yes, those changes are not updated in the CSR. It only contains information about the new APIs being added. @Phil, Sergey – could you mark the CSR reviewed, so that I can finalize it? Thanks, Krishna From: Phil Race Sent: Thursday, April 12, 2018 10:53 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) I was only expecting you to update your new code to use {@code .. } but since you've made the changes they are OK to keep. Just make sure you *exclude* those other changes from the CSR, as they aren't a spec. change and will clutter the CSR more than they are cluttering the webrev. +1 -phil. On 04/12/2018 03:46 AM, Krishna Addepalli wrote: Hi Phil, Thank you for the review. Here is the updated webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev04/"http://cr.openjdk.java.net/~kaddepalli/4842658/webrev04/ Thanks, Krishna From: Phil Race Sent: Wednesday, April 11, 2018 11:33 PM To: Krishna Addepalli HYPERLINK "mailto:krishna.addepa...@oracle.com"<krishna.addepa...@oracle.com>; 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) 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: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev03/"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 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. 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: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev02/"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: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev01/"http://cr.openjdk.java.net/~kaddepalli/4842658/webrev01/ Thanks, Krishna -----Original Message----- From: Andrej Golovnin HYPERLINK "mailto:andrej.golov...@gmail.com"<andrej.golov...@gmail.com> Sent: Monday, April 9, 2018 3:15 PM To: Krishna Addepalli HYPERLINK "mailto:krishna.addepa...@oracle.com"<krishna.addepa...@oracle.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 Krishna, Please review a simple fix for enhancement: JDK-4842658: https://bugs.openjdk.java.net/browse/JDK-4842658 Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev00/"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.