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?
2. dispose() method is never used in the test, so it could be removed.
3. Since testFailed field is accessed from different threads, it should
be marked volatile.
4. robot.delay(1000) in executeTest() is unnecessary, isn't it?
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.
6. Shouldn't test fail if PropertyVetoException is thrown for
f.setIcon(true)? I believe it's not expected in this case.
7. Probably any exception thrown during test execution should also be
considered as failure, otherwise catch in executeTest().Runnable{}.run()
is redundant.
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/codeconven
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
