<Swing Dev> [11] Review Request: JDK-6481195 ListSelectionListener indicates events on model.addElement after model.clear()

[Sergey Bylokhov]

+1

On 30/01/2018 01:21, Jayathirth D V wrote:
> Hi Pankaj,
> 
> Changes are fine.
> 
> Thanks,
> 
> Jay
> 
> *From:* Pankaj Bansal
> *Sent:* Tuesday, January 30, 2018 2:43 PM
> *To:* Jayathirth D V; 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,
> 
> I have made the small correction you suggested.
> 
> webrev: 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; 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/
> 
> 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/
> 
> 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
> 


-- 
Best regards, Sergey.