groovy
  1. groovy
  2. GROOVY-3177

Preserve SwingBuilder.build(Closure).

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6-rc-2
    • Fix Version/s: 1.6-rc-1
    • Component/s: Swing
    • Labels:
      None
    • Number of attachments :
      0

      Description

      Danno first "deprecated" static SwingBuilder.build(Closure) and has now introduced some crazy Groovy metaprogramming code into SwingBuilder as a "workaround" to the bugs caused by his attempts to make SwingBuilder threadsafe (which shouldn't be done and is a major and trouble plagued issue with no JIRA).

      static SwingBuilder.build(Closure) was introduced (by me) in Groovy 1.5 and is what I consider the preferred method for using SwingBuilder. It specifically addresses the common case of a user wanting a Swing UI with the minimum code and correct behavior wrt. EDT without having to even know there is one.

      http://www.nabble.com/SwingBuilder.build-%28was%3A-Re%3A--groovy-user--SwingBuilder-and-the-Event-Dispatch-Thread%29-p11354341.html

      It has been in use for more than a year now and is clearly being used by Groovy folks:

      http://www.nabble.com/Adding-a-GUI-to-a-program-td20437418.html#a20437418
      http://www.nabble.com/SwingBuilding-in-a-real-application--td19274585.html#a19276077
      http://www.nabble.com/Behavior-of-action-closure-in-SwingBuilder-td15348599.html#a15348599
      and many more...

        Activity

        Hide
        Danno Ferrin added a comment -

        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.

        Show
        Danno Ferrin added a comment - 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.
        Hide
        blackdrag blackdrag added a comment - - edited

        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

        Show
        blackdrag blackdrag added a comment - - edited 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
        Hide
        Jim White added a comment -

        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.

        Show
        Jim White added a comment - 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.
        Hide
        Andres Almiray added a comment -

        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()

        Show
        Andres Almiray added a comment - 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()
        Hide
        Paul King added a comment -

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

        Show
        Paul King added a comment - Still trying to understand this issue myself. What does deprecate mean? Will the API call be available in 1.6 but marked as deprecated?
        Hide
        blackdrag blackdrag added a comment -

        yes

        Show
        blackdrag blackdrag added a comment - yes
        Hide
        Jim White added a comment -

        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

        Show
        Jim White added a comment - 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
        Hide
        Andres Almiray added a comment -

        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.

        Show
        Andres Almiray added a comment - 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.
        Hide
        Jim White added a comment - - edited

        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.

        Show
        Jim White added a comment - - edited 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.
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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
        Hide
        Jim White added a comment -

        That the currently proposed solution is to have two methods with the same name ('build') with the same parameter ('Closure'), one static and one a member function should make it clear that it is a wrong approach.

        The common uses is not amongst you guys that are working on the trunk and snapshot builds but the installed base of Groovy 1.5 users. When Groovy 1.6 is released there is absolutely no reason to break any of the SwingBuilder code of those thousands of developers. That Griffon, a project in early development that cannot even work with Groovy 1.5 is driving the Groovy standard to break with them is wrong.

        I don't know what the deal is with this ThreadLocal jazz in FSB and SwingBuilder, but as I reported on groovy-dev, it apparently needs this static + member build(Closure) MOP hack in order to function with my Groovy 1.5 WingsConsole code. SwingBuilder 1.5.7 works fine in Groovy 1.6 and so FSB is not the problem. I'm guessing the problem is not with one SwingBuilder per thread but having more than one.

        Having more than one SB per thread is pretty normal doing threaded code because there is not reason to construct one on some thread and use it on another. The trouble in the multithreading is in allowing the possibility of more than one thread using a single SB concurrently (which my code is scrupulous to avoid).

        As for names for the new SwingBuilder that is incompatible with Groovy 1.5 there is also SwingBuilderNG and SwingBuilder2.

        Show
        Jim White added a comment - That the currently proposed solution is to have two methods with the same name ('build') with the same parameter ('Closure'), one static and one a member function should make it clear that it is a wrong approach. The common uses is not amongst you guys that are working on the trunk and snapshot builds but the installed base of Groovy 1.5 users. When Groovy 1.6 is released there is absolutely no reason to break any of the SwingBuilder code of those thousands of developers. That Griffon, a project in early development that cannot even work with Groovy 1.5 is driving the Groovy standard to break with them is wrong. I don't know what the deal is with this ThreadLocal jazz in FSB and SwingBuilder, but as I reported on groovy-dev, it apparently needs this static + member build(Closure) MOP hack in order to function with my Groovy 1.5 WingsConsole code. SwingBuilder 1.5.7 works fine in Groovy 1.6 and so FSB is not the problem. I'm guessing the problem is not with one SwingBuilder per thread but having more than one. Having more than one SB per thread is pretty normal doing threaded code because there is not reason to construct one on some thread and use it on another. The trouble in the multithreading is in allowing the possibility of more than one thread using a single SB concurrently (which my code is scrupulous to avoid). As for names for the new SwingBuilder that is incompatible with Groovy 1.5 there is also SwingBuilderNG and SwingBuilder2.
        Hide
        blackdrag blackdrag added a comment -

        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

        Show
        blackdrag blackdrag added a comment - 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
        Hide
        Jim White added a comment -

        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.

        Show
        Jim White added a comment - 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.
        Hide
        Jim White added a comment -

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

        Show
        Jim White added a comment - Added SwingBuilder.edtBuilder(Closure) to the 1.5 branch and marked build(Closure) deprecated.

          People

          • Assignee:
            Unassigned
            Reporter:
            Jim White
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: