Hi Pankaj,

I think it would be wrong to fix an issue by adding yet another case that contradicts the specs.

In the most usages the clearSelection() call is followed by setAnchorSelectionIndex(-1)/setLeadSelectionIndex(-1). I think that would be more safe way to fix the issue by adding those calls into the clearSelection(). And you will need to cleanup the usages.

Also, please file the issue about removal of anchor/selection with index !=0.

--Semyon


On 02/01/2018 11:25 PM, Pankaj Bansal wrote:

Hi Semyon,

<<I'm not sure that the spec should point to specific numbers.
<<Ok, I give you an example for clarification
<<list.setSelectedIndices(new int[] {1, 0}); // selection is [0 ,1] lead/anchor = 0/0
<<model.removeElementAt(0); // selection is [0] lead/anchor = 0/0
<<after you fix: lead/anchor = -1/-1.
<<the specs say
 <<    * Return the first/second index argument from the most recent call to  <<    * setSelectionInterval(), addSelectionInterval() or removeSelectionInterval().

Ok, I got your point. In the example you gave, as addSelectionInterval was called from setSelectedIndices with 0 at last, the lead/anchor should be 0/0 not -1/-1.

But what I am saying is, this is true for all the indices not just 0. For all other indices, the anchor/lead is incorrect. It is made correct for just 0 index by handling it specially in code.

Consider the same example you gave after adding 1 in every index.

list.setSelectedIndices(new int[] {2, 1}); // selection is [1 ,2] lead/anchor = 1/1 model.removeElementAt(1); // selection is [1] lead/anchor = 0/0  (before the fix. I have verified it)

If we go by the API description, even this is wrong as anchor/lead should be 1/1 and it will be wrong for all the indices except 0 as they have specially handled 0 to be correct. I have just made it incorrect for index 0 also. I think this is bug which needs to be addressed separately independent of the index.

Again, please let me know if I am missing something here.

Regards,

Pankaj Bansal

*From:*Semyon Sadetsky
*Sent:* Thursday, February 1, 2018 9:51 PM
*To:* Pankaj Bansal; swing-dev@openjdk.java.net
*Subject:* Re: <Swing Dev> [11] Review Request: JDK-6481195 ListSelectionListener indicates events on model.addElement after model.clear()

On 01/31/2018 11:04 AM, Pankaj Bansal wrote:

    Hi Semyon,

    Thanks for the review.

    << This is directly specified in getLeadSelectionIndex()/
    getAnchorSelectionIndex().

    Sorry but I am not able to find anything specific to handling of
    case of 0th item being deleted when first row is selected or
    anything which states that anchor or lead can’t be -1. I guess I
    may be missing something here or not able to infer what you are
    saying. Can you please point out which specific statement
    describes the behavior you suggested.

I'm not sure that the spec should point to specific numbers.

Ok, I give you an example for clarification

list.setSelectedIndices(new int[] {1, 0}); // selection is [0 ,1] lead/anchor = 0/0
model.removeElementAt(0); // selection is [0] lead/anchor = 0/0

after you fix: lead/anchor = -1/-1.

the specs say

     * Return the first/second index argument from the most recent call to
     * setSelectionInterval(), addSelectionInterval() or removeSelectionInterval().


    <<Anchor and lead indexes are used in lot of places. Which tests
    did you run after the fix?

    I have run following tests. I have run most of them(closed/jck) on
    Windows, but I think if this change is to affect something, it
    would not be platform specific. All the following runs have passed
    successfully (no new failures/errors).

    All automated tests  in: Open/test/jdk/javax/swing/JList,
     /JTable, /JTableHeader, /JTree

    All automated tests  in: Closed/test/jdk/javax/swing/JList,
    /Jtable, /JTableeHeader, /JTree, /DefaultListSelectionModel,
    /DefaultListModel, /DefaultListCellRenderer

    All tests  in: jck/swing_javax/JList, /JTable, /Tree,
    /DefaultListSelectionModel, /DefaultSingleSelectionModel ,
    /DefaultListModel, /DefaultListCellRenderer

    Few demos like SwingSet2, Java2D

    Please let me know if you think I should run some more tests.

    Regards,

    Pankaj Bansal

    *From:*Semyon Sadetsky
    *Sent:* Wednesday, January 31, 2018 9:49 PM
    *To:* Pankaj Bansal; swing-dev@openjdk.java.net
    <mailto:swing-dev@openjdk.java.net>
    *Subject:* Re: <Swing Dev> [11] Review Request: JDK-6481195
    ListSelectionListener indicates events on model.addElement after
    model.clear()

    Hi Pankaj,

    This is directly specified in getLeadSelectionIndex()/
    getAnchorSelectionIndex().

    Anchor and lead indexes are used in lot of places. Which tests did
    you run after the fix?

    --Semyon

    On 01/31/2018 12:38 AM, Pankaj Bansal wrote:

        Hi Semyon,

        << When only the first list element deleted the anchor and
        lead selection indexes become -1 after your fix while should be 0.

        Is it mentioned somewhere that it should be 0 ? I mean if lead
        index was 2 and only 3^rd element is deleted, lead becomes 1.
        Same should be true when lead is 0 and first element is
        deleted. Is it rule that lead or anchor can’t go beyond 0
        considering their default value is -1? I think there is a
        similar bug filled as why should not lead or anchor become -1.

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

        Regards,

        Pankaj Bansal

        *From:*Semyon Sadetsky
        *Sent:* Wednesday, January 31, 2018 12:17 PM
        *To:* Pankaj Bansal; Jayathirth D V;
        swing-dev@openjdk.java.net
        <mailto:swing-dev@openjdk.java.net>; Sergey Bylokhov; Prasanta
        Sadhukhan
        *Subject:* Re: <Swing Dev> [11] Review Request: JDK-6481195
        ListSelectionListener indicates events on model.addElement
        after model.clear()

        Hi Pankaj,

        When only the first list element deleted the anchor and lead
        selection indexes become -1 after your fix while should be 0.

        --Semyon

        On 01/30/2018 01:13 AM, Pankaj Bansal wrote:

            Hi Jay,

            I have made the small correction you suggested.

            webrev:
            http://cr.openjdk.java.net/~pbansal/6481195/webrev.02/
            <http://cr.openjdk.java.net/%7Epbansal/6481195/webrev.02/>

            Regards,

            Pankaj Bansal

            *From:* Jayathirth D V
            *Sent:* Tuesday, January 30, 2018 12:13 PM
            *To:* Pankaj Bansal; swing-dev@openjdk.java.net
            <mailto:swing-dev@openjdk.java.net>; Sergey Bylokhov;
            Semyon Sadetsky; Prasanta Sadhukhan
            *Subject:* RE: <Swing Dev> [11] Review Request:
            JDK-6481195 ListSelectionListener indicates events on
            model.addElement after model.clear()

            Hi Pankaj,

            Changes are fine.

            As mentioned in previous suggestion Java code convention
            for different statements like if () and for ()
            
loops(http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html)
            we have to maintain single space between if/for and
            braces. In test case this needs to be updated. There is no
            need for another webrev please update it before you push
            the change.

            Thanks,

            Jay

            *From:* Pankaj Bansal
            *Sent:* Monday, January 29, 2018 3:39 PM
            *To:* Jayathirth D V; swing-dev@openjdk.java.net
            <mailto:swing-dev@openjdk.java.net>; Sergey Bylokhov;
            Semyon Sadetsky; Prasanta Sadhukhan
            *Subject:* RE: <Swing Dev> [11] Review Request:
            JDK-6481195 ListSelectionListener indicates events on
            model.addElement after model.clear()

            Hi Jay,

            Thanks for the review.

            I have made the suggested changes.

            webrev:
            http://cr.openjdk.java.net/~pbansal/6481195/webrev.01/
            <http://cr.openjdk.java.net/%7Epbansal/6481195/webrev.01/>

            Regards,

            Pankaj Bansal

            *From:* Jayathirth D V
            *Sent:* Thursday, January 25, 2018 6:28 PM
            *To:* Pankaj Bansal; swing-dev@openjdk.java.net
            <mailto:swing-dev@openjdk.java.net>; Sergey Bylokhov;
            Semyon Sadetsky; Prasanta Sadhukhan
            *Subject:* RE: <Swing Dev> [11] Review Request:
            JDK-6481195 ListSelectionListener indicates events on
            model.addElement after model.clear()

            Hi Pankaj,

            Please find my inputs:

            Test case is working properly before and after changes.

            But I think we should make changes in test to actually
            detect all failure cases properly instead of throwing
            RuntimeException when we hit first failure:

            if (numberOfEvents > 2) {

            throw new RuntimeException("Wrong number of Events. " +

            "Expected: 2, Actual: " + numberOfEvents);

            }

            if (list.getLeadSelectionIndex() != -1) {

            throw new RuntimeException("Wrong Lead Index. " +

            "Expected: -1, Actual: " + list

              .getLeadSelectionIndex());

            }

            if (list.getAnchorSelectionIndex() != -1) {

            throw new RuntimeException("Wrong Anchor Index. " +

            "Expected: -1, Actual: " + list

                            .getAnchorSelectionIndex());

            }

            Before your code change we just hit first check throw
            exception which will not cover all the required
            conditions(proper number of events, proper lead selection
            index & proper anchor selection index).

            We can update a boolean variable in each of 3 cases and
            then check them and throw relevant RuntimeException.

            Also in test case there are some indentation problems in
            for loop, if condition and expressions please update them
            also.

            Thanks,

            Jay

            *From:* Pankaj Bansal
            *Sent:* Monday, January 22, 2018 6:43 PM
            *To:* swing-dev@openjdk.java.net
            <mailto:swing-dev@openjdk.java.net>; Sergey Bylokhov;
            Semyon Sadetsky; Prasanta Sadhukhan
            *Subject:* <Swing Dev> [11] Review Request: JDK-6481195
            ListSelectionListener indicates events on model.addElement
            after model.clear()

            Hi All,

            Please review the fix for JDK 11.

            Bug:

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

            Webrev:

            http://cr.openjdk.java.net/~pbansal/6481195/webrev.00/
            <http://cr.openjdk.java.net/%7Epbansal/6481195/webrev.00/>

            Issue:

            Invalid ListSelectionEvents are being fired, when the data
            is added after the clear() function has been called on
            DefaultListModel. This only happens when the index 0 was
            selected before calling clear.

            When the index 0 is selected on a JList and clear()
            function is called, anchor and lead are not being updated
            properly in DefaultSelectionModel as they are not being
            reset to -1 inside the removeIndexInterval() function.
            Because of this, when new elements are added in model, a
            ListSelectionEvent is fired from insertIndexInterval()
            function.

            Fix:

            The code inside removeIndexInterval function handles the
            case when 0 index is selected in special way. Removed that
            piece of code.

            Note:

            This also fixes
            https://bugs.openjdk.java.net/browse/JDK-4334792partially
            when removeAllElements or clear() is called on
            DefaultListModel.

            Regards

            Pankaj Bansal


Reply via email to