[Bug 35321] Implement 3rd party message send/receive API

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=35321

--- Comment #9 from Olli Salli <ollisal at gmail.com> 2011-03-18 02:06:33 PDT ---
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!

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.