Re: 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: 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: 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 previ
Re: Review request for 8041894: Test javax/swing/JSpinner/8008657/bug8008657.java failed on Mac
Hi, Avik On 21/12/15 07:17, Avik Niyogi wrote: Hi Sergey, I verified that the issue is only with Aqua Look and feel and not other look and feels. It will be better to prove it in the test, so the bug will not occur again in some other/new L&F. The type cast for ComponentOrientation was done in the code just for two reasons: 1) User readability for the component within the event e. 2) The cast for which type it is specified. For example, it can be noted that in the older code, This is unrelated to the code style. The cases which you selected are necessary because we pass these values to the replaceEditor(), note that parameters of replaceEditor are JComponent, so we must cast in these cases. Same for (ComponentOrientation) e.getNewValue() because we pass it to applyComponentOrientation(ComponentOrientation), but in case of getOldValue it is not necessary because we compare it to null only. Also please do not remove the empty line before package com.apple.laf; if ("editor".equals(propertyName)) { final JComponent oldEditor = *(JComponent)* e.getOldValue(); final JComponent newEditor = *(JComponent)* e.getNewValue(); ui.replaceEditor(oldEditor, newEditor); ui.updateEnabledState(); } else if ("componentOrientation".equals(propertyName)) { ComponentOrientation o = (ComponentOrientation) e.getNewValue(); if (o != (ComponentOrientation) e.getOldValue()) { JComponent editor = spinner.getEditor(); if (editor != null) { editor.applyComponentOrientation(o); } spinner.revalidate(); spinner.repaint(); } Casting of JComponent is done explicitly for the values there. Maintaining same standard and scoping out uncertainty seems correct in the context. With Regards, Avik Niyogi On 19-Dec-2015, at 4:47 am, Sergey Bylokhov mailto:sergey.bylok...@oracle.com>> wrote: Hi, Avik. Looks fine to me. It seems that the cast here is not necessary? if (o != (ComponentOrientation) e.getOldValue()) Can you confirm that this bug not affectes our other looks and feels? It will be good to update the test to prove that. On 17/12/15 10:21, Avik Niyogi wrote: Hi All, Kindly review the fix for JDK9. *Bug*:https://bugs.openjdk.java.net/browse/JDK-8041894 *Webrev*: http://cr.openjdk.java.net/~aniyogi/8041894/webrev.00/ *Issue*: Test case throws an exception as subcomponents of were not getting orientation property assignment. *Cause*: The case where property name as Component orientation not present to traverse the property to subcomponents of spinner for Aqua look and feel. *Fix*: The else if clause for this property name was added while checking for for editor to take charge of applying the component orientation to all subsequent subcomponents. With Regards, Avik Niyogi -- Best regards, Sergey. -- Best regards, Sergey.
Re: Review request for JDK-4769772 JInternalFrame.setIcon(true) before JDesktopPane.add(JIF) causes wrong state
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. 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: 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: 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 i
Re: 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: 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: 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/code