Looks fine to me.
On 03/01/2018 03:53, Pankaj Bansal wrote:
Hi Sergey,
I have made the changes you suggested for this bug. Please have a look.
Webrev: http://cr.openjdk.java.net/~pbansal/7108280/webrev.01/
I checked classes like JList, JTable which use DefaultListSelectionModel to
find the methods where DataModel and ListSelectionModel are used together.
In JList, getSelectedValues, getSelectedValuesList and getSelectedValue are
three APIs. I have made changes to these APIs.
In JTable, I could not find any API which uses both DataModel and
ListSelectionModel together.
Hi Andrej,
We cannot change the set methods (setSelectionInterval and
addSelectionInterval) here as they will break backward compatibility. Someone
can also set a custom ListSelectionModel which may cause exception in
get(getSelectedValues, getSelectedValuesList and getSelectedValue) methods. So
we will have to make changes to get methods only. In JTable, these exceptions
don’t because of the above mentioned reasons. But these exceptions can happen
in JList and we can't leave the code unchanged.
Regards,
Pankaj Bansal
-----Original Message-----
From: Andrej Golovnin [mailto:andrej.golov...@gmail.com]
Sent: Friday, December 8, 2017 1:10 PM
To: Pankaj Bansal
Cc: Sergey Bylokhov; Jayathirth D V; swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> [10] Review Request: JDK-7108280 :
JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list
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 <pankaj.b.ban...@oracle.com>
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:andrej.golov...@gmail.com]
Sent: Wednesday, December 6, 2017 2:00 PM
To: Sergey Bylokhov
Cc: Jayathirth D V; Pankaj Bansal; swing-dev@openjdk.java.net; 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 <sergey.bylok...@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@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.
--
Best regards, Sergey.
