<Swing Dev> [9] Review request for 8057791: Selection in JList is drawn with wrong colors in Nimbus L&F

[Alexander Potochkin]

Hello Anton

I like the latest version of this fix!
It looks good to me.

Thanks
alexp

On 6/21/2016 20:55, Anton Litvinov wrote:
> Hello Sergey and Semyon,
>
> Sergey, thank you for review of this fix. In the 1st version of the 
> fix I decided to return "null", because at least one implementation of 
> "javax.swing.plaf.synth.SynthStyle" which is 
> "sun.swing.plaf.synth.DefaultSynthStyle" returns "null" from its 
> implementation of "getColorForState(SynthContext, ColorType)", but 
> while I worked on the fix, I did not read the comment in 
> "NimbusStyle.java" which you quoted. Yes, I agree that it is incorrect 
> to return "null" from the method "NimbusStyle.getColorForState".
>
> Adding support of "Selected", "Disabled+Selected" states to 
> "List.cellRenderer" component in 
> "src/java.desktop/share/classes/javax/swing/plaf/nimbus/skin.laf" file 
> was considered before the review request of the 1st fix version, but 
> it was defined that in "SwingSet2" demo selected items in the list 
> were still drawn with wrong colors. However today I defined the reason 
> of this issue in "SwingSet2" demo, and it is caused by the fact that 
> in the demo a custom extension of 
> "javax.swing.DefaultListCellRenderer" class is used instead of the 
> default "javax.swing.plaf.synth.SynthListUI.SynthListCellRenderer" 
> which is capable of setting "SynthConstants.SELECTED" bit mask to its 
> state through the call sequence 
> "SynthListCellRenderer.getListCellRendererComponent" --> 
> "SynthLookAndFeel.setSelectedUI".
>
> The 2nd version of the fix was created. Could you please review the 
> 2nd version of the fix.
>
> Webrev 01: http://cr.openjdk.java.net/~alitvinov/8057791/jdk9/webrev.01
>
> In the new fix version the regression test stayed unchanged. The new 
> fix version adds support of "Selected", "Disabled+Selected" states to 
> "List.cellRenderer" component in 
> "src/java.desktop/share/classes/javax/swing/plaf/nimbus/skin.laf" to 
> make "NimbusStyle.getColorForState" method return correct colors for 
> "SynthListCellRenderer" instances used in "JList" in Nimbus L&F.
>
> The following 2 sets of regression tests were executed on Windows 
> platform:
> 1. Automatic regression tests from open and closed sets located in 
> "javax/swing/JList", "javax/swing/plaf/nimbus" directories.
> 2. JCK tests:
>     api/javax_swing/CellRendererPane
>     api/javax_swing/DefaultCellEditor
>     api/javax_swing/DefaultListCellRenderer
>     api/javax_swing/DefaultListModel
>     api/javax_swing/DefaultListSelectionModel
>     api/javax_swing/JList
>     api/javax_swing/plaf/ListUI
>     api/javax_swing/plaf/nimbus
>     api/javax_swing/plaf/synth/SynthListUI
>
> Thank you,
> Anton
>
> On 6/20/2016 3:13 PM, Sergey Bylokhov wrote:
>> Hi, Anton.
>> I am a little bit worried about the change in 
>> NimbusStyle.getColorForState(). Before the fix we intentionally never 
>> return null from this method, but return DEFAULT_COLOR which has this 
>> javadoc:
>>     /**
>>      * <p>The Color to return from getColorForState if it would 
>> otherwise have
>>      * returned null.</p>
>>      *
>>      * <p>Returning null from getColorForState is a very bad thing, 
>> as it causes
>>      * the AWT peer for the component to install a SystemColor, which 
>> is not a
>>      * UIResource. As a result, if <code>null</code> is returned from
>>      * getColorForState, then thereafter the color is not updated for 
>> other
>>      * states or on LAF changes or updates. This DEFAULT_COLOR is 
>> used to
>>      * ensure that a ColorUIResource is always returned from
>>      * getColorForState.</p>
>>      */
>>
>> And after the fix this method will return null for all ListCellRenderer.
>>
>> Can you please clarify why the change states of "List.cellRenderer" 
>> in plaf/nimbus/skin.laf does not solve the problem?
>>
>>> On 6/13/2016 12:12 AM, Anton Litvinov wrote:
>>>> Hello,
>>>>
>>>> Could you please review the following fix for the bug.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8057791
>>>> Webrev: http://cr.openjdk.java.net/~alitvinov/8057791/jdk9/webrev.00
>>>>
>>>> The bug is the fact that selected elements in "javax.swing.JList"
>>>> component are drawn with wrong background and foreground colors,
>>>> though "JList.getSelectionForeground()",
>>>> "JList.getSelectionBackground()" methods return the correct color
>>>> values for the tested JList instance.
>>>>
>>>> Wrong colors are returned in run time from the method
>>>> "javax.swing.plaf.synth.SynthStyle.getColor" for
>>>> "javax.swing.plaf.synth.SynthListUI.SynthListCellRenderer" instance,
>>>> because it can be only in 1 state "SynthConstants.ENABLED" and cannot
>>>> be in "SynthConstants.SELECTED" state, therefore the call from this
>>>> method to
>>>> "javax.swing.plaf.nimbus.NimbusStyle.getColorForState(SynthContext,
>>>> ColorType)" method always returns some wrong default L&F foreground,
>>>> background colors which are observed in this bug.
>>>>
>>>> All automatic regression tests from open and closed sets located in
>>>> "javax/swing/JList", "javax/swing/plaf/nimbus" directories were run on
>>>> MS Windows 7 OS during verification of the fix.
>>>>
>>>> Thank you,
>>>> Anton
>>>
>>
>>
>