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.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)
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.bylok...@oracle.com> <mailto:sergey.bylok...@oracle.com>;
Andrej Golovnin<andrej.golov...@gmail.com> <mailto:andrej.golov...@gmail.com>
Cc:swing-dev@openjdk.java.net <mailto: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: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.golov...@gmail.com>
<mailto:andrej.golov...@gmail.com>
Sent: Monday, April 9, 2018 3:15 PM
To: Krishna Addepalli<krishna.addepa...@oracle.com>
<mailto:krishna.addepa...@oracle.com>
Cc:swing-dev@openjdk.java.net <mailto: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/
<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.
