On Tue, 21 Nov 2023 12:06:41 GMT, Tejesh R <t...@openjdk.org> wrote:
>> Table contents does not follow right-left Orientation when Max width of
>> columns are set. This is due to not considering the offset in `x position`
>> while painting table grid and table cell. The fix handles the offset and
>> adjust the x position for each paint, similar to how header is painted. The
>> fix is applied to both Basic and Synth Look and Feel.
>> The fix is verified for all Look and Feel manually and test verifies on
>> Metal L&F since automatic test cannot be generalized throughout other Look
>> and Feel.
>> CI tested is green for regression check and test check.
>
> Tejesh R has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review fix
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java line
2031:
> 2029: damagedArea.x = SwingUtilities2.getXPosInRightToLeft(table,
> cMin);
> 2030: } else {
> 2031: damagedArea.x = SwingUtilities2.getXPosInRightToLeft(table,
> cMax);
The method name `getXPosInRightToLeft` is confusing when you call it for the
*left-to-right case*…
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java line
2031:
> 2029: damagedArea.x = SwingUtilities2.getXPosInRightToLeft(table,
> cMin);
> 2030: } else {
> 2031: damagedArea.x = SwingUtilities2.getXPosInRightToLeft(table,
> cMax);
Does it mean that `table.getCellRect` returns an incorrect value in
right-to-left case?
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java line
2088:
> 2086: cellRect = table.getCellRect(row, cMin, false);
> 2087: for (int column = cMin; column <= cMax; column++) {
> 2088: aColumn = cm.getColumn(column);
The bodies of the column loops are absolutely the same now, it asks for
refactoring… Yet it could be an overkill.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java line
2100:
> 2098: for (int row = rMin; row <= rMax; row++) {
> 2099: cellRect = table.getCellRect(row, cMax, false);
> 2100: cellRect.x =
> SwingUtilities2.getXPosInRightToLeft(table, cMax);
Here, I have the same question: _Does it mean that `table.getCellRect` returns
an incorrect value in right-to-left case?_
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java line
2108:
> 2106: paintCell(g, cellRect, row, column);
> 2107: }
> 2108: cellRect.x += columnWidth;
This is weird… if we paint columns in right-to-left order, x should decrease.
src/java.desktop/share/classes/sun/swing/SwingUtilities2.java line 2371:
> 2369: return table.getWidth();
> 2370: }
> 2371: return table.getParent().getWidth();
The condition `table != null` seem redundant — if it's `null`, the expression
`table.getParent()` in the return statement throws NPE.
test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 98:
> 96: table.table.getLocationOnScreen().y,
> 97: table.table.getWidth() -
> allColumnWidths,
> 98: table.table.getHeight()));
This is the reason why I dislike the model design: `table.table` doesn't look
good and is somewhat confusing. Moreover, `CustomTable` is not really _a table_
which makes the design even more confusing: a table contains a table.
I understand over-engineering a test is a waste of time, yet it feels too wrong
this way. A clean test code will ease the work of another engineer who will
need to deal with updating the test if anything changes.
test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 161:
> 159: "Salary",
> 160: };
> 161: List<CustomTable.Record> data = new ArrayList();
Suggestion:
List<CustomTable.Record> data = new ArrayList<>();
The right side still uses raw type.
test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 184:
> 182: this.salary = salary;
> 183: }
> 184: }
What I meant is using `record` instead of a *`class`*:
Suggestion:
record Data(String firstName, String lastName, float salary) {}
test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 204:
> 202: return item.lastname;
> 203: case COL_SALARY:
> 204: return Float.valueOf(item.salary);
Suggestion:
return item.salary;
Explicit boxing is redundant.
-------------
PR Review: https://git.openjdk.org/jdk/pull/16374#pullrequestreview-1742186893
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400701062
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400704002
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400717209
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400704758
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400721169
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400744578
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400782701
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400784213
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400786843
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400788820
