<Swing Dev> Review request for 7154030: java.awt.Component.hide() does not repaint parent container

[Jonathan Lu]

Verified!

Thanks, Charles

Regards

-Jonathan

On 04/26/2012 12:47 PM, Charles Lee wrote:
> Hi Jonathan,
>
> The patch has been committed @
>
> Changeset: 340cda7e1430
> Author:    luchsh
> Date:      2012-04-26 12:39 +0800
> URL:http://hg.openjdk.java.net/jdk8/awt/jdk/rev/340cda7e1430
>
> 7154030: java.awt.Component.hide() does not repaint parent component
> Reviewed-by: rupashka
>
> Please verify it.
>
> Hi Pavel,
>
> Thank you for reviewing it.
>
>
> On 04/24/2012 03:50 PM, Jonathan Lu wrote:
>> Hi Pavel,
>>
>> Thanks for the explanation, I've updated the patch accordingly, could 
>> you please take a look?
>> http://cr.openjdk.java.net/~luchsh/7154030_7/
>>
>> Best regards
>> -Jonathan
>>
>> On 04/22/2012 12:23 AM, Pavel Porvatov wrote:
>>> Hi Jonathan,
>>>> Hi Pavel,
>>>>
>>>> see my comments below,
>>>>
>>>> On 04/13/2012 06:39 PM, Pavel Porvatov wrote:
>>>>> Hi Jonathan,
>>>>>> Hi Pavel,
>>>>>>
>>>>>> Here's another updated patch.
>>>>>> http://cr.openjdk.java.net/~luchsh/7154030_5/
>>>>>>
>>>>>> My comments are inlined.
>>>>> See below
>>>>>>
>>>>>> On 04/11/2012 09:40 PM, Pavel Porvatov wrote:
>>>>>>> Hi Jonathan,
>>>>>>>> Hi Pavel,
>>>>>>>>
>>>>>>>> Here's the update patch,
>>>>>>>> http://cr.openjdk.java.net/~luchsh/7154030_4/
>>>>>>>>
>>>>>>> Could you please explain one change in the JComponent.java:
>>>>>>>
>>>>>>> +    public void hide() {
>>>>>>> +        if (isVisible()) {
>>>>>>> +            super.hide();
>>>>>>>
>>>>>>> I think super.hide() should be invoked always. You must also add 
>>>>>>> a javadoc to the new hide method (see {@inheritDoc} for details)
>>>>>>
>>>>>> Yes, the old patch may cause Component.isPacked not to be set 
>>>>>> correctly, so moved super.hide() up.
>>>>> Ok. The next question: I think we should use in "if (isVisible())" 
>>>>> isShowing instead of isVisible. What do you think about that?
>>>>
>>>> I do not think so, since the problem here is that the repaining 
>>>> work is not done after hiding a visible component, but the fields' 
>>>> status of this hidden component have been correct, so has its 
>>>> parent. And isShowing() 's implementation is just checking those 
>>>> fields in similar way as isVisible() except for additional checks 
>>>> for the parent, whose status also cannot reflect the visual picture 
>>>> from user's eyes. So in my opinion, isShowing does not help with 
>>>> this case better than isVisible but introduces more efforts of 
>>>> checking and branching. Pls fix me if anything wrong with my 
>>>> understanding.
>>> The java.awt.Component#isVisible method just returns value of the 
>>> "visible" field, but isShowing reflects visible component on screen 
>>> or not. E.g. almost all created components returns isVisible true, 
>>> even if they were not showed on screen...
>>>
>>> Regards, Pavel
>>>>
>>>> BTW, there's a stupid bug in previous patch caused by my local 
>>>> testing cache was not cleaned up completely, terribly sorry for 
>>>> that and here's the updated patch. 
>>>> http://cr.openjdk.java.net/~luchsh/7154030_6/
>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>> 3. Could you please follow our code conventions?  (see 
>>>>>>>>> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475)
>>>>>>>>>
>>>>>>>> Sorry for this problem, I was trying to keeping aligned with 
>>>>>>>> the original style of JComponent.java, which I later realized 
>>>>>>>> to be inappropriate. In the updated patch, code has been well 
>>>>>>>> formatted.
>>>>>>>>
>>>>>>>>> 4. Your test is not automatic one. I think you could use 
>>>>>>>>> java.awt.Robot#createScreenCapture and analyze result of hide 
>>>>>>>>> method.
>>>>>>>>
>>>>>>>> See the link, it should be automatic now.
>>>>>>> The test should be corrected:
>>>>>>> 1. All Swing components must be accessed from the EDT thread
>>>>>> Updated in the new test.
>>>>>>> 2. What is the reason to introduce the MyButton class? I think 
>>>>>>> the test can be simplified, if you will use the 
>>>>>>> Util#compareBufferedImages (see 
>>>>>>> test\javax\swing\regtesthelpers\Util.java) method. Just take 
>>>>>>> screen-shots between show/hide and compare them
>>>>>>
>>>>>> The reason of introducing MyButton class is to test the show/hide 
>>>>>> of non-opaque component.
>>>>> Why can't we use something like following:
>>>>>                 button.setOpaque(false);
>>>>>                 button.setBackground(new Color(255, 0, 0, 128));
>>>> updated in new patch.
>>>>>
>>>>>> I've changed pixel comparison to Util#compareBufferedImages approach.
>>>>>>
>>>>>>> 3. Thread.sleep(milisecond) should be replaced by the 
>>>>>>> SunToolkit#realSync method (many existing tests uses it)
>>>>>>>
>>>>>> Updated in new test.
>>>>> Looks good. Could you please remove unnecessary "volatile" modifiers?
>>>> done in the new patch.
>>>>>
>>>>> Regards, Pavel
>>>>
>>>> Regards
>>>> - Jonathan
>>>
>>
>>
>
>
> -- 
> Yours Charles

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