Re: [11][JDK-8153532] RFR: Add @throws NPE javadoc to UIManager.setLookAndFeel(String) method description

[Sergey Bylokhov]

Looks fine.

On 12/04/2018 00:10, Pankaj Bansal wrote:

Hi Sergey,

Re: [11][JDK-4842658] RFR: DefaultListModel and DefaultComboBoxModel should support addAll (Collection c)

[Phil Race]

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 ; Sergey 
Bylokhov ; Andrej Golovnin 


*Cc:* swing-dev@openjdk.java.net
*Subject:* Re:  [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/


Thanks,

Krishna

*From:*Phil Race
*Sent:* Wednesday, April 11, 2018 11:33 PM
*To:* Krishna Addepalli 
; Sergey Bylokhov
 ;
Andrej Golovnin 

*Cc:* swing-dev@openjdk.java.net 
*Subject:* Re:  [11][JDK-4842658] RFR: DefaultListModel
and DefaultComboBoxModel should support addAll (Collection c)

Use the syntax {@code c} instead of c 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/
   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 
; Andrej Golovnin 


Cc:swing-dev@openjdk.java.net 

Subject: RE:  [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  * " 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 IllegalArgumentExceptionif the 
index was invalid."

Removed.

  


   > - It is unclear what this text means:

   >"207 if c 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: