<Swing Dev> Review request for JDK-4769772 JInternalFrame.setIcon(true) before JDesktopPane.add(JIF) causes wrong state

[Rajeev Chamyal]

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 at 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.

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.

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.

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

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.

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 at 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/codeconve
>> 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
>>>