<Swing Dev> [11] Review Request: JDK-7007967 DefaultRowSorter: incorrect sorting due to not updating comparator cache
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Tue Jan 30 23:25:29 UTC 2018
+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.
More information about the swing-dev
mailing list