<Swing Dev> [11][JDK-4842658] RFR: DefaultListModel and DefaultComboBoxModel should support addAll (Collection c)

Phil Race philip.race at oracle.com
Fri Apr 13 18:06:39 UTC 2018



On 04/12/2018 10:36 PM, Krishna Addepalli wrote:
>
> 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?
>
We first need to settle whether you will update the method names as
proposed by Sergey.

-phil.

> Thanks,
>
> Krishna
>
> *From:*Phil Race
> *Sent:* Thursday, April 12, 2018 10:53 PM
> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>; Sergey 
> Bylokhov <sergey.bylokhov at oracle.com>; Andrej Golovnin 
> <andrej.golovnin at gmail.com>
> *Cc:* swing-dev at 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:
>     http://cr.openjdk.java.net/~kaddepalli/4842658/webrev04/
>     <http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev04/>
>
>     Thanks,
>
>     Krishna
>
>     *From:*Phil Race
>     *Sent:* Wednesday, April 11, 2018 11:33 PM
>     *To:* Krishna Addepalli <krishna.addepalli at oracle.com>
>     <mailto:krishna.addepalli at oracle.com>; Sergey Bylokhov
>     <sergey.bylokhov at oracle.com> <mailto:sergey.bylokhov at oracle.com>;
>     Andrej Golovnin <andrej.golovnin at gmail.com>
>     <mailto:andrej.golovnin at gmail.com>
>     *Cc:* swing-dev at openjdk.java.net <mailto:swing-dev at 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:http://cr.openjdk.java.net/~kaddepalli/4842658/webrev03/
>         <http://cr.openjdk.java.net/%7Ekaddepalli/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<sergey.bylokhov at oracle.com> <mailto:sergey.bylokhov at oracle.com>; Andrej Golovnin<andrej.golovnin at gmail.com> <mailto:andrej.golovnin at gmail.com>
>
>         Cc:swing-dev at openjdk.java.net <mailto:swing-dev at 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:http://cr.openjdk.java.net/~kaddepalli/4842658/webrev02/
>         <http://cr.openjdk.java.net/%7Ekaddepalli/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/
>             <http://cr.openjdk.java.net/%7Ekaddepalli/4842658/webrev01/>
>
>               
>
>             Thanks,
>
>             Krishna
>
>               
>
>             -----Original Message-----
>
>             From: Andrej Golovnin<andrej.golovnin at gmail.com> <mailto:andrej.golovnin at gmail.com>
>
>             Sent: Monday, April 9, 2018 3:15 PM
>
>             To: Krishna Addepalli<krishna.addepalli at oracle.com>
>             <mailto:krishna.addepalli at oracle.com>
>
>             Cc:swing-dev at openjdk.java.net <mailto:swing-dev at 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/
>                 <http://cr.openjdk.java.net/%7Ekaddepalli/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.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180413/932fa0c6/attachment.html>


More information about the swing-dev mailing list