<Swing Dev> [9] Review Request: [JDK-8169879] [TEST_BUG] javax/swing/text/GlyphPainter2/6427244/bug6427244.java - compilation failed

Wed Nov 30 04:05:58 UTC 2016

Thank you for your time in review Prasanta. 

Please find my responses inline.

> I guess JComponent.isVisible() does not throw IllegalStateException so 
> 141             } catch (IllegalStateException ex) {
> is not valid here.

As you rightly pointed, IllegalStateException is not invoked by JComponent.isVisble() method. 
Hence this has been corrected in the new webrev.

> I also think there is no need of storing in array as we are supposed to get only one point from getLocationOnScreen().

The code is absolutely normal. If we use a local variable (non-array) we will end up with a compilation error saying - " local variables accessed from inner class should be final or effectively final. " The only way you could work around this compilation error is by making the local variable- member of class. I prefer to have the code without deviating much from the author 's original code unless a bizzare bug exists.

The new changes are available for review under:
http://cr.openjdk.java.net/~pnarayanan/8169879/webrev.03/

Kindly review at your convenience & share feedback.

Thank you
Have a good day

Prahalad N.

From: Prasanta Sadhukhan 
Sent: Tuesday, November 29, 2016 7:15 PM
To: Prahalad Kumar Narayanan; swing-dev at openjdk.java.net
Cc: Sergey Bylokhov
Subject: Re: [9] Review Request: [JDK-8169879] [TEST_BUG] javax/swing/text/GlyphPainter2/6427244/bug6427244.java - compilation failed

I guess JComponent.isVisible() does not throw IllegalStateException so 
141             } catch (IllegalStateException ex) {
is not valid here. Probably you can call JComponent.getLocationOnScreen() which throws IllegalStateException in blockTillDisplayed() and update "Point".  
I also think there is no need of storing in array as we are supposed to get only one point from getLocationOnScreen().

Regards
Prasanta
On 11/29/2016 10:19 AM, Prahalad Kumar Narayanan wrote:
Hello Everyone

Request your time in reviewing the fix for - Bug ID: JDK-8169879

Bug Description: 
. A jtreg test case failed with a compilation error.

Fix Description:
. Prior to raising the review under the swing umbrella, the fix went through few reviews under 2D group.
. In the earlier fix revisions,
          . The compiler error was fixed and 
          . Dispose method was trigged on the JFrame object upon completion of the test case
. The subsequent suggestion was to include all swing API calls on EDT thread
          . This is addressed in the current fix.

. Changes are available for your review. 
Link: http://cr.openjdk.java.net/~pnarayanan/8169879/webrev.02/

Kindly review at your convenience and provide your suggestions

Thank you
Have a good day

Prahalad N.

-----Original Message-----
From: Phil Race 
Sent: Tuesday, November 22, 2016 12:21 AM
To: Prahalad Kumar Narayanan; Prasanta Sadhukhan; Sergey Bylokhov; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [9] Review Request: [JDK-8169879] [TEST_BUG] javax/swing/text/GlyphPainter2/6427244/bug6427244.java - compilation failed

The test does not have the "manual" tag/key so it is intended to be run as an automated test. User interaction will break a lot of automated tests that display a UI. Without looking at the specifics of this one, I'd guess it likely that it just an example of that .. rather than a unique problem.

-phil

On 11/21/2016 04:56 AM, Prahalad Kumar Narayanan wrote:
Thank you for your time in review Prasanta.
Appreciate your views.

As you noticed, I observed test-failures as well ( by simply moving the mouse )
        . There are quite some improvements that could be done to this test-case
        . But we could address them in a new bug
        . The current bug fix and related bug (JDK-8166003) relate to fixing critical compilation error that prevents the execution of the test-case.

Secondly blockTillDisplayed(tp) holds the current thread with Thread.sleep(). In this case, the Main thread is held from proceeding further. So it's better executed in non EDT thread. I've seen many swing test-cases that deploy same logic ( Correct me if my understanding is wrong ).

Thank you
Have a good day

Prahalad N.

-----Original Message-----
From: Prasanta Sadhukhan
Sent: Monday, November 21, 2016 3:48 PM
To: Prahalad Kumar Narayanan; Sergey Bylokhov; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [9] Review Request: [JDK-8169879] 
[TEST_BUG] javax/swing/text/GlyphPainter2/6427244/bug6427244.java - 
compilation failed

It seems if user minimize/maximize the frame (which is allowed as per testcase), then the test might fail. I guess we can set the frame to "undecorated" to prevent the user to play with minimize/maximize/close.

Also, I am not sure if we can call

blockTillDisplayed(tp);

outside EDT. Maybe Sergey can comment on that.

Lastly, this review should have been done in swing-dev and not 2d-dev.

Regards
Prasanta
On 11/21/2016 1:11 PM, Prahalad Kumar Narayanan wrote:
Thank you Prasanta & Sergey for your time in review.

Yes. As you pointed, the test file did not have JFrame.dispose() call at the completion of the test.
This has been added now and the changes are available for review.

Webrev Link: 
http://cr.openjdk.java.net/~pnarayanan/8169879/webrev.01/
Kindly review the code at your convenience

Thank you
Have a good day

Prahalad N.

-----Original Message-----
From: Sergey Bylokhov
Sent: Friday, November 18, 2016 7:42 PM
To: Prasanta Sadhukhan; Prahalad Kumar Narayanan; 
2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [9] Review Request: [JDK-8169879] 
[TEST_BUG] javax/swing/text/GlyphPainter2/6427244/bug6427244.java - 
compilation failed

On 18.11.16 14:33, Prasanta Sadhukhan wrote:
I guess in this test too

JFrame should be disposed at the end of the test(when the test 
passed or
failed)
Correct, good catch.

Regards
Prasanta
On 11/18/2016 2:06 PM, Sergey Bylokhov wrote:
Looks fine.

On 18.11.16 10:32, Prasanta Sadhukhan wrote:
+1

Regards
Prasanta
On 11/18/2016 12:24 PM, Prahalad Kumar Narayanan wrote:
Hello Everyone

Good day to you

Request your time in reviewing the fix for an error that I 
introduced in my submission.
         Bug ID: [JDK-8169879] [TEST_BUG] 
javax/swing/text/GlyphPainter2/6427244/bug6427244.java - 
compilation failed
         Relates to: [JDK-8166003] [PIT][TEST_BUG] missing helper 
for javax/swing/text/GlyphPainter2/6427244/bug6427244.java

Root Cause:
. I had missed to merge one-line of code while raising the webrev 
for JDK-8166003.
. This caused the compilation error to persist.
. Kindly excuse for this mistake.

Fix:
. The correction has been done
. The change has been verified to work on win7, ubuntu 14, and 
max sierra OS.

. Kindly review the change at your convenience.
            . Webrev link:
http://cr.openjdk.java.net/~pnarayanan/8169879/webrev.00/

Thank you for your time in review Have a good day

Prahalad N.
--
Best regards, Sergey.