On Mon, 3 Jun 2024 05:52:31 GMT, Abhishek Kumar <abhis...@openjdk.org> wrote:
>> 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](https://bugs.openjdk.org/browse/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](https://github.com/kumarabhi006/jdk/blob/c642f44bbe1e4cdbc23496a34ddaae30990ce7c0/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L767). >> In initial load JEditorPane, JTextArea and JTextPane components are opaque >> at >> [L718](https://github.com/kumarabhi006/jdk/blame/6c7656678916ff3f5c9fc70efcbb69ce76801458/jdk/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L718) >> but after the fix for >> [JDK-8145547](https://bugs.openjdk.org/browse/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](https://github.com/kumarabhi006/jdk/blame/73d2181d56063f6015e4fc42e130591bee39bc36/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L746C21-L746C21). >> JTextPane's issue was observed by >> [JDK-8218479](https://bugs.openjdk.org/browse/JDK-8218479) and then the >> default opacity is set to true for JTextPane [irrespective of GTK >> version](https://github.com/kumarabhi006/jdk/blame/c642f44bbe1e4cdbc23496a34ddaae30990ce7c0/src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java#L750C16-L750C16). >> 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. > > Abhishek Kumar has updated the pull request incrementally with one additional > commit since the last revision: > > Code formatting update Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java line 1: > 1: /* You should update the copyright year. test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 64: > 62: } catch (Exception e) { > 63: throw new SkippedException("GTK LAF is not supported on this > system;" + > 64: " test passes"); Suggestion: throw new SkippedException("GTK LAF is not supported on this system"); I'd remove the additional message. I believe it was `println` before. test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 116: > 114: } > 115: > 116: private void onBackgroundThread20() { This method should run on EDT because it accesses the state of the components. test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 124: > 122: Point loc = ref.getLocationOnScreen(); > 123: Rectangle refRect = > 124: new Rectangle(loc.x, loc.y, ref.getWidth(), > ref.getHeight()); Suggestion: Rectangle refRect = new Rectangle(loc, ref.getSize()); test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 133: > 131: Rectangle testRect = > 132: new Rectangle(loc.x, loc.y, > 133: test.getWidth(), test.getHeight()); Suggestion: loc = test.getLocationOnScreen(); Rectangle testRect = new Rectangle(test.getLocationOnScreen(), test.getSize()); Choose one style and adjust both statements, `refRect` and `testRect`. test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136: > 134: BufferedImage testimg = > robot.createScreenCapture(testRect); > 135: > 136: if (refimg.getWidth() != testimg.getWidth() I suggest moving the part that compares the images into a helper method. Alternatively, you can use [`Util.compareBufferedImages`](https://github.com/openjdk/jdk/blob/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L80). test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 137: > 135: > 136: if (refimg.getWidth() != testimg.getWidth() > 137: || refimg.getHeight() != testimg.getHeight()) Suggestion: if (refimg.getWidth() != testimg.getWidth() || refimg.getHeight() != testimg.getHeight()) One more space, it should be aligned to the inside of the parenthesis. test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 148: > 146: if (refPixel != testPixel) { > 147: fail("Image comparison failed at (" + > 148: x + "," + y + ") for image " + index); Adding `count` and `k` to diagnostic message could help identifying the failing case quicker. ------------- PR Review: https://git.openjdk.org/jdk/pull/19381#pullrequestreview-2096399951 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626048836 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626007721 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626017821 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626017048 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626024722 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626046506 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626023890 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626048214
