hypfvieh / dbus-java

Improved version of java DBus library provided by freedesktop.org (https://dbus.freedesktop.org/doc/dbus-java/)
https://hypfvieh.github.io/dbus-java/
MIT License
185 stars 73 forks source link

Request for API improvement with adding and removing signal handlers. #175

Closed brett-smith closed 2 years ago

brett-smith commented 2 years ago

This is not a bug, just a request for comments on possible API improvements to adding and removing signal handlers.

For example, say I have a GUI app. In this app, one of the windows I open will add a set of signal handlers when the window opens, and remove them when the windows. The app has various different types of windows that might listen, or stop listening to, various signals.

The code to achieve this a bit clunky, especially when used with lambda syntax, and may require keeping references to multiple objects to be able to remove signals.

private DBusSigHandler<SomeDbusObject.MySignal> mySigHandler;
private SomeDbusObject myObject;

void setupSignals() {
    myObject = connection.getRemoteObject(BUS_NAME, ROOT_OBJECT_PATH, SomeDbusObject.class).
    mySigHandler = (e) -> {
        // Do stuff
    };
    connection.addSigHandler(SomeDbusObject.MySignal.class, myObject, mySigHandler);
}

void tearDownSignals() {
    connection.removeSigHandler(SomeDbusObject.MySignal.class, myObject, mySigHandler);
}

This is not terrible, but it can get a bit excessive when you have many signals.

At the very least, it may be useful to do 2 things to reduce the typing a bit.

So the above code becomes.

private DBusSigHandler<SomeDbusObject.MySignal> mySigHandler;

void setupSignals() {
   mySigHandler =  connection.addSigHandler(SomeDbusObject.MySignal.class, 
     connection.getRemoteObject(BUS_NAME, ROOT_OBJECT_PATH, SomeDbusObject.class,  (e) -> {
        // Do stuff
    });
}

void tearDownSignals() {
    connection.removeSigHandler(mySigHandler);
}
hypfvieh commented 2 years ago

Sounds good to me. Feel free to provide a PR for this feature.

ghost commented 2 years ago

Return the handler reference in addSigHandler()

Provide a new removeSigHandler() than only takes the handler instance.

While that works, another approach is to return a Closeable implementation token. Such as:

final var token = connection.addSigHandler( ... );
token.close();

It's a bit more flexible because the code that performs the closing doesn't need a handle to the connection, only the token.

brett-smith commented 2 years ago

Having a Closeable of some sort sounds good to me too. I'll get a PR together.

hypfvieh commented 2 years ago

The change has been merged, can we close this issue?

ghost commented 2 years ago

Does any library usage documentation/example need to be updated as part of the PR?

hypfvieh commented 2 years ago

I don't think so. But if you want to add some more samples or documentation, that would be great