GNOME Bugzilla – Bug 599169
Should use new TpAccount API
Last modified: 2009-11-02 16:38:06 UTC
TSIA
http://git.collabora.co.uk/?p=user/jonny/empathy.git;a=shortlog;h=refs/heads/accountz libempathy-gtk/empathy-account-chooser.c | 171 ++-- libempathy-gtk/empathy-account-chooser.h | 10 +- libempathy-gtk/empathy-account-widget.c | 28 +- libempathy-gtk/empathy-chat-text-view.c | 1 - libempathy-gtk/empathy-chat.c | 65 +- libempathy-gtk/empathy-chat.h | 2 +- libempathy-gtk/empathy-contact-dialogs.c | 7 +- libempathy-gtk/empathy-contact-list-view.c | 18 +- libempathy-gtk/empathy-contact-widget.c | 6 +- libempathy-gtk/empathy-irc-network-dialog.c | 1 - libempathy-gtk/empathy-location-manager.c | 93 ++- libempathy-gtk/empathy-log-window.c | 76 +- libempathy-gtk/empathy-log-window.h | 4 +- libempathy-gtk/empathy-new-message-dialog.c | 1 - libempathy-gtk/empathy-presence-chooser.c | 56 +- libempathy-gtk/empathy-theme-adium.c | 6 +- libempathy-gtk/empathy-ui-utils.h | 1 - libempathy/Makefile.am | 4 - libempathy/empathy-account-manager.c | 1060 ---------------------- libempathy/empathy-account-manager.h | 111 --- libempathy/empathy-account-settings.c | 146 ++-- libempathy/empathy-account-settings.h | 8 +- libempathy/empathy-account.c | 1305 --------------------------- libempathy/empathy-account.h | 124 --- libempathy/empathy-chatroom-manager.c | 57 +- libempathy/empathy-chatroom-manager.h | 11 +- libempathy/empathy-chatroom.c | 16 +- libempathy/empathy-chatroom.h | 11 +- libempathy/empathy-contact-manager.c | 68 +- libempathy/empathy-contact.c | 24 +- libempathy/empathy-contact.h | 6 +- libempathy/empathy-dispatcher.c | 75 +- libempathy/empathy-idle.c | 161 +++- libempathy/empathy-idle.h | 7 + libempathy/empathy-log-manager.c | 10 +- libempathy/empathy-log-manager.h | 12 +- libempathy/empathy-log-store-empathy.c | 36 +- libempathy/empathy-log-store.c | 12 +- libempathy/empathy-log-store.h | 26 +- libempathy/empathy-tp-roomlist.c | 10 +- libempathy/empathy-tp-roomlist.h | 4 +- libempathy/empathy-utils.c | 69 ++- libempathy/empathy-utils.h | 4 + src/empathy-account-assistant.c | 6 +- src/empathy-accounts-dialog.c | 211 ++--- src/empathy-accounts-dialog.h | 4 +- src/empathy-chat-window.c | 27 +- src/empathy-chat-window.h | 5 +- src/empathy-chatrooms-window.c | 4 +- src/empathy-event-manager.c | 15 +- src/empathy-import-mc4-accounts.c | 10 +- src/empathy-import-widget.c | 57 +- src/empathy-main-window.c | 135 ++- src/empathy-new-chatroom-dialog.c | 6 +- src/empathy-status-icon.c | 57 +- src/empathy.c | 161 ++-- 56 files changed, 1178 insertions(+), 3443 deletions(-)
Some random comments, didn't review everything: - empathy_idle_get_requested_presence() is should either return const gchar* or be renamed to _dup_ - EmpathyIdle::account_status_changed_cb(): if the account is not connected anymore, you can remove from hash table. Why the hashtable contains account path as key instead of directly the TpAccount? - contact_manager_new_connection_cb should be renamed to status_changed_cb. I is not called for new accounts, only for accounts existing in the first place. Same in dispatcher.
(In reply to comment #2) > - empathy_idle_get_requested_presence() is should either return const gchar* > or be renamed to _dup_ No it shouldn't. This function is modelled around tp_account_get_current_presence and it follows the typical model of returning the most useful information (the TpConnectionPresenceType) and opting into more details (the status and status message). You can pass NULL to the latter two arguments for them not to be touched. > - EmpathyIdle::account_status_changed_cb(): if the account is not connected > anymore, you can remove from hash table. Why the hashtable contains account > path as key instead of directly the TpAccount? No reason. I've fixed both of these points. > - contact_manager_new_connection_cb should be renamed to status_changed_cb. I > is not called for new accounts, only for accounts existing in the first place. > Same in dispatcher. Renamed both of these functions.
(In reply to comment #3) > (In reply to comment #2) > > - empathy_idle_get_requested_presence() is should either return const gchar* > > or be renamed to _dup_ > > No it shouldn't. This function is modelled around > tp_account_get_current_presence and it follows the typical model of returning > the most useful information (the TpConnectionPresenceType) and opting into more > details (the status and status message). You can pass NULL to the latter two > arguments for them not to be touched. Well, IMO tp_account_get_current_presence() should not dup strings, there is no reason it does. get functions always return the internal string. But since that API can't be changed at this point, I guess it makes sense for empathy to make the same.
• Shouldn't you bump the tp-glib version? If yes, please send a mail to ddl about the bump. • empathy_get_account_for_connection : FIXME • empathy_get_account_for_connection: any reason why we don't have a tp_account_manager_get_account_for_connection method? • empathy_idle_do_set_presence: FIXME • empathy-idle: account_manager_ready_cb: I'd pass a GError to _prepare_finish and log the error if it fails • empathy-idle.c: please add a comment explaining the type of the keys and the values of the connect_times hash table (and if they are reffed/owned) • empathy_idle_account_is_just_connected: add a define instead of hardcoding "10". Also, add a comment explaining the semantic of this function • empathy_account_settings_account_ready_cb and empathy_account_settings_manager_ready_cb: log the error if _finish failed • empathy-chat-room: account_manager_ready_cb: idem • account_manager_prepared_cb : idem • dispatcher: account_manager_prepared_cb: idem • empathy_dispatcher_handle_channels: FIXME • log_store_empathy_get_filename_for_date: FIXME • account-chooser: account_manager_prepared_cb: log if _finish failed • chat: account_manager_prepared_cb; idem • contact_list_view_drag_data_received: FIXME • location: publish_to_all_am_prepared_cb and account_manager_prepared_cb: idem • publish_to_all_connections: Shouldn't PublishToAllData keep a ref on the self object to ensure it stays alive while the operation is flying? • log-window: account_manager_prepared_cb: log the error • presence-chooser: update_sensitivity_am_prepared_cb : idem • import-widget: account_manager_prepared_cb: idem • main-window: account_manager_prepared_cb : idem • status-icon: account_manager_prepared_cb: idem • empathy.c : account_manager_ready_for_accounts_cb and account_manager_ready_cb and account_manager_chatroom_ready_cb : idem • should_create_salut_account: FIXME For each FIXME, fix it if possible. If it's blocked on another feature or big refactoring then file a bug for it.
(In reply to comment #5) > • Shouldn't you bump the tp-glib version? If yes, please send a mail to ddl > about the bump. Yes, it should be bumped to 0.9.0. Sigh, do we really have to email d-d-l about it?! > • empathy_get_account_for_connection : FIXME Removed. > • empathy_get_account_for_connection: any reason why we don't have a > tp_account_manager_get_account_for_connection method? 13:50 < sjoerd> jonnylamb: because it's the wrong way round 13:50 < smcv> jonnylamb: probably YAGNI 13:50 < smcv> and that 13:50 < sjoerd> Empathy internally is slightly backwards because of historical reasons 13:50 < smcv> any time you have a Connection, you should already know the Account (and yes I realise that's not true in Empathy right now) > • empathy_idle_do_set_presence: FIXME Hm, updated. > • empathy-idle: account_manager_ready_cb: I'd pass a GError to _prepare_finish > and log the error if it fails Fixed. > • empathy-idle.c: please add a comment explaining the type of the keys and the > values of the connect_times hash table (and if they are reffed/owned) Done. > • empathy_idle_account_is_just_connected: add a define instead of hardcoding > "10". Also, add a comment explaining the semantic of this function Done. > • empathy_account_settings_account_ready_cb and > empathy_account_settings_manager_ready_cb: log the error if _finish failed > • empathy-chat-room: account_manager_ready_cb: idem > • account_manager_prepared_cb : idem > • dispatcher: account_manager_prepared_cb: idem Fixed. > • empathy_dispatcher_handle_channels: FIXME Opened bug #600111 > • log_store_empathy_get_filename_for_date: FIXME Referred to bug #599189. > • account-chooser: account_manager_prepared_cb: log if _finish failed Fixed. > • chat: account_manager_prepared_cb; idem Fixed. > • contact_list_view_drag_data_received: FIXME Boo, this would take some thought to fix, so I filed bug #600115 > • location: publish_to_all_am_prepared_cb and account_manager_prepared_cb: idem Fixed. > • publish_to_all_connections: Shouldn't PublishToAllData keep a ref on the self > object to ensure it stays alive while the operation is flying? Yes, fixed. > • log-window: account_manager_prepared_cb: log the error > • presence-chooser: update_sensitivity_am_prepared_cb : idem > • import-widget: account_manager_prepared_cb: idem > • main-window: account_manager_prepared_cb : idem > • status-icon: account_manager_prepared_cb: idem > • empathy.c : account_manager_ready_for_accounts_cb and > account_manager_ready_cb and account_manager_chatroom_ready_cb : idem Fixed. > • should_create_salut_account: FIXME Fixed. Branch is in the same place.
(In reply to comment #6) > Yes, it should be bumped to 0.9.0. Sigh, do we really have to email d-d-l about > it?! You do; please read http://live.gnome.org/TwoPointTwentynine/ExternalDependencies about the way external dependencies are handled.
(In reply to comment #6) > > • empathy_get_account_for_connection: any reason why we don't have a > > tp_account_manager_get_account_for_connection method? > > 13:50 < sjoerd> jonnylamb: because it's the wrong way round > 13:50 < smcv> jonnylamb: probably YAGNI > 13:50 < smcv> and that > 13:50 < sjoerd> Empathy internally is slightly backwards because of historical > reasons > 13:50 < smcv> any time you have a Connection, you should already know the > Account (and > yes I realise that's not true in Empathy right now) Could you open a bug describing this issue and explaining (in a few words) how Empathy should be refactored to do that properly? Your branch looks good, feel free to merge once the mail to ddl has been sent.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report. I opened bug #600417 about the empathy_account_for_connection thing.