[Bug 28200] TpBaseContactList

[bugzilla-daemon at freedesktop.org]

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

--- Comment #36 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-07-22 04:09:21 PDT ---
Branch updated as far as d9902722b7794fc35007fc10328cf83b55f0313d. I'll update
my Gabble branch (Bug #28199) to match.

(In reply to comment #35)
> > The contactlist example CM isn't particularly realistic: it behaves as if
> > incoming publish requests cause the contact to appear on our roster. It would
> > be more realistic to have a separate list of requests, and have the Telepathy
> > contact list be the union of the protocol roster and the requests list.
> >
> > Calling Unpublish() on a pending request would remove the contact from the
> > requests list, and therefore from the Telepathy contact list (assuming we
> > hadn't done anything that would cause them to be on our roster, like setting an
> > alias or putting them in a group).

I've made this change, and updated the test's assertions to match.

> > Methods that create groups as a side-effect (like adding members to a
> > nonexistent group) don't call the create_groups_async virtual method. I think
> > that's OK - for robustness, the subclass ought to auto-create groups anyway -
> > but would appreciate confirmation from reviewers.
> I guess I'll have to try using it before I can really comment on this.
> My initial thoughts are that on protocols where creating empty groups is a
> no-op, calling create_groups_async() is just adding complication to the
> bindings.

Right, in practice it was easier to have it be part of AddToGroup,
SetGroupMembers and SetContactGroups; in protocols that need to do actual work
to create a group, these methods can all call a helper function. I've made this
change.

> Also, if tp-glib includes auto-create groups logic, it might be
> expected to have auto-remove groups logic too, which is probably more difficult
> to get right in a generic way?

Auto-removing groups doesn't make sense: it is often possible to have a group
with no members, and on protocols where it isn't, we should emulate it within
the lifetime of the session to make UIs' lives easier.

(Consider: I have Alice in group A and Bob in group B. I want to exchange them,
in a UI with drag-and-drop. I drag Alice to group B, and group A is empty for a
moment, then I drag Bob to group A. If deletion of group A had been signalled
to the UI, there'd be nowhere to drop Bob.)

> > Alternatively, CreateChannel and EnsureChannel could call
> > set_group_members_async(group, []) or add_to_group_async(group, []) (which the
> > subclass has to support anyway, because they can be called from D-Bus),
> > removing the need for an explicit create method.
> Is the create method something that's going to disappear in telepathy-1.0 or
> something? If so, avoiding it entirely sounds like the way forwards. Again, I
> think implementing a CM or two and finding the edge cases is going to be more
> useful than lots of discussion here.

I've implemented this behaviour (I used add_to_group_async, but documented both
the methods I mentioned as having to create groups automatically), and deleted
create_groups_async.

> In other news: Trivial things that made me have to think when reviewing your
> code:
> d979ef8b54643fecc0717467aa6a75db0c968809
> +      GPtrArray *pa;
> doesn't really tell me what you're about to do with it. s/pa/new_interfaces/ or
> something? You use that variable name in a few places in base-contact-list.c,
> but nowhere else in tp-glib.

I've replaced all uses of this variable name.

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