[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-swing-dev
Subject:    Re: <Swing Dev> [9] Review request for 8051548 JColorChooser should have a way to disable transparen
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2015-04-27 15:15:12
Message-ID: 553E5280.6040402 () oracle ! com
[Download RAW message or body]

Hi, Alexander.
The fix looks fine.

On 21.04.15 18:32, Alexander Scherbatiy wrote:
>
>  Could you review the updated fix:
>    http://cr.openjdk.java.net/~alexsch/8051548/webrev.02/
>
> On 4/20/2015 6:34 PM, Sergey Bylokhov wrote:
>> Hi, Alexander.
>> A few notes:
>>  - Does this property control the "enable/disable" status or 
>> "visible/invisible" status?
>       Both. Setting invisible status means the getColor() returns 
> color ignoring alpha value.
>>  - Why the string property is soo long? Can it be simplified? 
>> Probably it will be good to wrap it using new String()?
>      The property is renamed to TRANSPARENCY_ENABLED_PROPERTY
>>  - Why SlidingSpinner.isVisible does not check the status of 
>> label/spinner?
>      Slider, label and spinner should always have the same visible state.
>>  - What about a new constructor which was requested in the CR also?
>     I have added the colorTransparencySelectionEnabled argument to the 
> JColorChooser.showDialog() method and updated the test.
>
>   Thanks,
>   Alexandr.
>>
>>
>> On 20.04.15 18:10, Alexander Scherbatiy wrote:
>>>
>>>  Hello,
>>>
>>>  Could you review the updated fix:
>>>     http://cr.openjdk.java.net/~alexsch/8051548/webrev.01/
>>>
>>>     - the state changing check is added in the 
>>> ColorPanel.setColorTransparencySelectionEnabled() method
>>>
>>>  Thanks,
>>>  Alexandr.
>>>
>>> On 4/16/2015 11:27 PM, Phil Race wrote:
>>>>
>>>>  210
>>>>  211     void setColorTransparencySelectionEnabled(boolean b) {
>>>>  212         spinners[model.getCount() - 1].setVisible(b);
>>>>  213         colorChanged();
>>>>  214     }
>>>>
>>>> do you want to check if there's a state change here before firing 
>>>> the events ?
>>>>
>>>> Other than that it looks OK to me.
>>>>
>>>> -phil.
>>>>
>>>> On 4/16/15 1:14 AM, Alexandr Scherbatiy wrote:
>>>>> 15.04.2015 22:22, Sergey Bylokhov пишет:
>>>>>> HI, Alexander.
>>>>>> What is the status of this fix?
>>>>>
>>>>> The fix requires at least two reviewers.
>>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>>>
>>>>>> On 25.07.14 16:45, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you review the fix:
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8051548
>>>>>>> webrev: http://cr.openjdk.java.net/~alexsch/8051548/webrev.00
>>>>>>>
>>>>>>> The following public methods are added to the 
>>>>>>> AbstractColorChooserPanel:
>>>>>>> - setColorTransparencySelectionEnabled(boolean b)
>>>>>>> - isColorTransparencySelectionEnabled()
>>>>>>>
>>>>>>> The request to the public API change will be created after the 
>>>>>>> technical review.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.

[prev in list] [next in list] [prev in thread] [next in thread]