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

[Pavel Porvatov]

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)

See also my comments below

> My comments are inlined.
>
> On 03/27/2012 11:58 PM, Pavel Porvatov wrote:
>> Hi Jonathan,
>>> Hello Pavel,
>>>
>>> Here's the updated patch and automatic test for bug 7154030, could 
>>> you please take another look?
>>> http://cr.openjdk.java.net/~luchsh/7154030_2/
>> I have several comments:
>>
>> 1. What about the following comment from Artem:
>> "Even if we accept the change in JComponent.hide(), we should then 
>> override show() as well (lightweight component may be non-opaque, so 
>> we should repaint from its parent)"
>> ?
>>
> I've updated my test case by including non-opaque components, but I do 
> no see a need for overriding show(), is there anything incorrect with 
> the updated testcase or my understanding?
>
>> 2. Could you please clarify your changes in setVisible method?
>> As I see in comments
>>             // Some (all should) LayoutManagers do not consider 
>> components
>>             // that are not visible. As such we need to revalidate 
>> when the
>>             // visible bit changes.
>>             revalidate();
>> but now this code is invoked only for setVisible(true)
>>
> For the setVisible(false) case, the repainting and revalidating 
> operations will be done in new method JComponent.hide(), so this 
> change is just to reduce some duplicated actions.
Ok
>
>> 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
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
3. Thread.sleep(milisecond) should be replaced by the 
SunToolkit#realSync method (many existing tests uses it)

Regards, Pavel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120411/77b1c774/attachment.html>