8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F #19381

kumarabhi006 · 2024-05-24T06:44:06Z

bug6492108.java test always fails in GTK L&F in single as well as dual screen linux machines. Since this test was not marked as "headful" in it's initial version, it never failed but after the fix of JDK-8287051 this test was problem listed as it always
failed which is captured in the JBS.
The reason of failure is the pixel color mismatch between JEditorPane and JTextArea/JTextPane which is caused by the JEditorPane's default opaque value which is false for GTK3 at L767.
In initial load JEditorPane, JTextArea and JTextPane components are opaque at L718 but after the fix for JDK-8145547 the implementation was changed to provide conditional support for GTK3 on linux where few components like Editor Pane, Formatted text Field, Password Field etc are opaque only if the GTK version is not 3.
JTextPane's issue was observed by JDK-8218479 and then the default opacity is set to true for JTextPane irrespective of GTK version.
Extending the fix in isOpaque() method in GTKStyle.java for JEditorPane similar to JTextPane resolves the issue.

Test verified in both single and dual screen linux machines.

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-8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F (Bug - P4)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Damon Nguyen (@DamonGuy - Committer)
Tejesh R (@TejeshR13 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19381

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-24T06:44:52Z

👋 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-05-24T06:45:20Z

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

8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F

Reviewed-by: aivanov, dnguyen, tr

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

9db7c95: 8332750: Broken link in CatalogFeatures.html
d826127: 8333462: Performance regression of new DecimalFormat() when compare to jdk11
67b9a08: 8333586: Improve syntax of @see tags in javax.lang.model
244f6ac: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share
a706e35: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
612b689: 8043226: Better diagnostics for non-applicable type annotations
dce9703: 8333226: Regressions 2-3% in Compress ZGC after 8331253
b101dcb: 8333312: Incorrect since tags on new ClassReader and ConstantPool methods
e0bab78: 8326951: since-checker - missing @ since tags
31f7039: 8316131: runtime/cds/appcds/TestParallelGCWithCDS.java fails with JNI error
... and 914 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-05-24T06:45:50Z

@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-05-24T16:39:17Z

Webrevs

TejeshR13 · 2024-05-30T06:06:47Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+        addTextComp(parent, text);
+
+        text = type.newInstance();
+        if (showText) text.setText(name + "\ndisabled");


showText is never initialized, meaning this condition is never executed. Can be removed?

TejeshR13 · 2024-05-30T06:07:41Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+        }
+    }
+
+    public void onBackgroundThread30() {


Unused, can be removed?

Actually it is not unused and is run on the background thread to allow enough time for visual verification. I will check once in the CI testing after removing this method and if there is no failure, it can be removed.

I verified with debug, its not used in background thread too. Please do verify once and remove if not used......

Actually I can see the debug output in my local run.
invoking: onEDT10 onEDT10 invoking: onBackgroundThread20 onBackgroundThread20 invoking: onBackgroundThread30 onBackgroundThread30

But as I told before this may not impact the result of the test, it is just to add extra delay to provide visual verification.

Yeah, true..

Tested in CI and there is no failure for multiple run. Removed this method.

But as I told before this may not impact the result of the test, it is just to add extra delay to provide visual verification.

In this case, it can be removed.

We want tests to run fast, don't we?

When debugging, it makes sense to increase the delays so that you can see what's going on on the screen.

Tested in CI and there is no failure for multiple run. Removed this method.

Thanks! I noticed it after I had written the comment.

TejeshR13 · 2024-05-30T06:10:29Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+ * @summary Verifies that the background is painted the same for
+ *          JTextArea, JTextPane, and JEditorPane.
+ * @library /javax/swing/regtesthelpers
+ * @build SwingTestHelper


Should this test be marked only for linux?

Since test was written originally to run in GTK L&F, updated the test to run only on linux machine.

alisenchung · 2024-05-30T19:24:32Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+            UIManager.setLookAndFeel(
+                "com.sun.java.swing.plaf.gtk.GTKLookAndFeel");
+        } catch (Exception e) {
+            System.out.println("GTK LAF is not supported on this system; test passes");


should this be jtreg.testSkipped exception?

alisenchung · 2024-05-30T19:56:15Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+        return panel;
+    }
+
+    private void onEDT10() {


is this method required? can it be rewritten to not use onEDT10 and onBackgroundThread20?

It can be re-written without using onEDT10 and onBackgroundThread20.. but I kept it as it was in original test. Don't see any strong reason to change also and there are many other tests which used SwingTestHelper utility to perform the testing.

alisenchung · 2024-05-30T19:58:31Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+    private static void addTextComps(Container parent,
+                                     Class<? extends JTextComponent> type)
+        throws Throwable
+    {


I think formatting here looks a little odd, could probably move the bracket onto the same line as throws Throwable and shift it over to the right to match Class<? extends JTextComponent> type)

Previous formatting was less confusing and aligned with Java Coding Style Guidelines.

The parameters to the method are aligned to the opening parenthesis; the throws clause is not part of the parameters and it's placed on another line according to rules of wrapping lines (it should've used double-indentation of 8 spaces rather than 4 spaces to indicate a continuation line).

Reverted back to previous formatting with double-indentation. Hopefully this should be ok now.

This formatting looks good to me.

alisenchung · 2024-05-30T19:59:40Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+
+                if (refimg.getWidth() != testimg.getWidth() ||
+                    refimg.getHeight() != testimg.getHeight())
+                {


move bracket to previous line to be consistent

Initially I thought of doing so but in one of the PR it was mentioned that "We generally prefer the { on a new line if the conditional is multi-line" #18703 (comment).

So I left like that.

I can't say that “We generally prefer the { on a new line if the conditional is multi-line” really holds, yet I agree it makes code less confusing by separating the condition and its continuation lines from the inside block.

To make the continuation line stand out as continuation line, place the || at the start of wrapped line.

Again, there's no agreed upon set of style guidelines.

To make the continuation line stand out as continuation line, place the || at the start of wrapped line.

Updated.

kumarabhi006 · 2024-06-04T04:48:32Z

@aivanov-jdk @alisenchung All review comments are addressed. Please review.

aivanov-jdk · 2024-06-04T13:25:25Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+            throw new SkippedException("GTK LAF is not supported on this system;" +
+                    " test passes");


Suggested change

throw new SkippedException("GTK LAF is not supported on this system;" +

" test passes");

throw new SkippedException("GTK LAF is not supported on this system");

I'd remove the additional message. I believe it was println before.

aivanov-jdk · 2024-06-04T13:25:49Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+    private static void addTextComps(Container parent,
+                                     Class<? extends JTextComponent> type)
+        throws Throwable
+    {


This formatting looks good to me.

aivanov-jdk · 2024-06-04T13:31:04Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+            Rectangle refRect =
+                new Rectangle(loc.x, loc.y, ref.getWidth(), ref.getHeight());


Suggested change

Rectangle refRect =

new Rectangle(loc.x, loc.y, ref.getWidth(), ref.getHeight());

Rectangle refRect = new Rectangle(loc, ref.getSize());

aivanov-jdk · 2024-06-04T13:31:32Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+        requestAndWaitForFocus(panel);
+    }
+
+    private void onBackgroundThread20() {


This method should run on EDT because it accesses the state of the components.

Updated to run this on EDT.

aivanov-jdk · 2024-06-04T13:35:16Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+                if (refimg.getWidth() != testimg.getWidth()
+                   || refimg.getHeight() != testimg.getHeight())


Suggested change

if (refimg.getWidth() != testimg.getWidth()

|| refimg.getHeight() != testimg.getHeight())

if (refimg.getWidth() != testimg.getWidth()

|| refimg.getHeight() != testimg.getHeight())

One more space, it should be aligned to the inside of the parenthesis.

Used Util to compare images, so these lines are removed now.

aivanov-jdk · 2024-06-04T13:35:50Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+                loc = test.getLocationOnScreen();
+                Rectangle testRect =
+                    new Rectangle(loc.x, loc.y,
+                                  test.getWidth(), test.getHeight());


Suggested change

loc = test.getLocationOnScreen();

Rectangle testRect =

new Rectangle(loc.x, loc.y,

test.getWidth(), test.getHeight());

loc = test.getLocationOnScreen();

Rectangle testRect = new Rectangle(test.getLocationOnScreen(),

test.getSize());

Choose one style and adjust both statements, refRect and testRect.

Updated to use same style for both places.

aivanov-jdk · 2024-06-04T13:44:21Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+                                  test.getWidth(), test.getHeight());
+                BufferedImage testimg = robot.createScreenCapture(testRect);
+
+                if (refimg.getWidth() != testimg.getWidth()


I suggest moving the part that compares the images into a helper method. Alternatively, you can use Util.compareBufferedImages.

Ok.. Updated Util.compareBufferedImages to compare images.

aivanov-jdk · 2024-06-04T13:45:24Z

test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java

+                            fail("Image comparison failed at (" +
+                                 x + "," + y + ") for image " + index);


Adding count and k to diagnostic message could help identifying the failing case quicker.

Don't find adding k to diagnostic message useful.. failure message is more relevant now about the images which are compared.

The images themselves are more useful for sure. Thanks!

aivanov-jdk · 2024-06-04T13:45:44Z

src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java

You should update the copyright year.

I can't see it in the PR. Didn't push?

Oh my bad, I missed to add that changes. Pushed now.

DamonGuy

Ran bug6492108.java with changes. LGTM.

TejeshR13

LGTM.

kumarabhi006 · 2024-06-05T16:02:02Z

/integrate

openjdk · 2024-06-05T16:03:38Z

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

438121b: 8332785: Replace naked uses of UseSharedSpaces with CDSConfig::is_using_archive
d7d1afb: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
7acfba2: 8327650: Test java/nio/channels/DatagramChannel/StressNativeSignal.java timed out
c5c0867: 8333252: C2: assert(assertion_predicate_has_loop_opaque_node(iff)) failed: must find OpaqueLoop* nodes
d85b0ca: 8332457: Examine startup overheads from JDK-8294961
326dbb1: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
9a8096f: 8330047: ASAN build error with gcc 13
6882b38: 8333590: UnmodifiableHeaders.toString() returns a value that represents empty headers
cbb6747: 8329581: Java launcher no longer prints a stack trace
789f704: 8322732: ForkJoinPool may underutilize cores in async mode
... and 933 more: https://git.openjdk.org/jdk/compare/b9c76dedf4aa2248a5e561a535c9e3e181f7836a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-05T16:03:44Z

@kumarabhi006 Pushed as commit 5dcb7a6.

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

JEditorPane issue fix and test moved to open

952c80f

openjdk bot added client client-libs-dev@openjdk.org rfr Pull request is ready for review labels May 24, 2024

kumarabhi006 changed the title ~~8160755: closed/com/sun/java/swing/plaf/gtk/6492108/bug6492108.java test fails in GTK L&F~~ bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F May 24, 2024

openjdk bot removed the rfr Pull request is ready for review label May 24, 2024

kumarabhi006 changed the title ~~bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F~~ 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F May 24, 2024

openjdk bot added the rfr Pull request is ready for review label May 24, 2024

TejeshR13 reviewed May 30, 2024

View reviewed changes

kumarabhi006 added 2 commits May 30, 2024 12:08

Review comment update

63ea19e

background method removed

2d836f0

alisenchung reviewed May 30, 2024

View reviewed changes

kumarabhi006 added 2 commits May 31, 2024 10:28

code formatting and jtreg skippedException added

69fedf2

Code formatting update

f94e4b0

aivanov-jdk suggested changes Jun 4, 2024

View reviewed changes

kumarabhi006 added 2 commits June 4, 2024 20:12

Review comment update

0afa8d8

copyright year update

649c3ad

aivanov-jdk approved these changes Jun 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 4, 2024

DamonGuy approved these changes Jun 4, 2024

View reviewed changes

TejeshR13 approved these changes Jun 5, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 5, 2024

openjdk bot closed this Jun 5, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Jun 5, 2024

openjdk bot removed the rfr Pull request is ready for review label Jun 5, 2024

		throw new SkippedException("GTK LAF is not supported on this system;" +
		" test passes");

		Rectangle refRect =
		new Rectangle(loc.x, loc.y, ref.getWidth(), ref.getHeight());

	Rectangle refRect =
	new Rectangle(loc.x, loc.y, ref.getWidth(), ref.getHeight());
	Rectangle refRect = new Rectangle(loc, ref.getSize());

		if (refimg.getWidth() != testimg.getWidth()
		\|\| refimg.getHeight() != testimg.getHeight())

		fail("Image comparison failed at (" +
		x + "," + y + ") for image " + index);

8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F #19381

8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F #19381

Uh oh!

Conversation

kumarabhi006 commented May 24, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 24, 2024

Uh oh!

openjdk bot commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 24, 2024

Uh oh!

mlbridge bot commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarabhi006 May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarabhi006 commented Jun 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kumarabhi006 commented May 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 24, 2024 •

edited

Loading

mlbridge bot commented May 24, 2024 •

edited

Loading

kumarabhi006 May 30, 2024 •

edited

Loading

aivanov-jdk May 31, 2024 •

edited

Loading