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.
