Hi Semyon, <<what will be the return from JList.getSelectedIndices() in the issue scenario? I have not made any changes to JList.getSelectedIndices method. It will return the selected indices in selection model.
Regards, Pankaj Bansal -----Original Message----- From: Semyon Sadetsky Sent: Friday, March 9, 2018 7:17 AM To: Pankaj Bansal; swing-dev@openjdk.java.net Subject: Re: <Swing Dev> [10] Review Request: JDK-7108280 : JList.getSelectedValuesList fails if JList.setSelectionInterval larger than list Hi Pankaj, what will be the return from JList.getSelectedIndices() in the issue scenario? --Semyon On 01/03/2018 03:53 AM, 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.
