<Swing Dev> Review Request: JDK-7012008 JDesktopPane - Wrong background color with Win7+WindowsLnf

[Alexey Ivanov]

Hi Prem,

Thank you for updating the test, it looks much simpler now.

Could you please move the comment with JTreg @test tags to the class 
declaration? I mean put it below imports.

The array of look-and-feels to test would be better readable if 
formatted like this:

         String[] lookAndFeel = new String[] {
             "com.sun.java.swing.plaf.windows.WindowsLookAndFeel",
"com.sun.java.swing.plaf.windows.WindowsClassicLookAndFeel"
         };

Don't you agree?

I'm sure the line
         desktopPane.setVisible(true)
is redundant in this case.

Why do you catch the four exceptions in main? I see no reason to catch 
any exceptions in main: an exception will fail the test and you will see 
the real problem rather than the generic message "Setting 
WindowsLookAndFeel Failed".


Just to confirm: Does the test pass on Linux or Mac?

I also suggest renaming 'lookAndFeel1' variable in loop to 'laf'.

Regards,
Alexey

On 10.03.2016 9:45, Prem Balakrishnan wrote:
>
> Hi Semyon and Alexey,
>
> Thankyou for the Review.
>
> I have updated the test as per review comments.
>
> http://cr.openjdk.java.net/~arapte/prem/7012008/webrev.01/ 
> <http://cr.openjdk.java.net/%7Earapte/prem/7012008/webrev.01/>
>
> Regards,
> Prem
>
> *From:*Alexey Ivanov
> *Sent:* Wednesday, March 09, 2016 3:58 PM
> *To:* Prem Balakrishnan; Rajeev Chamyal; Sergey Bylokhov; Semyon 
> Sadetsky; swing-dev at openjdk.java.net
> *Subject:* Re: <Swing Dev> Review Request: JDK-7012008 JDesktopPane - 
> Wrong background color with Win7+WindowsLnf
>
> Hi Prem,
>
> The fix itself looks good.
> Yet I have some questions to the test.
>
> Could you please place the comment with @test tag and other before the 
> class declaration rather than before imports?
> Could you place asterisk in front of each line in this comment block? 
> It will look consistent with Java style doc comments and other tests.
>
> Do you really need to create UI?
> Wouldn't it be enough to create JDesktopPane and check its background 
> color? I believe it will significantly simplify the test.
>
> I think converting Color to ColorUIResource is unnecessary here. 
> ColorUIResource is Color decorated with UIResource interface: 
> Color.equals method does not take this distinction into account.
>
> In main(), you can create and initialize the LaF array in one go.
>
> In executeTest(), you should dispose mainFrame on EDT rather than on 
> main thread. I'd recommend using finally block in run() to dispose 
> mainFrame: it will handle both success and failure.
>
> Additionally, you access variables from different threads. The way the 
> code is written now does not guarantee memory consistency between the 
> different threads, thus the test could fail or pass sporadically.
>
> And I agree with Semyon, the test should fail on other platforms, you 
> should just skip the test.
>
>
> Regards,
> Alexey
>
> On 09.03.2016 9:59, Prem Balakrishnan wrote:
>
>     Hi*,*
>
>     Please review fix for JDK9,
>
>     *Bug:*https://bugs.openjdk.java.net/browse/JDK-7012008**
>
>     *Webrev:*http://cr.openjdk.java.net/~arapte/prem/7012008/webrev.00/ <http://cr.openjdk.java.net/%7Earapte/prem/7012008/webrev.00/>
>
>     *Issue:*
>
>     JDesktopPane - Wrong background color with Win7+WindowsLnf
>
>     *Cause:*
>
>     Desktop.background property set to "win.desktop.backgroundColor"
>
>     *Fix:*
>
>     Changed Desktop.background property  to "win.mdi.backgroundColor"
>
>     Regards,
>     Prem
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160310/b5a4aad4/attachment.html>