Hello All, I need one more review for this webrev. http://cr.openjdk.java.net/~rchamyal/4769772/webrev.03/
Regards, Rajeev Chamyal -----Original Message----- From: Alexey Ivanov Sent: 28 December 2015 19:15 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, The updated version looks good to me. Regards, Alexey On 28.12.2015 16:25, Rajeev Chamyal wrote: > Hello Alexey, > > Thanks for the review. I have updated the code. > http://cr.openjdk.java.net/~rchamyal/4769772/webrev.03/ > > 1) 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? > [Rajeev Chamyal] Updated the code as suggested. > > 2) 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? > [Rajeev Chamyal] Test updated to make errorMessage volatile. errorMessage > stores all exception messages which may occur in different LAFs and on test > completion message is thrown with RunTimeException to mark test failure. I got your idea: you iterate over all LaFs accumulating failed LaFs rather than fail the test at the first failure. > > 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 >>>>>
