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; HYPERLINK
"mailto:swing-dev@openjdk.java.net"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 3rd 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; HYPERLINK
"mailto:swing-dev@openjdk.java.net"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: HYPERLINK
"http://cr.openjdk.java.net/%7Epbansal/6481195/webrev.02/"http://cr.openjdk.java.net/~pbansal/6481195/webrev.02/
Regards,
Pankaj Bansal
From: Jayathirth D V
Sent: Tuesday, January 30, 2018 12:13 PM
To: Pankaj Bansal; HYPERLINK
"mailto:swing-dev@openjdk.java.net"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; HYPERLINK
"mailto:swing-dev@openjdk.java.net"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: HYPERLINK
"http://cr.openjdk.java.net/%7Epbansal/6481195/webrev.01/"http://cr.openjdk.java.net/~pbansal/6481195/webrev.01/
Regards,
Pankaj Bansal
From: Jayathirth D V
Sent: Thursday, January 25, 2018 6:28 PM
To: Pankaj Bansal; HYPERLINK
"mailto:swing-dev@openjdk.java.net"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: HYPERLINK "mailto:swing-dev@openjdk.java.net"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:
HYPERLINK
"http://cr.openjdk.java.net/%7Epbansal/6481195/webrev.00/"http://cr.openjdk.java.net/~pbansal/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-4334792 partially when
removeAllElements or clear() is called on DefaultListModel.
Regards
Pankaj Bansal
