3) robot.delay(1000) in executeTest() is unnecessary, isn't it?
[Rajeev Chamyal] Removed the robot.delay and added robot.waitForIdle() call.
4) You change the code for Synth and Aqua LaF but your test does not cover
them. I suggest iterating over all installed LaFs and run the test case in all
the LaFs.
Does it make it sense to print the current LaF at least for the failed test?
[Rajeev Chamyal] Test prints all exception messages along with failed LaF
names on test completion.
5) Shouldn't test fail if PropertyVetoException is thrown for f.setIcon(true)?
I believe it's not expected in this case.
[Rajeev Chamyal] PropertyVetoException exception is considered test failure.
Regards,
Rajeev Chamyal
-----Original Message-----
From: Alexey Ivanov
Sent: 28 December 2015 16:53
To: Rajeev Chamyal; swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> Review request for JDK-4769772
JInternalFrame.setIcon(true) before JDesktopPane.add(JIF) causes wrong
state
Hi Rajeev,
Thank you for updating the code and the test.
Please see my comments inline.
On 28.12.2015 10:37, Rajeev Chamyal wrote:
Hello Alexey,
Thanks for the review. I have updated the code as suggested.
http://cr.openjdk.java.net/~rchamyal/4769772/webrev.02/
Please see response to questions inline.
Regards,
Rajeev Chamyal
-----Original Message-----
From: Alexey Ivanov
Sent: 25 December 2015 18:14
To: Rajeev Chamyal; swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> Review request for JDK-4769772
JInternalFrame.setIcon(true) before JDesktopPane.add(JIF) causes
wrong state
Hi Rajeev,
Thank you for updating the code.
I have a couple more questions though.
1. SynthDesktopPaneUI
What is the purpose of this line?
d.setBounds(r.x, r.y, r.width, r.height);
Since d is a JDesktopPane which contains the frame, you change the location and
size of the desktop pane to those of the iconified frame.
It doesn't seem right to me. Could you please clarify your intention?
[Rajeev Chamyal] It's a typo updated the code. We need to set bounds for
desktopIcon instead of desktop pane.
I suggest moving these lines:
if (c == null || d == null) {
return;
}
above calculations of desktopIcon position. Why shall we waste time calculating
the position if we ignore the operation eventually?
2. dispose() method is never used in the test, so it could be removed.
[Rajeev Chamyal] Removed from test code.
3. Since testFailed field is accessed from different threads, it should be
marked volatile.
[Rajeev Chamyal] Test updated to check all LAF. testFailed variable is not used
now.
Yet your new errorMessage filed has the same issue. It should be marked
volatile since it's accessed from different threads without synchronization.
Actually I suggest storing the exception itself rather than the error message
only. If exception is null, the test passed, if it's not, the test failed and
you just print the exception and re-throw it to the test framework. What do you
think?
4. robot.delay(1000) in executeTest() is unnecessary, isn't it?
[Rajeev Chamyal] As its UI test so added delay for user to check visually as
well.
Think about it this way: you have, let's say, 20 automated UI tests which are
similar to this one. There are five LaFs available on Windows, so the test
takes at least 12 seconds. Thus you need at least 4 minutes to execute all 20
tests which is spent in delays. I don't think it's good.
So I propose to remove all delays. If the test fails, the user could add a
delay to visually inspect the result of the test.
At the same time, to make test more stable, you should keep
robot.waitForIdle();
after createUI() which was in the test code in the previous webrevs.
5. You change the code for Synth and Aqua LaF but your test does not cover
them. I suggest iterating over all installed LaFs and run the test case in all
the LaFs.
[Rajeev Chamyal] Updated the test for all LAF's
Does it make it sense to print the current LaF at least for the failed test?
6. Shouldn't test fail if PropertyVetoException is thrown for f.setIcon(true)?
I believe it's not expected in this case.
[Rajeev Chamyal] Updated code. But this exception is not expected.
Since this exception is not expected, it could be considered a test failure.
Regards,
Alexey
7. Probably any exception thrown during test execution should also be
considered as failure, otherwise catch in executeTest().Runnable{}.run() is
redundant.
[Rajeev Chamyal] Test code is updated to handle all exceptions as failures.
Regards,
Alexey
On 24.12.2015 10:19, Rajeev Chamyal wrote:
Hello Alexey,
Thanks for the review.
I have updated webrev as per review comments.
http://cr.openjdk.java.net/~rchamyal/4769772/webrev.01/
Regards,
Rajeev Chamyal
-----Original Message-----
From: Alexey Ivanov
Sent: 23 December 2015 19:27
To: swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> Review request for JDK-4769772
JInternalFrame.setIcon(true) before JDesktopPane.add(JIF) causes
wrong state
Hi Rajeev,
One more comment:
You should call dispose() in finally block in main so that the frame is closed
automatically if the test fails.
Regards,
Alexey
On 23.12.2015 16:46, Alexey Ivanov wrote:
Hi Rajeev,
There's a potential NullPointerException in this line of
BasicInternalFrameUI.java:
if(value.equals(Boolean.FALSE))
I suggest eliminating it using this construct:
if (Boolean.FALSE.equals(value))
This way the code is safer.
Can the test be simplified? Is it really required to create menu
and use robot to invoke commands?
Wouldn't it be enough to programmatically add minimized internal
frame and then check its properties?
JFrame in test should also be instantiated on EDT, in createUI() method.
And a general recommendation to follow Java Coding Style [1] and in
particular to add a space [2] after 'if', after comma in argument
lists, and after cast operator.
Regards,
Alexey
[1] http://www.oracle.com/technetwork/java/codeconvtoc-136057.html
[2]
http://www.oracle.com/technetwork/java/javase/documentation/codecon
v
e
n
tions-141388.html#682
On 18.12.2015 11:44, Rajeev Chamyal wrote:
Hello All,
Please review the following fix for Jdk9:
Bug: https://bugs.openjdk.java.net/browse/JDK-4769772
Webrev:http://cr.openjdk.java.net/~rchamyal/4769772/webrev.00/
<http://cr.openjdk.java.net/%7Erchamyal/4769772/webrev.00/>
Issue: Iconifying a frame before adding it to desktop pane is not
working.
Cause: Setting setIcon property of a JInternalFrame before
addition to desktop pane fails to find the desktop as a result its
always in maximized state.
Fix: Added method to check if frame is already iconified before
addition.
Verified the fix on windows,Ubuntu and Mac with all layouts.
Also, removed unused imports from JDesktopPane.java as part of fix.
Regards,
Rajeev Chamyal
