Delayed DBus answer

[Kaffeine]

We need to be able to delay DBus answers, but it is not possible with current design.
Some methods need to make a network request before complete the call.

Examples:

• Connection.Interface.Addressing: method GetContactsByURI
• Connection.Interface.ClientTypes: method RequestClientTypes
• Connection.Interface.ContactInfo: method RequestContactInfo
• Channel.Interface.Password: method ProvidePassword

Telepathy-Morse also needs to delay Connection.Interface.Requests CreateChannel() to get a native identifier of created group chat.

[Kaffeine]

@rmescandon suggested to replace the last callback parameter (Tp::DBusError) by the dbus context.

Pros:

• Adaptors are already TP_QT_EXPORT'ed, so we don't have to expose more things
• All input arguments are passed to callback as usual C++ arguments (the code is not generated, so we can keep some input args compatibility if needed)
• Method (return) type is a pretty stable thing. We can bump the ABI if the change is that serious.
• We don't have to add new more arguments, just replace one by another.
• error->set() is not any better than context->setFinishedWithError(), so we don't lose much here

Cons:

• Spec changes can affect ABI
• We have to include big _gen/svc-{connection,channel,...}.h in the public headers

Still, this sounds good for me.

Opinions? @gkiagia ?

[gkiagia]

That was also one of my thoughts. The other thought I had was to use Tp::PendingOperation and wrap Tp::MethodInvocationContext inside it, somehow.

In both cases, though, there is something that I don't like about the whole design...

Historically, telepathy-qt's design originates in telepathy-glib's design, meaning that, telepathy-qt has always tried to copy the design of tp-glib (explains the refcounted objects for one ;). For inspiration, I had a look at how tp-glib implements the service-side BaseSomething classes. So, in tp-glib, the generated bindings are classes that resemble a lot our adaptees, mixing with the high-level bindings. Some sample (generated) API:

typedef void (*tp_svc_channel_close_impl) (TpSvcChannel *self, DBusGMethodInvocation *context);
void tp_svc_channel_implement_close (TpSvcChannelClass *klass, tp_svc_channel_close_impl impl);
void tp_svc_channel_return_from_close (DBusGMethodInvocation *context);

// and in the .c file...
static void
tp_svc_channel_close (TpSvcChannel *self, DBusGMethodInvocation *context) {
    // calls the saved tp_svc_channel_close_impl callback
}

This is exactly the same as our "high-level" tricks with the callbacks. Think that
tp_svc_channel_close (TpSvcChannel *self, DBusGMethodInvocation *context) is the equivalent of
void BaseChannel::Adaptee::close(const Tp::Service::ChannelAdaptor::CloseContextPtr &context) and void tp_svc_channel_implement_close (TpSvcChannelClass *klass, tp_svc_channel_close_impl impl); would be the equivalent of void BaseChannel::setCloseCallback(const CloseCallback &cb), which we don't have because this specific method is implemented trivially in our bindings, but you get the idea...

Now looking at the higher level bindings, you will notice that not all the classes represent dbus objects directly. And vice-versa, there are interfaces that are nowhere to be found on the high level. glib-based CMs simply mix low-level and high-level bindings. Methods like the ones we are talking about in this ticket, are mostly implemented with the low level bindings.

This brings into question the bindings that we are writing in tp-qt. In tp-qt we have a lot of duplication. Basically, the Adaptor API is reflected in the Adaptee, which is reflected on the high-level class, and we have so many high-level classes that you even started to generate them with a tool... sounds like something is wrong there.

Bridging this with the original subject, I have no objection exposing the low-level API, but the point I am trying to make is that if we do that, then we may as well try to use it directly instead of wrapping it again and again.

The problem, though, is that it is not so obvious how to mix those two with the current API (and it is actually a bit complex). So, I want to propose some changes around the service-side API in general:

1. Generate the Adaptees and put them in the Tp::Service namespace.
2. Promote the Adaptees to include API for managing user callbacks. So, for example, Tp::Service::ChannelAdaptee, apart from including a void close(const Tp::Service::ChannelAdaptor::CloseContextPtr &context) method, it should also include a void setCloseCallback(const CloseCallback &cb) that can be used to implement the close() method without delegating the call to any higher level class.
3. Adaptees should create their adaptors internally, essentially making adaptors "hidden" API.
4. High level objects should allow plugging Adaptees directly. I imagine something like this happening:

auto *passwdAdaptee = new Tp::Service::ChannelInterfacePasswordAdaptee();
passwdAdaptee->setProvidePasswordCallback(...);
baseChannel->plugInterface(passwdAdaptee);

This will eliminate the need for high-level BaseFooInterface classes that we have lying around and will allow us build real high-level bindings. Essential interfaces, like Conn.I.Requests, should be part of the main high-level classes (BaseConnection in this case) and others can be arranged in a similar fashion as in tp-glib. For example, Chan.T.Text and Chan.I.Messages can be made into a single BaseMessagesChannel. Or, many of the Connection interfaces that deal with contacts can perhaps make a single BaseContactList class that manages much of the complexity.

Now all these are just my thoughts after spending the whole morning thinking about the problem... Since I have some spare time these days, I'll try to prototype it and we can see... In any case, I don't object to the original idea, but I'd like to try something better.

[gkiagia]

Here is a first version of my idea:
https://gist.github.com/gkiagia/6583a6e1c621fc28a8109e20a08463a9

So far I have modified the generator to output this, so it's not hand-written. (I only removed the generated doc strings to make the gist more readable)

Main points of this implementation:

On the base class level:

• DBusObject is the main class exported on the bus. Unlike before, it now derives from Tp::Object, so it can be put in a SharedPtr etc... The idea is then to have DBusService inherit DBusObject and provide the additional service registration functionality.
• AbstractDBusInterfaceAdaptee is the base class of all adaptees. This is a class that provides the mechanism that will allow 'plugging' adaptees on the main DBusObject
• There is no base class for the Adaptor anymore.

On the generated level:

• The Adaptor looks like before, api-wise, but is not exported
• It relies purely on C++, there is no QMetaObject magic
• The Adaptee creates the Adaptor internally when the object is about to be registered. The Adaptee is still fully usable when it is not registered
• Adaptee properties act as cache variables for the values exported on the bus. They are always read/write, even if they are read-only on the bus, so that the implementation can set the values internally.
• Writable properties have an additional signal on the Adaptee that notifies the implementation when a property was set from the bus. The property is set anyway, even if the implementation doesn't handle the new value. (See the URI property on the specific example)
• DBus API methods on the Adaptee can be implemented with a callback. They always return void and the return values are returned from the context (async API, yay!)
• DBus API signals are implemented as emitFoo() slots on the Adaptee, which feels more natural than having Q_SIGNALS that are meant to be emitted instead of connected to.

Comments?

[gkiagia]

So, opinions? @Kaffeine ?

[olesalscheider]

I like the proposal and think that we should do it that way for 1.0.

[rmescandon]

+1

[boiko]

+1

Em 8 de nov de 2016 12:53, "Niels Ole Salscheider" notifications@github.com
escreveu:

I like the proposal and think that we should do it that way for 1.0.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#5 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPioV-ZsTqLvF6uXxWsCYsYrVDY35Zoks5q8IlLgaJpZM4JIQWS
.

[Kaffeine]

@gkiagia I'm sorry, I would like to read it carefully, but I have a very limited time (I'm changing the job and have to get done so many tasks for my former project).

I like the idea, but I have one point, one question and a few nit-picks (may be I just misreading something):

• Q_PROPERTY macros in ChannelTypeFileTransferAdaptee? I think that we don't need it.
• Why did you choose FileTransfer? :) This is the interface which I carefully crafted and covered with a nice test. I think it is the most high-level class on the service side.

Nit-picks (probably you posted a quick draft, so they makes no sense):

• Please remove the virtual keywords from overrides. (virtual void createAdaptor() Q_DECL_OVERRIDE; and so on.)
• Methods, defined in a line of a class definition, are already inlined. You don't need "inline" keyword there:

inline bool isRegistered() const
{ return dbusObject() ? dbusObject()->isRegistered() : false; }

May be it would be better this way:

bool isRegistered() const { return dbusObject() && dbusObject()->isRegistered(); }

• Qt, KDE and TelepathyQt (mostly) follow the convension of asterisks and ampersand aligned to the name, rather then type. (QDBusVariant& accessControlParam -> QDBusVariant &accessControlParam).

Still, +1 :)

[gkiagia]

Take your time in checking details, I was just unsure whether the whole design sounded good or not.

Q_PROPERTY macros at ChannelTypeFileTransferAdaptee? I think that we don't need it.

We don't need it, but I thought that maybe it can be useful for some people. I can remove them and see if we find any need for them in the future. We can always add those later.

Why you choosed FileTransfer? :) This is the interface which I carefully crafted and covered with a nice test. I think it is the most high-level class on the service side.

Totally random, I just needed an interface that would showcase all bindings: at least one read-only property, at least one read-write property, at least one method and at least one signal. Most interfaces have all of those except the read-write property, and I remembered this one had the "URI" property because I had happened to look at its spec recently, so I picked it... I didn't even see what the high level class looks like.

Please remove the virtual keywords from overrides. (virtual void createAdaptor() Q_DECL_OVERRIDE; and so on.)
You don't need inline here.

A matter of style, I would say :) I usually like to put keywords, even though they can be inferred easily, since that makes the statement implicitly clear to the human eye about what it does. At least this was very much needed with virtuals in C++03, as there was no override keyword. Now I guess I can live without the redundant "virtual" keyword...

bool isRegistered() const { return dbusObject() && dbusObject()->isRegistered(); }

Of course :S /me feels stupid

Qt, KDE and TelepathyQt (mostly) follow the convension of asterisks moved to the name, rather then type. (QDBusVariant& accessControlParam -> QDBusVariant &accessControlParam).

Actually, Qt and KDE follow the convention that you say. TelepathyQt follows the opposite. The existing generated bindings use the same style that I have used there, I just chose not to change it. I prefer the Qt/KDE one, though.

[Kaffeine]

Adaptee::Private constructor and POD-members initialization is missing.
We need to change generated code in follow manner:

struct TP_QT_NO_EXPORT DebugAdaptee::Private
{
+   Private() :
+       isEnabled(false)
+   {
+   }
+
    QPointer<DebugAdaptor> adaptor;
    bool isEnabled;
    GetMessagesCallback getMessagesCb;
};

[gkiagia]

I have pushed a commit in my svc-refactoring branch to address the POD-member initialization. I have used C++11 non-static member initializers instead of a constructor, as it was easier (it's hard to do the commas between the member initializers when you are going through them in a for loop!)