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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #10 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-03-18 07:46:33 PDT ---
(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.

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