<Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

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

<Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Pankaj Bansal

Hi All,

 

Please review the fix.

 

Bug:

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

 

Webrev:

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

 

Issue:

JList.getSelectedValuesList crashes if the JList.setSelectionInterval or JList.addSelectionInterval had been called earlier with interval having lead greater than the size of List

 

Fix:

Made changes in JList.getSelectedValuesList to check the if the max selection index is greater than the actual size of the List. If yes, the max is changed to last element index of List.

 

Note:

It makes sense to change the behavior of JList.setSelectionInterval or JList.addSelectionInterval to not allow to set the selection with interval having indices not present in the list. But it will change the behavior of this API and will result in failure of 2 JCK tests.

Also, we will still have to put checks inside the JList.getSelectedValuesList as the selection can be changed by setting selection interval on DefualtListSelectionModel and there is no way to check if the supplied interval range actually exist in the List inside DefualtListSelectionModel.

 

If changing the JList.setSelectionInterval or JList.addSelectionInterval is possible, the potential fix can be following webrev.

http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/

 

 

Regards,

Pankaj Bansal

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Jayathirth D v

Hello Pankaj,

 

Please find my observation:

 

As you have mentioned I also feel that adding check in setSelectionInterval() or addSelectionInterval() would be a good approach. Since I am not aware of swing component code I will leave this decision to others.

 

Regarding http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/ :

May be we are not handling the case where validateLeadIndex() fails and we don’t set selection interval and it is resulting in JCK test fail. If you can share what is behavior of JCK test failure after your change it would be helpful.

 

                Also specification of setSelectionInterval() or addSelectionInterval() mentions that “{@code anchor} doesn't have to be less than or equal to {@code lead}”. So while validating arguments for setSelectionInterval() or addSelectionInterval() I think we should verify the value of anchor first and then check the value of (anchor + lead) instead of just checking whether lead < size.

 

Thanks,

Jay

 

From: Pankaj Bansal
Sent: Friday, December 01, 2017 3:02 PM
To: [hidden email]; Sergey Bylokhov; Semyon Sadetsky
Subject: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

 

Hi All,

 

Please review the fix.

 

Bug:

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

 

Webrev:

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

 

Issue:

JList.getSelectedValuesList crashes if the JList.setSelectionInterval or JList.addSelectionInterval had been called earlier with interval having lead greater than the size of List

 

Fix:

Made changes in JList.getSelectedValuesList to check the if the max selection index is greater than the actual size of the List. If yes, the max is changed to last element index of List.

 

Note:

It makes sense to change the behavior of JList.setSelectionInterval or JList.addSelectionInterval to not allow to set the selection with interval having indices not present in the list. But it will change the behavior of this API and will result in failure of 2 JCK tests.

Also, we will still have to put checks inside the JList.getSelectedValuesList as the selection can be changed by setting selection interval on DefualtListSelectionModel and there is no way to check if the supplied interval range actually exist in the List inside DefualtListSelectionModel.

 

If changing the JList.setSelectionInterval or JList.addSelectionInterval is possible, the potential fix can be following webrev.

http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/

 

 

Regards,

Pankaj Bansal

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Pankaj Bansal

Hi Jay,

 

Thanks for the review.

 

<<May be we are not handling the case where validateLeadIndex() fails and we don’t set selection interval and it is resulting in JCK test fail. If you can share what is <<behavior of JCK test failure after your change it would be helpful.

No, the JCK test expects the selection to be actually set to the given interval irrespective of the number of items in the List. Following test case is failing

   

public Status JList2067() {

        JList c = new JList(); // Create JList object

        c.setSelectionInterval(10, 0);

        if (c.getSelectedIndices().length != 11) {

            ref.println("Wrong selection");

            return Status.failed("FAILED");

        }

 

 

<<Also specification of setSelectionInterval() or addSelectionInterval() mentions that “{@code anchor} doesn't have to be less than or equal to {@code lead}”. So while <<validating arguments for setSelectionInterval() or addSelectionInterval() I think we should verify the value of anchor first and then check the value of (anchor + lead) <<instead of just checking whether lead < size.

Yes, I think we have to check the condition for both anchor and lead both as they both can be greater than size of the List. I will make that change if we decide to go this route depending upon the other reviewers comments. But I feel not making the change in setSelectionInterval and addSelectionInterval is the preferred way as we will anyway have the checks in the getSelectedValuesList as wrong selection can be directly set using DefaultListSelectionModel.

 

Regards

Pankaj Bansal

 

 

Regards

Pankaj Bansal

From: Jayathirth D V
Sent: Friday, December 1, 2017 4:17 PM
To: Pankaj Bansal; [hidden email]; Sergey Bylokhov; Semyon Sadetsky
Subject: RE: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

 

Hello Pankaj,

 

Please find my observation:

 

As you have mentioned I also feel that adding check in setSelectionInterval() or addSelectionInterval() would be a good approach. Since I am not aware of swing component code I will leave this decision to others.

 

Regarding http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/ :

May be we are not handling the case where validateLeadIndex() fails and we don’t set selection interval and it is resulting in JCK test fail. If you can share what is behavior of JCK test failure after your change it would be helpful.

 

                Also specification of setSelectionInterval() or addSelectionInterval() mentions that “{@code anchor} doesn't have to be less than or equal to {@code lead}”. So while validating arguments for setSelectionInterval() or addSelectionInterval() I think we should verify the value of anchor first and then check the value of (anchor + lead) instead of just checking whether lead < size.

 

Thanks,

Jay

 

From: Pankaj Bansal
Sent: Friday, December 01, 2017 3:02 PM
To: [hidden email]; Sergey Bylokhov; Semyon Sadetsky
Subject: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

 

Hi All,

 

Please review the fix.

 

Bug:

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

 

Webrev:

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

 

Issue:

JList.getSelectedValuesList crashes if the JList.setSelectionInterval or JList.addSelectionInterval had been called earlier with interval having lead greater than the size of List

 

Fix:

Made changes in JList.getSelectedValuesList to check the if the max selection index is greater than the actual size of the List. If yes, the max is changed to last element index of List.

 

Note:

It makes sense to change the behavior of JList.setSelectionInterval or JList.addSelectionInterval to not allow to set the selection with interval having indices not present in the list. But it will change the behavior of this API and will result in failure of 2 JCK tests.

Also, we will still have to put checks inside the JList.getSelectedValuesList as the selection can be changed by setting selection interval on DefualtListSelectionModel and there is no way to check if the supplied interval range actually exist in the List inside DefualtListSelectionModel.

 

If changing the JList.setSelectionInterval or JList.addSelectionInterval is possible, the potential fix can be following webrev.

http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/

 

 

Regards,

Pankaj Bansal

 

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Sergey Bylokhov
In reply to this post by Jayathirth D v
Hello.
On 01/12/2017 02:47, Jayathirth D V wrote:
> As you have mentioned I also feel that adding check in
> setSelectionInterval() or addSelectionInterval() would be a good
> approach. Since I am not aware of swing component code I will leave this
> decision to others.

I also have no preference where to change this. If we will change
setSelectionInterval()/addSelectionInterval() then we will need to
update selection model on every change of datamodel. But if we decide
like in the current fix to change the get methods, then we will need to
verify all places where we use datamodel and selection model:
for example JList.getSelectedValue() and others.
Also we should check other classes which use the same selection model
like JTable.

>
> Regarding http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/:
>
> May be we are not handling the case where validateLeadIndex() fails and
> we don’t set selection interval and it is resulting in JCK test fail. If
> you can share what is behavior of JCK test failure after your change it
> would be helpful.
>
>                  Also specification of setSelectionInterval() or
> addSelectionInterval() mentions that “{@code anchor} doesn't have to be
> less than or equal to {@code lead}”. So while validating arguments for
> setSelectionInterval() or addSelectionInterval() I think we should
> verify the value of anchor first and then check the value of (anchor +
> lead) instead of just checking whether lead < size.
>
> Thanks,
>
> Jay
>
> *From:* Pankaj Bansal
> *Sent:* Friday, December 01, 2017 3:02 PM
> *To:* [hidden email]; Sergey Bylokhov; Semyon Sadetsky
> *Subject:* <Swing Dev> [10] Review Request: JDK-7108280 :
> JList.getSelectedValuesList fails if JList.setSelectionInterval larger
> than list
>
> Hi All,
>
> Please review the fix.
>
> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-7108280
>
> Webrev:
>
> http://cr.openjdk.java.net/~pbansal/7108280/webrev.00/
>
> Issue:
>
> JList.getSelectedValuesList crashes if the JList.setSelectionInterval or
> JList.addSelectionInterval had been called earlier with interval having
> lead greater than the size of List
>
> Fix:
>
> Made changes in JList.getSelectedValuesList to check the if the max
> selection index is greater than the actual size of the List. If yes, the
> max is changed to last element index of List.
>
> Note:
>
> It makes sense to change the behavior of JList.setSelectionInterval or
> JList.addSelectionInterval to not allow to set the selection with
> interval having indices not present in the list. But it will change the
> behavior of this API and will result in failure of 2 JCK tests.
>
> Also, we will still have to put checks inside the
> JList.getSelectedValuesList as the selection can be changed by setting
> selection interval on DefualtListSelectionModel and there is no way to
> check if the supplied interval range actually exist in the List inside
> DefualtListSelectionModel.
>
> If changing the JList.setSelectionInterval or JList.addSelectionInterval
> is possible, the potential fix can be following webrev.
>
> http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/
>
> Regards,
>
> Pankaj Bansal
>


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

Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Andrej Golovnin-2
Hi all,

as a long Swing user I would like to vote against the proposed
changes. The fact is that the #setSelectionInterval and
#addSelectionInterval methods of the JList class exist in this form
for a very long time and any change in the behaviour of this methods
may break existing applications.

Technically this methods should throw an IllegalArgumentException when
the arguments are out of bounds. For example the
#setRowSelectionInterval and #addRowSelectionInterval methods of the
JTable class throw an IllegalArgumentException.

I think you should ask someone from the Java core team who has deeper
understanding, when a change is backward compatible and when not, if
it is OK to add a check to this methods and throw an
IllegalArgumentException when the arguments are out of bounds. If it
is not OK, then you can improve at least the JavaDocs of this methods
and explain in the JavaDocs that the arguments must be in the bounds
of the current ListModel.

I would also not change the behaviour of JList#getSelectedValuesList.
The current behaviour, i.g. throwing the
ArrayIndexOutOfBoundsException, helps me to find bugs in applications.
For me setting a wrong selection interval is a bug.

Best regards,
Andrej Golovnin

On Tue, Dec 5, 2017 at 11:29 PM, Sergey Bylokhov
<[hidden email]> wrote:

> Hello.
> On 01/12/2017 02:47, Jayathirth D V wrote:
>>
>> As you have mentioned I also feel that adding check in
>> setSelectionInterval() or addSelectionInterval() would be a good approach.
>> Since I am not aware of swing component code I will leave this decision to
>> others.
>
>
> I also have no preference where to change this. If we will change
> setSelectionInterval()/addSelectionInterval() then we will need to update
> selection model on every change of datamodel. But if we decide like in the
> current fix to change the get methods, then we will need to verify all
> places where we use datamodel and selection model:
> for example JList.getSelectedValue() and others.
> Also we should check other classes which use the same selection model like
> JTable.
>
>>
>> Regarding http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/:
>>
>> May be we are not handling the case where validateLeadIndex() fails and we
>> don’t set selection interval and it is resulting in JCK test fail. If you
>> can share what is behavior of JCK test failure after your change it would be
>> helpful.
>>
>>                  Also specification of setSelectionInterval() or
>> addSelectionInterval() mentions that “{@code anchor} doesn't have to be less
>> than or equal to {@code lead}”. So while validating arguments for
>> setSelectionInterval() or addSelectionInterval() I think we should verify
>> the value of anchor first and then check the value of (anchor + lead)
>> instead of just checking whether lead < size.
>>
>> Thanks,
>>
>> Jay
>>
>> *From:* Pankaj Bansal
>> *Sent:* Friday, December 01, 2017 3:02 PM
>> *To:* [hidden email]; Sergey Bylokhov; Semyon Sadetsky
>> *Subject:* <Swing Dev> [10] Review Request: JDK-7108280 :
>> JList.getSelectedValuesList fails if JList.setSelectionInterval larger than
>> list
>>
>> Hi All,
>>
>> Please review the fix.
>>
>> Bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-7108280
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~pbansal/7108280/webrev.00/
>>
>> Issue:
>>
>> JList.getSelectedValuesList crashes if the JList.setSelectionInterval or
>> JList.addSelectionInterval had been called earlier with interval having lead
>> greater than the size of List
>>
>> Fix:
>>
>> Made changes in JList.getSelectedValuesList to check the if the max
>> selection index is greater than the actual size of the List. If yes, the max
>> is changed to last element index of List.
>>
>> Note:
>>
>> It makes sense to change the behavior of JList.setSelectionInterval or
>> JList.addSelectionInterval to not allow to set the selection with interval
>> having indices not present in the list. But it will change the behavior of
>> this API and will result in failure of 2 JCK tests.
>>
>> Also, we will still have to put checks inside the
>> JList.getSelectedValuesList as the selection can be changed by setting
>> selection interval on DefualtListSelectionModel and there is no way to check
>> if the supplied interval range actually exist in the List inside
>> DefualtListSelectionModel.
>>
>> If changing the JList.setSelectionInterval or JList.addSelectionInterval
>> is possible, the potential fix can be following webrev.
>>
>> http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/
>>
>> Regards,
>>
>> Pankaj Bansal
>>
>
>
> --
> Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Pankaj Bansal
Hi All,

Thank you for your reviews.

I think we need to finalize on where to make changes, before going any further . We have following few options

1. Change setSelectionInterval and addSelectionInterval to throw Exception: Make change to these functions to throw IllegalArgumentException when the interval is out of range. This is looks correct as it does not make sense to set selection out of bounds of the list. This also makes the JList more compatible with the JTable. But this will break 2 jck tests which expect the out of bound selection to be allowed. Also this can break existing applications which are setting the wrong selection and expecting it not to throw an exception. Also someone can still set the wrong selection by setting the selection directly on SelectionModel instead of calling it on JList.

2. Change setSelectionInterval and addSelectionInterval to just return without exception: Make changes to these function to just return if the interval is out of bound. But this will also break the same 2 jck tests and may break existing applications.

3. Change the getSelectedValues and getSelectedValuesList: Make changes to these functions to check the selection and return the selected objects. If the selection is out of bound, return an empty List or partial List depending on the selection interval, instead of throwing the ArrayIndexOutOfBoundsException.

Option 1 stops someone from setting the wrong selection in the first place and find bugs in case someone tries to set wrong selection. But it may break existing application and can still cause ArrayIndexOutOfBoundsException is someone is setting wrong selection on SelectionModel instead of going through JList.
Option 3 does not seem to break any existing application and looks immune to ArrayIndexOutOfBoundsException in getMethods. So maybe this is correct way.


Regards,
Pankaj Bansal




-----Original Message-----
From: Andrej Golovnin [mailto:[hidden email]]
Sent: Wednesday, December 6, 2017 2:00 PM
To: Sergey Bylokhov
Cc: Jayathirth D V; Pankaj Bansal; [hidden email]; Semyon Sadetsky
Subject: Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Hi all,

as a long Swing user I would like to vote against the proposed changes. The fact is that the #setSelectionInterval and #addSelectionInterval methods of the JList class exist in this form for a very long time and any change in the behaviour of this methods may break existing applications.

Technically this methods should throw an IllegalArgumentException when the arguments are out of bounds. For example the #setRowSelectionInterval and #addRowSelectionInterval methods of the JTable class throw an IllegalArgumentException.

I think you should ask someone from the Java core team who has deeper understanding, when a change is backward compatible and when not, if it is OK to add a check to this methods and throw an IllegalArgumentException when the arguments are out of bounds. If it is not OK, then you can improve at least the JavaDocs of this methods and explain in the JavaDocs that the arguments must be in the bounds of the current ListModel.

I would also not change the behaviour of JList#getSelectedValuesList.
The current behaviour, i.g. throwing the ArrayIndexOutOfBoundsException, helps me to find bugs in applications.
For me setting a wrong selection interval is a bug.

Best regards,
Andrej Golovnin

On Tue, Dec 5, 2017 at 11:29 PM, Sergey Bylokhov <[hidden email]> wrote:

> Hello.
> On 01/12/2017 02:47, Jayathirth D V wrote:
>>
>> As you have mentioned I also feel that adding check in
>> setSelectionInterval() or addSelectionInterval() would be a good approach.
>> Since I am not aware of swing component code I will leave this
>> decision to others.
>
>
> I also have no preference where to change this. If we will change
> setSelectionInterval()/addSelectionInterval() then we will need to
> update selection model on every change of datamodel. But if we decide
> like in the current fix to change the get methods, then we will need
> to verify all places where we use datamodel and selection model:
> for example JList.getSelectedValue() and others.
> Also we should check other classes which use the same selection model
> like JTable.
>
>>
>> Regarding http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/:
>>
>> May be we are not handling the case where validateLeadIndex() fails
>> and we don’t set selection interval and it is resulting in JCK test
>> fail. If you can share what is behavior of JCK test failure after
>> your change it would be helpful.
>>
>>                  Also specification of setSelectionInterval() or
>> addSelectionInterval() mentions that “{@code anchor} doesn't have to
>> be less than or equal to {@code lead}”. So while validating arguments
>> for
>> setSelectionInterval() or addSelectionInterval() I think we should
>> verify the value of anchor first and then check the value of (anchor
>> + lead) instead of just checking whether lead < size.
>>
>> Thanks,
>>
>> Jay
>>
>> *From:* Pankaj Bansal
>> *Sent:* Friday, December 01, 2017 3:02 PM
>> *To:* [hidden email]; Sergey Bylokhov; Semyon Sadetsky
>> *Subject:* <Swing Dev> [10] Review Request: JDK-7108280 :
>> JList.getSelectedValuesList fails if JList.setSelectionInterval
>> larger than list
>>
>> Hi All,
>>
>> Please review the fix.
>>
>> Bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-7108280
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~pbansal/7108280/webrev.00/
>>
>> Issue:
>>
>> JList.getSelectedValuesList crashes if the JList.setSelectionInterval
>> or JList.addSelectionInterval had been called earlier with interval
>> having lead greater than the size of List
>>
>> Fix:
>>
>> Made changes in JList.getSelectedValuesList to check the if the max
>> selection index is greater than the actual size of the List. If yes,
>> the max is changed to last element index of List.
>>
>> Note:
>>
>> It makes sense to change the behavior of JList.setSelectionInterval
>> or JList.addSelectionInterval to not allow to set the selection with
>> interval having indices not present in the list. But it will change
>> the behavior of this API and will result in failure of 2 JCK tests.
>>
>> Also, we will still have to put checks inside the
>> JList.getSelectedValuesList as the selection can be changed by
>> setting selection interval on DefualtListSelectionModel and there is
>> no way to check if the supplied interval range actually exist in the
>> List inside DefualtListSelectionModel.
>>
>> If changing the JList.setSelectionInterval or
>> JList.addSelectionInterval is possible, the potential fix can be following webrev.
>>
>> http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/
>>
>> Regards,
>>
>> Pankaj Bansal
>>
>
>
> --
> Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list

Andrej Golovnin-2
Hi all,

there is one more option:

4. Close the issue as "won't fix" and suggest the reporter to fix his code.

Option 3 may not break any existing application. But it may hide bugs
in applications because in my opinion setting a wrong selection
interval is a bug.

Just my 2 cents.

Best regards,
Andrej Golovnin

On Fri, Dec 8, 2017 at 8:13 AM, Pankaj Bansal
<[hidden email]> wrote:

> Hi All,
>
> Thank you for your reviews.
>
> I think we need to finalize on where to make changes, before going any further . We have following few options
>
> 1. Change setSelectionInterval and addSelectionInterval to throw Exception: Make change to these functions to throw IllegalArgumentException when the interval is out of range. This is looks correct as it does not make sense to set selection out of bounds of the list. This also makes the JList more compatible with the JTable. But this will break 2 jck tests which expect the out of bound selection to be allowed. Also this can break existing applications which are setting the wrong selection and expecting it not to throw an exception. Also someone can still set the wrong selection by setting the selection directly on SelectionModel instead of calling it on JList.
>
> 2. Change setSelectionInterval and addSelectionInterval to just return without exception: Make changes to these function to just return if the interval is out of bound. But this will also break the same 2 jck tests and may break existing applications.
>
> 3. Change the getSelectedValues and getSelectedValuesList: Make changes to these functions to check the selection and return the selected objects. If the selection is out of bound, return an empty List or partial List depending on the selection interval, instead of throwing the ArrayIndexOutOfBoundsException.
>
> Option 1 stops someone from setting the wrong selection in the first place and find bugs in case someone tries to set wrong selection. But it may break existing application and can still cause ArrayIndexOutOfBoundsException is someone is setting wrong selection on SelectionModel instead of going through JList.
> Option 3 does not seem to break any existing application and looks immune to ArrayIndexOutOfBoundsException in getMethods. So maybe this is correct way.
>
>
> Regards,
> Pankaj Bansal
>
>
>
>
> -----Original Message-----
> From: Andrej Golovnin [mailto:[hidden email]]
> Sent: Wednesday, December 6, 2017 2:00 PM
> To: Sergey Bylokhov
> Cc: Jayathirth D V; Pankaj Bansal; [hidden email]; Semyon Sadetsky
> Subject: Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list
>
> Hi all,
>
> as a long Swing user I would like to vote against the proposed changes. The fact is that the #setSelectionInterval and #addSelectionInterval methods of the JList class exist in this form for a very long time and any change in the behaviour of this methods may break existing applications.
>
> Technically this methods should throw an IllegalArgumentException when the arguments are out of bounds. For example the #setRowSelectionInterval and #addRowSelectionInterval methods of the JTable class throw an IllegalArgumentException.
>
> I think you should ask someone from the Java core team who has deeper understanding, when a change is backward compatible and when not, if it is OK to add a check to this methods and throw an IllegalArgumentException when the arguments are out of bounds. If it is not OK, then you can improve at least the JavaDocs of this methods and explain in the JavaDocs that the arguments must be in the bounds of the current ListModel.
>
> I would also not change the behaviour of JList#getSelectedValuesList.
> The current behaviour, i.g. throwing the ArrayIndexOutOfBoundsException, helps me to find bugs in applications.
> For me setting a wrong selection interval is a bug.
>
> Best regards,
> Andrej Golovnin
>
> On Tue, Dec 5, 2017 at 11:29 PM, Sergey Bylokhov <[hidden email]> wrote:
>> Hello.
>> On 01/12/2017 02:47, Jayathirth D V wrote:
>>>
>>> As you have mentioned I also feel that adding check in
>>> setSelectionInterval() or addSelectionInterval() would be a good approach.
>>> Since I am not aware of swing component code I will leave this
>>> decision to others.
>>
>>
>> I also have no preference where to change this. If we will change
>> setSelectionInterval()/addSelectionInterval() then we will need to
>> update selection model on every change of datamodel. But if we decide
>> like in the current fix to change the get methods, then we will need
>> to verify all places where we use datamodel and selection model:
>> for example JList.getSelectedValue() and others.
>> Also we should check other classes which use the same selection model
>> like JTable.
>>
>>>
>>> Regarding http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/:
>>>
>>> May be we are not handling the case where validateLeadIndex() fails
>>> and we don’t set selection interval and it is resulting in JCK test
>>> fail. If you can share what is behavior of JCK test failure after
>>> your change it would be helpful.
>>>
>>>                  Also specification of setSelectionInterval() or
>>> addSelectionInterval() mentions that “{@code anchor} doesn't have to
>>> be less than or equal to {@code lead}”. So while validating arguments
>>> for
>>> setSelectionInterval() or addSelectionInterval() I think we should
>>> verify the value of anchor first and then check the value of (anchor
>>> + lead) instead of just checking whether lead < size.
>>>
>>> Thanks,
>>>
>>> Jay
>>>
>>> *From:* Pankaj Bansal
>>> *Sent:* Friday, December 01, 2017 3:02 PM
>>> *To:* [hidden email]; Sergey Bylokhov; Semyon Sadetsky
>>> *Subject:* <Swing Dev> [10] Review Request: JDK-7108280 :
>>> JList.getSelectedValuesList fails if JList.setSelectionInterval
>>> larger than list
>>>
>>> Hi All,
>>>
>>> Please review the fix.
>>>
>>> Bug:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-7108280
>>>
>>> Webrev:
>>>
>>> http://cr.openjdk.java.net/~pbansal/7108280/webrev.00/
>>>
>>> Issue:
>>>
>>> JList.getSelectedValuesList crashes if the JList.setSelectionInterval
>>> or JList.addSelectionInterval had been called earlier with interval
>>> having lead greater than the size of List
>>>
>>> Fix:
>>>
>>> Made changes in JList.getSelectedValuesList to check the if the max
>>> selection index is greater than the actual size of the List. If yes,
>>> the max is changed to last element index of List.
>>>
>>> Note:
>>>
>>> It makes sense to change the behavior of JList.setSelectionInterval
>>> or JList.addSelectionInterval to not allow to set the selection with
>>> interval having indices not present in the list. But it will change
>>> the behavior of this API and will result in failure of 2 JCK tests.
>>>
>>> Also, we will still have to put checks inside the
>>> JList.getSelectedValuesList as the selection can be changed by
>>> setting selection interval on DefualtListSelectionModel and there is
>>> no way to check if the supplied interval range actually exist in the
>>> List inside DefualtListSelectionModel.
>>>
>>> If changing the JList.setSelectionInterval or
>>> JList.addSelectionInterval is possible, the potential fix can be following webrev.
>>>
>>> http://cr.openjdk.java.net/~pbansal/7108280/webrev.002/
>>>
>>> Regards,
>>>
>>> Pankaj Bansal
>>>
>>
>>
>> --
>> Best regards, Sergey.