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

[Sergey Bylokhov]

Hi, Krishna.
Please update the test to catch the difference between v00 and v01.
Some comments about javadoc:
  - "183      * <p>" is unnecessary
  - Do not use dots at the end of @param/@return tags
  - 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."
  - 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
  - Why in one class you use addElements/addElementsAt names and in 
another addAll?
  - 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"

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?

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.