The branch is in the URL field. Note that implementing the send functionality specifically requires some further service-side work.

(In reply to comment #0)
> the bug 28974 he filed

That actually is bug 28753 but written in my very special numeral system.

Comments on the API:

First of all, the API must allow selecting just a single contact to communicate with. This is for convenience, but also for efficiency purposes: for a single-contact API, we can register an observer filter with the TargetID key for that contact. I imagine most uses for this API (except, if you want to write some simple text logger, say) is for communicating with a single contact at a time, so making the API reflect that we can prevent being woken up when unrelated text channels appear.

Note that this API is necessarily somewhat inefficient though: it will be invoked for the same contact ID being communicated with on ALL accounts, as we can't declare a client filter to only catch communication on a single account. We just need to filter the other accounts out client-side (note that we can't signal them: the same contact ID can mean a different person on different accounts).

The "I really want to write a simple text logger" usecase we could support by allowing passing a NULL ContactPtr / empty contact identifier to the create() method. This comes with the limitation that you can't send messages as you have no recipient though. That is, the contact parameter should be removed from the send methods and signals, as it'll be passed to the create() method already. Following that, the simple text logger usecase looks even more distinct: maybe a separate class for that, which we add later - so this API would always require a valid contact passed to it? Then you don't need to worry about how to signal the recipient in the signals either, because it'll be the one you passed.

There should be a public accessor for the contact ID the object is communicating with (NOT a ContactPtr - these are only temporary from this API's POV, as they're specific to a Connection, not valid between different Connections to an Account).

The send(Tp::Message) overload doesn't make sense here. Tp::Message is exclusively for signaling sent and received messages by Tp::TextChannel (and consequently, us) - not for passing in messages to send. You can't construct anything else than text part only -message using its public API anyway (why it allows even that is beyond me, though).

For sending simple text part only -messages, you'll use the QString textPart overloads. For sending multi-part complex messages, you use the MessagePartList overload. Though, we should really wrap MessagePart with one of our friendly variant map wrappers for that (can be just an API/ABI placeholder empty class for now). Currently it's a bit hostile to use, especially in a "simple" API like this.

Now on to naming. So, this'll be a single-contact API. ContactMessenger?

One point I tried to make earlier is that while you don't need to worry about designing and implementing other things than "send, but don't present the UI" from http://www.mail-archive.com/telepathy@lists.freedesktop.org/msg04261.html, you need to make the method naming / parameters cater for adding those later, as they're all very useful usecases.

presentUIForSending is I think a good name for one of the usecases, and doesn't easily conflict with the others two, which are more problematic: Would send() (the current one), sendAndPresentUI() and presentUIForSending() be a triple that make sense? I think that's cleaner than Will's original proposal of having a bool parameter to send() which tells whether the UI should be presented or not. That could be made better by using an enum { PresentUI, DontPresentUI } and including a parameter which defaults to DontPresentUI, though - please consider which of these look better, and if it's the bool or enum variant, add that parameter as a placeholder to send() for now.

Will's proposal from the observer bug to include the actual TextChannel the events happen on is a good one IMO. Doing that, one can take the channel up for further observation, e.g. for observing remote chat state changes on the channel. If we don't signal the Channel, a Client wanting to do such a thing would need to go the full way of implementing AbstractClientObserver themselves. We can signal it at no cost, since we've already had to build it to get the signals in the first place. If it's the last argument of the signal, you can also totally ignore it in applications if desired due to Qt's extra signal argument ignore rule. You can also use the actual channel the event happened at to get the Contact object, if needed.

We can implement the observer the same way as you implemented the Handler for the R&H API: creating a private ClientRegistrar with a fake account factory returning just the account this ContactMessenger was constructed for. The factory needs to be altered a bit though, because we can't register an observer filter just for a single Account - we'll be invoked for text channels for all Accounts which coincidentally have an equal target ID (but can be a different contact!). We'll just need to ignore these client-side, without failing or warning at the factory (so change it to return a non-ready Account proxy for the signaled object path, and in the observeChannels implementation ignore channels for which the Account proxy is not the expected one?).

Oh, also if we call the class ContactMessenger or something like that (so that the name already includes some word having roots in "message") we don't need to repeat the "message" prefix/suffix in the method/signal names. Any kind of consistency in naming with the full API in TextChannel is not important: you'll use one or the other for your text messaging needs, not both.

So, updated the API to conform with Olli's suggestions with a minor point:
- I chose to use sendMessage/messageSent/messageReceived instead of send/sent/received as I believe it looks better, but I am ok if you still want to change.
  Eg.: pseudo code:
       messenger->sendMessage(...) vs messender->send(...)
       connect(messenger, messageSent) vs connect(messenger, sent)

And for those that didn't realize yet, we are about to introduce yet another CM acronym in tp-qt4 (ContactManager, ConnectionManager, ContactMessenger), who is going to be next?? heh.

Yeah, put that way I agree with you on the method names actually. The long versions are indeed clearer.

Umm, you now have the overloads twice for sendMessage() :) So clean that up, please.

The MessageSent signal should include the Channel as well. The signal is supposed to be emitted from Observing channels, not as a direct response to sendMessage() we generate ourselves. In particular, this signal is useful when somebody else than you sends a message to the contact you're messaging with. The direct response to sending messages ourselves is finishing the PendingSend.

MessageParts are message parts, they're not specifications for message parts! The "spec" I added in names for (Requestable)ChannelClassSpec means "specification" as in "formal specification" - it's not applicable here, as the message parts are data, not format specifications or such for data. For a name which doesn't clash with the auto-generated MessagePart struct, how about MessageContentPart? Or RichMessagePart, as that API would only be used for various kinds of "rich messages"?

Otherwise looks good now.

Pushed all the requested changes to my branch in URL

The branch in URL is updated with send/receive support.

Notes:
1 - The branch simple-send-receive is on top a branch called 0.21.13.UNRELEASED that will be updated once the spec is released with the CD.I.Message addition. We will have to also generate low-level bindings for other new stuff added to the spec.
2 - I changed a bit CD.I.Message found here https://people.internal.collabora.co.uk/~vivek/Channel_Dispatcher_Interface_Message_DRAFT.xml, by removing the Status signal and the TargetHandleType param from Send. I also did other minor changes to make our autogen tools happy.
3 - The code is _untested_ and bugs are expected. Once we merge this branch with the tests Olli is writing we can properly fix them.

A new class called SimpleTextObserver was added (I believe it suffices #28753), and can be used standalone if the user only wants to observe channels. ContactMessenger uses it internally for the receiving support.

I fully agree with your changes to the spec, but I think we need one more: The current name CD.I.Message would imply representing a single message. That should be changed to CD.I.Messages, which is also consistent with Chan.I.Messages naming. This is of course just a trivial change, but please prepare the branch for it in advance.

I had actually modified my copy of the spec in the unit tests's adaptor code in the exact same fashion, except for Message vs Messages :)

SimpleTextObserver is very much a ++ idea to include in this branch already, as indeed, it's useful as an implementation helper too. I'll write a test for it as well.

I believe adding a default param of ContactPtr contact = ContactPtr for STO::create() would be equivalent to having the current separate overload for passing no contact (ptr or id). You've probably made it a separate overload for documentation purposes, with the intent that the contact-less overload would be used whenever you're going to observe all contacts? In that case, I'd make the ContactPtr variant warn if you pass it NULL.

+        // this shouldn't happen, but in any case
+        if (!textChannel || !textChannel->isValid()) {
+            continue;
+        }

This might happen in particular if the user has set on the channel factory a custom subclass for text channels which doesn't derive from Tp::TextChannel. We should do either of:

a) warn here, specifically for the cast failing by telling that the ChannelFactory subclass for text chats should be a TextChannel subclass if you want to use this API and document that in both ChannelFactory and for this API

OR

b) Construct a TextChannel on our own to use based on the object path. The user-specified Channel subclass will have its ref dropped and be freed. I don't think this is a great idea, however, as then no features set on the factory would apply either.

I'd lean towards a) myself.

+      AbstractClientObserver(channelFilter, true),

Recovering doesn't make sense for this observer here, as it's not service-activatable. Remember that recovery means that on an observer disappearing from the bus (crashing), MC should reactivate it using D-Bus service activation and redispatch all channels matching its filters to it.

You could perhaps connect the Wrapper signals directly to the STO signals as the slots do nothing with them in between.

Otherwise STO looks very cleanly implemented to me - good job there!

On to ContactMessenger:

+    if (contactIdentifier.isEmpty()) {
+        return ContactMessengerPtr();
+    }

warn

PendingSendMessage:

+/**
+ * Return the channel used to send the message if this instance was created using
+ * TextChannel. If it was created using ContactMessenger, return a null TextChannelPtr.
+ *
+ * \return A TextChannelPtr object.
+ */

Perhaps add the complementary accessor: return the Messenger if that was used instead? The messenger is useful at least for discovering the original target ID where the messages were supposed to be headed.

OH CRAP! I suddenly realize we've made a terrible overlook here: we're registering an observer with a strict ID match on the user-passed target ID, without normalizing it in between! This means that if I pass "044 123 4567" as the target ID, the real channel will probably have "0441234567" as its target ID and I'll lose the events! Note that we can't filter the events even at client side in the STO (registering a generic filter): we would have to compare the observed channel's target ID with our unnormalized target ID just the same as the CD does.

To fix this we must normalize the target ID using http://telepathy.freedesktop.org/spec/Protocol.html#Method:NormalizeContact. This in effect means two things:

1a) ContactMessenger construction must be async, so that we can do the async round trip to that function

OR 

1b) We deliberately miss messageSent and messageReceived events until we've discovered the normalized ID. This might be the better solution but needs to be documented.

AND 

2) we require that API to be implemented in the CM.

Now, train arriving on station. I believe the above will keep you busy for a while when you wake up!

(In reply to comment #9)
> I fully agree with your changes to the spec, but I think we need one more: The
> current name CD.I.Message would imply representing a single message. That
> should be changed to CD.I.Messages, which is also consistent with
> Chan.I.Messages naming. This is of course just a trivial change, but please
> prepare the branch for it in advance.
> 
> I had actually modified my copy of the spec in the unit tests's adaptor code in
> the exact same fashion, except for Message vs Messages :)
Agreed. I changed it to Messages now and moved it to future-foo as we will depend on a draft iface. Had to push -f the branch to "fix" this.

> SimpleTextObserver is very much a ++ idea to include in this branch already, as
> indeed, it's useful as an implementation helper too. I'll write a test for it
> as well.
Great, looking forward for the tests.

> I believe adding a default param of ContactPtr contact = ContactPtr for
> STO::create() would be equivalent to having the current separate overload for
> passing no contact (ptr or id). You've probably made it a separate overload for
> documentation purposes, with the intent that the contact-less overload would be
> used whenever you're going to observe all contacts? In that case, I'd make the
> ContactPtr variant warn if you pass it NULL.
So I decided to use 3 constructors as I think it makes the API cleaner. I didn't want to warn here as using the variant create(account, contactId) won't warn if the id is empty, so I just added docs explaining what happens if id/contact params are invalid.
 
> +        // this shouldn't happen, but in any case
> +        if (!textChannel || !textChannel->isValid()) {
> +            continue;
> +        }
> 
> This might happen in particular if the user has set on the channel factory a
> custom subclass for text channels which doesn't derive from Tp::TextChannel. We
> should do either of:
> 
> a) warn here, specifically for the cast failing by telling that the
> ChannelFactory subclass for text chats should be a TextChannel subclass if you
> want to use this API and document that in both ChannelFactory and for this API
> 
> OR
> 
> b) Construct a TextChannel on our own to use based on the object path. The
> user-specified Channel subclass will have its ref dropped and be freed. I don't
> think this is a great idea, however, as then no features set on the factory
> would apply either.
> 
> I'd lean towards a) myself.
I used a) here and added some other warnings also.

> +      AbstractClientObserver(channelFilter, true),
> 
> Recovering doesn't make sense for this observer here, as it's not
> service-activatable. Remember that recovery means that on an observer
> disappearing from the bus (crashing), MC should reactivate it using D-Bus
> service activation and redispatch all channels matching its filters to it.
Obviously, fixed :D
 
> You could perhaps connect the Wrapper signals directly to the STO signals as
> the slots do nothing with them in between.
Done.

> Otherwise STO looks very cleanly implemented to me - good job there!
Tnx
 
> On to ContactMessenger:
> 
> +    if (contactIdentifier.isEmpty()) {
> +        return ContactMessengerPtr();
> +    }
> 
> warn
Done
 
> PendingSendMessage:
> 
> +/**
> + * Return the channel used to send the message if this instance was created
> using
> + * TextChannel. If it was created using ContactMessenger, return a null
> TextChannelPtr.
> + *
> + * \return A TextChannelPtr object.
> + */
> 
> Perhaps add the complementary accessor: return the Messenger if that was used
> instead? The messenger is useful at least for discovering the original target
> ID where the messages were supposed to be headed.
Done
 
> OH CRAP! I suddenly realize we've made a terrible overlook here: we're
> registering an observer with a strict ID match on the user-passed target ID,
> without normalizing it in between! This means that if I pass "044 123 4567" as
> the target ID, the real channel will probably have "0441234567" as its target
> ID and I'll lose the events! Note that we can't filter the events even at
> client side in the STO (registering a generic filter): we would have to compare
> the observed channel's target ID with our unnormalized target ID just the same
> as the CD does.
> 
> To fix this we must normalize the target ID using
> http://telepathy.freedesktop.org/spec/Protocol.html#Method:NormalizeContact.
> This in effect means two things:
> 
> 1a) ContactMessenger construction must be async, so that we can do the async
> round trip to that function
> 
> OR 
> 
> 1b) We deliberately miss messageSent and messageReceived events until we've
> discovered the normalized ID. This might be the better solution but needs to be
> documented.
> 
> AND 
> 
> 2) we require that API to be implemented in the CM.
So this is the only part missing from this and I would like to discuss it. One other idea would be to just use the id as is and document that we require normalized ids. Using the create(account, contact) variant will work anyway as the contact->id is already normalized. Let's see what we should do here.

Fixed in tp-qt4 0.5.12.