RFR: 8301606: JFileChooser file chooser details view "size" label cut off in Metal Look&Feel [v3]

Alexey Ivanov aivanov at openjdk.org
Tue Jul 25 13:17:58 UTC 2023


On Fri, 14 Jul 2023 07:43:00 GMT, Tejesh R <tr at openjdk.org> wrote:

>> "size" label which is _RIGHT_ aligned is cut off on header cell. The issue is not only w.r.t to `JFileChooser` rather it is part of `JTable`. The root caused is found to be that in metal L&F the border insets is set to `(2,2,2,0)` meaning the right most inset value is 0. Hence when UIScaling increases the issue will be visible clearly. The fix addresses the issue by setting the right `inset` to 2 similar to other `inset` values. (Though the reason for setting it to 0 is unclear since it was initial load). 
>> CI testing shows green.
>> After the fix at 225% scaling:
>> ![image](https://github.com/openjdk/jdk/assets/94159358/f3e5a88a-1710-4ee0-84aa-338bc93010b2)
>
> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Spacing fix

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 36:

> 34: 
> 35: import java.io.File;
> 36: import javax.imageio.ImageIO;

`java.*` and `javax.*` packages shouldn't be mixed like that.

Usually, in OpenJDK, `java.*` packages are preceded by `javax.*` packages, it's a matter of consistency; in a way, having `javax.*` packages above `java.*` ones is reasonable, the test is for Swing which is in `javax.*` space.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 42:

> 40:  * @bug 8301606
> 41:  * @library /java/awt/regtesthelpers
> 42:  * @build PassFailJFrame

Suggestion:


The test is no longer manual, it uses no libraries.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 44:

> 42:  * @build PassFailJFrame
> 43:  * @summary Test to check if the Right aligned header
> 44:  * label doesn't cut off Metal Look&Feel

Suggestion:

 * @summary Test to check if the Right aligned header
 *          label doesn't cut off Metal Look&Feel

This way the summary is easier to grasp by scanning the tags quickly, don't you agree?

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 48:

> 46:  */
> 47: 
> 48: public class JTableHeaderLabelRightAlignTest {

Suggestion:

 */
public class JTableHeaderLabelRightAlignTest {

No blank line between the class declaration and the comment above it.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 49:

> 47: 
> 48: public class JTableHeaderLabelRightAlignTest {
> 49:     static JTable table;

Suggestion:

    private static JTable table;


It's better to separate it from the following constants with a blank line.

Even better, the `table` field can be converted to a local variable inside the `test` method.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 62:

> 60:                 throw new RuntimeException(e);
> 61:             }
> 62:         });

Suggestion:

        SwingUtilities.invokeAndWait(JTableHeaderLabelRightAlignTest::test);


You can call your own methods without specifying the class. Even better, remove the `throws Exception` from the `test` method declaration and handle `IOException` when saving the image.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 65:

> 63:     }
> 64: 
> 65:     public static void Test() throws Exception {

Suggestion:

    private static void test() throws Exception {

It doesn't need to be public, does it?

Method names in Java start with lower-case letter.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 70:

> 68:         BufferedImage imgHeader;
> 69:         double w;
> 70:         double h;

Declare variables where they're used, there's not much sense in declaring them in advance.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 76:

> 74:         };
> 75: 
> 76:         String[] columnNames = { "Size", "Size", "Size"};

Do you really need three columns and two rows?

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 79:

> 77: 
> 78:         table = new JTable(data, columnNames);
> 79:         table.setSize(WIDTH,HEIGHT);

Suggestion:

        table.setSize(WIDTH, HEIGHT);

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 85:

> 83:         final JTableHeader header = table.getTableHeader();
> 84:         TableCellRenderer renderer = header.getDefaultRenderer();
> 85:         header.setDefaultRenderer(renderer);

This doesn't make sense to me: you get the renderer from the header and set it back. Nothing's changed here, is it?

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 91:

> 89:         header.setSize(size);
> 90:         w = SCALE * size.width;
> 91:         h = SCALE * size.height;

Perhaps, it make more sense to have both `w` and `h` as `int` and use `ceil` to ensure the buffered image has enough space to contain the entire image without clipping.

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 108:

> 106:         for (int i = 1; i < imgHeader.getHeight()-3; i++) {
> 107:             for (int j = verticalLineCol; j < verticalLineCol + 1; j++) {
> 108:                 if (expectedRGB != imgHeader.getRGB(j, i)) {

Suggestion:

        for (int i = 1; i < imgHeader.getHeight()-3; i++) {
            for (int j = verticalLineCol; j < verticalLineCol + 1; j++) {
                if (expectedRGB != imgHeader.getRGB(j, i)) {

These are coordinates, so name them as `x` and `y`. Adding the coordinates would help an engineer dealing with the test failure the clue what went wrong, that engineer could be you.

The height should also be scaled: `(int) (imgHeader.getHeight() * SCALE - 3);`

test/jdk/javax/swing/JTableHeader/JTableHeaderLabelRightAlignTest.java line 115:

> 113:             }
> 114:         }
> 115:         System.out.println("Test Pass!!");

Suggestion:

        System.out.println("Test Passed");

-------------

PR Review: https://git.openjdk.org/jdk/pull/14464#pullrequestreview-1545367488
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273507961
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273509468
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273510820
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273475982
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273476947
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273500298
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273501354
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273490371
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273524852
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273490538
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273492672
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273494630
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273499942
PR Review Comment: https://git.openjdk.org/jdk/pull/14464#discussion_r1273495855



More information about the client-libs-dev mailing list