<Swing Dev> [11] Review Request: JDK-7007967 DefaultRowSorter: incorrect sorting due to not updating comparator cache

[Sergey Bylokhov]

+1

On 30/01/2018 01:04, Jayathirth D V wrote:
> Hi Pankaj,
> 
> Changes are fine.
> 
> Thanks,
> 
> Jay
> 
> *From:* Pankaj Bansal
> *Sent:* Tuesday, January 30, 2018 2:12 PM
> *To:* Jayathirth D V; swing-dev at openjdk.java.net; Sergey Bylokhov; 
> Semyon Sadetsky
> *Subject:* RE: <Swing Dev> [11] Review Request: JDK-7007967 
> DefaultRowSorter: incorrect sorting due to not updating comparator cache
> 
> Hi Jay,
> 
> Thanks for the review.
> 
> I have incorporated the suggested changes.
> 
> webrev: http://cr.openjdk.java.net/~pbansal/7007967/webrev.02/
> 
> Regards,
> 
> Pankaj Bansal
> 
> *From:* Jayathirth D V
> *Sent:* Tuesday, January 30, 2018 12:04 PM
> *To:* Pankaj Bansal; swing-dev at openjdk.java.net 
> <mailto:swing-dev at openjdk.java.net>; Sergey Bylokhov; Semyon Sadetsky
> *Subject:* RE: <Swing Dev> [11] Review Request: JDK-7007967 
> DefaultRowSorter: incorrect sorting due to not updating comparator cache
> 
> Hi Pankaj,
> 
> It’s better to make a multiline condition by dividing different 
> conditions instead of making new line within a condition:
> 
> 1036         if (!sorted || viewToModel.length == 0 || (lastRow - 
> firstRow) >
> 
> 1037                 viewToModel.length / 10) {
> 
> to
> 
> 1036         if (!sorted || viewToModel.length == 0 ||
> 
> 1037              (lastRow - firstRow) > viewToModel.length / 10) {
> 
> In test case if need to throw multiple RuntimeException please make sure 
> that there is difference between them instead of throwing same message 
> “Wrong Sorting”. We can use “Wrong sorting before …” and “Wrong sorting 
> after …”. Also in jtreg comments section maintain the indentation of 
> multiline comment.
> 
> Thanks,
> 
> Jay
> 
> *From:* Pankaj Bansal
> *Sent:* Monday, January 29, 2018 2:31 PM
> *To:* Jayathirth D V; swing-dev at openjdk.java.net 
> <mailto:swing-dev at openjdk.java.net>; Sergey Bylokhov; Semyon Sadetsky
> *Subject:* RE: <Swing Dev> [11] Review Request: JDK-7007967 
> DefaultRowSorter: incorrect sorting due to not updating comparator cache
> 
> Hello Jay,
> 
> I have made suggested changes.
> 
> webrev: http://cr.openjdk.java.net/~pbansal/7007967/webrev.01/
> 
> << Test case is working fine before and after the change.
> 
> <<I think there is no need for us to verify row index before we delete 
> and update the <<contents again.
> 
>   << 65         for (int row = 0; row < model.getRowCount(); row++) {
> 
>   << 66             // values are in sorted ascending. so the row index and
> 
>   << 67             // view index from sorter should be same
> 
>   << 68             if (row != rowSorter.convertRowIndexToView(row)) {
> 
>   << 69                 throw new RuntimeException("Wrong sorting");
> 
>   << 70             }
> 
>   << 71         }
> 
> <<We can remove the above code in the test case because we never hit it 
> and throw RuntimeException.
> 
> This is required to make sure that the row sorted is in proper condition 
> and the test passes before making changes. This makes sure that our 
> changes are not causing the error if there are any.
> 
> Regards,
> 
> Pankaj Bansal
> 
> *From:* Jayathirth D V
> *Sent:* Thursday, January 25, 2018 8:32 PM
> *To:* Pankaj Bansal; swing-dev at openjdk.java.net 
> <mailto:swing-dev at openjdk.java.net>; Sergey Bylokhov; Semyon Sadetsky
> *Subject:* RE: <Swing Dev> [11] Review Request: JDK-7007967 
> DefaultRowSorter: incorrect sorting due to not updating comparator cache
> 
> Hello Pankaj,
> 
> Please find my inputs:
> 
> At line 1036 number of characters per line is more than 80, we can 
> divide the conditions into multiple lines.
> 
> 1036         if (!sorted || viewToModel.length == 0 || (lastRow - 
> firstRow) > viewToModel.length / 10) {
> 
> 1037             // We either weren't sorted, or to much changed, sort 
> it all or
> 
> 1038             // this is the first row added and we have to update 
> different
> 
> 1039             // caches
> 
> Test case is working fine before and after the change.
> 
> I think there is no need for us to verify row index before we delete and 
> update the contents again.
> 
>    65         for (int row = 0; row < model.getRowCount(); row++) {
> 
>    66             // values are in sorted ascending. so the row index and
> 
>    67             // view index from sorter should be same
> 
>    68             if (row != rowSorter.convertRowIndexToView(row)) {
> 
>    69                 throw new RuntimeException("Wrong sorting");
> 
>    70             }
> 
>    71         }
> 
> We can remove the above code in the test case because we never hit it 
> and throw RuntimeException.
> 
> Thanks,
> 
> Jay
> 
> *From:* Pankaj Bansal
> *Sent:* Wednesday, January 17, 2018 5:24 PM
> *To:* swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>; 
> Sergey Bylokhov; Semyon Sadetsky
> *Subject:* <Swing Dev> [11] Review Request: JDK-7007967 
> DefaultRowSorter: incorrect sorting due to not updating comparator cache
> 
> Hi All,
> 
> Please review the fix for JDK 11.
> 
> Bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-7007967
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~pbansal/7007967/webrev.00/
> 
> Issue:
> 
> The caching flags like useToString, comparators etc are not always 
> updated in shouldOptimizeChange function. It can cause wrong cache to be 
> used in some cases and produce wrong sorting results.
> 
> For example, in the submitted test case, the integer data is added and 
> sorted. This updates the useToString etc cache flags according to the 
> data type provided.  //Sorts 1,2,11 as 1->2->11 (correct).
> 
> Then all the data is removed from the model and when the sorter is 
> indicated of this, the cache flags get updated accordingly. Because of 
> this, the useToString is set to true for the column and comparator is 
> set to String Comparator and sorted flag is still true.
> 
> Now if Integer data rows are again added one by one, these flags are not 
> updated and it will use the wrong caches of useToString, comparators etc 
> and produce wrong sorting results. //Sorts 1,2,11 as 1->11->2 
> (Incorrect. The data is sorted according to String representation).
> 
> Fix:
> 
> There can be different approaches to solve this issue. In the current 
> approach, changes have been done in shouldOptimizeChange function to 
> call the sort function when first row is added to the model. This will 
> ensure that the cache flags like useToString, comparators are updated 
> according to the data added for the column.
> 
> Regards,
> 
> Pankaj Bansal
> 


-- 
Best regards, Sergey.