GNOME Bugzilla – Bug 577427
Port to latest tp-glib API
Last modified: 2009-04-22 10:22:56 UTC
Telepathy-glib now provides more high level API like TpContact and tp_channel_group_*. We should also stop using *_run_* API which is probably going to be deprecated at some point. Most of that is done in my tp-contact branch: http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=shortlog;h=refs/heads/tp-contact
These are my comments after a (very) quick review on this huge branch: http://people.collabora.co.uk/~cassidy/Empathy%20TpContact.html
*** Bug 579612 has been marked as a duplicate of this bug. ***
empathy-contact * I'm not sure we can relicence these files It has nothing to do with original code, it is now a TpContact wrapper, it is perfectly fine IMO. * tp_contact_notify_cb: don't handle "presence-status" change Because we don't have that property. * properties doesn't have G_PARAM_STATIC_STRINGS Not related to this branch, but fixed. * fix FIXME They are there because EmpathyContact still need to refer to its McAccount in some places. That's left for future improvement. For example logs contain the account name, avatar cache is stored in a path that contain account name, etc... * we should have an empathy_contact_new_from_tp_contact (TpContact) and a empathy_contact_gadget (...) used by the log mgr so g_object_new is never used directly Added empathy_contact_new_static(). empathy-account-manager * in EmpathyAccountManagerPriv document accounts and connections hash table (type of the key, type of the value, reffed or not, etc) Fixed. * connection_invalidated_cb: I'd assert that _lookup's don't return NULL fixed. * empathy_account_manager_get_connection: doc should say if the connection is reffed or not *_get_* should never return new ref, I use _dup_ for that. But anyway, it's better to make it clear in the doc. Fixed. * empathy_account_manager_dup_connections needs doc Why that function and not all others? But anyway, fixed. empathy-tp-contact-factory.c * why not avatar token in contact_features? Because we can't use that because TpContact don't get the avatar data. I kept old code for avatar. * /* FIXME: This should be done by TpContact */ indeed, it should It's outside the scope of this branch, will change that when TpContact gets those features. empathy-tp-contact-list.c * empathy_tp_contact_list_remove_all: loop on members and fire members-changed for each contact is bong. Signal should aggregate all the members Why? It's the purpose of EmpathyTpContactList to de-aggregate contacts. CM groups contacts when emitting signals because dbus round trips are heavy. Here we just emit gobject signals. Changing this API is outside the scope of this branch, if we have reason to change, we'll do later. * tp_contact_list_group_add_data_unref: (--data->ref_count == 0) confusing++ fixed. * some comment explaining GroupAddData and the semantic of its fields would be good fixed. * empathy_tp_contact_list_init: explicitely document the type of each key/value and who owns them I added for group ht, but there is nothing more to say for members and pendings. * tp_contact_list_add_to_group: handles is leaked * tp_contact_list_rename_group: handles and members are leaked, * tp_contact_list_remove_group: idem No, tp_contact_list_group_add takes ownership of it. Added a comment to make it explicit. * 4b95f5cbe3b606eb20f7521aa1374cd51ac6efa6 : why? Some comments would be good Rebase occured while migrating to git.gnome.org, that commit does not exist anymore. * tp_connection_parse_object_path is released now so you should use it Fixed. empathy-dispatcher.c * dispatcher_connection_new_channel: we don't pass the contact object to empathy_dispatch_operation_new if the handle type is contact. Seems it's now done in EmpathyDispatchOperation's constructor. What's the rationnal of this change? empathy-tp-call * I'm not convinced that removing tp_call_members_changed_cb is a worth change. Having the right callback signature is cleaner imho We don't need the details of the members change, lookup in a TpIntSet is fast anyway. We are not going to have hundred handles member of a call :D empathy-call-handler * empathy_call_handler_start_call request hash table is leaked Dispatcher takes ownership of it. empathy-tp-chat * I'd add tp_chat_property_{new,free} helper functions for extra clarity It has nothing to do with this branch, we are going to drop that when moving to Messages interface anyway. empathy-contact-manager: * 669a1d6896f897f3cdf2f3cb6cbf787fb7336a28 I'd rename manager to self Commit does not exist, since git migration. Where is that? empathy-contact-list-view.c * + /* FIXME: We assume we have already an account manager */ what happen if we have not? We need to find the connection from the account. EmpathyAccountManager keeps a mapping. if we just created that object the map will be empty, so it won't find the connection and the DnD will fail. There is not much we can do actually. I hope McAccount will keep directly a ref to its TpConnection with MC5 api.
f47e98eaea8fa799da621471097a7f6be39bf9b4 Would be good to document if key/values are owned or not. empathy_contact_new_static seems a poor name to me (static already has to many meanings in C). Can't really figure a better name though. How can we be 100% sure about the relicensing? Do you have a diff against the original file? Maybe open a bug saying Empathy should get avatar from TpContact once this branch is merged? What about this comment? * dispatcher_connection_new_channel: we don't pass the contact object to empathy_dispatch_operation_new if the handle type is contact. Seems it's now done in EmpathyDispatchOperation's constructor. What's the rationnal of this change? * I'm not convinced that removing tp_call_members_changed_cb is a worth change. Having the right callback signature is cleaner imho We don't need the details of the members change, lookup in a TpIntSet is fast anyway. We are not going to have hundred handles member of a call :D Even if we don't use them, I think having the right callback signature is a lot cleaner that a truncated version of it. 4b95f5cbe3b606eb20f7521aa1374cd51ac6efa6 is now http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=commitdiff;h=41cd15a4d906138b44e43dcdb834d422d642268b 669a1d6896f897f3cdf2f3cb6cbf787fb7336a28 is now http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=commitdiff;h=9fffa8d42ffc2f6e1df0309de459eb60e3249c9f
(In reply to comment #4) > f47e98eaea8fa799da621471097a7f6be39bf9b4 > Would be good to document if key/values are owned or not. It is obvious, destroy functions are passed to g_hash_table_new_full(). Hash tables always owns its data. When it does not own data, there is comment telling that. > empathy_contact_new_static seems a poor name to me (static already has to many > meanings in C). Can't really figure a better name though. empathy_contact_new_for_log() ? > How can we be 100% sure about the relicensing? Do you have a diff against the > original file? The diff won't help, we did a s/gossip/empathy/. But trust me, there is nothing in common, really. > Maybe open a bug saying Empathy should get avatar from TpContact once this > branch is merged? There is already a bug about that on tp-glib. Of course we can add a bug in empathy too. > What about this comment? > * dispatcher_connection_new_channel: we don't pass the contact object to > empathy_dispatch_operation_new if the handle type is contact. Seems it's now > done in EmpathyDispatchOperation's constructor. What's the rationnal of this > change? didn't look yet, I don't remember that change. > * I'm not convinced that removing tp_call_members_changed_cb is a worth > change. Having the right callback signature is cleaner imho > We don't need the details of the members change, lookup in a TpIntSet is fast > anyway. We are not going to have hundred handles member of a call :D > > Even if we don't use them, I think having the right callback signature is a lot > cleaner that a truncated version of it. But it's ugly for the call from constructor. It is common to do that with g_signal_connect_swapped(). > 4b95f5cbe3b606eb20f7521aa1374cd51ac6efa6 is now > http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=commitdiff;h=41cd15a4d906138b44e43dcdb834d422d642268b This was to avoid adding a 2nd time channel requested in tp_contact_list_group_request_channel_cb. But this is bogus in fact, suppress_handler should not be used. I'll fix that. > 669a1d6896f897f3cdf2f3cb6cbf787fb7336a28 is now > http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=commitdiff;h=9fffa8d42ffc2f6e1df0309de459eb60e3249c9f Fixed.
(In reply to comment #4) > What about this comment? > * dispatcher_connection_new_channel: we don't pass the contact object to > empathy_dispatch_operation_new if the handle type is contact. Seems it's now > done in EmpathyDispatchOperation's constructor. What's the rationnal of this > change? Because we get contacts in a cb now, so it's a bit tricky to do in dispatcher_connection_new_channel(), we should create a struct to give all params the the got_contact cb, then create the operation. I think it's easier to get the contact inside the operation. I made sure the operation don't get ready before both channel and contact are ready. And in dispatcher_start_dispatching() it waits for the operation to get ready. So it should be OK. > > 4b95f5cbe3b606eb20f7521aa1374cd51ac6efa6 is now > http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=commitdiff;h=41cd15a4d906138b44e43dcdb834d422d642268b fixed
empathy_contact_new_for_log () seems good Please open a bug about tp-contact and avatars and link the relevant tp-glib bug in it.
I opened bug #579812, bug #579811 and bug #579813
Merged!