<Swing Dev> [11] Review Request: JDK-6481195 ListSelectionListener indicates events on model.addElement after model.clear()
Semyon Sadetsky
semyon.sadetsky at oracle.com
Thu Feb 1 16:20:52 UTC 2018
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 at 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 at openjdk.java.net
> <mailto:swing-dev at 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 at openjdk.java.net
> <mailto:swing-dev at 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 at openjdk.java.net
> <mailto:swing-dev at 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 at openjdk.java.net
> <mailto:swing-dev at 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 at openjdk.java.net
> <mailto:swing-dev at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180201/8d4e4bf6/attachment.html>
More information about the swing-dev
mailing list