Closed GoogleCodeExporter closed 9 years ago
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:
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
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
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
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
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
Original comment by allain.lalonde
on 2 Feb 2010 at 7:28
What's the revision number for the fix?
Original comment by cmal...@pixelzoom.com
on 2 Feb 2010 at 10:41
Sorry, forgot to record it here.
Revision number is r965.
Original comment by allain.lalonde
on 3 Feb 2010 at 2:17
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
Original comment by heue...@gmail.com
on 3 Feb 2010 at 5:54
Original issue reported on code.google.com by
cmal...@pixelzoom.com
on 1 Feb 2010 at 11:01Attachments: