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
        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: