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

[Andrej Golovnin]

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
<Sergey.Bylokhov at oracle.com> 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:* swing-dev at openjdk.java.net; 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.