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

[Krishna Addepalli]

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.golovnin at gmail.com>
> Sent: Monday, April 9, 2018 3:15 PM
> To: Krishna Addepalli <krishna.addepalli at oracle.com>
> Cc: 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/
>>
>> 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.