-
Notifications
You must be signed in to change notification settings - Fork 9
[nemo-qml-plugin-dbus] Add an auto quit feature for DBus adaptors. Contributes to JB#54789 #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ntributes to JB#54789 Add an option to a acquire an event loop locker at startup and release it after a timeout. This will close the application if no windows are visible at that time. And allow additional interfaces to defined for an object by grouping multiple adaptors into an object for a path. For applications which need to support multiple interfaces but can group them on a common path this allows them to continue letting an object register the service without conflicts or race conditions between the adaptors. Applications which have to support multiple objects will also have to register themselves.
DeclarativeDBusAbstractObject::~DeclarativeDBusAbstractObject() | ||
{ | ||
QDBusConnection conn = DeclarativeDBus::connection(m_bus); | ||
conn.unregisterObject(m_path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old, but should have isEmpty() check?
|
||
void DeclarativeDBusAbstractObject::setService(const QString &service) | ||
{ | ||
if (m_service != service) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also old code, but what should happen if this or later properties get adjusted after Component.onComplete? If we don't want to support that, could at least warn?
|
||
void DeclarativeDBusAbstractObject::setQuitOnTimeout(bool quit) | ||
{ | ||
if (m_quitOnTimeout != quit && (!m_complete || !quit)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this supports turning off the autoquit but not back on later? The documentation example is "quitOnTimeout: !Qt.application.active"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the app won't be active initially so auto quit is enabled on component complete, when the application becomes active later it be disabled cancelling the timeout and it can't be turned on again.
In theory, on the application becoming active there are guaranteed visible windows and removing the event loop locker won't exit the application. And it's less laborious than explicitly cancelling the quit on every method call that results in a window being shown, and in the case where the application isn't started by dbus. I would have preferred to use ApplicationWindow.visible but that was true initially despite item visibility following window visibility and the window not yet having been shown.
Unfortunately I forgot about starting a second application while a first is still launching, in which case the first application will never hit that application.active state despite having shown a window and it will exit after a bit. So that's no good either, and we're trending back to so convoluted to use that you're better off rolling your own so you can at least follow what's happening territory.
</interface>" | ||
|
||
DBusAdaptor { | ||
iface: "com.example.service" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: one extra space on indentation.
|
||
// It is still valid to publish an object on the bus without first registering a service name, | ||
// a remote process would have to connect directly to the DBus address. | ||
if (!m_path.isEmpty() && !conn.registerVirtualObject(m_path, this)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, the QDBusVirtualObject is documented but registerVirtualObject() is marked \internal.
on the system or session bus. | ||
|
||
Each interface implemented by the object is defined in an individual DBusAdaptor in the body | ||
of the DBusObject and will be registered as belonging to the same path and the same optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not either uncommon that one service has multiple objects in different paths, or that different interfaces have different object paths. We can have support only for this variation first, but should have some plan on going forward.
What do you think about that case? DBusService { service: "foo.bar"; DBusObject { path: "/"; DBusAdaptor { ... } } or something like that, optionally the object part skpiped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have applications that mix QML dbus objects with C++ objects, some instantiated in the QML application, some in main or in some other construct.
Having the only dbus object in your application register the service for you is a convenience that can't really be rolled back now. But if you have multiple objects for a single service the solution that handles every combination is to not have an object that is responsible for registering the service for that application but to register it yourself in main after you've constructed everything.
Q_PROPERTY(QString xml READ xml WRITE setXml NOTIFY xmlChanged) | ||
Q_PROPERTY(DeclarativeDBus::BusType bus READ bus WRITE setBus NOTIFY busChanged) | ||
Q_PROPERTY(bool quitOnTimeout READ quitOnTimeout WRITE setQuitOnTimeout NOTIFY quitOnTimeoutChanged) | ||
Q_PROPERTY(int quitTimeout READ quitTimeout WRITE setQuitTimeout NOTIFY quitTimeoutChanged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings for these autoquit properties here. They do support d-bus related use-case, but then again the implementation side is not much related to d-bus adaptors, and the same property is available on two different components. Potentially later on third if we add a component to represent the d-bus service.
No clear home for the functionality, but pondering if a separate component in Silica could be justified. Perhaps not all auto-quit needs are related to d-bus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even focusing on the really specific use case of exiting because a d-bus thing didn't happen or happened but didn't result in an application window being shown it's difficult to come up with something that isn't a foot gun.
It's here because I was looking for a way to cancel the quit after handling a method if there were visible windows or something, but that doesn't cover non-prestart, or the existance other dbus adaptors. But the other reason is that keeps expectations low, it does this one thing and so long as you follow the example your applications not going to exit because you got a phone call that coincided with an exit if not seemingly open use timeout. Something in silica that could be used for hypothetical non dbus auto start timeout use cases will come with an expectation that it will actually work for those use cases.
} | ||
|
||
DBusAdaptor { | ||
iface: 'org.freedesktop.Application' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetics, but could use "" strings consistently?
Add an option to a acquire an event loop locker at startup and release
it after a timeout. This will close the application if no windows are
visible at that time.
And allow additional interfaces to defined for an object by grouping
multiple adaptors into an object for a path. For applications which
need to support multiple interfaces but can group them on a common path
this allows them to continue letting an object register the service
without conflicts or race conditions between the adaptors. Applications
which have to support multiple objects will also have to register
themselves.