[Bug 28200] TpBaseContactList

[bugzilla-daemon at freedesktop.org]

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

--- Comment #40 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-08-04 03:02:12 PDT ---
(In reply to comment #39)
> Needing to handle multiple contacts at the same time is a pain in the ass, if
> you have to do everything async *before* calling the callback, as the API asks
> you to (requires looping and counting the number of async calls you make). What
> is the use-case for adding multiple contacts with the same message?

At the D-Bus level, the "change" methods are:

* RequestSubscription: being plural is only useful if you're in a protocol like
SIP/SIMPLE where the contact list is transient. Empathy could store the contact
list locally (in libfolks?), and drop it into both AuthorizePublication and
RequestSubscription every time you connect. On the other hand, perhaps we want
to declare that connection managers for SIMPLE should store your contact list
in the XDG_DATA_HOME anyway?

* AuthorizePublication: same as RequestSubscription, but being plural is also
useful if you want to apply an "auto-authorize people we're subscribed to" UI
policy. Initially you GetContactListAttributes, filter for people we're
subscribed to, and drop them back into AuthorizePublication; on changes, you
filter and send back in the same way. If that's your UI's policy, then the
"pre-authorization" semantics of AuthorizePublication make it work exactly like
you want it to :-)

* RemoveContacts: being plural is nonessential

* Unsubscribe, Unpublish: should be rare anyway

* SetContactGroups: not plural for contacts

* SetGroupMembers: has to be plural to be useful!

* AddToGroup, RemoveFromGroup: nonessential

* RemoveGroup, RenameGroup: not plural, but when implemented on a protocol
where they're not an atomic network operation, the effects are necessarily
plural

So to implement SetGroupMembers, RemoveGroup and RenameGroup, you need
operation-counting anyway.

For each change, we can classify protocols into two sets:

1. the change is done per-contact

2. the change is done atomically

XMPP is in set 1 for all operations, but other protocols might not be.

If we don't implement methods as plural at both the D-Bus level and the C
level, we lose the opportunity to make changes atomically - we'd essentially
make all protocols look like set 1, even if they could be in set 2.

Perhaps it'd be worth making the list (but not group) operations singular in C,
I don't know. If we do, we should probably make them singular on D-Bus too?

I don't think it's really all that onerous to count changes, though - Gabble
now has gabble_simple_async_counter_new(), which we could provide in
telepathy-glib if people like it enough.

> Also, doing it GAsyncResult-based means that your function has two pointers
> that get passed into it.

Not our bug; this is how GAsyncResult works, and being consistent with other
GObject libraries (potentially gaining "free" CM bindings later) is important.

> If you immediately convert that into a
> GSimpleAsyncResult, it brings it down to one, but I can't help thinking it
> would be simpler to be passed in an opaque token for the request (which could
> easily be a pointer to a GSimpleAsyncResult, with the agreement that you will
> call tp_mutable_contact_list_request_subscription_finished (self, token); when
> you're done.

Part of the point of GAsyncResult is that not all async results need to be
Simple - you can subclass GSimpleAsyncResult to encapsulate extra state if you
need to, or make your own implementation of GAsyncResult that encapsulates all
the extra state you're tracking. gabble_simple_async_counter_new() is a
simplistic version of this (but it abuses the result pointer, which it happens
to not need, to be the counter).

> Also, I can't think of a use-case for implementing a finish function,
> especially if the whole point of the GAsyncResult API is that it doesn't
> guarantee that it will ever be called.

The finish function pulls out the "return" from the async result. It's
necessary to support non-Simple async results, and is conventional; bindings
can presumably safely require it.

In our case, the finish function is very simple, because in the successful
case, we "return void". The same's true for (for instance) GAsyncInitable,
though.

> If you want to do cleanup, you can
> either finish not in an idle

No, one of the simplifications made by GAsyncResult is that callbacks are
always called from the (calling thread's) main loop, never synchronously. The
only place where it's acceptable to finish not-in-an-idle is if you know for
sure that you're being called from the calling thread's main loop (which
basically means you're finishing a GAsyncResult from another GAsyncResult's
callback).

> or do it in the function you pass into
> set_op_res_gpointer().

... or in the dispose/finalize of your GAsyncResult implementation, if you
reimplemented it.

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