groovy
  1. groovy
  2. GROOVY-1884

GEP: Adding Bound Properties to GroovyBeans

    Details

    • Type: New Feature New Feature
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1-beta-2
    • Fix Version/s: 1.6-beta-1
    • Component/s: GEP
    • Labels:
      None
    • Number of attachments :
      4

      Description

      Abstract:

      The current support of properties in Groovy beans does not auto-generate support for bound properties, a standard feature defined in the JavaBeans specification. This GEP proposes that there be some standard terse Groovy way to indicate to the compiler that such property support should be generated automatically

      Specification:

      The proposed solution is to introduce a new annotation groovy.lang.BoundProperty. This annotation will have source level retention and will be valid only on fields.

      When this property is present on one or more GroovyBeans properties then the class that contains the property will add a field $propertyChangeSupport of type java.beans.PropertyChangeSupport. Five methods will also be added, addPropertyChangeListener(PropertyChagneLsitener), removePropertyChangeListener(PropertyChagneListener), getPropertyChangeListeners(), addPropertyChangeListener(String, PropertyChagneLsitener), and removePropertyChangeListener(String, PropertyChagneListener). All of these methods will delegate directly to the appropriate method on $propertyChangeSupport. The auto-generated setter method will also call the firePropertyChanged(String, Object, Object} method of $propertyChangeSupport.

      A compile time error MUST be raised when a property is declared as @BoundProperty where the default named setter method is also declared. (i.e.

      @BoundProperty int x
      setX(int i) { x = i }

      is invalid)

      A compile time error MUST be raised when a class has any properties declared as a @BoundProperty and any or all of the five automatically added methods are also declared in the class.

      A compile time error SHOULD be raised when the @BoundProperty is used on a field with declared visibility, hence not a GroovyBean Property.

      Classes compiles with this annotation SHOULD execute under a 1.4 JVM, although compilation using a Java 5 JVM MAY be required.

      Motivation:

      While it is possible to add such wiring today to a GroovyBeans property by essentially implementing it the way Java does, (i.e. explicitly do everything yourself), it would be cleaner if there was a way to declare a property as bound, and have groovy handle the wiring like it already does for simple property declarations. Either a language keyword or annotation would be possible ways to do it.

      I am proposing the built in annotation because the addition of an additional language keyword may break older scripts where a previously unreserved keyword is now reserved. The use of syntactical punctuation is also not recommended since (a) there is no other example of using punctuation to add this aspect to a property and (b) because of that readability would suffer for those not familiar with the Groovy language.

      I am also proposing annotations because I will be able to submit a proof of concept implementation myself much easier.

      Backwards Compatibility:

      This enhancement will not be source code compatible with versions of Groovy prior to 1.1. The sole reason for this is the use of annotations. (use of a keyword or punctuation would similarly cause incompatibility).

      In instances where a user wants to use a Groovy script with @BoundProperty notation they user will need to remove the annotation, and either (a) not use automatically generated PropertyChangeListeners or (b) implement them by hand.

      1. BoundProperty.java
        0.5 kB
        Danno Ferrin
      2. BuiltinAnnotations.java
        1 kB
        Danno Ferrin
      3. GEP-BoundProperties.txt
        11 kB
        Danno Ferrin
      4. GEP-BoundPropertries-java14safe.txt
        23 kB
        Danno Ferrin

        Activity

        Hide
        Danno Ferrin added a comment -

        fixed spelling, wiki formatting

        Show
        Danno Ferrin added a comment - fixed spelling, wiki formatting
        Hide
        Aaron Digulla added a comment -

        I see no reason why the setter should be illegal for bound properties. Why not simply wrap it if it exists?

        Just add two flags to @BoundProperty:

        @BoundProperty(checkMod=true, before=true)

        checkMod will add this code:

        if (this.x != newVal)

        { ... }

        before move the call of the setter before the check (so you can modify the value):

        def oldVal = this.x
        callOldSetter(...)
        if (this.x != oldVal)

        { ... invoke listeners ... }

        It would be nice if one could set the default values for both at the class level to avoid repeating code.

        Show
        Aaron Digulla added a comment - I see no reason why the setter should be illegal for bound properties. Why not simply wrap it if it exists? Just add two flags to @BoundProperty: @BoundProperty(checkMod=true, before=true) checkMod will add this code: if (this.x != newVal) { ... } before move the call of the setter before the check (so you can modify the value): def oldVal = this.x callOldSetter(...) if (this.x != oldVal) { ... invoke listeners ... } It would be nice if one could set the default values for both at the class level to avoid repeating code.
        Hide
        Danno Ferrin added a comment -

        A new 1.4 safe version of the bound properties. BoundProperty is pre-defined as a built in annotation, and class resolution is trapped out to use the internal definition, and error checking is told not to flag it as an error. Hence code that has the @BoundProperty annotation can be used in 1.4, both written and compiled.

        Show
        Danno Ferrin added a comment - A new 1.4 safe version of the bound properties. BoundProperty is pre-defined as a built in annotation, and class resolution is trapped out to use the internal definition, and error checking is told not to flag it as an error. Hence code that has the @BoundProperty annotation can be used in 1.4, both written and compiled.
        Hide
        Danno Ferrin added a comment -

        Regarding check mod, the PropertyChangeSupport already traps out such non-changes and doesn't fire change events for non-modifying sets.

        As for the before annotation, if we write the code for that I don't see why it should be optional. If we are wrapping a non-bound super method then the wrapped portion should only do the event notification. However, this conflicts with the GroovyBeans way of generating the field and the getter/setter. We would also need to overwrite code to not generate the field, and IMHO at that point you may as well just write an overriding setter that generates the events. Such a situation is rare enough the explicit coding of the setter would have some value in clarity of understanding.

        Show
        Danno Ferrin added a comment - Regarding check mod, the PropertyChangeSupport already traps out such non-changes and doesn't fire change events for non-modifying sets. As for the before annotation, if we write the code for that I don't see why it should be optional. If we are wrapping a non-bound super method then the wrapped portion should only do the event notification. However, this conflicts with the GroovyBeans way of generating the field and the getter/setter. We would also need to overwrite code to not generate the field, and IMHO at that point you may as well just write an overriding setter that generates the events. Such a situation is rare enough the explicit coding of the setter would have some value in clarity of understanding.
        Hide
        Aaron Digulla added a comment -

        My comment refers to the situation, when you need to do additional processing in the setter (for example, convert a String to an int):

        class A {
            int x
        
            void setX (val) {
                if (val instanceof String) {
                    x = Integer.parseInt(val)
                } else {
                    x = val
                }
            }
        }
        

        In this case, you can't compare the value before the custom setter has run. In other cases, the setter can be expensive and you want it to stop early (i.e. compare first and not call the custom setter). So in my book, we need to be able to handle these three cases:

        • Setter is generated by Groovy
        • Setter is supplied by user; check for changes and notification added at the end
        • Setter is supplied by user; check needs to wrap the whole setter

        I'd also opt for checkMod=false so you can disable the compare for expensive objects (which would otherwise start to load stuff from a database, for example).

        Show
        Aaron Digulla added a comment - My comment refers to the situation, when you need to do additional processing in the setter (for example, convert a String to an int): class A { int x void setX (val) { if (val instanceof String ) { x = Integer .parseInt(val) } else { x = val } } } In this case, you can't compare the value before the custom setter has run. In other cases, the setter can be expensive and you want it to stop early (i.e. compare first and not call the custom setter). So in my book, we need to be able to handle these three cases: Setter is generated by Groovy Setter is supplied by user; check for changes and notification added at the end Setter is supplied by user; check needs to wrap the whole setter I'd also opt for checkMod=false so you can disable the compare for expensive objects (which would otherwise start to load stuff from a database, for example).
        Hide
        Danno Ferrin added a comment -

        The bytecode generated foe setX(val) would actually generate a setX(Object) instead of a setX(int) which is what is needed for the JavaBeans sepecification. JavaBeans was based on an explicitly typed system. In reality the groovy would deletage the 'x =' lines to the setX(int) methods, so in this example there really wouldn't be any conflict.

        Show
        Danno Ferrin added a comment - The bytecode generated foe setX(val) would actually generate a setX(Object) instead of a setX(int) which is what is needed for the JavaBeans sepecification. JavaBeans was based on an explicitly typed system. In reality the groovy would deletage the 'x =' lines to the setX(int) methods, so in this example there really wouldn't be any conflict.
        Hide
        blackdrag blackdrag added a comment -

        you wrote:
        """
        A compile time error MUST be raised when a class has any properties declared as a @BoundProperty and any or all of the five automatically added methods are also declared in the class.

        A compile time error SHOULD be raised when the @BoundProperty is used on a field with declared visibility, hence not a GroovyBean Property.
        """

        That's funny, because I would go another policy... if it is used on a field -> error, if getter/setter are used -> error, if any of

        addPropertyChangeListener(PropertyChagegListener)
        removePropertyChangeListener(PropertyChangeListener)
        getPropertyChangeListeners()
        addPropertyChangeListener(String, PropertyChangeLsitener)
        removePropertyChangeListener(String, PropertyChagneListener)
        $propertyChangeSupport

        is used, I would not throw an error. I would even go as far as not using $propertyChangeSupport as field name, but propertyChangeSupport. Maybe even allowing to customize the field name through the annotation? Maybe it should be possible to apply the annotation to the class too andlet the user specify the field there.

        One thing about the "patch"... first, thx for it. second, don't be angry if I have to modify that big times... the place for the code change is wrong and several other things that don't fit the usual way. Or do you want to change that? Then we would have to discuss how to.

        Show
        blackdrag blackdrag added a comment - you wrote: """ A compile time error MUST be raised when a class has any properties declared as a @BoundProperty and any or all of the five automatically added methods are also declared in the class. A compile time error SHOULD be raised when the @BoundProperty is used on a field with declared visibility, hence not a GroovyBean Property. """ That's funny, because I would go another policy... if it is used on a field -> error, if getter/setter are used -> error, if any of addPropertyChangeListener(PropertyChagegListener) removePropertyChangeListener(PropertyChangeListener) getPropertyChangeListeners() addPropertyChangeListener(String, PropertyChangeLsitener) removePropertyChangeListener(String, PropertyChagneListener) $propertyChangeSupport is used, I would not throw an error. I would even go as far as not using $propertyChangeSupport as field name, but propertyChangeSupport. Maybe even allowing to customize the field name through the annotation? Maybe it should be possible to apply the annotation to the class too andlet the user specify the field there. One thing about the "patch"... first, thx for it. second, don't be angry if I have to modify that big times... the place for the code change is wrong and several other things that don't fit the usual way. Or do you want to change that? Then we would have to discuss how to.
        Hide
        Aaron Digulla added a comment -

        I'm confused with "@BoundProperty is used on a field with declared visibility". Can you give an example here, please?

        > setX(val) would actually generate a setX(Object) instead of a setX(int)

        I guess this is true but still, why forbid users to write a fancy setter? Is there a good reason you can think of or just a general uneasyness about it?

        Show
        Aaron Digulla added a comment - I'm confused with "@BoundProperty is used on a field with declared visibility". Can you give an example here, please? > setX(val) would actually generate a setX(Object) instead of a setX(int) I guess this is true but still, why forbid users to write a fancy setter? Is there a good reason you can think of or just a general uneasyness about it?
        Hide
        Danno Ferrin added a comment -

        Jochen:

        The patch is a proof of concept. Tear it to shreds if necessary, I wanted to demonstrate how relativly low touch it was for Java 1.5. I was considering committing it myself, but after the large amount of discussion this seemingly small bit of code prompted I thought I should do a bit more of a formal process.

        The 'should' on a field was mostly for ease of implementation. we can raise it to a must.

        As to the exact semantics of the property change support, I was considering exposing a method 'protected void firePropertyChange(name, old, new)' in addition to the add and remove listener methods, and making the support private. That lines up with the way Swing handles it and would make extending swing properties easier. The two arg add and remove and getListeners is just nice to haves, not required, but JavaBeans requires the add and the remove to reflect properly. We probably should add some re-use logic when extending. Perhaps the rule could be fire(prop, old, new), add(listener), and remove(listener) must be present or it is an error, but if we are making our own add(prop, listener), remove (prop, listener), and getListeners() would also be generated.

        Show
        Danno Ferrin added a comment - Jochen: The patch is a proof of concept. Tear it to shreds if necessary, I wanted to demonstrate how relativly low touch it was for Java 1.5. I was considering committing it myself, but after the large amount of discussion this seemingly small bit of code prompted I thought I should do a bit more of a formal process. The 'should' on a field was mostly for ease of implementation. we can raise it to a must. As to the exact semantics of the property change support, I was considering exposing a method 'protected void firePropertyChange(name, old, new)' in addition to the add and remove listener methods, and making the support private. That lines up with the way Swing handles it and would make extending swing properties easier. The two arg add and remove and getListeners is just nice to haves, not required, but JavaBeans requires the add and the remove to reflect properly. We probably should add some re-use logic when extending. Perhaps the rule could be fire(prop, old, new), add(listener), and remove(listener) must be present or it is an error, but if we are making our own add(prop, listener), remove (prop, listener), and getListeners() would also be generated.
        Hide
        Danno Ferrin added a comment -

        Aaron:

        It's not about forbidding users from write fancy setters, it's about playing nice with java.beans.Introspector. To allow that much free reigh we may need to generate BeanInfos for each annotated class. May be doable, but looks more like a 2.0 horizon item. Of course IIRC the Introspector relies on the order of the methods, so we could re-order the class file to make the generated get/set show up first or last (as needed)

        Show
        Danno Ferrin added a comment - Aaron: It's not about forbidding users from write fancy setters, it's about playing nice with java.beans.Introspector. To allow that much free reigh we may need to generate BeanInfos for each annotated class. May be doable, but looks more like a 2.0 horizon item. Of course IIRC the Introspector relies on the order of the methods, so we could re-order the class file to make the generated get/set show up first or last (as needed)
        Hide
        blackdrag blackdrag added a comment -

        The ordering of the methods is not fixed in the VM or for reflection. So doing logic based on the ordering the method has in bytecode is not a good idea.

        Show
        blackdrag blackdrag added a comment - The ordering of the methods is not fixed in the VM or for reflection. So doing logic based on the ordering the method has in bytecode is not a good idea.
        Hide
        Danno Ferrin added a comment -

        This was addressed as part of the AST Transformations. Annotations are @Bindable and @Vetoable

        Show
        Danno Ferrin added a comment - This was addressed as part of the AST Transformations. Annotations are @Bindable and @Vetoable

          People

          • Assignee:
            Danno Ferrin
            Reporter:
            Danno Ferrin
          • Votes:
            4 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: