Preserve SwingBuilder.build(Closure).

The engineering rational for the decision is that build(Closure) has been mistakenly considered a valid alternative for build(Script), from the parent FactoryBuilderSupport. The concerns of building a Script in context of the builder should not automatically imply that the operation must be performed on the EDT. Method renaming is warrented in this case: newBuilderOnEDT. The name reflects both concerns that happen: being build on the EDT and being build in a new Builder not previously existing. The refactoring of WingsBuilderConsole illustrate how the implications can get lost in the shuffle.

being an error or not is not the most important part here I think. If the method should be removed, then it should be made deprecated first, so it can be removed in later major versions. The documentation of the method should state what is wrong with that method and how it should be done instead. So simply removing the method is a no-go. Changing the method from bing static to virtual is also not good, since the idea of deprecating something is, that we keep compatibility first

My point is that the deprecation action itself is wrong.

I've cited a few examples out of many (including one from Danno) that people are using static SwingBuilder.build(Closure) and recommending that as the way to use SB. That is no surprise because it works correctly, is the shortest form of code, and highly mnemonic. That is about as groovy as it gets.

So this issue here is whether anyone else supports deprecation. Obviously we're currently at a net 0 in votes.

After reading about this issue at the mailing lists and glancing at the current patch, it is my belief that the proposal from Danno and Jochen are better suited for SwingBuilder's future and every other builder based on FBS.

+1 on deprecation and addition of newBuilderOnEDT()

Still trying to understand this issue myself. What does deprecate mean? Will the API call be available in 1.6 but marked as deprecated?

yes

When static SwingBuilder.build was introduced in 1.5, Jochen described it as a positive part of SwingBuilder evolution.
http://blackdragsview.blogspot.com/2007/07/about-swingbuilder.html
No one has provided any explanation why any Groovy user should expect that it should be anything but the best practice so far devised, at least in Groovy 1.x.

Andres showed static SwingBuilder.build as the very first thing on how to use SwingBuilder on slide 6 at JavaOne as well as on his blog (including one explaining new features for 1.6).
http://www.slideshare.net/aalmiray/javaone-ts5098-groovy-swingbuilder
http://www.jroller.com/aalmiray/entry/swingbuilder_s_binding_revisited
http://www.groovygrails.com/gg/blog/view/125601
http://www.therichwebexperience.com/blog/andres_almiray/2008/02/gfx_doodles.html

A year ago Danno promoted static SwingBuilder.build as useful new feature:
http://shemnon.com/speling/2007/09/new-and-mildly-entertaining-fe.html

Danno misrepresents my experience with SwingBuilder.build with WingsConsole, so it certainly is not a data point in favor of deprecation.

There are no doubt thousands of applications using static SwingBuilder.build. And no reason to deprecate. I certainly haven't heard an explanation that is going to make sense to Java enterprise folks (many of which just paid a bunch of money for Groovy training this last year) on why the first thing they learned about SwingBuilder is deprecated in less than a year. I'm sure the only impression is going to be instability.

But if the consensus is to add @Deprecated, so be it. My strongest objection and the thing that sent me ballistic (besides Danno reverting my commits) is this:

    /**
     * re-director to deail with reprtative method names
     */
    public static SwingBuilder '$static_methodMissing'(String method, Object args) {
        if (method == 'build' && args.length == 1 && args[0] instanceof Closure) {
            LOG.warning("The build(Closure) API will be removed in a future API.  Please use the edt(Closure) instance method or newBuilderInEDT(Closure) static method")
            return newBuilderInEDT(args[0])
        } else {
            throw new MissingMethodException(method, SwingBuilder, args, true)
        }
    }

as a replacement for:

    /**
     * Utility method to create a SwingBuilder, and run the
     * the closure in the EDT
     *
     * @param c run this closre in the builder using the edt method
     */
    public static SwingBuilder build(Closure c) {
        SwingBuilder builder = new SwingBuilder()
        return builder.edt(c)
    }

The real problem with SwingBuilder in trunk is that it has incompatible changes with the way it tries to deal with threading. As I demonstrated yesterday with a very concise example, SwingBuilder 1.5 is totally fine for multithreading when used correctly.
http://docs.codehaus.org/display/GROOVY/Multithreading+with+SwingBuilder

Jim, the problem with SwingBuilder.build() is that it opens the door to the following code

def buildMyPanels = {
   SwingBuilder.build {
      panel( id: 'mypanel' ) { ... }
   }
}

def swing = SwingBuilder.build {
   frame( title: "title" ) {
      buildMyPanels()
   }
}

That is just plain wrong!

• 2 instances of SwingBuilder will be created when only one is fine
• assert swing.mypanel -> false! because mypanel is a variable in the second SwingBuilder instance, which as the code shows we did not save a reference to

While it is true Danno and myself promoted the use of SwingBuilder.build() in the past we have come to realize it is not a good practice anymore, given that its side effects are not well documented and even if they were a developer may still write that code, resulting in weird UIs, exceptions and frustration. Thus the deprecation flag and the newBuilderInEDT() option.

About the replacement code, that is a valid wrapper. It is even better than other deprecation mechanisms as it will alert the user that something will change in the future and provides a workaround, still provides the expected behavior.

I think your example is a positive one, not a negative. Reusing SwingBuilders is not the norm, hence the popularity of the current behavior.

As my threading example points out, you must use multiple builders when doing threaded building if you want sane and simple code that is also predictable, correct, and backwards compatible.

Moreover it is the attempt at making SwingBuilders secretly hook up with each other via ThreadLocal that is causing the current trouble.

What really needs to happen here is whatever the new functionality that is being added which is doing this behind-the-scenes jazz with ThreadLocal needs to go into a new builder that is a subclass of SwingBuilder.

The attempt at putting a member function and static function with the same name and parameters types into the same class (a direct violation of Java naming rules <http://java.sun.com/docs/books/jls/third_edition/html/names.html#6.4.3>) clearly demonstrates that this change is incompatible and needs to rely on the user opting-in by using a new class name.

SwingBuilderX is taken, but I'm sure there's something suitable and descriptive like SwingBuilderPlus or whatever.

And the log message is highly objectionable on many levels.

you say it is not the norm, but if you go and use bindings and labeled components, then it becomes the norm very fast. I use SwingBuilder like that, Andres does and I think Danno does too. and maybe you "must" use a builder per thread, but who does this? Moreover you don't do this too, because you use the same swing builder inside and outside the EDT. Your example on http://docs.codehaus.org/display/GROOVY/Multithreading+with+SwingBuilder shows that.

"""Moreover it is the attempt at making SwingBuilders secretly hook up with each other via ThreadLocal that is causing the current trouble.""" ... Jim, that you need to explain, because that is very new, so have to explain what exactly is the current trouble you are talking about and how the threadlocal is causing that. If you still use a builder per thread, you will not get any negative results from the thread locals. The threadlocal astuff is also not really in SwingBuilder, but in FactoryBuilderSupport, so doing a subclass of swingbuilder to implement that won't do it. And as I said... I am unclear what disadvantages you get from that. Please try to explain them

Currently I don't see why both build(Closure) methods are needed. One of them should be enough and should handle all the cases well already

I agree with the decision to add the member function 'Object build(Closure)' in order to be consistent with 'Object build(Script)' and 'Object build(Class)'. I think however it belongs in FBS along with those other methods. It is probably a fairly safe change there (as we've seen it is difficult to have it affect subclasses that define it otherwise). Also 1.6 is the time to do it rather than a future bug fix release like 1.6.1.

That then means 'static SwingBuilder build(Closure)' is necessarily deprecated and we need the MOP dispatch to preserve backwards compatibility.

Although I remain concerned that the ThreadLocal change to SwingBuilder is likely to cause trouble for other users, the GROOVY_1_6 (cs14271) branch works for me and I am ready to resolve this issue.

The deprecation also means we need to add SwingBuilder.edtBuilder(Closure) to the 1.5 branch and mark build(Closure) deprecated.

Added SwingBuilder.edtBuilder(Closure) to the 1.5 branch and marked build(Closure) deprecated.