<Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Pankaj Bansal

Hi All,

 

Please review the fix for JDK 11.

 

Bug:

https://bugs.openjdk.java.net/browse/JDK-5076761

 

Webrev:

http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/

 

Issue:

When setSelectedValue is called on JList with null object, it should clear all selection. But it is not doing anything.

 

Fix:

In setSelectedValue if the object is null, the setSelectedIndex is called with -1. but this does not clear the selection. We should be calling clearSelection instead of SetSelectedIndex.

 

Regards,

Pankaj Bansal

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Sergey Bylokhov
Hi, Pankaj.
The fix looks fine, but I suggest to update the spec as well and
describe the behavior if "null" value is passed.

On 02/01/2018 02:07, Pankaj Bansal wrote:

> Hi All,
>
> Please review the fix for JDK 11.
>
> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-5076761
>
> Webrev:
>
> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>
> Issue:
>
> When setSelectedValue is called on JList with null object, it should
> clear all selection. But it is not doing anything.
>
> Fix:
>
> In setSelectedValue if the object is null, the setSelectedIndex is
> called with -1. but this does not clear the selection. We should be
> calling clearSelection instead of SetSelectedIndex.
>
> Regards,
>
> Pankaj Bansal
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Pankaj Bansal
Hi Sergey,

Thanks for the review.

I have made the suggested changes. Please have a look.
webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/

Regards,
Pankaj Bansal


-----Original Message-----
From: Sergey Bylokhov
Sent: Wednesday, January 3, 2018 3:31 AM
To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
Subject: Re: [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Hi, Pankaj.
The fix looks fine, but I suggest to update the spec as well and describe the behavior if "null" value is passed.

On 02/01/2018 02:07, Pankaj Bansal wrote:

> Hi All,
>
> Please review the fix for JDK 11.
>
> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-5076761
>
> Webrev:
>
> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>
> Issue:
>
> When setSelectedValue is called on JList with null object, it should
> clear all selection. But it is not doing anything.
>
> Fix:
>
> In setSelectedValue if the object is null, the setSelectedIndex is
> called with -1. but this does not clear the selection. We should be
> calling clearSelection instead of SetSelectedIndex.
>
> Regards,
>
> Pankaj Bansal
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Sergey Bylokhov
Looks fine.
Please change the dates in the test header before the push. Note that
you need to create a CSR request for the spec update.

On 02/01/2018 23:26, Pankaj Bansal wrote:

> Hi Sergey,
>
> Thanks for the review.
>
> I have made the suggested changes. Please have a look.
> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>
> Regards,
> Pankaj Bansal
>
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Wednesday, January 3, 2018 3:31 AM
> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
> Subject: Re: [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything
>
> Hi, Pankaj.
> The fix looks fine, but I suggest to update the spec as well and describe the behavior if "null" value is passed.
>
> On 02/01/2018 02:07, Pankaj Bansal wrote:
>> Hi All,
>>
>> Please review the fix for JDK 11.
>>
>> Bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>
>> Issue:
>>
>> When setSelectedValue is called on JList with null object, it should
>> clear all selection. But it is not doing anything.
>>
>> Fix:
>>
>> In setSelectedValue if the object is null, the setSelectedIndex is
>> called with -1. but this does not clear the selection. We should be
>> calling clearSelection instead of SetSelectedIndex.
>>
>> Regards,
>>
>> Pankaj Bansal
>>
>
>
> --
> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Semyon Sadetsky
In reply to this post by Pankaj Bansal
Hi Pankaj,

Can JList contain null value?

--Semyon


On 01/02/2018 11:26 PM, Pankaj Bansal wrote:

> Hi Sergey,
>
> Thanks for the review.
>
> I have made the suggested changes. Please have a look.
> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>
> Regards,
> Pankaj Bansal
>
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Wednesday, January 3, 2018 3:31 AM
> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
> Subject: Re: [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything
>
> Hi, Pankaj.
> The fix looks fine, but I suggest to update the spec as well and describe the behavior if "null" value is passed.
>
> On 02/01/2018 02:07, Pankaj Bansal wrote:
>> Hi All,
>>
>> Please review the fix for JDK 11.
>>
>> Bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>
>> Issue:
>>
>> When setSelectedValue is called on JList with null object, it should
>> clear all selection. But it is not doing anything.
>>
>> Fix:
>>
>> In setSelectedValue if the object is null, the setSelectedIndex is
>> called with -1. but this does not clear the selection. We should be
>> calling clearSelection instead of SetSelectedIndex.
>>
>> Regards,
>>
>> Pankaj Bansal
>>
>
> --
> Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Pankaj Bansal
Hi Semyon/Sergey,

Yes, JList can contain null elements as it can be added to data model. I think this changes a lot in this bug. I think the selection needs to be cleared when the object passed is not present in the list, irrespective of it being null or not. I have made code changes. Please have a look.

Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/

Regards,
Pankaj Bansal

-----Original Message-----
From: Semyon Sadetsky
Sent: Friday, January 5, 2018 6:59 AM
To: Pankaj Bansal; [hidden email]
Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Hi Pankaj,

Can JList contain null value?

--Semyon


On 01/02/2018 11:26 PM, Pankaj Bansal wrote:

> Hi Sergey,
>
> Thanks for the review.
>
> I have made the suggested changes. Please have a look.
> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>
> Regards,
> Pankaj Bansal
>
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Wednesday, January 3, 2018 3:31 AM
> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
> Subject: Re: [11] Review Request: JDK-5076761 :
> JList.setSelectedValue(null, ...) doesn't do anything
>
> Hi, Pankaj.
> The fix looks fine, but I suggest to update the spec as well and describe the behavior if "null" value is passed.
>
> On 02/01/2018 02:07, Pankaj Bansal wrote:
>> Hi All,
>>
>> Please review the fix for JDK 11.
>>
>> Bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>
>> Issue:
>>
>> When setSelectedValue is called on JList with null object, it should
>> clear all selection. But it is not doing anything.
>>
>> Fix:
>>
>> In setSelectedValue if the object is null, the setSelectedIndex is
>> called with -1. but this does not clear the selection. We should be
>> calling clearSelection instead of SetSelectedIndex.
>>
>> Regards,
>>
>> Pankaj Bansal
>>
>
> --
> Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Andrej Golovnin-2
Hi Pankaj,

> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/

src/java.desktop/share/classes/javax/swing/JList.java

You can simplify the code in lines

2373         if((anObject == selectedValue) ||
2374                 (anObject != null && anObject.equals(selectedValue))) {

and

2383                 if ((anObject == object) ||
2384                         anObject!= null && anObject.equals(object)) {

by using Objects.equals(Object, Object):

And please add spaces in the line 2379 where needed (see
http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-whitespace
for details).

Best regards,
Andrej Golovnin
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Pankaj Bansal
Hi Andrej,

Thanks for the review.

I have made changes according to your suggestions. Please have a look.
Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.03/

Regards,
Pankaj Bansal

-----Original Message-----
From: Andrej Golovnin [mailto:[hidden email]]
Sent: Friday, January 5, 2018 4:33 PM
To: Pankaj Bansal
Cc: Semyon Sadetsky; [hidden email]; Sergey Bylokhov
Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Hi Pankaj,

> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/

src/java.desktop/share/classes/javax/swing/JList.java

You can simplify the code in lines

2373         if((anObject == selectedValue) ||
2374                 (anObject != null && anObject.equals(selectedValue))) {

and

2383                 if ((anObject == object) ||
2384                         anObject!= null && anObject.equals(object)) {

by using Objects.equals(Object, Object):

And please add spaces in the line 2379 where needed (see http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-whitespace
for details).

Best regards,
Andrej Golovnin
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Andrej Golovnin-2
Hi Pankaj,

> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.03/

src/java.desktop/share/classes/javax/swing/JList.java

2379             for(i = 0, c = dm.getSize(); i < c; i++) {

There should be a space after "for". Otherwise looks good. Thanks!

Best regards,
Andrej Golovnin
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Pankaj Bansal
Hi Andrej,

Sorry I missed this small thing. Following is the new webrev
webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.04/

Regards,
Pankaj Bansal

-----Original Message-----
From: Andrej Golovnin [mailto:[hidden email]]
Sent: Friday, January 5, 2018 6:52 PM
To: Pankaj Bansal
Cc: Semyon Sadetsky; [hidden email]; Sergey Bylokhov
Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Hi Pankaj,

> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.03/

src/java.desktop/share/classes/javax/swing/JList.java

2379             for(i = 0, c = dm.getSize(); i < c; i++) {

There should be a space after "for". Otherwise looks good. Thanks!

Best regards,
Andrej Golovnin
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Semyon Sadetsky
Hi Pankaj,

Note that before the selection was never cleared if the object is not
found and the DefaultListSelecetionModel is used. This may cause
compatibility issue in old applications.

Since the clearing selection behavior is now mentioned in
setSelectedValue() javadoc it should be also clarified for

setSelectedIndex()
setSelectedIndices()
setSelectionInterval()

and their implementation may need to be changed as well to be consistent.

--Semyon

On 01/05/2018 06:09 AM, Pankaj Bansal wrote:

> Hi Andrej,
>
> Sorry I missed this small thing. Following is the new webrev
> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.04/
>
> Regards,
> Pankaj Bansal
>
> -----Original Message-----
> From: Andrej Golovnin [mailto:[hidden email]]
> Sent: Friday, January 5, 2018 6:52 PM
> To: Pankaj Bansal
> Cc: Semyon Sadetsky; [hidden email]; Sergey Bylokhov
> Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything
>
> Hi Pankaj,
>
>> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.03/
> src/java.desktop/share/classes/javax/swing/JList.java
>
> 2379             for(i = 0, c = dm.getSize(); i < c; i++) {
>
> There should be a space after "for". Otherwise looks good. Thanks!
>
> Best regards,
> Andrej Golovnin

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Sergey Bylokhov
In reply to this post by Pankaj Bansal
I doubt that support of null values in the data model is intentional
because JList is "A component that displays a list of *objects* and
allows the user to select one or more items."

The null behavior was not specified and work in most cases because some
of the getXXX methods in ListModel sub-classes and JList have a special
meaning of null:

JList.getSelectedValue(): Returns {@code null} if there is no selection.
ListModel.getElementAt(int index): In most cases implemented to return
null if index is incorrect.

Similar method TableModel.getValueAt(int,int) is also usually
implemented to return null if value is not found or some error occured.

JComboBox.setSelectedItem(): use <code>null</code> to clear the selection
ComboBoxModel.setSelectedItem(): <code>null</code> to clear the selection
ComboBoxModel.getSelectedItem(): <code>null</code> if there is no selection

So I suggest to follow initial intention here, because it is too late to
add validation of the values.
  - The null value in get() will mean no-selection.
  - The null value passed to set() will clear selection.

On 05/01/2018 02:15, Pankaj Bansal wrote:

> Hi Semyon/Sergey,
>
> Yes, JList can contain null elements as it can be added to data model. I think this changes a lot in this bug. I think the selection needs to be cleared when the object passed is not present in the list, irrespective of it being null or not. I have made code changes. Please have a look.
>
> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/
>
> Regards,
> Pankaj Bansal
>
> -----Original Message-----
> From: Semyon Sadetsky
> Sent: Friday, January 5, 2018 6:59 AM
> To: Pankaj Bansal; [hidden email]
> Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything
>
> Hi Pankaj,
>
> Can JList contain null value?
>
> --Semyon
>
>
> On 01/02/2018 11:26 PM, Pankaj Bansal wrote:
>> Hi Sergey,
>>
>> Thanks for the review.
>>
>> I have made the suggested changes. Please have a look.
>> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>>
>> Regards,
>> Pankaj Bansal
>>
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Wednesday, January 3, 2018 3:31 AM
>> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
>> Subject: Re: [11] Review Request: JDK-5076761 :
>> JList.setSelectedValue(null, ...) doesn't do anything
>>
>> Hi, Pankaj.
>> The fix looks fine, but I suggest to update the spec as well and describe the behavior if "null" value is passed.
>>
>> On 02/01/2018 02:07, Pankaj Bansal wrote:
>>> Hi All,
>>>
>>> Please review the fix for JDK 11.
>>>
>>> Bug:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>>
>>> Webrev:
>>>
>>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>>
>>> Issue:
>>>
>>> When setSelectedValue is called on JList with null object, it should
>>> clear all selection. But it is not doing anything.
>>>
>>> Fix:
>>>
>>> In setSelectedValue if the object is null, the setSelectedIndex is
>>> called with -1. but this does not clear the selection. We should be
>>> calling clearSelection instead of SetSelectedIndex.
>>>
>>> Regards,
>>>
>>> Pankaj Bansal
>>>
>>
>> --
>> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Semyon Sadetsky
Can you explain what is wrong with the suggested  approach?  It is
flexible to handle both null and non-null containing JList.


On 01/05/2018 04:25 PM, Sergey Bylokhov wrote:

> I doubt that support of null values in the data model is intentional
> because JList is "A component that displays a list of *objects* and
> allows the user to select one or more items."
>
> The null behavior was not specified and work in most cases because
> some of the getXXX methods in ListModel sub-classes and JList have a
> special meaning of null:
>
> JList.getSelectedValue(): Returns {@code null} if there is no selection.
> ListModel.getElementAt(int index): In most cases implemented to return
> null if index is incorrect.
>
> Similar method TableModel.getValueAt(int,int) is also usually
> implemented to return null if value is not found or some error occured.
>
> JComboBox.setSelectedItem(): use <code>null</code> to clear the selection
> ComboBoxModel.setSelectedItem(): <code>null</code> to clear the selection
> ComboBoxModel.getSelectedItem(): <code>null</code> if there is no
> selection
>
> So I suggest to follow initial intention here, because it is too late
> to add validation of the values.
>  - The null value in get() will mean no-selection.
>  - The null value passed to set() will clear selection.
>
> On 05/01/2018 02:15, Pankaj Bansal wrote:
>> Hi Semyon/Sergey,
>>
>> Yes, JList can contain null elements as it can be added to data
>> model. I think this changes a lot in this bug. I think the selection
>> needs to be cleared when the object passed is not present in the
>> list, irrespective of it being null or not. I have made code changes.
>> Please have a look.
>>
>> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/
>>
>> Regards,
>> Pankaj Bansal
>>
>> -----Original Message-----
>> From: Semyon Sadetsky
>> Sent: Friday, January 5, 2018 6:59 AM
>> To: Pankaj Bansal; [hidden email]
>> Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 :
>> JList.setSelectedValue(null, ...) doesn't do anything
>>
>> Hi Pankaj,
>>
>> Can JList contain null value?
>>
>> --Semyon
>>
>>
>> On 01/02/2018 11:26 PM, Pankaj Bansal wrote:
>>> Hi Sergey,
>>>
>>> Thanks for the review.
>>>
>>> I have made the suggested changes. Please have a look.
>>> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>>>
>>> Regards,
>>> Pankaj Bansal
>>>
>>>
>>> -----Original Message-----
>>> From: Sergey Bylokhov
>>> Sent: Wednesday, January 3, 2018 3:31 AM
>>> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
>>> Subject: Re: [11] Review Request: JDK-5076761 :
>>> JList.setSelectedValue(null, ...) doesn't do anything
>>>
>>> Hi, Pankaj.
>>> The fix looks fine, but I suggest to update the spec as well and
>>> describe the behavior if "null" value is passed.
>>>
>>> On 02/01/2018 02:07, Pankaj Bansal wrote:
>>>> Hi All,
>>>>
>>>> Please review the fix for JDK 11.
>>>>
>>>> Bug:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>>>
>>>> Webrev:
>>>>
>>>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>>>
>>>> Issue:
>>>>
>>>> When setSelectedValue is called on JList with null object, it should
>>>> clear all selection. But it is not doing anything.
>>>>
>>>> Fix:
>>>>
>>>> In setSelectedValue if the object is null, the setSelectedIndex is
>>>> called with -1. but this does not clear the selection. We should be
>>>> calling clearSelection instead of SetSelectedIndex.
>>>>
>>>> Regards,
>>>>
>>>> Pankaj Bansal
>>>>
>>>
>>> --
>>> Best regards, Sergey.
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Sergey Bylokhov
On 05/01/2018 18:01, Semyon Sadetsky wrote:
> Can you explain what is wrong with the suggested  approach?  It is
> flexible to handle both null and non-null containing JList.

Because it is wrong direction try to support some approach which is
undefined behavior. The null has a special meaning in
JList.getSelectedValue/setSelectedValue() as well as in a few other
methods mentioned below. The usage of unspecified null values may result
in a run time exception either immediately or at some later time.

>
> On 01/05/2018 04:25 PM, Sergey Bylokhov wrote:
>> I doubt that support of null values in the data model is intentional
>> because JList is "A component that displays a list of *objects* and
>> allows the user to select one or more items."
>>
>> The null behavior was not specified and work in most cases because
>> some of the getXXX methods in ListModel sub-classes and JList have a
>> special meaning of null:
>>
>> JList.getSelectedValue(): Returns {@code null} if there is no selection.
>> ListModel.getElementAt(int index): In most cases implemented to return
>> null if index is incorrect.
>>
>> Similar method TableModel.getValueAt(int,int) is also usually
>> implemented to return null if value is not found or some error occured.
>>
>> JComboBox.setSelectedItem(): use <code>null</code> to clear the selection
>> ComboBoxModel.setSelectedItem(): <code>null</code> to clear the selection
>> ComboBoxModel.getSelectedItem(): <code>null</code> if there is no
>> selection
>>
>> So I suggest to follow initial intention here, because it is too late
>> to add validation of the values.
>>  - The null value in get() will mean no-selection.
>>  - The null value passed to set() will clear selection.
>>
>> On 05/01/2018 02:15, Pankaj Bansal wrote:
>>> Hi Semyon/Sergey,
>>>
>>> Yes, JList can contain null elements as it can be added to data
>>> model. I think this changes a lot in this bug. I think the selection
>>> needs to be cleared when the object passed is not present in the
>>> list, irrespective of it being null or not. I have made code changes.
>>> Please have a look.
>>>
>>> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/
>>>
>>> Regards,
>>> Pankaj Bansal
>>>
>>> -----Original Message-----
>>> From: Semyon Sadetsky
>>> Sent: Friday, January 5, 2018 6:59 AM
>>> To: Pankaj Bansal; [hidden email]
>>> Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 :
>>> JList.setSelectedValue(null, ...) doesn't do anything
>>>
>>> Hi Pankaj,
>>>
>>> Can JList contain null value?
>>>
>>> --Semyon
>>>
>>>
>>> On 01/02/2018 11:26 PM, Pankaj Bansal wrote:
>>>> Hi Sergey,
>>>>
>>>> Thanks for the review.
>>>>
>>>> I have made the suggested changes. Please have a look.
>>>> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>>>>
>>>> Regards,
>>>> Pankaj Bansal
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Sergey Bylokhov
>>>> Sent: Wednesday, January 3, 2018 3:31 AM
>>>> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
>>>> Subject: Re: [11] Review Request: JDK-5076761 :
>>>> JList.setSelectedValue(null, ...) doesn't do anything
>>>>
>>>> Hi, Pankaj.
>>>> The fix looks fine, but I suggest to update the spec as well and
>>>> describe the behavior if "null" value is passed.
>>>>
>>>> On 02/01/2018 02:07, Pankaj Bansal wrote:
>>>>> Hi All,
>>>>>
>>>>> Please review the fix for JDK 11.
>>>>>
>>>>> Bug:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>>>>
>>>>> Webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>>>>
>>>>> Issue:
>>>>>
>>>>> When setSelectedValue is called on JList with null object, it should
>>>>> clear all selection. But it is not doing anything.
>>>>>
>>>>> Fix:
>>>>>
>>>>> In setSelectedValue if the object is null, the setSelectedIndex is
>>>>> called with -1. but this does not clear the selection. We should be
>>>>> calling clearSelection instead of SetSelectedIndex.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Pankaj Bansal
>>>>>
>>>>
>>>> --
>>>> Best regards, Sergey.
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Pankaj Bansal
Hi Sergey/Semyon,

>> So I suggest to follow initial intention here, because it is too late
>> to add validation of the values.
>>  - The null value in get() will mean no-selection.
>>  - The null value passed to set() will clear selection.
So I assume we are going with our initial approach.


<<Looks fine.
<<Please change the dates in the test header before the push. Note that you need to create a CSR request for the spec update.
The code changes are in following webrev. I have made the changes in dates in test header. I will raise a CSR.
Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.05/


Regards,
Pankaj Bansal

-----Original Message-----
From: Sergey Bylokhov
Sent: Saturday, January 6, 2018 11:46 AM
To: Semyon Sadetsky; Pankaj Bansal; [hidden email]
Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

On 05/01/2018 18:01, Semyon Sadetsky wrote:
> Can you explain what is wrong with the suggested  approach?  It is
> flexible to handle both null and non-null containing JList.

Because it is wrong direction try to support some approach which is undefined behavior. The null has a special meaning in
JList.getSelectedValue/setSelectedValue() as well as in a few other methods mentioned below. The usage of unspecified null values may result in a run time exception either immediately or at some later time.

>
> On 01/05/2018 04:25 PM, Sergey Bylokhov wrote:
>> I doubt that support of null values in the data model is intentional
>> because JList is "A component that displays a list of *objects* and
>> allows the user to select one or more items."
>>
>> The null behavior was not specified and work in most cases because
>> some of the getXXX methods in ListModel sub-classes and JList have a
>> special meaning of null:
>>
>> JList.getSelectedValue(): Returns {@code null} if there is no selection.
>> ListModel.getElementAt(int index): In most cases implemented to
>> return null if index is incorrect.
>>
>> Similar method TableModel.getValueAt(int,int) is also usually
>> implemented to return null if value is not found or some error occured.
>>
>> JComboBox.setSelectedItem(): use <code>null</code> to clear the
>> selection
>> ComboBoxModel.setSelectedItem(): <code>null</code> to clear the
>> selection
>> ComboBoxModel.getSelectedItem(): <code>null</code> if there is no
>> selection
>>
>> So I suggest to follow initial intention here, because it is too late
>> to add validation of the values.
>>  - The null value in get() will mean no-selection.
>>  - The null value passed to set() will clear selection.
>>
>> On 05/01/2018 02:15, Pankaj Bansal wrote:
>>> Hi Semyon/Sergey,
>>>
>>> Yes, JList can contain null elements as it can be added to data
>>> model. I think this changes a lot in this bug. I think the selection
>>> needs to be cleared when the object passed is not present in the
>>> list, irrespective of it being null or not. I have made code changes.
>>> Please have a look.
>>>
>>> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/
>>>
>>> Regards,
>>> Pankaj Bansal
>>>
>>> -----Original Message-----
>>> From: Semyon Sadetsky
>>> Sent: Friday, January 5, 2018 6:59 AM
>>> To: Pankaj Bansal; [hidden email]
>>> Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 :
>>> JList.setSelectedValue(null, ...) doesn't do anything
>>>
>>> Hi Pankaj,
>>>
>>> Can JList contain null value?
>>>
>>> --Semyon
>>>
>>>
>>> On 01/02/2018 11:26 PM, Pankaj Bansal wrote:
>>>> Hi Sergey,
>>>>
>>>> Thanks for the review.
>>>>
>>>> I have made the suggested changes. Please have a look.
>>>> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>>>>
>>>> Regards,
>>>> Pankaj Bansal
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Sergey Bylokhov
>>>> Sent: Wednesday, January 3, 2018 3:31 AM
>>>> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
>>>> Subject: Re: [11] Review Request: JDK-5076761 :
>>>> JList.setSelectedValue(null, ...) doesn't do anything
>>>>
>>>> Hi, Pankaj.
>>>> The fix looks fine, but I suggest to update the spec as well and
>>>> describe the behavior if "null" value is passed.
>>>>
>>>> On 02/01/2018 02:07, Pankaj Bansal wrote:
>>>>> Hi All,
>>>>>
>>>>> Please review the fix for JDK 11.
>>>>>
>>>>> Bug:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>>>>
>>>>> Webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>>>>
>>>>> Issue:
>>>>>
>>>>> When setSelectedValue is called on JList with null object, it
>>>>> should clear all selection. But it is not doing anything.
>>>>>
>>>>> Fix:
>>>>>
>>>>> In setSelectedValue if the object is null, the setSelectedIndex is
>>>>> called with -1. but this does not clear the selection. We should
>>>>> be calling clearSelection instead of SetSelectedIndex.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Pankaj Bansal
>>>>>
>>>>
>>>> --
>>>> Best regards, Sergey.
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Semyon Sadetsky
In reply to this post by Sergey Bylokhov
On 01/05/2018 10:16 PM, Sergey Bylokhov wrote:

> On 05/01/2018 18:01, Semyon Sadetsky wrote:
>> Can you explain what is wrong with the suggested  approach?  It is
>> flexible to handle both null and non-null containing JList.
>
> Because it is wrong direction try to support some approach which is
> undefined behavior. The null has a special meaning in
> JList.getSelectedValue/setSelectedValue() as well as in a few other
> methods mentioned below. The usage of unspecified null values may
> result in a run time exception either immediately or at some later time.
I didn't find any spec that mentions null as a special value as well as
any pointer that JList couldn't contain null.
getSelectedValue() may not be an indicator because there are several
replacements setSelectedIndex(), setSelectedIndices(),
getSelectedValues(), getSelectedValuesList() which supports null.
Other components are unrelated.

>
>>
>> On 01/05/2018 04:25 PM, Sergey Bylokhov wrote:
>>> I doubt that support of null values in the data model is intentional
>>> because JList is "A component that displays a list of *objects* and
>>> allows the user to select one or more items."
>>>
>>> The null behavior was not specified and work in most cases because
>>> some of the getXXX methods in ListModel sub-classes and JList have a
>>> special meaning of null:
>>>
>>> JList.getSelectedValue(): Returns {@code null} if there is no
>>> selection.
>>> ListModel.getElementAt(int index): In most cases implemented to
>>> return null if index is incorrect.
>>>
>>> Similar method TableModel.getValueAt(int,int) is also usually
>>> implemented to return null if value is not found or some error occured.
>>>
>>> JComboBox.setSelectedItem(): use <code>null</code> to clear the
>>> selection
>>> ComboBoxModel.setSelectedItem(): <code>null</code> to clear the
>>> selection
>>> ComboBoxModel.getSelectedItem(): <code>null</code> if there is no
>>> selection
>>>
>>> So I suggest to follow initial intention here, because it is too
>>> late to add validation of the values.
>>>  - The null value in get() will mean no-selection.
>>>  - The null value passed to set() will clear selection.
>>>
>>> On 05/01/2018 02:15, Pankaj Bansal wrote:
>>>> Hi Semyon/Sergey,
>>>>
>>>> Yes, JList can contain null elements as it can be added to data
>>>> model. I think this changes a lot in this bug. I think the
>>>> selection needs to be cleared when the object passed is not present
>>>> in the list, irrespective of it being null or not. I have made code
>>>> changes. Please have a look.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.02/
>>>>
>>>> Regards,
>>>> Pankaj Bansal
>>>>
>>>> -----Original Message-----
>>>> From: Semyon Sadetsky
>>>> Sent: Friday, January 5, 2018 6:59 AM
>>>> To: Pankaj Bansal; [hidden email]
>>>> Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 :
>>>> JList.setSelectedValue(null, ...) doesn't do anything
>>>>
>>>> Hi Pankaj,
>>>>
>>>> Can JList contain null value?
>>>>
>>>> --Semyon
>>>>
>>>>
>>>> On 01/02/2018 11:26 PM, Pankaj Bansal wrote:
>>>>> Hi Sergey,
>>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> I have made the suggested changes. Please have a look.
>>>>> webrev: http://cr.openjdk.java.net/~pbansal/5076761/webrev.01/
>>>>>
>>>>> Regards,
>>>>> Pankaj Bansal
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Sergey Bylokhov
>>>>> Sent: Wednesday, January 3, 2018 3:31 AM
>>>>> To: Pankaj Bansal; [hidden email]; Prasanta Sadhukhan
>>>>> Subject: Re: [11] Review Request: JDK-5076761 :
>>>>> JList.setSelectedValue(null, ...) doesn't do anything
>>>>>
>>>>> Hi, Pankaj.
>>>>> The fix looks fine, but I suggest to update the spec as well and
>>>>> describe the behavior if "null" value is passed.
>>>>>
>>>>> On 02/01/2018 02:07, Pankaj Bansal wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Please review the fix for JDK 11.
>>>>>>
>>>>>> Bug:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-5076761
>>>>>>
>>>>>> Webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~pbansal/5076761/webrev.00/
>>>>>>
>>>>>> Issue:
>>>>>>
>>>>>> When setSelectedValue is called on JList with null object, it should
>>>>>> clear all selection. But it is not doing anything.
>>>>>>
>>>>>> Fix:
>>>>>>
>>>>>> In setSelectedValue if the object is null, the setSelectedIndex is
>>>>>> called with -1. but this does not clear the selection. We should be
>>>>>> calling clearSelection instead of SetSelectedIndex.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Pankaj Bansal
>>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Sergey.
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Sergey Bylokhov
On 08/01/2018 08:25, Semyon Sadetsky wrote:

>> On 05/01/2018 18:01, Semyon Sadetsky wrote:
>>> Can you explain what is wrong with the suggested  approach?  It is
>>> flexible to handle both null and non-null containing JList.
>>
>> Because it is wrong direction try to support some approach which is
>> undefined behavior. The null has a special meaning in
>> JList.getSelectedValue/setSelectedValue() as well as in a few other
>> methods mentioned below. The usage of unspecified null values may
>> result in a run time exception either immediately or at some later time.
> I didn't find any spec that mentions null as a special value

I already listed a methods which use null as an absent of the value. And
current methods were implemented with this intention.

>  as well as any pointer that JList couldn't contain null.

There are no references to null in the javadoc which means that null is
unsupported. By default the null value is unsupported, and it can be
considered as supported only if spec accept that.

> getSelectedValue() may not be an indicator because there are several
> replacements setSelectedIndex(), setSelectedIndices(),
> getSelectedValues(), getSelectedValuesList() which supports null.
> Other components are unrelated.
Why JComboBox.setSelectedItem() and JList.setSelectedValue() are
unrelated? Both are quite similar and have similar logic. Moreover
JComboBox.setSelectedItem() has much more detailed description which we
should use as an example.

--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Semyon Sadetsky


On 01/16/2018 09:47 AM, Sergey Bylokhov wrote:

> On 08/01/2018 08:25, Semyon Sadetsky wrote:
>>> On 05/01/2018 18:01, Semyon Sadetsky wrote:
>>>> Can you explain what is wrong with the suggested  approach?  It is
>>>> flexible to handle both null and non-null containing JList.
>>>
>>> Because it is wrong direction try to support some approach which is
>>> undefined behavior. The null has a special meaning in
>>> JList.getSelectedValue/setSelectedValue() as well as in a few other
>>> methods mentioned below. The usage of unspecified null values may
>>> result in a run time exception either immediately or at some later
>>> time.
>> I didn't find any spec that mentions null as a special value
>
> I already listed a methods which use null as an absent of the value.
> And current methods were implemented with this intention.
But JList is fully usable without them.
>
>>  as well as any pointer that JList couldn't contain null.
>
> There are no references to null in the javadoc which means that null
> is unsupported. By default the null value is unsupported, and it can
> be considered as supported only if spec accept that.
That is a good argument that proves that null is not considered as a
special value.

>
>> getSelectedValue() may not be an indicator because there are several
>> replacements setSelectedIndex(), setSelectedIndices(),
>> getSelectedValues(), getSelectedValuesList() which supports null.
>> Other components are unrelated.
> Why JComboBox.setSelectedItem() and JList.setSelectedValue() are
> unrelated? Both are quite similar and have similar logic. Moreover
> JComboBox.setSelectedItem() has much more detailed description which
> we should use as an example.
>
Those are different type of controls. For example, in JComboBox
setSelectedIndex(-1) clears selection unlike JList. Why you don't
propose to fix the current issue by making setSelectedIndex(-1) clearing
selection in JList?
And JComboBox may contain null as well.
If we look at JFX ListView it may also contain null.

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Sergey Bylokhov
On 17/01/2018 08:41, Semyon Sadetsky wrote:
>> I already listed a methods which use null as an absent of the value.
>> And current methods were implemented with this intention.
> But JList is fully usable without them.

No because getSelectedValue() will return null if no elements is selected.

>>
>>>  as well as any pointer that JList couldn't contain null.
>>
>> There are no references to null in the javadoc which means that null
>> is unsupported. By default the null value is unsupported, and it can
>> be considered as supported only if spec accept that.
> That is a good argument that proves that null is not considered as a
> special value.

What argument? There are no description that null is supported, but
there are a description that null is used in somecases like
getSelectedValue().


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

Pankaj Bansal
Hi Sergey/Semyon,

I have updated the code to clear selection if the object passed is null. It makes sense to do that as GetSelectedValue returns null if nothing is selected. This makes these two functions compatible in JList.
I think we can deal with clearing selection if the object passed is not present in the JList in some other bug. Present bug only talks about the case when the Object passed in null.

Please share your thoughts on this.

webrev:
http://cr.openjdk.java.net/~pbansal/5076761/webrev.06/

Regards,
Pankaj Bansal

-----Original Message-----
From: Sergey Bylokhov
Sent: Wednesday, January 17, 2018 10:23 PM
To: Semyon Sadetsky; Pankaj Bansal; [hidden email]
Subject: Re: <Swing Dev> [11] Review Request: JDK-5076761 : JList.setSelectedValue(null, ...) doesn't do anything

On 17/01/2018 08:41, Semyon Sadetsky wrote:
>> I already listed a methods which use null as an absent of the value.
>> And current methods were implemented with this intention.
> But JList is fully usable without them.

No because getSelectedValue() will return null if no elements is selected.

>>
>>>  as well as any pointer that JList couldn't contain null.
>>
>> There are no references to null in the javadoc which means that null
>> is unsupported. By default the null value is unsupported, and it can
>> be considered as supported only if spec accept that.
> That is a good argument that proves that null is not considered as a
> special value.

What argument? There are no description that null is supported, but there are a description that null is used in somecases like getSelectedValue().


--
Best regards, Sergey.
12