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
*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-4334792 partially
when removeAllElements or clear() is called on DefaultListModel.
Regards
Pankaj Bansal
