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

[Pavel Porvatov]

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

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