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:
>> 
>
> 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