endless calls to PSwing's ComponentListener

[GoogleCodeExporter]

What steps will reproduce the problem?

Run the attached example (DebugPSwingComponentAdapter) using Piccolo 1.3-rc1.

What is the expected output? What do you see instead?

The application should start with 2 radio buttons visible, and no check box
visible.  Instead, all 3 controls are visible and an endless sequence of
calls to componentHidden and componentShown occurs (see System.out) for the
check box.

Please use labels and text to provide additional information.

See the example's javadoc, and the comments labeled "WORKAROUNDS".

See also this Piccolo Developer thread:
http://groups.google.com/group/piccolo2d-dev/browse_thread/thread/7c87eca14c244e
2a

Original issue reported on code.google.com by cmal...@pixelzoom.com on 1 Feb 2010 at 11:01

Attachments:

• DebugPSwingComponentAdapter.java

[GoogleCodeExporter]

Example originally attached to report had debug output attached to the wrong 
PSwing.
Attached is a corrected example.

Original comment by cmal...@pixelzoom.com on 1 Feb 2010 at 11:15

Attachments:

• DebugPSwingComponentAdapter.java

[GoogleCodeExporter]

Found the root cause.

While constructing the scene, you are setting the visibility twice.

First time is to false, and then the second time is to true.  

The net effect is an infinite chain of off ons for the Callbacks from the 
component 
to itself.

I'm thinking that it's not worth it at this point. We can't expect client 
behaviour 
to change to accommodate a change that is solely for hypothetical optimzations.
Thoughts?

Original comment by allain.lalonde on 2 Feb 2010 at 6:02

• Changed state: Accepted
• Added labels: Type-Defect, Priority-Critical, Effort-Low, OpSys-All, Toolkit-Piccolo2D.Java

[GoogleCodeExporter]

A comment on the example code, which may or may not be related to the actual 
issue
(perhaps workaround #1), using ArrayLists to hold listeners is not thread-safe. 
 You
may wish to consider using javax.swing.event.EventListenerList or
java.util.concurrent.CopyOnWriteArrayList.

Allain, what are you suggesting above?

Original comment by heue...@gmail.com on 2 Feb 2010 at 6:15

[GoogleCodeExporter]

In this case, I think the cure may be worse than the disease ;-)

I vote for removing the ComponentAdapter that's added in the PSwing 
constructor, and
keeping the setVisible override.  The setVisible override seems worthwhile, 
since it
keeps the JComponent's visibility in-sync with the PNode (unless the client 
calls
JComponent.setVisible).

More thoughts...

Setting visibility a couple of times during scenegraph initialization is 
something
that's easy to do, and not necessarily a client error.  It's sometimes 
intentional,
as in our simulations that are driven by a complex physical model that is also
initializing itself.  I don't think this is something that we can expect 
clients to
avoid, and the consequence seem rather severe and difficult for clients to 
diagnose.
So the price of this optimization is a little too high.

Without the optimization there is the situation you mentioned previously 
(invisible
PSwing node and visible JComponent or visible PSwing and invisible JComponent). 
Overriding setVisible keeps the visibility in-sync if the client uses
PSwing.setVisible, which is arguably what they should be doing after 
instantiating a
PSwing.  Calling JComponent.setVisible could be documented as a bad practice in 
the
Javadoc for PSwing's constructor; once a JComponent is wrapped with a PSwing, 
the
scenegraph mechanism (and not Swing) should be used to control visibility. 
(Child
components of the JComponent can still be make visible/invisible via Swing's
setVisible.)  

Will clients still make calls to JComponent.setVisible, and cause the 
visibility of
PSwing and JComponent to be out of sync?  Yes, probably, but it's much easier to
diagnose.  And this situation is also easy to detect programmatically, so 
consider
throwing an exception with a very clear message.  Maybe something like this?:

         component.addComponentListener(new ComponentAdapter() {

            /** {@inheritDoc} */
            public void componentHidden(final ComponentEvent e) {
                if ( e.getVisible() != getVisible() ) {
                    visibilitySyncCheck();
                }
            }

            /** {@inheritDoc} */
            public void componentShown(final ComponentEvent e) {
                if ( e.getVisible() != getVisible() ) {
                     visibilitySyncCheck();
                }
            }
        });

        private void visibilitySyncCheck() {
            if ( component.isVisible() != getVisible() ) {
                throw new IllegalStateException( "Visibility of PSwing and JComponent
is out of sync. Did you call setVisible directly on the JComponent?" );
}           }
        }

Caveat: Might be problems with the above, too. Like what happens if a 
JComponent is
wrapped by a new PSwing? (And is this handled properly in general?)

Original comment by cmal...@pixelzoom.com on 2 Feb 2010 at 6:29

[GoogleCodeExporter]

Other than adding some documentation to PSwing,  I don't recommend trying to 
handle
the visibility-out-of-sync issue in 1.3-rc1.  Treating it as an exception was 
the
first thing that occurred to me, but I think it's likely to cause some other 
problem.  

Perhaps create a new ticket and address in a future release?

Original comment by cmal...@pixelzoom.com on 2 Feb 2010 at 6:43

[GoogleCodeExporter]

Sorry... In comment 5 I meant "I don't recommend trying to handle
the visibility-out-of-sync issue for Piccolo 1.3 release".

Original comment by cmal...@pixelzoom.com on 2 Feb 2010 at 6:44

[GoogleCodeExporter]

Original comment by allain.lalonde on 2 Feb 2010 at 7:28

• Changed state: Fixed

[GoogleCodeExporter]

What's the revision number for the fix?

Original comment by cmal...@pixelzoom.com on 2 Feb 2010 at 10:41

[GoogleCodeExporter]

Sorry, forgot to record it here.

Revision number is r965.

Original comment by allain.lalonde on 3 Feb 2010 at 2:17

[GoogleCodeExporter]

I verified that r965 fixes the example and our application.

Marking this issue as verified.

Original comment by cmal...@pixelzoom.com on 3 Feb 2010 at 4:51

• Changed state: Verified

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 3 Feb 2010 at 5:54

• Added labels: Milestone-1.3