[libcommhistory] Remove DBus serialisation from public headers.

[dcaliste]

If you look at the public recipient.h header file, it is including QDBusArgument. If you're using libcommhistory, but you're not using DBus directly, including this header is failing.

The solution proposed here is a bit quick and dirty. If you look at why QDBusArgument is included, it is for the operator<<() for DBus serialisation. It is even not exported. So these may be removed from the header, or move to a private one. But, if you look at event.h, it has the same serialisation, but these ones are exported. So QtDBus may be part of the API after all, and the quick patch here be the solution.

@pvuorela , what do you think ?

[pvuorela]

I could guess that the event.h export is by accident. It shouldn't be that easy to actually use that part, right? I'd say ideally those should be moved to internal headers or cpp files.

[dcaliste]

I've removed the serialisation from the header files and tried to compile commhistory-daemon. There is one issue though, because of src/mmshandler.cpp line 93. It is registering qDBusRegisterMetaType<QList<CommHistory::Event> >();.

I'm still looking to see if it can be done differently there.

[dcaliste]

This registration was added by commit cbefa3e9. I think it is only used in the signal listening EVENTS_UPDATED_SIGNAL connected to slot onEventsUpdated().

What is strange is that this commit is also adding a listener on GROUPS_UPDATED_FULL_SIGNAL, while it is not adding a registration for CommHistory::Group::List... I'm wondering if the registration that creates an issue is actualy needed...

But it's a bit hard to test. I'm afraid my knowledge on QtDBus is too limited here. Having the registration makes sense though, because it should know how to deserialise a DBus structure to create a QList from it. But then why it is not registering anything for the group list...

[dcaliste]

Maybe a good solution would be to use a UpdatesEmitter (see libcommhistory/src/updatesemitter.h) there instead of a direct connection to the session bus. But UpdatesEmitter is in the private headers...

In that case, I may use the Adaptor it is inheriting from directly.

[dcaliste]

I've removed the serialisation with DBus arguments from the public headers. It requires sailfishos/commhistory-daemon#11 though. But I think, it's cleaner like that, particularly with the Adaptor class being public : the DBus layer is then completely hidden and you can just connect to this adaptor object signals.

It breaks the possibility for third party to send Commhistory::Event and friends over DBus though. But I think it's something that is marginaly needed, if needed at all. All operations being handled by EventModel for instance.

[pvuorela]

Generally I do like this. Though the Adaptor in the api feels a bit strange and technically does create that qdbus dependency from libcommhistory. But I guess if one is using that the app is quite much opted into qdbus and we could justify not having a hard dependency on the whole thing.

Btw didn't seem originally intended public, but commit 417279e added export so it can be used in unit test. Maybe that's sort of semi private and commhistoryd is enough coupled with this to use more private parts.

[dcaliste]

Though the Adaptor in the api feels a bit strange and technically does create that qdbus dependency from libcommhistory.

Ah indeed, good point. I missed the #include <QtDbus> since I only looked for headers like #include <QDBus. Having it in the public headers defeats the purpose of this PR…

Btw didn't seem originally intended public, but commit 417279e added export so it can be used in unit test. Maybe that's sort of semi private and commhistoryd is enough coupled with this to use more private parts.

Maybe I can create a public UpdateListener class, like the UpdateEmitter, but that is just relaying the signals of the adaptor, without exposing the public inheritance on QDBusAbstractAdaptor class ? Like that libcommhistory will completely hide the fact that internally DBus is used. What do you think ?

[pvuorela]

Maybe I can create a public UpdateListener class, like the UpdateEmitter, but that is just relaying the signals of the adaptor, without exposing the public inheritance on QDBusAbstractAdaptor class ? Like that libcommhistory will completely hide the fact that internally DBus is used. What do you think ?

Sure. Was thinking something like that myself even.

There is constants.h now included in installed headers, having some d-bus names exposed, but the actual d-bus api seemed a bit complicated and not really documented. Didn't check, but I'm guessing that with commhistoryd using such listener there might not even be other needs for having the constants exported, but I can also live with that installed.

[dcaliste]

So, I've pushed it a bit further, implementing a UpdatesListener class that completely hides the underlying DBus machinery. It has two constructors:

1. UpdatesListener() that listens on signals emitted by any service with an object called "/CommHistoryModel",
2. UpdatesListener(objectPath) that listens on signals emitted by any service with the provided object. If objectPath is empty it is listening on any objects.

This is because, in some places (like in the tests) the connections are done on any objects, while in other places (like MmsHandler from the daemon) it is listening only on the "official" object. I've adjusted the tests to use this new UpdatesListener object and checked that the ut_eventmodel is green. In the tests in ut_eventmodel, it was creating an adaptor and registering a specific object for the test, but this object was actually not used. So I've removed it. It's the same in the ModelWatcher, it was registering itself on the bus, for no apparent reason (well, I didn't find any, but I may be wrong) since it has no adaptor.

I've not switched the internal code in eventmodel_p.cpp and groupmanager.cpp to use the new UpdatesListener object, but I kept the direct connections to the session bus. I'm wondering if using the new object can have a performance penalty here since it is listening to more signals than required, which means more DBus deserialisations, right ? Or is there an optimisation that avoid the deserialisation if there is no connection to the signals ?

Talking about the listeners in eventmodel_p.cpp and groupmanager.cpp, they are listening on any objects with the given interface. Do you think it is correct ? Shouldn't it listen to the COMM_HISTORY_OBJECT_PATH only ? Because they emit only on this path, but listen on any ? For instance, if I modify the emitter to be able to use a different object path, the tests could emit on a different object, and avoid the system commhistory daemon to react on changes done by tests. It's another topic than the one of the PR here, but what do you think about it ?

[pvuorela]

But these are signals?

[dcaliste]

Yes, sorry, I was not clear. I was wondering if in this case:

UpdatesListener listener;

connect(&listener, &UpdatesListener::eventsAdded, this, &MyObj::onEventsAdded);

Does Qt notice that nothing is connected to let say, eventDeleted signal of listener object, and thus don't trigger the internall connection from the DBus signal eventDeleted and there is a quick return when the DBus signal is received without deserialisation.

But well, as you said, it may not be required anyway due to the low traffic.

[pvuorela]

That's another thing, but here I meant that eventsAdded and others are signals, not a SLOT()s.

I commented the connection optimization on the pull request generic discussion, but from Qt POV the dbus signal is connected somewhere, namely the eventsAdded. If an optimization is wanted, this class can track which of its signals are connected and do d-bus connections accordingly, but yea, shouldn't maybe matter enough to warrant such.

[dcaliste]

That's another thing, but here I meant that eventsAdded and others are signals, not a SLOT()s.

Ah good point, sorry. I definitely prefer the connections with the references to functions. I don't know why it's not possible to do it in the DBus special connect() functions… I guess it's not possible since it was not added in the QDBusConnection API when it was added to the QObject one.

[pvuorela]

I've not switched the internal code in eventmodel_p.cpp and groupmanager.cpp to use the new UpdatesListener object, but I kept the direct connections to the session bus. I'm wondering if using the new object can have a performance penalty here since it is listening to more signals than required, which means more DBus deserialisations, right ? Or is there an optimisation that avoid the deserialisation if there is no connection to the signals ?

Likely the libcommhistory d-bus traffic is low enough not to matter. I wouldn't think there's any optimizations to avoid, though. Would need to probably do that in the UpdatesListener level with QObject::connectNotify(). But I think the existing connections in the mentioned classes are good enough too.

Talking about the listeners in eventmodel_p.cpp and groupmanager.cpp, they are listening on any objects with the given interface. Do you think it is correct ? Shouldn't it listen to the COMM_HISTORY_OBJECT_PATH only ? Because they emit only on this path, but listen on any

Guess so.

Created btw JB#62199 for this. You can use on commit message or I just annotate the tag.

[dcaliste]

I've added the JB number to the commit message. I think I'll do a separated PR to internally use the UpdateListener and listen only on the COMM_HISTORY_OBJECT_PATH object and not all objects, if asked so.

[dcaliste]

I've changed SLOT for SIGNAL in the connections in UpdatesListener constructor.