|
[
Permalink
| « Hide
]
Jochen Theodorou added a comment - 24/Jun/07 04:20 PM
implemented
Using r6669, I find that Christopher Brown's program behaves strangely. In the panel, the string 'EDT-friendly' is displayed. However the window title is 'I like deadlock'. It seems that the SU.isEventDispathThread is true in the label construction but false in the frame construction.
Attaching my variant of Christopher Brown's program.
Sorry, that should have been r6672.
yes, that's a nice one, but the fix is working as expected, your groovy program does not work as you expect. If you do
foo (a:b) {bar()}
then, you call the method foo with a map and a closure.. so far so good. But both of them are already created at the point foo is called. That means the map is already there and the closure instance too. When I change the closure to be executed during the EDT, then I still have the map and the closure creation outside of the EDT. So "frame" is really executed in the EDT and because frame calls the closure, the closure too. But the map creation is not. No my question. How bad is that? I mean we could add a noop method to SwingBuilder doing exactly nothing, but beeing executed in the EDT. As a result all calls done in the closure given to the noop method are executed in the EDT. So if I change your code to def frame
swing.noop {
frame = swing.frame (
title : SU.isEventDispatchThread ( ) ? 'Built on EDT' : 'I like deadlocks' ,
defaultCloseOperation : WC.DO_NOTHING_ON_CLOSE ,
windowClosing : { event ->
println ( 'feeling sleepy' )
Thread.sleep ( 500 )
println ( 'goodbye' )
System.exit ( 0 )
}
) {
panel ( layout : new BL ( ) ) {
label ( constraints : BL.NORTH , text : SU.isEventDispatchThread ( ) ? 'EDT-friendly' : 'Deadlock-friendly' )
button ( constraints : BL.CENTER , text : 'hello', actionPerformed : {
println ( 'button pushed' )
})
}
}
}
then I get the map creation in the EDT too... But maybe it isn't so bad that the map is created outside the EDT... comments? I tried out a slightly modified version of Rod Cope's Infamous JavaOne Getting Image from Excel Chart and using it for Icon Image demo (AKA so, do you think Groovy is fast enough? - I think so). So long as I created the Scriptom proxy on the EDT then everything worked fine and no Excel phantom process was left behind which can sometimes be a problem with Scriptom. It also works on XP with Office 2003 and Vista with Office 2007. So, I think it is fine from my point of view - note: I was having problems wth this demo prior to this patch because of the EDT issue.
The point about the separation of parameter processing and closure processing is a good and important one. I agree it is entirely reasonable for parameter processing to be in the current thread and only forcing closure processing to the EDT. We have to accept that some people will trip over this issue and therefore must be careful in documentation. I think the current state is entirely reasonable (though I can imagine that there are going to be awkward cases), so I am reclosing the issue as agreed on the mail list.
Reopening issue until we clarify what to do with regard to Jim White's comments:
This partial and "behind-the-scenes" fix in SwingBuilder for the Swing EDT problem simply reduces the number of vulnerable calls but leaves folks who are unaware of the issue still exposed (and with a frequency of occurrence that could be even more erratic). Also you're penalizing correct Swing code with a bunch of spurious "isEDT" calls. A typical SwingBuilder sample looks like: def swing = new SwingBuilder()
def frame = swing.frame ...
frame.pack()
frame.show()
Those "pack" and "show" calls critically need to be on the EDT, even more so than many SwingBuilder calls (because Swing construction calls typically are not actually in a race - the correct term for the problem here, not deadlock - because they haven't been added to a "live" display tree yet). So I suggest finding an idiom for Swing usage that is EDT-safe rather than hacking up SwingBuilder resulting in slower but still incorrect applications. What the Groovy Swing examples should look like is: SwingUtilities.invokeAndWait {
def swing = new SwingBuilder()
def frame = swing.frame ...
frame.pack()
frame.show()
}
That code will be correct because the callbacks will be events getting dispatched. And to help out folks who have long running code that needs to be divvied up between the EDT and background threads, you could bundle SwingWorker. http://java.sun.com/products/jfc/tsc/articles/threads/threads3.html I vote to remove the http://jira.codehaus.org/browse/GROOVY-1952 Jim to keep all those happy following the opinion of Jim I removed the code in invokeMethod and put it into a new method called edt. edt is taking a CLosure that is executed using SwingUtilities#invokeAndWait. The example in this issue could then be written as
import groovy.swing.SwingBuilder import java.awt.BorderLayout as BL import javax.swing.SwingUtilities as SU import javax.swing.WindowConstants as WC def swing = new SwingBuilder() swing.edt { def frame = frame ( title : SU.isEventDispatchThread ( ) ? 'Built on EDT' : 'I like deadlocks' , defaultCloseOperation : WC.DO_NOTHING_ON_CLOSE , windowClosing : { event -> println ( 'feeling sleepy' ) Thread.sleep ( 500 ) println ( 'goodbye' ) System.exit ( 0 ) } ) { panel ( layout : new BL ( ) ) { label ( constraints : BL.NORTH , text : SU.isEventDispatchThread ( ) ? 'EDT-friendly' : 'Deadlock-friendly' ) button ( constraints : BL.CENTER , text : 'hello', actionPerformed : { println ( 'button pushed' ) }) } } frame.pack ( ) frame.locationRelativeTo = null frame.visible = true } I also followed his suggestion on the mailing list and added a static method build that creates an instance of SwingBuilder, which allows us to write import groovy.swing.SwingBuilder import java.awt.BorderLayout as BL import javax.swing.SwingUtilities as SU import javax.swing.WindowConstants as WC SwingBuilder.build { def frame = frame ( title : SU.isEventDispatchThread ( ) ? 'Built on EDT' : 'I like deadlocks' , defaultCloseOperation : WC.DO_NOTHING_ON_CLOSE , windowClosing : { event -> println ( 'feeling sleepy' ) Thread.sleep ( 500 ) println ( 'goodbye' ) System.exit ( 0 ) } ) { panel ( layout : new BL ( ) ) { label ( constraints : BL.NORTH , text : SU.isEventDispatchThread ( ) ? 'EDT-friendly' : 'Deadlock-friendly' ) button ( constraints : BL.CENTER , text : 'hello', actionPerformed : { println ( 'button pushed' ) }) } } frame.pack ( ) frame.locationRelativeTo = null frame.visible = true } the differences are minimal. both methods get the SwingBuilder instance as parameter and both methods return the SwingBuilder instance at the end. The return value of the closure is ignored. In fact that made the change quite easy, because we no longer need that nasty array to sotre the return values and we no longer need an annonymous inner class... ok, the alter is not really corret, because the closure is one as well somehow. Just to complete this, here is the code for the new methods: public SwingBuilder edt(Closure c) { c.setDelegate(this); if (headless || SwingUtilities.isEventDispatchThread()) { c.call(this); } else { try { SwingUtilities.invokeAndWait(c.curry(new Object[]{this})); } catch (InterruptedException e) { throw new GroovyRuntimeException("interrupted swing interaction", e); } catch (InvocationTargetException e) { throw new GroovyRuntimeException("exception in event dispatch thread", e.getTargetException()); } } return this; } public static SwingBuilder build(Closure c) { SwingBuilder builder = new SwingBuilder(); return builder.edt(c); } I closure this issue now as fixed... I hope this time it is really left close
 |
|||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
force SwingBulder to execute builder methods in EDT
