JDK-8016524: [macosx] Bottom line is not visible for JTableHeader #7219

honkar-jdk · 2022-01-26T00:12:17Z

Previously, the JTableHeader Bottom line wasn't visible on MacOS LAF (Aqua). With the fix the bottom line (light grey) is visible.

Changes were made to AquaTableHeaderBorder.paintBorder method and the height of the component+border was adjusted in paint method.

A new test case (JTHeaderBorderTest.java) was added to test the fix as there was no corresponding test case present for this issue previously. The test checks if the border is visible by checking the color at the border location with the background color of the table and table-header, if both are different test case passes and it fails if the pixel color at border location matches either the table-header or table background color.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8016524: [macosx] Bottom line is not visible for JTableHeader

Reviewers

Prasanta Sadhukhan (@prsadhuk - Reviewer)
Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7219/head:pull/7219
$ git checkout pull/7219

Update a local copy of the PR:
$ git checkout pull/7219
$ git pull https://git.openjdk.java.net/jdk pull/7219/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7219

View PR using the GUI difftool:
$ git pr show -t 7219

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7219.diff

bridgekeeper · 2022-01-26T00:29:28Z

Hi @honkar-jdk, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user honkar-jdk" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2022-01-26T00:32:20Z

@honkar-jdk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

honkar-jdk · 2022-01-26T00:48:35Z

/covered

bridgekeeper · 2022-01-26T00:53:03Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

src/java.desktop/macosx/classes/com/apple/laf/AquaTableHeaderBorder.java

test/jdk/javax/swing/JTableHeader/8016524/Bug8016524.java

mlbridge · 2022-01-27T00:40:47Z

Webrevs

victordyakov · 2022-01-27T00:55:30Z

@prsadhuk please review

test/jdk/javax/swing/JTableHeader/8016524/Bug8016524.java

prsadhuk · 2022-01-27T09:25:04Z

test/jdk/javax/swing/JTableHeader/8016524/Bug8016524.java

+        {
+            throw new RuntimeException(e);
+        }
+        frame.dispose();


frame should be disposed irrespective of whether it fails or pass.
Also, it should be under EDT too.

The test is not marked for mac only and I see it fails in my ubuntu locally.
Did you try running the test in CI system?

@prsadhuk I have changed the test case to explicitly set the table and header background to white. Mach5 tests are passing on all platforms including linux now. Can you please let me know in case you are seeing any issues on Ubuntu locally, with the new test case?

I now see it fail locally in windows system also along with ubuntu. windows screenshot here where the border is white which is why it fails

It seems there's a anomaly for JTableHeader border in native and jdk in Windows L&F..
The system table shows here doesn't show any line

whereas in our SwingSet2 we have a light grey line

Technically, it might be construed as a bug but I guess I will prefer a demarcation line between header and body.
It's upto you if you want to take a look at this in a separate issue or in this.

@prsadhuk Thank you for looking into it. I think this fix will require a change in Window related L&F classes. I think it would help me if I track this as separate issue.

prsadhuk · 2022-01-27T09:31:21Z

test/jdk/javax/swing/JTableHeader/8016524/Bug8016524.java

+        // if pixel color is white then border not visible, throw Exception
+        if(lowerLeft.getRGB() == WHITE_RGB || lowerRight.getRGB() == WHITE_RGB)
+        {
+            throw new RuntimeException("JTableHeader Bottom Border not visible");


Not sure this check is correct. If we have dark theme or the background is changed in system, then I guess the test will fail even with fix. Maybe you can set the JTable background to white explicitly.

@prsadhuk I have explicitly set the background to white for the table as well as the header.

@prsadhuk The fix and test case is ready for review. I have marked the testcase as mac-only.

I see it fails for me in mac 10.15..DId you see it pass locally for you and in CI systems? Also, it should show 3 rows including row showing "A""B""C" as it is shown in windows/ubuntu, why it is showing only 1 row here?

I got the 2nd part of the question myself which is because of white gridlines used in AquaLookAndFeel so invisible..

@prsadhuk It passes in my local (mac 11.6.2) and in CI systems as well. Are there any system specific configurations that I need to look into to fix this issue?

@prsadhuk Ready for review. Changed the test case to paint JFrame to BufferedImage. The test case is marked headful though robot calls are not used, as the JFrame rendering requires it to be headful.

I believe you could raise a awt Robot issue for not getting correct pixel color via getPixelColor(x,y) on retina display (if you are sure JDK-8274939 patch didn't help)

test/jdk/javax/swing/JTableHeader/8016524/Bug8016524.java

mrserb · 2022-02-01T02:44:04Z

src/java.desktop/macosx/classes/com/apple/laf/AquaTableHeaderBorder.java

@@ -105,7 +105,7 @@ public void paintBorder(final Component c, final Graphics g, final int x, final
        final int newWidth = width;
        final int newHeight = height;

-        painter.paint(g, c, newX - 1, newY - 1, newWidth + 1, newHeight);
+        painter.paint(g, c, newX - 1, newY - 1, newWidth + 1, newHeight + 1);


I would like to clarify the change here, what is the coordinates/size passed here is it a size of the component+border, or just a size of the component? So if decrease the newX/Y will we draw in the component area, or accidentally draw outside?

@mrserb It is the size of component + border. The component being painted here is DefaultTableCellHeaderRenderer.

Tested under two cases

Decreasing x & y, keeping width & height same as above - Border gets drawn inside the component area making header look smaller.

Decreasing width & height, keeping x & y same - Border is not visible

If it is a size of the component+border then how we can draw outside of it by decreasing the x/y?

@mrserb Sorry, what I meant to convey is that the coordinate arguments sent to painter.paint() is including the component + border, and newX, newY, newHeight, newWidth are coordinates of component only.
So when newX and newY are decreased by more than 1 in paint() method, border seems to be drawn inside the component area and not outside (in this case within the table header)

…g issues

test/jdk/javax/swing/JTableHeader/8016524/JTHeaderBorderTest.java

prsadhuk · 2022-02-11T07:30:28Z

test/jdk/javax/swing/JTableHeader/8016524/JTHeaderBorderTest.java

+                String tableColor = Integer.toHexString(table.getBackground().getRGB());
+                String headerColor = Integer.toHexString(table.getTableHeader().getBackground().getRGB());
+                String pixelColor = tableColor;
+                boolean isBottomLineVisible = false;


To nitpick, I will much rather use Color instead of converting to String as we are trying to check and compare for Color but it's not a dealbreaker.

@prsadhuk BufferedImage's getRGB() method returns a int value and does not have equivalent method that returns a Color object (at specified coordinates), I was able to change string comparisons to int. Please let me know if this should be okay?

You could have used getBackground() itself to get a Color object but amnot finicky about it, its upto you.

prsadhuk · 2022-02-11T07:35:27Z

test/jdk/javax/swing/JTableHeader/8016524/JTHeaderBorderTest.java

+                boolean isBottomLineVisible = false;
+
+                // scan table header region to check if bottom border of JTableHeader is visible
+                for (int y = Y_OFFSET; y <= Y_OFFSET+25; y++) {


Can't we use X_OFFSET for hardcoded 25 here?

The X_OFFSET is different from the 25 specified in the loop, this specifies the vertical scan range. To avoid confusion I have changed it as Y_OFFSET_START and Y_OFFSET_END.

openjdk · 2022-02-14T04:49:11Z

@honkar-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8016524: [macosx] Bottom line is not visible for JTableHeader

Reviewed-by: psadhukhan, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 259 new commits pushed to the master branch:

1a7b70a: 8269091: javax/sound/sampled/Clip/SetPositionHang.java failed with ArrayIndexOutOfBoundsException: Array index out of range: -4
16f649b: 8281678: appcds/dynamicArchive/ArchiveConsistency.java fails after JDK-8279997
88fc3bf: 8280473: CI: Support unresolved JVM_CONSTANT_Dynamic constant pool entries
f07b816: 8280940: gtest os.release_multi_mappings_vm is racy
9d0a4c3: 8274238: Inconsistent type for young_list_target_length()
2604a88: 8281585: Remove unused imports under test/lib and jtreg/gc
534e557: 8256368: Avoid repeated upcalls into Java to re-resolve MH/VH linkers/invokers
95f198b: 8274980: Improve adhoc build version strings
c61d629: 8281553: Ensure we only require liveness from mach-nodes with barriers
2597206: 8280783: Parallel: Refactor PSCardTable::scavenge_contents_parallel
... and 249 more: https://git.openjdk.java.net/jdk/compare/c1e4f3dd1b42474c9abc22c7b981a98f9c36e0d5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prsadhuk, @prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

honkar-jdk · 2022-02-15T00:58:32Z

/integrate

openjdk · 2022-02-15T00:59:32Z

@honkar-jdk
Your change (at version 9d1bc7a) is now ready to be sponsored by a Committer.

prsadhuk · 2022-02-15T05:02:13Z

/sponsor

openjdk · 2022-02-15T05:03:44Z

Going to push as commit f33329e.
Since your change was applied there have been 260 commits pushed to the master branch:

d4cd8df: 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters
1a7b70a: 8269091: javax/sound/sampled/Clip/SetPositionHang.java failed with ArrayIndexOutOfBoundsException: Array index out of range: -4
16f649b: 8281678: appcds/dynamicArchive/ArchiveConsistency.java fails after JDK-8279997
88fc3bf: 8280473: CI: Support unresolved JVM_CONSTANT_Dynamic constant pool entries
f07b816: 8280940: gtest os.release_multi_mappings_vm is racy
9d0a4c3: 8274238: Inconsistent type for young_list_target_length()
2604a88: 8281585: Remove unused imports under test/lib and jtreg/gc
534e557: 8256368: Avoid repeated upcalls into Java to re-resolve MH/VH linkers/invokers
95f198b: 8274980: Improve adhoc build version strings
c61d629: 8281553: Ensure we only require liveness from mach-nodes with barriers
... and 250 more: https://git.openjdk.java.net/jdk/compare/c1e4f3dd1b42474c9abc22c7b981a98f9c36e0d5...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-15T05:03:59Z

@prsadhuk @honkar-jdk Pushed as commit f33329e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

honkar-jdk added 4 commits January 21, 2022 18:18

JtableHeader Bottom line issue fix

5a7a8de

Merge branch 'master' into JTableHeader_8016524

1e1a41c

added copyright year and removed commented lines

16d2c39

removed jtreg author tag

f8a4bfd

bridgekeeper bot added the oca label Jan 26, 2022

openjdk bot added the client label Jan 26, 2022

honkar-jdk changed the title ~~JDK-8016524: Bottom line is not visible for JTableHeader~~ JDK-8016524: [macosx] Bottom line is not visible for JTableHeader Jan 26, 2022

bridgekeeper bot added the oca-verify label Jan 26, 2022

mrserb reviewed Jan 26, 2022

View reviewed changes

incorporated review changes

1d1c632

bridgekeeper bot removed oca oca-verify labels Jan 27, 2022

openjdk bot added the rfr label Jan 27, 2022

prsadhuk suggested changes Jan 27, 2022

View reviewed changes

honkar-jdk added 3 commits January 30, 2022 19:46

added white background to table and header

8f306a5

aligned the jtreg header

90a5a7a

tagged test case as mac only

516985d

mrserb reviewed Feb 1, 2022

View reviewed changes

honkar-jdk added 2 commits February 3, 2022 17:19

changed the test case condition and marked the test as mac-only

552d409

Changed test case to use BufferedImage to prevent retina-pixel scalin…

52374b8

…g issues

prsadhuk reviewed Feb 11, 2022

View reviewed changes

Revised test case - changed string to int comparison, expanded imports

9d1bc7a

prsadhuk approved these changes Feb 14, 2022

View reviewed changes

openjdk bot added the ready label Feb 14, 2022

prrace approved these changes Feb 15, 2022

View reviewed changes

openjdk bot added the sponsor label Feb 15, 2022

openjdk bot added the integrated label Feb 15, 2022

openjdk bot closed this Feb 15, 2022

openjdk bot removed ready rfr sponsor labels Feb 15, 2022

aivanov-jdk mentioned this pull request Mar 1, 2022

8015854: [macosx] JButton's HTML ImageView adding unwanted padding #7310

Closed

3 tasks

JDK-8016524: [macosx] Bottom line is not visible for JTableHeader #7219

JDK-8016524: [macosx] Bottom line is not visible for JTableHeader #7219

Conversation

honkar-jdk commented Jan 26, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 26, 2022

openjdk bot commented Jan 26, 2022

honkar-jdk commented Jan 26, 2022

bridgekeeper bot commented Jan 26, 2022

mlbridge bot commented Jan 27, 2022 • edited Loading

Webrevs

victordyakov commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkar-jdk Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 14, 2022 • edited Loading

honkar-jdk commented Feb 15, 2022

openjdk bot commented Feb 15, 2022

prsadhuk commented Feb 15, 2022

openjdk bot commented Feb 15, 2022

openjdk bot commented Feb 15, 2022

honkar-jdk commented Jan 26, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 27, 2022 •

edited

Loading

honkar-jdk Jan 31, 2022 •

edited

Loading

honkar-jdk Feb 7, 2022 •

edited

Loading

honkar-jdk Feb 3, 2022 •

edited

Loading

openjdk bot commented Feb 14, 2022 •

edited

Loading