+++ This bug was initially created as a clone of Bug #26171 +++ We should have telepathy-qt4-style API for the contact lists: a Feature on the Connection that provides a list of TpContact objects for the union of the contact lists, plus some views onto that list (subscribe, publish, blocked, subscribe-or-publish, etc.). TpContact objects should also have subscription state as a Feature, which would enable the corresponding Connection Feature behind the scenes. After Bug #21787 in telepathy-spec is fixed, this can become a considerably thinner wrapper, since it'll no longer have to worry about ContactList channels and can just access the ContactList interface directly.
So, I wrote a small Telepathy bot in Python this week-end and the lake of high level API to access to the contact list made me cry. We should have something like: Feature ------- TP_CONNECTION_FEATURE_CONTACT_LIST (which depends on TP_CONNECTION_FEATURE_CONNECTED and TP_IFACE_CHANNEL_TYPE_CONTACT_LIST). Methods ------- void tp_connection_contact_list_get_contacts_async (TpConnection *self, guint n_features, const TpContactFeature *features, GAsyncReadyCallback callback, gpointer user_data); Or should we use a GArray instead? gboolean tp_connection_contact_list_get_contacts_finish (TpConnection *self, GAsyncResult *result, GList **contacts, GError **error); with @contacts containing TpContact objects having all the features prepared. void tp_connection_contact_list_request_subscription_async (TpConnection *self, guint n_ids, const gchar * const *ids, const gchar *message, GAsyncReadyCallback callback, gpointer user_data); Do we want an handle variant ? void tp_connection_contact_list_authorize_publication_async (TpConnection *self, GList *contacts, GAsyncReadyCallback callback, gpointer user_data); With @contacts containing TpContact objects. Same signature for _remove_contacts_async(), _unsubscribe_async() and _unpublish_async(). I didn't mention the _finish() functions when they are trivial. Signal ------ TpConnection::contact-list-changed (GList *added, GList *removed); with both being list of TpContact. Or a GArray? I'm wondering if we should just signals addition/removal of contacts and ignore subscribe, publish and publish-request attributes changes are they are already announced on TpContact. Properties ---------- TpConnnection:contact-list-persists (gboolean) TpConnnection:contact-list-can-be-changed (gboolean) TpConnnection:contact-list-request-uses-message (gboolean) + the obvious _get_() accessors.
(In reply to comment #1) > TP_CONNECTION_FEATURE_CONTACT_LIST (which depends on > TP_CONNECTION_FEATURE_CONNECTED and TP_IFACE_CHANNEL_TYPE_CONTACT_LIST). Some of the ContactList information is useful before CONNECTED, like whether we have one at all, and the boolean flags. One alternative would be: * FEATURE_CONTACT_LIST does not depend on FEATURE_CONNECTED * FEATURE_CONTACT_LIST also includes (a mapping of) ContactListState * FEATURE_CONTACT_LIST also includes contact-list-error: GError or NULL, to report why the contact list (last) failed, or NotAvailable if it's just not there yet * the contact list appears to be empty until we get to the SUCCESS state, with change notification * FEATURE_CONNECTED doesn't require SUCCESS (rationale: it can take a while to get the contact list, particularly on XMPP where it's a separate async request, and MSN where the separate contact list server might even be down) Another would be: * as above, plus * FEATURE_CONTACT_LIST_FULL (better name needed) requires that SUCCESS or FAILURE state has been reached Someone objected to features that require CONNECTED; Sjoerd, perhaps?
I think it's nice to have a feature ensuring that the contact list has been fetched (or failed); FEATURE_CONTACT_LIST_RETRIEVED? FEATURE_CONTACT_LIST_FETCHED?
(In reply to comment #1) > TpConnection::contact-list-changed (GList *added, GList *removed); If we fire this signal as soon as we get the ContactsChangedWithID, most clients (if not all) will have to call tp_connection_upgrade_contacts() on the newly added contacts as they can't do much whitout having the contacts features. That' a bit of a shame for an high level API whose goal is to make client's life easier. One solution would be to queue all D-Bus signals (to keep ordering) and not fire the GObject ones until at set of features have been prepared. This set of feature could be either be set implicitely or just be the intersection of the ones passed to tp_connection_contact_list_get_contacts_async(). Actually I'm wondering if we shouldn't go for an approach similar to tp-qt4: having a contact-factory (similar to our channel-factory) doing the contact preparation. We could maybe even have a TpContactList object created from the TpConnection to which we pass the contact factory we want to use. That would avoid the case where a plugin wants to prepare some extra feature and so delay all the other components to get contacts (assuming they share the same proxy objects).
(In reply to comment #1) > > void tp_connection_contact_list_get_contacts_async (TpConnection *self, > guint n_features, > const TpContactFeature *features, > GAsyncReadyCallback callback, > gpointer user_data); > > Or should we use a GArray instead? I think the point here is to not have to do async call for this. We should define contact features we want on the connection and then we can just get the internal list of contacts the connection already prepared. That's how tp-qt4 API works.
(In reply to comment #5) > (In reply to comment #1) > > > > void tp_connection_contact_list_get_contacts_async (TpConnection *self, > > guint n_features, > > const TpContactFeature *features, > > GAsyncReadyCallback callback, > > gpointer user_data); > > > > Or should we use a GArray instead? > > I think the point here is to not have to do async call for this. We should > define contact features we want on the connection and then we can just get the > internal list of contacts the connection already prepared. That's how tp-qt4 > API works. I basically agree: there should be a connection feature that means “I care about contacts”, and once that's prepared everything should be sync.
TpContact already has features to track its subscription states and groups, those are using ContactList interface already: TP_CONTACT_FEATURE_SUBSCRIPTION_STATES, TP_CONTACT_FEATURE_CONTACT_GROUPS Here is one extra step to get ContactList properties on TpConnection: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=contact-list What's missing: 1) a feature on TpConnection telling to create TpContact for all known contacts, with a method to get them all, and a signal telling those added/removed. 2) a way to define on TpConnection the features we want to be prepared on all known contacts before exposing them. 3) tp_connection_*_async(TpConnection *, array<TpContact>) and tp_contact_*_async() for all those methods: RequestSubscription AuthorizePublication RemoveContacts Unsubscribe Unpublish Let's do this step by step :)
(In reply to comment #7) > 1) a feature on TpConnection telling to create TpContact for all known > contacts, with a method to get them all, and a signal telling those > added/removed. s/all known contacts/all contacts on the contact list/ (yes, there is a distinction - consider blocked contacts) This shouldn't imply "wait til the contact list is ready" because that might never terminate (consider MSN when the contact list server has fallen over - you can still talk to people, and having everything wait for this desired feature to be ready wouldn't be beneficial.) If getting the contact list has failed, there should be an accessor for the error we saw last time we tried? The state property added by your branch should probably delay moving to Succeeded until we have actually downloaded the complete list of contacts, if we're trying to do that? > 2) a way to define on TpConnection the features we want to be prepared on all > known contacts before exposing them "features we want to be prepared on new contacts before signalling them"? Or we could just say "you will get at least subscription states, everything else you have to wait for". > 3) tp_connection_*_async(TpConnection *, array<TpContact>) and > tp_contact_*_async() for all those methods: > > RequestSubscription > AuthorizePublication > RemoveContacts > Unsubscribe > Unpublish I think this is more important than (2). While you're writing boilerplate you might also want block, unblock and block-and-report-abuse (the Blocking interface). Regarding your branch: > + "ContactList can change", "Whether the contact list can change", No, it might be able to change even though *we* can't change it (consider Facebook). > + * TP_CONNECTION_FEATURE_CONTACT_LIST: I'm a bit wary about using this feature name for something that isn't your stage (1) above. If I activate the "contact list" feature, surely I should have a list of contacts? You could either rename this to CONTACT_LIST_PROPERTIES, or decide that it will imply "contact list will be present as a list of TpContact, if the CM-side state allows it" (i.e. combine it with (1)).
(In reply to comment #7) > 1) a feature on TpConnection telling to create TpContact for all known > contacts, with a method to get them all, and a signal telling those > added/removed. This is now done, I've called this all_known_contacts which is the name used in tpqt4. I'm open to better naming. I'm using GList as container, this is not really consistent with other APIs, maybe I should have pairs of (guint n_contacts, TpContact * const *contacts) ? > 2) a way to define on TpConnection the features we want to be prepared on all > known contacts before exposing them. This is done, but I'm not really happy with my API for it. ATM, I have a setter that must be called before preparing the ALL_KNOWN_CONTACTS feature: void tp_connection_set_desired_contact_features (TpConnection *self, guint n_features, const TpContactFeature *features); I'm thinking about replacing that, by a specialized version of tp_proxy_prepare_async() just for connections: tp_connection_prepare_async (TpConnection self, const GQuark *features, guint n_contact_features, const TpContactFeature *contact_features, GAsyncReadyCallback callback, gpointer user_data); Calling that if ALL_KNOWN_CONTACTS is already prepared, would upgrade contacts. Known issue: TpContact refs its connection, and connection->priv->all_known_contacts refs the contacts => ref cycle and nothing gets freed. I don't know how to fix this, suggestions?
(In reply to comment #8) > (In reply to comment #7) > > 1) a feature on TpConnection telling to create TpContact for all known > > contacts, with a method to get them all, and a signal telling those > > added/removed. > > s/all known contacts/all contacts on the contact list/ > > (yes, there is a distinction - consider blocked contacts) Right, that's the name in tpqt4, probably because of 'known' list (now renamed to 'stored'). Maybe I should just rename to list<TpContact> tp_connection_get_contact_list() ? > This shouldn't imply "wait til the contact list is ready" because that might > never terminate (consider MSN when the contact list server has fallen over - > you can still talk to people, and having everything wait for this desired > feature to be ready wouldn't be beneficial.) My current implementation waits for connected and status success. But I agree this might not be correct. Maybe it should wait if state is WAITING, and stop on FAILURE or SUCCESS. Once state goes to SUCCESS or FAILURE, I assume it can't change anymore, can it? > If getting the contact list has failed, there should be an accessor for the > error we saw last time we tried? Something like that? TpContactListState tp_connection_get_contact_list_state(connection, &error); > The state property added by your branch should probably delay moving to > Succeeded until we have actually downloaded the complete list of contacts, if > we're trying to do that? I'm considering removing the FEATURE_CONTACT_LIST and keep only FEATURE_ALL_KNOWN_CONTACTS that would do both. and actually that feature could then be renamed to FEATURE_CONTACT_LIST? I agree there is probably no interest in having contact list properties without actually having contacts. > > 2) a way to define on TpConnection the features we want to be prepared on all > > known contacts before exposing them > > "features we want to be prepared on new contacts before signalling them"? > > Or we could just say "you will get at least subscription states, everything > else you have to wait for". GetContactListAttributes allows giving all the features we want, to fetch everything at once. That would be faster than fetching only subscription state, then let user upgrade contacts again, no? What about my proposal in comment #9 ? > > 3) tp_connection_*_async(TpConnection *, array<TpContact>) and > > tp_contact_*_async() for all those methods: > > > > RequestSubscription > > AuthorizePublication > > RemoveContacts > > Unsubscribe > > Unpublish > > I think this is more important than (2). > > While you're writing boilerplate you might also want block, unblock and > block-and-report-abuse (the Blocking interface). Still on todo-list. There are also AddToGroup, etc. I didn't start with that because they are just plain wrapper around the dbus call, nothing smart is needed there. Boring copy-pasting :D > Regarding your branch: > > > + "ContactList can change", "Whether the contact list can change", > > No, it might be able to change even though *we* can't change it (consider > Facebook). Doc wording needs love everywhere. Suggestions for that one? :) > > + * TP_CONNECTION_FEATURE_CONTACT_LIST: > > I'm a bit wary about using this feature name for something that isn't your > stage (1) above. If I activate the "contact list" feature, surely I should have > a list of contacts? > > You could either rename this to CONTACT_LIST_PROPERTIES, or decide that it will > imply "contact list will be present as a list of TpContact, if the CM-side > state allows it" (i.e. combine it with (1)). I've replied this above, I agree with you.
I've update my branch with 2 commits: 1) merge ALL_KNOWN_CONTACTS into CONTACT_LIST feature, as I don't think contact list properties are useful without having the contacts anyway. 2) Implement my idea of tp_connection_prepare_async() instead of the _set_desired_features(). Open questions: a) Ref cycle between TpContact and TpConnection. b) What to do in case ContactListState goes to FAILURE. c) What container to use for contacts, atm it is GList<TpContact>, but (len, TpContact**) pairs seems more consistent with existing APIs.
btw, I've ported ssh-contact to use this new API, so you can see how this API works from a really simple case: http://cgit.collabora.com/git/user/xclaesse/telepathy-ssh-contact.git/log/?h=contact-list
Just though about something: should desired_features always contain at least TP_CONTACT_FEATURE_SUBSCRIPTION_STATES as we get them for free anyway? I've also noticed that tp_connection_upgrade_contacts() could be smarter to not refetch contact-attributes that are already ready on all contacts, and ignore the contacts that are already have all features ready.
(In reply to comment #13) > Just though about something: should desired_features always contain at least > TP_CONTACT_FEATURE_SUBSCRIPTION_STATES as we get them for free anyway? I've done this.
(In reply to comment #7) > 3) tp_connection_*_async(TpConnection *, array<TpContact>) and > tp_contact_*_async() for all those methods: > > RequestSubscription > AuthorizePublication > RemoveContacts > Unsubscribe > Unpublish Done for TpConnection, still need on-contact variant on TpContact (In reply to comment #11) > Open questions: > > a) Ref cycle between TpContact and TpConnection. > > b) What to do in case ContactListState goes to FAILURE. > > c) What container to use for contacts, atm it is GList<TpContact>, but (len, > TpContact**) pairs seems more consistent with existing APIs. two more questions: d) Blocked contacts are not included in contact list. Another feature on TpConnection would tell to fetch them as well, do we want to prepare the same features on them? I vote for yes. e) On the Groups interface, are we sure all contacts are from the contact list, or can we get additional contacts there? If we have other contacts, do we want to prepare the same features as well on them?
and here is wrapper for ContactGroups: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=groups missing TpContact variants for AddToGroup and RemoveFromGroup
For info here is an empathy branch using the new APIs: http://cgit.collabora.com/git/user/xclaesse/empathy.git/log/?h=contact-list
Bah, I just realized that I've ported dead code in empathy, since all this is done in folks instead. So I didn't actually test anything from this TpConnection APIs :(
If the idea of tp_connection_prepare_async() is accepted, I think we could also add void tp_account_prepare_async (TpAccount *self, GQuark *features, GQuark *connection_features, guint n_contact_features, TpContactFeature *contact_features); void tp_account_manager_prepare_async (TpAccount *self, GQuark *features, GQuark *account_features, GQuark *connection_features, guint n_contact_features, TpContactFeature *contact_features); Like that, preparing the AM and *everything* is ready to use.
argh, sadly those function names are already taken for legacy reasons... guess we'll have to go with _full() variant then :(
I've remade the commit history in my branches to make them easier to review. Now commits could go one by one I think. http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=contact-list http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=contact-groups
For the refcycle problem I see 2 possibilities: 1) An intermediary TpContactList object, like Tp::ContactManager: <smcv> strong refs: contact list -> contact -> connection, contact list -> connection if neither contacts nor the connection ref the contact list, it would fix the cycles? That way, user will have to keep strong ref on the TpContactList. if user keeps only a strong ref on a TpContact but not TpContactList, then the TpContact object won't update anymore, and any action on it will fail. This means I would have to change my code to have API like that: TpContactList *tp_connection_dup_contact_list(TpConnection*self); void tp_contact_list_prepare_async (TpContactList *self, guint n_features, TpContactFeature *contact_features); 2) Stop TpContact doing a strong-ref on its TpConnection, and make TpConnection strong-ref the roster TpContact. This means the user could have to keep a strong ref on the TpConnection otherwise TpContact objects won't update anymore, and any action on them will fail. That way, I can keep the same API as in my branch, but introduce a API break since existing apps that only keep ref on TpContact but not their TpConnection will stop working properly. If this goes together with the breaks for the "telepathy 6.0" plans, I would personally vote for solution 2. If we want to merge this sooner, then solution 1 is the only choice we have. Suggestions?
Forgot to tell what I don't like in solution 1 is that you'll always have that extra step of first getting the TpContactList object then preparing it. That means we couldn't have way to prepare the TpAccountManager in one call and have *everything* ready to use in one single step. So not breaking the way ref are done has a cost on long term, IMO.
(In reply to comment #22) > For the refcycle problem I see 2 possibilities: > > TpContactList *tp_connection_dup_contact_list(TpConnection*self); > > void tp_contact_list_prepare_async (TpContactList *self, guint n_features, > TpContactFeature *contact_features); > If going this route, I'd rather have a tp_connection_dup_contact_list_async (or tp_connection_request_contact_list or whatever naming you use) which takes the features and a callback, and only gives you the contact list once successfully retrieved. > > 2) Stop TpContact doing a strong-ref on its TpConnection, and make TpConnection > strong-ref the roster TpContact. > > ... > > If this goes together with the breaks for the "telepathy 6.0" plans, I would > personally vote for solution 2. If we want to merge this sooner, then solution > 1 is the only choice we have. > > Suggestions? I vote for this as the long term solution as well.
(In reply to comment #23) > Forgot to tell what I don't like in solution 1 is that you'll always have that > extra step of first getting the TpContactList object then preparing it. That > means we couldn't have way to prepare the TpAccountManager in one call and have > *everything* ready to use in one single step. So not breaking the way ref are > done has a cost on long term, IMO. ++
oh, solution 3) just accept the refcycle for now and fix it when we can break guarantees. Note that in any case, when TpConnection is invalidated it should drop refs on its roster contacts, so that breaks refcycle when account goes offline anyway (note that currently my branch does not do that)
I've added that commit in my branch to fix refcycle: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=contact-groups&id=8bfa4ed600ac3dfbd48d601d5f844c9ec0f35c6c I think that's the best solution we have, unless someone is really against it?
I've added a python example that prints the roster using this API, in my contact-groups branch. Unfortunately it hits a pygobject bug: https://bugzilla.gnome.org/show_bug.cgi?id=652115. 40 lines of python to print the whole roster, that's really good \o/ I can still be improved by passing the connection/account/contact features to manager.prepare_async().
I didn't look at the code, just the API. I'm wondering if we shouldn't use a GPtrArray rather than a GList to return the contacts. It offers at least those advantages: - Easier memory management using the free func - Can be reffed so user can easily keep it around - Better memory usage Actually, I'm starting to think we should consider using GPtrArray as our 'default' container when returning collections. The main advantages of GList is to be more flexible to modifications, but in most case we don't care. I'm not convinced that tp_{account,connection}_prepare_async() is the way to go. Maybe a factory approach defining the features we want on every objects would be better. We could have a TpFactory object implementing TpClientChannelFactoryInterface and futures TpClient{Account,Connection,Contact}FactoryInterface. We'd pass this TpFactory to the most top lewel object (TpAccountManager) which will pass it back to all the other objects created. Isn't tp-qt4 doing something like that? Is "guint n_contacts, TpContact * const *contacts," the best pattern for such functions? In C, user will always have to create a GPtrArray a pass array->len, array->data, so why not pass the GPtrArray directly? This is different from the tp_proxy_prepare_async() case as there the array of feature is always static so we can use G_N_ELEMENTS.
(In reply to comment #29) > I'm not convinced that tp_{account,connection}_prepare_async() is the way to > go. Maybe a factory approach defining the features we want on every objects > would be better. We could have a TpFactory object implementing > TpClientChannelFactoryInterface and futures > TpClient{Account,Connection,Contact}FactoryInterface. > > We'd pass this TpFactory to the most top lewel object (TpAccountManager) which > will pass it back to all the other objects created. > > Isn't tp-qt4 doing something like that? > Yes, tp-qt4 has most exactly that, although the factories are separate objects rather than interfaces on a single object. Using multiple glib interfaces on a single object allows you to binary compatibly add more factory-constructible types though, which is cool (in C++ you can't add base classes without breaking ABI).
(In reply to comment #29) > I'm not convinced that tp_{account,connection}_prepare_async() is the way to > go. Maybe a factory approach defining the features we want on every objects > would be better. We could have a TpFactory object implementing > TpClientChannelFactoryInterface and futures > TpClient{Account,Connection,Contact}FactoryInterface. That would also allow us to ensure that the TpContact signaled by TpTextChannel have a bunch of features prepared making the life all most (all?) clients much easier.
Branch updated with factories \o/
Created attachment 48574 [details] [review] ContactList example in Vala This is the last commit from this branch, based on Xavier's: http://cgit.collabora.com/git/user/treitter/telepathy-glib.git/log/?h=contact-list-2 It's just a port of the Python example, to make sure it works as expected in Vala. Feel free to either merge or ignore this.
(In reply to comment #33) > Created an attachment (id=48574) [details] > ContactList example in Vala > > This is the last commit from this branch, based on Xavier's: > > http://cgit.collabora.com/git/user/treitter/telepathy-glib.git/log/?h=contact-list-2 > > It's just a port of the Python example, to make sure it works as expected in > Vala. > > Feel free to either merge or ignore this. Based on this patch, I think the API looks good for the needs of Folks as well. I didn't check the implementation in detail, but the end result seems fine to me.
For the record, we are going to need this API for the Shell: https://bugzilla.gnome.org/show_bug.cgi?id=653941
http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=contact-list-properties looks good to me, although I'm not sure it really makes sense to merge without the actual contacts part.
already merged that part to have a better view of what's remaining in my branches, and avoid any conflicts. But indeed it is not yet useful as it is. contact-list branch contains the remaining bits, but will need small change to build with master (wjt slightly changed an internal API)
Created attachment 49903 [details] [review] export some internal functions of contact.c
Created attachment 49904 [details] [review] Add _tp_contacts_from_values
Created attachment 49905 [details] [review] Prepare TpContact objects for the ContactList If TpConnection has TP_CONNECTION_FEATURE_CONTACT_LIST, also create TpContact objects for the roster. Add tp_connection_dup_contact_list() to get them.
Created attachment 49906 [details] [review] add contact-list.{c,py} example It demonstrate how to get all prepared contacts using a factory.
Created attachment 49907 [details] [review] Include contact-list.c example into TpAccountManager's doc
Created attachment 49908 [details] [review] Tests: verify TP_CONNECTION_FEATURE_CONTACT_LIST also prepare TpContact
Review of attachment 49903 [details] [review]: ++
Review of attachment 49904 [details] [review]: ::: telepathy-glib/util.c @@ +1923,3 @@ + +GPtrArray * +_tp_contacts_from_values (GHashTable *table) I'd add a comment explaining the kind of hash table expected. @@ +1934,3 @@ + g_hash_table_iter_init (&iter, table); + while (g_hash_table_iter_next (&iter, NULL, &value)) + gpointer value; Add an assertion/check ensuring that's a TpContact ?
Review of attachment 49905 [details] [review]: ::: telepathy-glib/connection-contact-list.c @@ +94,3 @@ + + contact = g_hash_table_lookup (self->priv->roster, key); + tp_clear_pointer (&item->identifiers, g_hash_table_unref); How can contact be NULL? CM bug? Maybe it's worth displaying a debug in this case. @@ +101,3 @@ + } + +} This comment is bit miss leading as you didn't filter out any contact at this stage (you already removed contacts which was already in the roster in process_queued_contacts_changed). @@ +278,3 @@ + const gchar **supported_interfaces; + + TpContact *contact = g_hash_table_lookup (self->priv->roster, key); display the path of the connection @@ +697,3 @@ + * @self: a #TpConnection + * + * The list of contacts associated with the user, but MAY contain other the doc should mention @self somewhere. "associated with the user on @self"? @@ +700,3 @@ + * contacts. This list does not contain every visible contact: for instance, + * contacts seen in XMPP or IRC chatrooms does not appear here. Blocked contacts + * does not appear here, unless they still have a non-No subscribe or publish This isn't very clear. Can't we express that in term of TpContact flags or something? @@ +709,3 @@ + * For this method to be valid, you must first call tp_proxy_prepare_async() + * with the feature %TP_CONNECTION_FEATURE_CONTACT_LIST and verify the + * does not appear here, unless they still have a non-No subscribe or publish Does that mean that having the feature prepared is not enough to safely call this function? ::: telepathy-glib/connection.c @@ +2071,3 @@ _tp_marshal_VOID__STRING_STRING, G_TYPE_NONE, 2, G_TYPE_STRING, G_TYPE_STRING); + /** missing \n
Review of attachment 49906 [details] [review]: ::: examples/client/contact-list.c @@ +71,3 @@ + 0 }; + const GQuark connection_features[] = { + TP_CONNECTION_FEATURE_CONTACT_LIST, Maybe those should be automatically prepared by the automatic factory? ::: examples/client/python/contact-list.py @@ +11,3 @@ + + accounts = manager.get_valid_accounts() + for account in accounts: for account in manager.get_valid_accounts(): @@ +13,3 @@ + for account in accounts: + connection = account.get_connection() + if connection != None: if connection is not None:
Review of attachment 49907 [details] [review]: ::: telepathy-glib/account-manager.c @@ +63,3 @@ + * #TpAccountManager is the "top level" object, its #TpProxy:factory will be + * propagated to all other objects like #TpAccountManager -> #TpAccount -> + * #TpConnection -> #TpContact and #TpChannel. I'd make clearer that imply all the features from this factory will be prepared.
Review of attachment 49908 [details] [review]: ++
Oh, and we should probably make clear that this API only works on Connection implementing ContactList.
(In reply to comment #44) > Review of attachment 49903 [details] [review]: > > ++ ok, picked that one to master
(In reply to comment #45) > Review of attachment 49904 [details] [review]: > > ::: telepathy-glib/util.c > @@ +1923,3 @@ > + > +GPtrArray * > +_tp_contacts_from_values (GHashTable *table) > > I'd add a comment explaining the kind of hash table expected. > > @@ +1934,3 @@ > + g_hash_table_iter_init (&iter, table); > + while (g_hash_table_iter_next (&iter, NULL, &value)) > + gpointer value; > > Add an assertion/check ensuring that's a TpContact ? fixed and picked to master
Review of attachment 49905 [details] [review]: ::: telepathy-glib/connection-contact-list.c @@ +94,3 @@ + + contact = g_hash_table_lookup (self->priv->roster, key); + if (contact != NULL) right, broken CM. Added a DEBUG(). @@ +101,3 @@ + } + + /* Add contacts to roster and build a list of contacts really added */ right, removed the "really" since we add it even if it already exists, which is fine tbh :) @@ +278,3 @@ + const gchar **supported_interfaces; + + DEBUG ("CM has the roster, fetch it now"); done @@ +700,3 @@ + * contacts. This list does not contain every visible contact: for instance, + * contacts seen in XMPP or IRC chatrooms does not appear here. Blocked contacts + * does not appear here, unless they still have a non-No subscribe or publish this is copy/paste from spec tbh. Blocked is not yet defined on TpContact, but the "have a non-No subscribe or publish" can be expressed as TpContact properties, indeed. @@ +709,3 @@ + * For this method to be valid, you must first call tp_proxy_prepare_async() + * with the feature %TP_CONNECTION_FEATURE_CONTACT_LIST and verify the + * #TpConnection:contact-list-state is set to %TP_CONTACT_LIST_STATE_SUCCESS. if state is not SUCCESS, even if feature is prepared, returned list will be empty. Feature being prepared means "we know CM's state" but does not mean the CM already fetched from server. Doc could be improved I guess...
Review of attachment 49906 [details] [review]: ::: examples/client/contact-list.c @@ +71,3 @@ + 0 }; + const GQuark connection_features[] = { + TP_CONNECTION_FEATURE_CONTACT_LIST, hm, the automatic factory is the one used by default, this means by default it would prepare the contact-list unless explicitly asking for a TpSimpleClientFactory. atm the automatic only ask for CORE features... IMO it's cheap enough to list features when creating the factory. it's just that CORE is required for the proxy to be useful at all. no? ::: examples/client/python/contact-list.py @@ +11,3 @@ + + accounts = manager.get_valid_accounts() + for account in accounts: done @@ +13,3 @@ + for account in accounts: + connection = account.get_connection() + if connection != None: done
(In reply to comment #55) > Review of attachment 49906 [details] [review]: > > ::: examples/client/contact-list.c > @@ +71,3 @@ > + 0 }; > + const GQuark connection_features[] = { > + TP_CONNECTION_FEATURE_CONTACT_LIST, > > hm, the automatic factory is the one used by default, this means by default it > would prepare the contact-list unless explicitly asking for a > TpSimpleClientFactory. > > atm the automatic only ask for CORE features... IMO it's cheap enough to list > features when creating the factory. it's just that CORE is required for the > proxy to be useful at all. I think you're right. Always prepare the CL seems overkil as much apps won't need it (when handling an incoming channel for example).
Enough of the factories have landed in master, so I rebased on master to merge this separately. branch is now contact-list-rebase
Hum maybe TP_ACCOUNT_FEATURE_CONNECTION could be prepared by the automatic factory though. In most case (TpBaseClient, etc) we always create a TpAcount and a TpConnection, so preparing this feature should be basically free, right?
(In reply to comment #48) > Review of attachment 49907 [details] [review]: > > ::: telepathy-glib/account-manager.c > @@ +63,3 @@ > + * #TpAccountManager is the "top level" object, its #TpProxy:factory will be > + * propagated to all other objects like #TpAccountManager -> #TpAccount -> > + * #TpConnection -> #TpContact and #TpChannel. > > I'd make clearer that imply all the features from this factory will be > prepared. You didn't fix this one. (In reply to comment #54) > Review of attachment 49905 [details] [review]: > @@ +709,3 @@ > + * For this method to be valid, you must first call tp_proxy_prepare_async() > + * with the feature %TP_CONNECTION_FEATURE_CONTACT_LIST and verify the > + * #TpConnection:contact-list-state is set to %TP_CONTACT_LIST_STATE_SUCCESS. > > if state is not SUCCESS, even if feature is prepared, returned list will be > empty. Feature being prepared means "we know CM's state" but does not mean the > CM already fetched from server. Doc could be improved I guess... Then the doc of the feature lies: "When this feature is prepared (...) all #TpContact objects has been prepared with the desired features" Also the examples are wrong as they assumed that once the feature has been prepared the list of contacts is ready to be used.
(In reply to comment #58) > Hum maybe TP_ACCOUNT_FEATURE_CONNECTION could be prepared by the automatic > factory though. In most case (TpBaseClient, etc) we always create a TpAcount > and a TpConnection, so preparing this feature should be basically free, right? I've discussed this with Will, in TpAccountManager we made it part of CORE to prepare its accounts because an AM without prepared accounts would be useless. But he thinks that an account without prepared connection could still be useful (account settings app for example) since account itself has lots of properties/methods that does not need a connection afaik.
(In reply to comment #59) > (In reply to comment #48) > > Review of attachment 49907 [details] [review] [details]: > > > > ::: telepathy-glib/account-manager.c > > @@ +63,3 @@ > > + * #TpAccountManager is the "top level" object, its #TpProxy:factory will be > > + * propagated to all other objects like #TpAccountManager -> #TpAccount -> > > + * #TpConnection -> #TpContact and #TpChannel. > > > > I'd make clearer that imply all the features from this factory will be > > prepared. > > You didn't fix this one. Right, added "This means that desired features * set on that factory will be prepared on all those objects." > > (In reply to comment #54) > > Review of attachment 49905 [details] [review] [details]: > > @@ +709,3 @@ > > + * For this method to be valid, you must first call tp_proxy_prepare_async() > > + * with the feature %TP_CONNECTION_FEATURE_CONTACT_LIST and verify the > > + * #TpConnection:contact-list-state is set to %TP_CONTACT_LIST_STATE_SUCCESS. > > > > if state is not SUCCESS, even if feature is prepared, returned list will be > > empty. Feature being prepared means "we know CM's state" but does not mean the > > CM already fetched from server. Doc could be improved I guess... > > Then the doc of the feature lies: > "When this feature is prepared (...) all #TpContact objects has been prepared > with the desired features" right, fixed the doc to say this apply only if contact-list-state is SUCCESS. > Also the examples are wrong as they assumed that once the feature has been > prepared the list of contacts is ready to be used. if state is not success, the list of contacts would just be empty. But to be more demonstrative, I added a check that state is SUCCESS.
(In reply to comment #60) > (In reply to comment #58) > > Hum maybe TP_ACCOUNT_FEATURE_CONNECTION could be prepared by the automatic > > factory though. In most case (TpBaseClient, etc) we always create a TpAcount > > and a TpConnection, so preparing this feature should be basically free, right? > > I've discussed this with Will, in TpAccountManager we made it part of CORE to > prepare its accounts because an AM without prepared accounts would be useless. > But he thinks that an account without prepared connection could still be useful > (account settings app for example) since account itself has lots of > properties/methods that does not need a connection afaik. I'm not convinced. The automatic factory should be designed for most use cases, corner cases can still use the simple factory. And most app will use TpAccount/TpConnection and expect to have get_account/get_connection() working. I still think that this is unclear for API doc: "Blocked contacts does not appear here, unless they still have a non-No subscribe or publish attribute for some reason." Tbh, I don't understand what it means at all.
++
Branch merged \o/ Thanks all for comments and reviews
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.