Added implementation for FxProperty

[Rawi01]

This PR fixes #521

I just updated the branch and adjusted a few minor things to be comaptible with the current master. It seems to be complete as there were no complaints about missing stuff in the linked issue. The implementation is based on the JavaFX Property Architecture documentation. For each field @FxProperty generates a final getter, a final setter (optional) and a property getter.

[rzwitserloot]

The problem is: I'm totally unfamiliar with JavaFX. So is @rspilker. However, you've more than deserved the right to sign off on stuff like this. Take my review with a grain of salt; I am not capable of providing feedback on whether this is suitable for the problem domain.

A few nitpicks before we integrate this into the main branch:

• Primarily, all test cases have a field of a type of something from the javafx.beans.property package. But it makes more sense to my javafx-unaware-layperson's view that one would want to write @FxProperty int foo; in order to get the bells and whistles: A field of type IntegerProperty or possibly ReadOnlyIntegerProperty, and the various methods that go with it: A setter, a getter, and a 'get me that property' method. Lombok's @FxProperty feature should either [A] also handle it fine if the field's type is one of these javafx.beans.property types, or [B] I think this is better: Error out and throw in the towel, telling the user that they should just use the type they want (be it int or be it BigDecimal). The handler will figure it out and make an IntegerProperty, ListProperty<String>, ObjectProperty<BigDecimal>, etc.
• Assuming the above is sensible advice, then surely a final field defaults to a ReadOnly variant. The @FxProperty(readOnly=true) syntax exists solely to 'export' a mutable int field as a read only property, i.e. it can be mutated by the class code just fine, but external sources that grab the property merely get a 'view' object that can be used to read it, not to set it. Alternatively if none of this makes sense, just generate an error on the appropriate AST node and throw in the towel. If someone comes up with a sensible explanation of what the code means later, we can future-version-upgrade code that currently emits errors to code that does not, but not the other way around (not without breaking backwards compatibility).
• Is @FxProperty on an enum even sensible? I know @Getter makes sense there, perhaps you just copied functionality. If you can't immediately think of a sensible scenario there, I'm tempted to just error out if you try until someone crafts a sensible use case for this.
• /src/core/lombok/javac/handlers/HandleFxProperty::PRIMITIVE_TYPE_MAP feels like it should be in src/core/lombok/javac/JavacHandlerUtil. If it shouldn't be, it must be loaded in from HandleGetter (the static map is already public, so that part should be trivial).
• (minor nit) possibly the getter handler code for FxProperty and Getter itself needs to be unified: The static class FieldInfo def needs to move up, the HandleFxProperty class extends HandleGetter or calls HandleGetter methods for most of its work, FieldInfo moves 'up', etc. The only code left that can generate getters across all of javac-space lombok is your createGetter method from HandleFxProperty (and, same deal, createGetter). Where the actual code of handleGetter/handleSetter lives - some sensible place, which is presumably either some common superclass or utility class used by both HandleGetter/HandleSetter and HandleFxProperty, or in HandleGetter/HandleSetter itself, where we consider HandleFxProperty to 'extend' the functionality in these two (not literally class HandleFxProperty extends HandleGetter, but spiritually, if you get my drift). The code duplication bothers me, and I'm definitely going to forget about updating HandleFxProperty when I add new features or fix bugs in HandleGetter, hence why code duplication in this case really bothers me. Unlike the previous points, this isn't a showstopper.
• As far as I can tell there's no documentation. I'll gladly volunteer to write it if you prefer.
• If issues/feature requests come up that are specifically referring to the nature of javafx's property system, I'm probably just going to have to defer to your knowledge on this subject. Is that okay?

[Rawi01]

Thanks for this detailed review 👍

My knowledge of JavaFX is also quite limited, the implementation is based on the input in the linked issue and the JavaFX wiki. It was just a feature that seems to be quite easy to implement and maintain and also removes a lot of boilerplate so it fulfills the requirements for a new feature.

• I think that is possible but I'm not sure if this actually the better solution. I think that an annotation that only adds new methods is easier to understand and changing the type of the field might lead to a confusing situation if someone tries to assign a value directly (e.g. this.integer = 1;). We also have to add some kind of exclude annotation for fields that should not be exported as property. It is also way harder to update an existing codebase that already uses the recommended property architecture. I will collect some feedback in the linked issue.
• I agree that using final for read-only properties seems to be a good idea if we want to use java standard types.
• 👍
• 👍
• I will check if I can refactor it a bit to reduce code duplication. I think I created a different method because the generated body is different and I don't want to apply most of the other getter/setter stuff.

[ChibiChu]

Finally! Thank you so much! Is there any updates on when the merge might happen?

[nilssen98]

Any updates on this?

[Rawi01]

Please read the linked issue. TL;DR unclear requirements