Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Mon, 3 Jun 2024 05:48:18 GMT, Abhishek Kumar wrote: >> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626008412
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Fri, 31 May 2024 20:47:52 GMT, Alexey Ivanov wrote: >> Updated. > > 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. > To make the continuation line stand out as continuation line, place the || at > the start of wrapped line. Updated. - PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1623813651 PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1623814147
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Fri, 31 May 2024 04:39:24 GMT, Abhishek Kumar wrote:
>> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136:
>>
>>> 134: if (refimg.getWidth() != testimg.getWidth() ||
>>> 135: refimg.getHeight() != testimg.getHeight())
>>> 136: {
>>
>> 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"
> https://github.com/openjdk/jdk/pull/18703#discussion_r1571126135.
>
> 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1622939563
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Fri, 31 May 2024 05:02:16 GMT, Abhishek Kumar wrote:
>> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 70:
>>
>>> 68: Class
>>> type)
>>> 69: throws Throwable
>>> 70: {
>>
>> 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 type)`
>
> Updated.
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).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1622937272
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Thu, 30 May 2024 19:24:32 GMT, Alisen Chung wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> background method removed
>
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 61:
>
>> 59: "com.sun.java.swing.plaf.gtk.GTKLookAndFeel");
>> 60: } catch (Exception e) {
>> 61: System.out.println("GTK LAF is not supported on this system;
>> test passes");
>
> should this be jtreg.testSkipped exception?
Updated.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 70:
>
>> 68: Class
>> type)
>> 69: throws Throwable
>> 70: {
>
> 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 type)`
Updated.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 110:
>
>> 108: }
>> 109:
>> 110: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621720308
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621720428
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621720186
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Thu, 30 May 2024 19:59:40 GMT, Alisen Chung wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> background method removed
>
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136:
>
>> 134: if (refimg.getWidth() != testimg.getWidth() ||
>> 135: refimg.getHeight() != testimg.getHeight())
>> 136: {
>
> 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"
https://github.com/openjdk/jdk/pull/18703#discussion_r1571126135.
So I left like that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621700494
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Thu, 30 May 2024 07:33:31 GMT, Abhishek Kumar 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:
>
> background method removed
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 61:
> 59: "com.sun.java.swing.plaf.gtk.GTKLookAndFeel");
> 60: } catch (Exception e) {
> 61: System.out.println("GTK LAF is not supported on this system;
> test passes");
should this be jtreg.testSkipped exception?
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 70:
> 68: Class type)
> 69: throws Throwable
> 70: {
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 type)`
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 110:
> 108: }
> 109:
> 110: private void onEDT10() {
is this method required? can it be rewritten to not use onEDT10 and
onBackgroundThread20?
test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136:
> 134: if (refimg.getWidth() != testimg.getWidth() ||
> 135: refimg.getHeight() != testimg.getHeight())
> 136: {
move bracket to previous line to be consistent
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621332748
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621363444
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621361476
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621364434
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
> 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: background method removed - Changes: - all: https://git.openjdk.org/jdk/pull/19381/files - new: https://git.openjdk.org/jdk/pull/19381/files/63ea19e7..2d836f01 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19381&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19381&range=01-02 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19381.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19381/head:pull/19381 PR: https://git.openjdk.org/jdk/pull/19381
Re: RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v3]
On Thu, 30 May 2024 07:16:20 GMT, Tejesh R wrote: >> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1620138304
