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