8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+ #18644

kumarabhi006 · 2024-04-05T06:54:46Z

Test was failing on GTK and Windows LAF due to pixel color mismatch. Th reason behind this issue was the size of image which is different and that results in incorrect pixel comparison. Fix is to ensure that correct pixel is matched and the pixel color should remain within tolerance.
For windows LAF the background color is not an exact match and thus added a TOLERANCE field to check if the RGB difference is within limits.

@key headful added in jtreg tag to ensure that test run for GTK LAF as well which was not the case before as it is mentioned in JBS It does not fail in mach5 because on linux + headless mode the gtk L&F is not supported.

CI testing is green for the modified test. Link attached in JBS.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+ (Bug - P4)

Reviewers

Damon Nguyen (@DamonGuy - Committer)
Jayathirth D V (@jayathirthrao - Reviewer)
Tejesh R (@TejeshR13 - Committer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18644/head:pull/18644
$ git checkout pull/18644

Update a local copy of the PR:
$ git checkout pull/18644
$ git pull https://git.openjdk.org/jdk.git pull/18644/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18644

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18644.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-05T06:55:14Z

👋 Welcome back abhiscxk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-05T06:55:32Z

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

8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+

Reviewed-by: dnguyen, jdv, tr, serb

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 307 new commits pushed to the master branch:

32946e1: 8295111: dpkg appears to have problems resolving symbolically linked native libraries
6ee8407: 8324342: Doclet should default @SInCE for a nested class to that of its enclosing class
235ba9a: 8330458: Add missing @SInCE tag to ClassFile.JAVA_23_VERSION
f713766: 8329322: Convert PageFormat/Orient.java to use PassFailJFrame
ec180d4: 8324950: IGV: save the state to a file
571e6bc: 8330419: Unused code in ConnectionGraph::specialize_castpp
5853a2c: 8330533: JFR: LocalDateTime should not use milliseconds since epoch
b648ed0: 8329997: Add provisions for checking memory segment alignment constraints
60b65e6: 8325872: Make GuaranteedSafepointInterval default 0
b049609: 8330094: RISC-V: Save and restore FRM in the call stub
... and 297 more: https://git.openjdk.org/jdk/compare/b9c76dedf4aa2248a5e561a535c9e3e181f7836a...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-05T06:55:51Z

@kumarabhi006 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.

mlbridge · 2024-04-05T06:58:19Z

Webrevs

DamonGuy · 2024-04-05T18:22:18Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

-        } else {
-            failingLafs.append(lafName + ", ");
-        }
+        System.out.println("Test Passed: "+lafName);


Suggested change

System.out.println("Test Passed: "+lafName);

System.out.println("Test Passed: " + lafName);

DamonGuy · 2024-04-05T18:23:42Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

+        int redDiff = c1.getRed() - c2.getRed();
+        int blueDiff = c1.getBlue() - c2.getBlue();
+        int greenDiff = c1.getGreen() - c2.getGreen();


Does this need Math.abs to get the absolute value of the difference?

That makes sense. Updated.

DamonGuy · 2024-04-05T18:25:45Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

+                // In Windows LAF, the ComboBox sizes are different.
+                // Combo size is 116 x 22 where as Combo2 size is 115 X 20 and
+                // that results in pixel offset of 1px for width and 1px for height.
+                eColor1 = new Color(enabledImage.getRGB(x, y));
+                eColor2 = new Color(enabledImage2.getRGB(x + 1, y - 1));
+                dColor1 = new Color(disabledImage.getRGB(x, y));
+                dColor2 = new Color(disabledImage2.getRGB(x + 1, y - 1));
+            } else if (lafName.equals("GTK+")) {
+                // In GTK LAF, the ComboBox sizes are different.
+                // Combo size is 159 x 30 where as Combo2 size is 179 X 34 and
+                // that results in pixel offset of 10px for width and 2px for height.
+                eColor1 = new Color(enabledImage.getRGB(x, y));
+                eColor2 = new Color(enabledImage2.getRGB(x + 10, y + 2));
+                dColor1 = new Color(disabledImage.getRGB(x, y));
+                dColor2 = new Color(disabledImage2.getRGB(x + 10, y + 2));


I noticed with these components, sometimes the size varies slightly between different desktop scaling values or resolutions. You have exact combobox pixel sizes in the comments, but does this stay the same when you set your device to 200% or other values?

Maybe best to leave out the exact combo size from the comments regardless.

I noticed with these components, sometimes the size varies slightly between different desktop scaling values or resolutions. You have exact combobox pixel sizes in the comments, but does this stay the same when you set your device to 200% or other values?
Maybe best to leave out the exact combo size from the comments regardless.

Checked with UIScale of 2 and the images are of same size. But your point seems valid that may vary between different machine for different resolution or scale.
Removed the exact size from the comments.

DamonGuy

Changes look good to me now.

jayathirthrao · 2024-04-15T08:34:43Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

@@ -44,13 +37,22 @@

 import static java.awt.image.BufferedImage.TYPE_INT_ARGB;

+/*


Usually we keep jtreg comment section before imports.

There was a discussion going on regarding placement of jtreg tags. Since, we updated jtreg tags aftre imports section in recent test sprint, kept it same here as well.

jayathirthrao · 2024-04-15T08:39:04Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

@@ -99,11 +101,10 @@ private static void testMethod() throws IOException {
        ImageIO.write(disabledImage2, "png", new File(testDir


We are saving these images even in the case where test passes. In my local macOS run i see 16 images getting saved in JTwork folder. This will end up taking good amount of space in our CI test systems.

We should save image only in case of test failure foe debugging purpose.

Also i see images with Mac OS X***.png filename and all of them are just blank and there is no combobox. Is that fine?

We are saving these images even in the case where test passes. In my local macOS run i see 16 images getting saved in JTwork folder. This will end up taking good amount of space in our CI test systems.
We should save image only in case of test failure foe debugging purpose.

Updated to save the image only when test fails.

Also i see images with Mac OS X***.png filename and all of them are just blank and there is no combobox. Is that fine?

Even I also observed the same but not sure about this.

jayathirthrao · 2024-04-15T08:41:00Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

 public class DisabledComboBoxFontTestAuto {
    private static JComboBox combo, combo2;
    private static BufferedImage enabledImage, disabledImage, enabledImage2, disabledImage2;
    private static Path testDir;
    private static String lafName;
    private static StringBuffer failingLafs;
    private static int COMBO_HEIGHT, COMBO_WIDTH, COMBO2_HEIGHT, COMBO2_WIDTH;
+    private static final int TOLERANCE = 20;

    private static void createCombo() {
        combo = new JComboBox();


If we can use unicode blank character instead of "Simple JComboBox" text. I think we make this test more robust and avoid tolerance checks.

Can the initial bug "JComboBox has correct font color when disabled" be verified with this change?

This change works fine with initial bug. Test failed if I comment out the fix proposed in initial bug for Nimbus LAF.

jayathirthrao · 2024-04-15T08:43:28Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

+                dColor1 = new Color(disabledImage.getRGB(x, y));
+                dColor2 = new Color(disabledImage2.getRGB(x + 1, y - 1));
+            } else if (lafName.equals("GTK+")) {
+                // In GTK LAF, the ComboBox sizes are different and


Is there any way to get these internal offsets programmatically and not use constants?

Removed the offsets as after having unicode blank character there is no need of these.

kumarabhi006 · 2024-04-16T05:53:13Z

@jayathirthrao Ran the updated test in CI, no failure observed across platforms.

jayathirthrao

Updated changes look good to me.

mrserb · 2024-04-16T07:13:48Z

As far as I understand the test creates two enabled-JComboBox, then disables both, and then compares the disabled and enabled images. Why there are some differences in sizes and colors?

kumarabhi006 · 2024-04-16T09:29:03Z

As far as I understand the test creates two enabled-JComboBox, then disables both, and then compares the disabled and enabled images. Why there are some differences in sizes and colors?

Not sure about this.. but there is a mention in initial bug also regarding size difference #12390 (comment)

DamonGuy

Updated changes LGTM

TejeshR13

I guess testDir can be moved inside testMethod() method where it is used.

TejeshR13 · 2024-04-17T04:25:43Z

test/jdk/javax/swing/JComboBox/DisabledComboBoxFontTestAuto.java

-                || (c1.getBlue() != c2.getBlue())
-                || (c1.getGreen() != c2.getGreen())) {
-
+            || (c1.getBlue() != c2.getBlue())


This condition can be optimized since the test is done for all LAF and for each this method is called twice. Instead of using ||, using && would optimized slightly. You can check for true where it checks for == and returns true. Its just a suggestion.

This condition can be optimized since the test is done for all LAF and for each this method is called twice. Instead of using ||, using && would optimized slightly. You can check for true where it checks for == and returns true. Its just a suggestion.

I think current comparison with || is ok as if any of the RGB color component doesn't match, test should fail.

I meant to use && so that comparisons for failure case fails faster than ||.
Similar to this:
` if ((c1.getRed() == c2.getRed())
&& (c1.getBlue() == c2.getBlue())
&& (c1.getGreen() == c2.getGreen())) {
return true;

} else {

System.out.println(lafName + " Enabled RGB failure: "
+ c1.getRed() + ", "
+ c1.getBlue() + ", "
+ c1.getGreen() + " vs "
+ c2.getRed() + ", "
+ c2.getBlue() + ", "
+ c2.getGreen());
return false;
}`

The logical || operator doesn't check second condition if first condition is true. It checks second condition only if first one is false.
So as per current logic, if RED component is not equal, it doesn't check for GREEN and BLUE component and fails.

Exactly, similar thing applies to && and hence in failure condition || checks for all the colors while && drops off at first failure only. Which is why I suggested it would be better for failure cases.

Yeah, its != here right, then it'll fail fast for failure conditions...... Then no need to optimize......

In && operator if condition evaluate to false it will not check further.
Similarly, in || operator if condition evaluate to true it will not check further.
So if c1.getRed() != c2.getRed() evaluates to true, it will not check further and fail immediately.

kumarabhi006 · 2024-04-17T04:35:34Z

I guess testDir can be moved inside testMethod() method where it is used.

Updated.

TejeshR13

LGTM

kumarabhi006 · 2024-04-17T09:55:43Z

As far as I understand the test creates two enabled-JComboBox, then disables both, and then compares the disabled and enabled images. Why there are some differences in sizes and colors?

Difference in size is specific to LAFs like in case on Metal and Motif the sizes are same but in Nimbus and GTK it differs in size.
For Nimbus when DefaultListCellRenderer is used the width is 2px more than when SynthComboBoxRenderer is used. This is due to the difference in Inset values. In case of DefaultListCellRenderer the Insets value (left and right) are 1 px more than the insets value for SynthComboBoxRenderer.

Similarly in GTK LAF also the insets values are different in DefaultListCellRenderer and SynthComboBoxRenderer.

Color difference is only in the background color for Windows LAF which is slightly different.

kumarabhi006 · 2024-04-18T05:58:00Z

@mrserb Do you have any other suggestion ?

DamonGuy · 2024-04-18T15:53:28Z

As far as I understand the test creates two enabled-JComboBox, then disables both, and then compares the disabled and enabled images. Why there are some differences in sizes and colors?

Difference in size is specific to LAFs like in case on Metal and Motif the sizes are same but in Nimbus and GTK it differs in size. For Nimbus when DefaultListCellRenderer is used the width is 2px more than when SynthComboBoxRenderer is used. This is due to the difference in Inset values. In case of DefaultListCellRenderer the Insets value (left and right) are 1 px more than the insets value for SynthComboBoxRenderer.

Similarly in GTK LAF also the insets values are different in DefaultListCellRenderer and SynthComboBoxRenderer.

Color difference is only in the background color for Windows LAF which is slightly different.

This sounds about right. I looked into this difference briefly in my original fix, and checked the insets, bounds, and borders. Now that you mention it, I remember it was the insets that differed between the two renderers. Didn't seem related to the issue/fix and may be intended, so I left it as is and modified my test to use a midline scan to compare the pixel colors between the two comboboxes.

kumarabhi006 · 2024-04-19T05:01:46Z

/integrate

kumarabhi006 · 2024-04-19T05:21:35Z

/dummy

openjdk · 2024-04-19T10:10:18Z

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

85261bc: 8330266: RISC-V: Restore frm to RoundingMode::rne after JNI
9f2a4fa: 8330572: jdk.internal.net.http.HttpConnection calls an expensive checkOpen() when returning a HTTP/1.1 connection to the pool
8990864: 8330156: RISC-V: Range check auipc + signed 12 imm instruction
46a2ce4: 8330063: Upgrade jQuery to 3.7.1
252c62a: 8330524: Linux ppc64le compile warning with clang in os_linux_ppc.cpp
32946e1: 8295111: dpkg appears to have problems resolving symbolically linked native libraries
6ee8407: 8324342: Doclet should default @SInCE for a nested class to that of its enclosing class
235ba9a: 8330458: Add missing @SInCE tag to ClassFile.JAVA_23_VERSION
f713766: 8329322: Convert PageFormat/Orient.java to use PassFailJFrame
ec180d4: 8324950: IGV: save the state to a file
... and 302 more: https://git.openjdk.org/jdk/compare/b9c76dedf4aa2248a5e561a535c9e3e181f7836a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-19T10:11:39Z

@kumarabhi006 Pushed as commit eb60822.

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

openjdk · 2024-04-19T10:14:51Z

@kumarabhi006 Unknown command dummy - for a list of valid commands use /help.

ComboBox color issue fix

12a1bf4

openjdk bot added the rfr Pull request is ready for review label Apr 5, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Apr 5, 2024

DamonGuy suggested changes Apr 5, 2024

View reviewed changes

Review comment update

170d45c

DamonGuy approved these changes Apr 5, 2024

View reviewed changes

jayathirthrao reviewed Apr 15, 2024

View reviewed changes

Review comment update, tolerance check removed

d3a4520

jayathirthrao approved these changes Apr 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2024

DamonGuy approved these changes Apr 16, 2024

View reviewed changes

TejeshR13 reviewed Apr 17, 2024

View reviewed changes

testDir moved to method

5e7bcf3

TejeshR13 approved these changes Apr 17, 2024

View reviewed changes

DamonGuy approved these changes Apr 18, 2024

View reviewed changes

mrserb approved these changes Apr 18, 2024

View reviewed changes

jayathirthrao approved these changes Apr 19, 2024

View reviewed changes

Empty-Commit

c67ec7e

openjdk bot added the integrated Pull request has been integrated label Apr 19, 2024

openjdk bot closed this Apr 19, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 19, 2024

	System.out.println("Test Passed: "+lafName);
	System.out.println("Test Passed: " + lafName);

		@@ -44,13 +37,22 @@

		import static java.awt.image.BufferedImage.TYPE_INT_ARGB;

		/*

		@@ -99,11 +101,10 @@ private static void testMethod() throws IOException {
		ImageIO.write(disabledImage2, "png", new File(testDir

8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+ #18644

8310072: JComboBox/DisabledComboBoxFontTestAuto: Enabled and disabled ComboBox does not match in these LAFs: GTK+ #18644

Conversation

kumarabhi006 commented Apr 5, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 5, 2024

openjdk bot commented Apr 5, 2024 • edited Loading

openjdk bot commented Apr 5, 2024

mlbridge bot commented Apr 5, 2024 • edited Loading

Webrevs

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

kumarabhi006 Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

DamonGuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarabhi006 Apr 16, 2024 • 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

kumarabhi006 commented Apr 16, 2024

jayathirthrao left a comment

Choose a reason for hiding this comment

mrserb commented Apr 16, 2024

kumarabhi006 commented Apr 16, 2024

DamonGuy left a comment

Choose a reason for hiding this comment

TejeshR13 left a 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

TejeshR13 Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarabhi006 commented Apr 17, 2024

TejeshR13 left a comment

Choose a reason for hiding this comment

kumarabhi006 commented Apr 17, 2024 • edited Loading

kumarabhi006 commented Apr 18, 2024

DamonGuy commented Apr 18, 2024

kumarabhi006 commented Apr 19, 2024

kumarabhi006 commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

kumarabhi006 commented Apr 5, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 5, 2024 •

edited

Loading

mlbridge bot commented Apr 5, 2024 •

edited

Loading

kumarabhi006 Apr 5, 2024 •

edited

Loading

kumarabhi006 Apr 16, 2024 •

edited

Loading

TejeshR13 Apr 17, 2024 •

edited

Loading

kumarabhi006 commented Apr 17, 2024 •

edited

Loading