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
