RFR: 5108458: JTable does not properly layout its content [v11]
Alexey Ivanov
aivanov at openjdk.org
Thu Nov 30 12:44:24 UTC 2023
On Thu, 30 Nov 2023 10:03:47 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/JTable.java line 3013:
> 3011: }
> 3012: //x position is updated to handled the offset to which the table
> 3013: //has to be moved towards right side in right-to-left Orientation.
Suggestion:
// x position is updated to take into account the offset to
// which the table column is moved in the right-to-left orientation.
I still don't like any variant because it doesn't clarify the main point: _the columns are laid out starting from the right side_.
src/java.desktop/share/classes/javax/swing/JTable.java line 3017:
> 3015: r.x = getWidthInRightToLeft() - r.x - cm.getColumn(column).getWidth();
> 3016: }
> 3017: r.width = cm.getColumn(column).getWidth();
Suggestion:
final int columnWidth = cm.getColumn(column).getWidth();
if( !getComponentOrientation().isLeftToRight() ) {
r.x = getWidthInRightToLeft() - r.x - columnWidth;
}
r.width = columnWidth;
Just to avoid getting the same value twice.
src/java.desktop/share/classes/javax/swing/JTable.java line 9809:
> 9807: }
> 9808: return super.getWidth();
> 9809: }
Can you elaborate on why it's needed? Presumably both versions return the same width because `JTable` doesn't override `getWidth`.
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 48:
> 46: import java.io.IOException;
> 47: import java.util.ArrayList;
> 48: import java.util.List;
I wonder why these imports are below `javax.*`, it is very uncommon.
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 55:
> 53: * @bug 5108458
> 54: * @summary Test to check Right alignment of JTable data
> 55: * (Fix affects all L&F, test verifies only Metal L&F)
Suggestion:
This line doesn't apply any longer.
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 73:
> 71: try {
> 72: SwingUtilities.invokeAndWait(() -> {
> 73: frame = new JFrame("Test JTable");
Be more specific:
Suggestion:
frame = new JFrame("JTable RTL column layout");
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 97:
> 95: }
> 96: tableLocation = customTableObj.table.getLocationOnScreen();
> 97: tableSize = customTableObj.table.getSize();
Perform all the calculation here and publish the object with the result:
Suggestion:
int allColumnWidths = 0;
for (int i = 0; i < customTableObj.table.getColumnCount(); i++) {
allColumnWidths += customTableObj.table.getTableHeader().getColumnModel()
.getColumn(i)
.getWidth();
}
Point tableLocation = customTableObj.table.getLocationOnScreen();
Dimension tableSize = customTableObj.table.getSize();
tableSize.width -= allColumnWidths;
tableBounds = new Rectangle(tableLocation, tableSize);
where `tableBounds` is a `static volatile` field of type `Rectangle` that needs to be declared instead of the three fields `allColumnWidths`, `tableLocation`, `tableSize` which has now become local variables.
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 104:
> 102: tableLocation.y,
> 103: (int)tableSize.getWidth() - allColumnWidths,
> 104: (int)tableSize.getHeight()));
Suggestion:
BufferedImage bufferedImage = robot.createScreenCapture(tableBounds);
This is much cleaner, isn't it?
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 130:
> 128: private static void saveImage(BufferedImage image, String fileName) {
> 129: try {
> 130: ImageIO.write(image, "png", new File(fileName));
Suggestion:
ImageIO.write(image, "png", new File("failureImage.png"));
Perhaps, you can inline the file name… It never changes.
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 148:
> 146: }
> 147:
> 148: class CustomTable {
I still think `CustomTable` is a bad name for this class because it represents table model. It requires further refactoring to clean up the code but I won't insist on doing so.
Even though it's only test code and therefore it shouldn't be as robust and clean as product code, I believe it's important to keep the test code clean as well so that tests can serve as samples. Any ways…
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 151:
> 149: public final static int COL_FIRSTNAME = 0;
> 150: public final static int COL_LASTNAME = 1;
> 151: public final static int COL_SALARY = 2;
Suggestion:
private static final int COL_FIRSTNAME = 0;
private static final int COL_LASTNAME = 1;
private static final int COL_SALARY = 2;
Let's use the blessed modifier order; these can be `private`.
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 153:
> 151: public final static int COL_SALARY = 2;
> 152:
> 153: static final Class[] classes = {
Suggestion:
static final Class<?>[] classes = {
Let's get rid of raw types. The test code was presumably written before generics were introduced into Java.
test/jdk/javax/swing/JTable/JTableRightOrientationTest.java line 206:
> 204: }
> 205:
> 206: public Class getColumnClass(int columnIndex) {
Suggestion:
public Class<?> getColumnClass(int columnIndex) {
-------------
PR Review: https://git.openjdk.org/jdk/pull/16374#pullrequestreview-1757169103
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410497393
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410615475
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410613628
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410580754
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410582328
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410616870
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410586207
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410588365
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410589855
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410604922
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410591553
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410592492
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1410593653
More information about the client-libs-dev
mailing list