On Tue, 17 Aug 2021 15:50:56 GMT, lawrence.andrews
<github.com+87324768+lawrence-and...@openjdk.org> wrote:
>> 1) Automated the manual test case.
>> 2) Removed html dependent file
>> 3) Removed javax.swing.JApplet dependent.
>> 4) Test case can be executed independently as well with jtreg framework.
>> 5) Added methods to know that JFrame and Other component is visible before
>> starting the user action via Robot.
>>
>> @shurymury
>
> lawrence.andrews has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixed review comments
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/im/4959409/bug4959409.java line 24:
> 22: */
> 23:
> 24: /**
I suggest to remove the second `*` to avoid the warning from the IDE about the
dangling Javadoc comment.
test/jdk/java/awt/im/4959409/bug4959409.java line 65:
> 63: final CountDownLatch keyPressedEventLatch = new CountDownLatch(1);
> 64: final Point points[] = new Point[1];
> 65: final Rectangle rect[] = new Rectangle[1];
Suggestion:
final Point[] points = new Point[1];
final Rectangle[] rect = new Rectangle[1];
The IDE issues a warning about C-style array declaration.
test/jdk/java/awt/im/4959409/bug4959409.java line 103:
> 101: } else {
> 102: jLabel.setText("Did not received KEY PRESS for
> Shift+1");
> 103: System.out.println("Did not received KEY PRESS
> for Shift+1");
Suggestion:
jLabel.setText("Did not receive KEY PRESS for Shift+1");
System.out.println("Did not receive KEY PRESS for
Shift+1");
Probably it makes sense to spell “KEYPRESS”, without the space, for consistency.
test/jdk/java/awt/im/4959409/bug4959409.java line 147:
> 145:
> 146: if (!keyPressedEventLatch.await(TIMEOUT, TimeUnit.SECONDS)) {
> 147: throw new RuntimeException("Did not received KEY PRESS for
> Shift + 1 , test failed");
Suggestion:
throw new RuntimeException("Did not receive KEY PRESS for Shift + 1
, test failed");
And the same for “KEY PRESS”.
test/jdk/java/awt/im/4959409/bug4959409.java line 162:
> 160: }
> 161:
> 162: public static void checkSwingTopLevelVisible(javax.swing.JFrame
> jFrame, CountDownLatch topLevelVisibleLatch) throws InterruptedException,
> InvocationTargetException {
Is there a reason why you use the fully qualified `javax.swing.JFrame` when
it's imported?
I suggest wrapping `throws` declaration to next line; this line is longer than
100.
Both comments apply to the method below.
test/jdk/java/awt/im/4959409/bug4959409.java line 170:
> 168: }
> 169:
> 170: public static boolean isTopLevelVisible(javax.swing.JFrame jFrame,
> CountDownLatch topLevelVisibleLatch) throws Exception {
The logic in this method polls `jFrame.isVisible` instead of waiting for it to
become visible. Nothing wrong with this approach yet the last call
`topLevelVisibleLatch.await(TIMEOUT, TimeUnit.SECONDS)` doesn't make sense in
this case as the latch can't reach 0 if it's not zero.
So, if something goes wrong, the test still waits for `TIMEOUT` (30) seconds
for the condition that can never be satisfied. I suggest returning
`topLevelVisibleLatch.getCount() == 0` and save 30 seconds.
Probably it's enough to check whether the text field is visible. The text field
can become visible only if frame is visible, both likely become visible at the
same moment anyway.
test/jdk/java/awt/im/4959409/bug4959409.java line 177:
> 175: TimeUnit.SECONDS.sleep(1);
> 176: checkSwingTopLevelVisible(jFrame, topLevelVisibleLatch);
> 177: if (topLevelVisibleLatch.getCount() == 0) {
Maybe this condition could be in the while loop?
while (count <= 5 && topLevelVisibleLatch.getCount() != 0)
Then there's no need for explicit premature break from the loop.
test/jdk/java/awt/im/4959409/bug4959409.java line 186:
> 184: }
> 185:
> 186: public static void checkJComponentVisible(javax.swing.JComponent
> jComponent, CountDownLatch componentVisibleLatch) throws
> InterruptedException, InvocationTargetException {
You're using fully qualified `javax.swing.JComponent` here: `JComponent` isn't
imported. Why not import it?
Please also wrap `throws` declaration to the next line.
test/jdk/java/awt/im/4959409/bug4959409.java line 216:
> 214: if (frame != null) {
> 215: SwingUtilities.invokeAndWait(frame::dispose);
> 216: }
This has inconsistency where `frame != null` is run on main thread without
synchronisation. The null check should also be run on the EDT as I originally
suggested.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5058
