+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@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@openjdk.java.net
<mailto:swing-dev@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@openjdk.java.net
<mailto:swing-dev@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@openjdk.java.net
<mailto:swing-dev@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@openjdk.java.net <mailto:swing-dev@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.
