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

[Alexey Ivanov]

Hi Prem,

Looks good.
Thanks!

On 10.03.2016 13:57, Prem Balakrishnan wrote:
>
> Hi Alexey,
>
> Updated the test as per review comments.
>
> http://cr.openjdk.java.net/~arapte/prem/7012008/webrev.02/ 
> <http://cr.openjdk.java.net/%7Earapte/prem/7012008/webrev.02/>
>
> *JTreg @test tags:*
>
> As per TAG SYNTAX, Test tags must be enclosed in a comment at the head 
> of the file, refer below link
>
> *http://openjdk.java.net/jtreg/tag-spec.html#TAG_SYNTAX , *
>
> and we are following the same conventions in all the JDK9 tests.
>
I see. I remember I saw some tests had this JTreg tag comments before 
the class, and I find it more convenient because IDE doesn't collapse 
the comment automatically.
>
> *Just to confirm: Does the test pass on Linux or Mac?*
>
> *@requires (os.family == "windows") is added to tag, *
>
> which doesn’t allow the following test to run on any other platform 
> except windows.
>
> (When executed using JTreg)
>
Thanks! I haven't seen that notation before, I just wanted to be sure.


Regards,
Alexey
>
> Regards,
> Prem
>
> *From:*Alexey Ivanov
> *Sent:* Thursday, March 10, 2016 3:07 PM
> *To:* Prem Balakrishnan; Semyon Sadetsky; Rajeev Chamyal; Sergey 
> Bylokhov; swing-dev at openjdk.java.net
> *Subject:* Re: <Swing Dev> Review Request: JDK-7012008 JDesktopPane - 
> Wrong background color with Win7+WindowsLnf
>
> 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
>     <mailto: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/5236bd50/attachment.html>