RFR: 5108458: JTable does not properly layout its content [v6]

[Alexey Ivanov]

On Tue, 21 Nov 2023 12:06:41 GMT, Tejesh R <tr at 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