Makes it possible to map DBUS properties directly to setter / getters

[brett-smith]

This patch makes it possible to map DBUS properties directly to setter / getter methods in any DBusInterface. This has a number of advantages.

• Less code. No need to implement Get(), Set() and GetAll() from org.freedesktop.DBus.Properties. In fact, you do not need to implement this interface at all.
• More natural. Java programmers are familiar with this pattern, and I was finding every time I wanted to use properties, I had to go refresh my memory as to how dbus-java does it
• Makes it much easier to share interfaces between client and server code. This was particularly difficult due to properties being wrapped in Variant on one side, but not the other.

If there is a disadvantage, it's that there is no equivalant of GetAll(). However, there is nothing stopping you mixing this new API with the old. So just implement GetAll() and stub out Get() and Set().

This is what an interface with properties would now look like.

@DBusInterfaceName("com.acme.InterfaceWithProperties")
public interface InterfaceWithProperties extends DBusInterface {

    String sayHello();

    int getJustAnInteger();

    @DBusProperty(access = Access.READ, name = "MyProperty")
    String getMyProperty();

    @DBusProperty(access = Access.WRITE, name = "MyProperty")
    void setMyProperty(String _property);

    @DBusProperty(access = Access.READ, name = "ZZZZZZZ")
    long getMyAltProperty();

    @DBusProperty(access = Access.WRITE, name = "ZZZZZZZ")
    void setMyAltProperty(long _property);

    @DBusProperty
    boolean isMyOtherProperty();

    @DBusProperty
    void setMyOtherProperty(boolean  _property);
}

And the implementation ..

public class ObjectWithProperties implements InterfaceWithProperties {

    private String myProperty = "Initial value";
    private boolean myOtherProperty;
    private long myAltProperty = 123;

    @Override
    public String getObjectPath() {
        return "/com/acme/ObjectWithProperties";
    }

    @Override
    public String sayHello() {
        return "Hello!";
    }

    @Override
    public long getMyAltProperty() {
        return myAltProperty;
    }

    @Override
    public void setMyAltProperty(long _myAltProperty) {
        this.myAltProperty = _myAltProperty;
    }

    @Override
    public int getJustAnInteger() {
        return 99;
    }

    @Override
    public String getMyProperty() {
        return myProperty;
    }

    @Override
    public void setMyProperty(String _property) {
        myProperty = _property;
    }

    @Override
    public boolean isMyOtherProperty() {
        return myOtherProperty;
    }

    @Override
    public void setMyOtherProperty(boolean _property) {
        myOtherProperty = _property;
    }

}

Some points to note about the above.

• @DBusProperty now may be attached to methods.
• Any method with @DBusProperty will not be exported as a method, instead it and it's related setter or getter will be presented as a property.
• When used on methods, all attributes of the annotation are optional. If not present, they will be inferred from the name itself, the parameter and return types and the name for access ("set*" will WRITE and "is" and "get" for READ).

Other changes made.

• The code generator now has an option --propertyMethods that will generate codes using this method instead of class annotations.
• name attribute of @DBusProperty was given a default value of "". Runtime checks are now used instead. This may be slightly controversial, but the alternative was using a totally separate annotation.
• Also added a --package option to generate in a different namespace (and inject @DBusInterfaceName).
• For my sanity, I refactored some of the code in AbstractConnection. This is where the most complex part of this patch was concentrated. Im 99% certain its functionally identical (apart from the new property features), but any review should pay particular attention here.

A new example has been added, but not yet tests. If you are happy with how this looks, I'll finish up with adding appropraite unit tests.

[hypfvieh]

As always thanks for your PR.

I like the idea of making the usage of properties/getters/setters more straight forward. The reason why the Properties interface uses Get and Set with upper case is because DBus specification defines it that way. I'm thinking of adding get and set with lowercase as default implementation in the interface to be more Java like. But I have to think a bit about that (does this cause side effects? Break existing code?).

Anyway, there are some points I would prefer to change. First: It would be great to have this patch for 5x-develop branch instead of master. As Java 21 was just released, I'm planing to release 4.3.1 soon and switch development to 5.x (requiring Java 17 as minimum version). Therefore I want to avoid bigger changes for 4.x and espacially for 4.3.1 right now.

Second: It does not feel so good to use DBusProperty annotation for both, some sort of method-magic and for properties. I would prefer to use a extra annotation like DBusPropertyMethod or similar. Then we don't need to support empty name parameter or other ugly workarounds and can completely focus on the options we need for this method magic.

Third: I'm not a fan of the var type. Please use proper variable type when ever possible.

Fourth: As you said: Test cases.

And last but not least: Additional documentation in maven site. This feature feels valuable and so we should add a page to maven site for this. The description in this PR might be a good starting point.

[brett-smith]

1. Re using branch 5.x, yup sure. I had a feeling you might say this. With Java 17, another possibility opens up too. I wonder if record could be used. Records are immutable (sorta), so you would need to use some kind of wrapper for properties that can be written to. E.g.

public interface MyInterface extends DBusInterface {

    record Props(String prop1, int prop2, boolean prop3, Mutable<String> propWithGetSet);

    void myMethod();

    @DBusPropertyRecordOrSomething
     Props props();
}

This would eliminate yet more code, both in the interface and particularly the implementation. Exactly how this might look would depend on if it's possible to create Proxy records. I'll do a bit of research.

2. Understood. Will create a new separate annotation.
3. The var keyword. Heh yeh. I personally love it, it saves so much typing and I personally think it usually makes more readable code if used consistently. But I understand it's not universally loved, so will of course bow to your wishes.
4. Of course, will get tests and docs etc added.

[hypfvieh]

I released 4.3.1 and updated master to 5x-develop branch. Would you please resolve the conflicts introduced by the 5x merge so that I can merge this PR?

[brett-smith]

That's great (been waiting for 4.3.1 for other reasons too!).

Re this PR, I've got the Maven site documentation written, just got the tests to do now. I should be able to get some time on this later today.

[brett-smith]

Nearly there. To recap what is done so far,

• Entirely new annotation DBusBoundProperty
• Example ExportObjectWithProperties
• Maven site documents (properties.md)
• Test class (covers most types, BoundPropertiesTest)
• Rebased from v5 on master.

What remains is what the tests highlighted, a problem with List<SomeType> and Map<SomeKey, SomeVal>. This is certainly something to do with type erasure, but I am not yet sure exactly what is going on or how to correct it.

By using TypeRef, I can see that the types have correct introspection data (looking in d-feet), but attempt to set the value fails in RemoteInvocationHandler.

java.lang.IllegalArgumentException: Can't wrap class java.util.Arrays$ArrayList in an unqualified Variant (Exporting non-exportable type: class java.util.Arrays$ArrayList).
    at org.freedesktop.dbus.types.Variant.<init>(Variant.java:41)
    at org.freedesktop.dbus.Marshalling.convertParameters(Marshalling.java:461)
    at org.freedesktop.dbus.RemoteInvocationHandler.executeRemoteMethod(RemoteInvocationHandler.java:168)
    at org.freedesktop.dbus.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:94)
    at jdk.proxy2/jdk.proxy2.$Proxy33.setAList(Unknown Source)
    at org.freedesktop.dbus.test.BoundPropertiesTest.testProperties(BoundPropertiesTest.java:57)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

I'll keep at it, but any pointers you might have would be useful. I think https://github.com/hypfvieh/dbus-java/issues/74 seems sort of related, so I wonder if there are some extra hoops you have to jump through with these types on properties using the existing method.

[hypfvieh]

This problem look familiar... But I don't have any idea on how to fix that.

That's what I've try to explain at different occasions. It's the same issue that libraries like Jackson have. Without setting up something like TypeRef (Jackson has the same thing called TypeReference) you don't get any hint on which type was used in a container parametrized by generics. I guess that this will be a limitation one cannot fix, because it is the way Java deals with generics (= type erasure)

[brett-smith]

Yeh, this is proving quite hard. If you are happy for this PR to go in as it is, I'm fine with that. If this collections problem gets too annoying I'll take another shot at it.

I'm sure it is possible, as I said the introspect data seems correct when you do use TypeRef, so something has got the type right! The problem appears to be in convertParameters(). I think this needs some special handling for collections at the very least. I did try to convert List to an array there, but then got stuck with some error about the signature when actually writing the message.

[hypfvieh]

The issue is the use of Variant<?>. I used your unit test enabled the setAList()> method (disabled everything unrelated for easier debugging) and then started debugging the method call.

First of all, the Introspection says the expected format for the method is "as" (Array of String). This is correct. In the background DBus Properties interface is used so the call to setAList() will be mapped to Properties.Set(String, String, Object). At that point, the passed in List of setAList() must be wrapped to a Variant because the DBus Properties interface is defined that Set or Get will take/return a Variant.

Wrapping a Map/Collection will then fail because it is not possible to determine the correct type. Variant constructor is called with the value which should be wrapped. Inside the constructor Marshalling.getDBusType() is used and only the class of the given value is provided. Calling getClass() on the provided list will return e.g. ArrayList class without any information about the containing type.

I tried to get around this by determining the generic type using the given value object instead of just asking for the class. But I did not find any solution getting the actually used wrapped type.

There is another constructor for Variant allowing to provide the DBus signature to use for the wrapping type. The information is present in RemoteInvokationHandler.invoke by using the type() information of the annotation. The problem is, there is no way to pass this information to the point the Variant is created ... -.-

[Edit] There is also a bug in the unit test. The annotation for 'getAList' and 'setAList' are defined as 'StringList.class', but the lists used are containing integers. This will probably fail when we find a way to provide annotation information to Variant constructor.

[hypfvieh]

I think I got it working. I will cleanup my mess and will merge the results upstream

[brett-smith]

Wow! Good job. I'll have to review your patch see exactly what was done :)

I've already ported several project to use the new annotation, and they've all gone really well so far. None have required using this list / map pattern yet, but I can think of one that is yet to be done that does.

[brett-smith]

I just noticed I made a stupid typo in an exception message in PropertyRef.checkMethod().

throw new IllegalArgumentException("WRITE properties must have exaclty 1 parameter, and return void.");

should read ..

throw new IllegalArgumentException("WRITE properties must have exactly 1 parameter, and return void.");

[hypfvieh]

I just noticed I made a stupid typo in an exception message in PropertyRef.checkMethod().

throw new IllegalArgumentException("WRITE properties must have exaclty 1 parameter, and return void.");

should read ..

throw new IllegalArgumentException("WRITE properties must have exactly 1 parameter, and return void.");

fixed