GNOME Bugzilla – Bug 590165
Should use MC5
Last modified: 2009-08-25 12:11:27 UTC
Empathy should use Mission Control 5.
These are my commments after a (quick) review of http://git.collabora.co.uk/?p=user/sjoerd/empathy.git;a=shortlog;h=refs/heads/mc5 • EMPATHY_ARRAY_TYPE_OBJECT: isn't there an equivalent in tp-glib now? • e700d8f398b27a98ef24476e3909570aa91c329a I don't understand this patch. What's changed? • e9266689e7b55d9be53afc46c5b3d131ba9837cd would be good to port these tests. At least add a FIXME and open a bug about that • I think we shouldn't merge MC5 branch while MC4 accounts are not automatically imported when starting the MC5-enabled Empathy for the first time empathy-account • empathy_account_update, empathy_account_new, empathy_account_parse_unique_name : function declaration style is wrong • empathy_account_got_all_cb: use DEBUG and a more informative message if an error occurs • empathy_account_requested_presence_cb: same here empathy-account-manager • empathy_account_manager_request_global_presence: FIXME is not fixed • empathy_account_manager_request_global_presence: would be easier/clearer to use empathy_account_is_ready • emp_account_manager_update_global_presence: a comment explaining the logic of this function would be good • empathy_account_manager_created_ready_cb empathy_account_manager_create_account_async : declaration style is wrong • empathy_account_manager_created_ready_cb: shouldn't complete the operation with an error if !empathy_account_is_ready (account)? And remove the account? empathy-dispatcher • dispatcher_constructor: is Empathy 100% garanteed to be single instance? • empathy_dispatcher_class_init: useless double \n at the end of the function • empathy_dispatcher_handle_channels: 2 FIXME empathy-idle • empathy_idle_do_set_presence: statusses shouldn't be statuses ? empathy-account-settings • empathy_account_settings_set_display_name_async and empathy_account_settings_set_display_name_finish are empty • style of few declarations is wrong
- avatar cache should be in ~/.cache/Empathy/avatars/<CM>/<protocol>/<account>/<token> We could even remove /Empathy/ to share the cache with other clients?
(In reply to comment #1) > • EMPATHY_ARRAY_TYPE_OBJECT: isn't there an equivalent in tp-glib now? Nope > • e700d8f398b27a98ef24476e3909570aa91c329a I don't understand this patch. > What's changed? Me failing to include changes in the right commit seems what has happened, i don't really want to rebase at this point though. > • e9266689e7b55d9be53afc46c5b3d131ba9837cd would be good to port these tests. > At least add a FIXME and open a bug about that See 590285 > • I think we shouldn't merge MC5 branch while MC4 accounts are not > automatically imported when starting the MC5-enabled Empathy for the first >time No we should merge it asap so people can develop against it, the MC4 importing should be there before the gnome beta release. > empathy-account-manager > • empathy_account_manager_request_global_presence: FIXME is not fixed Fixed in cosimocs branch > • empathy_account_manager_request_global_presence: would be easier/clearer to > use empathy_account_is_ready this code was written before that function existed :) > • empathy_account_manager_created_ready_cb: shouldn't complete the operation > with an error if !empathy_account_is_ready (account)? And remove the account? No, the check is just defensive programming (a notify of the ready property doesn't guarantee that it's true now) > empathy-dispatcher > • dispatcher_constructor: is Empathy 100% garanteed to be single instance? no, it's on my todo list of things to fix > empathy-account-settings > • empathy_account_settings_set_display_name_async and > empathy_account_settings_set_display_name_finish are empty yes, being implemented in cosimocs branch The comments i removed from my reply are fixed in the mc5 branch now, thanks for the reivew
Next release (2.27.90) is on 11th August. If the MC4 importer is merged by then I'm ok to merge the mc5 branch before (a release blocker bug should be opened about that).
The MC5 port now has its one upstream branch: http://git.gnome.org/cgit/empathy/log/?h=mc5
Branch has been merged to master \o/