I agree. Note that with MC 5.x the store format will change from gconf to a key file. When moving to MC 5.x we'll need a transition script either in MC directly or in Empathy. I think we should make sure that script could be extended to import other client accounts.

Created attachment 114087 [details] [review]
Initial imported dialog

So, I wrote this little dialog that does little more than import accounts from Pidgin. Getting the account info from pidgin's ~/.purple/accounts.xml does contain a few hacks though.

Thoughts and feedback on this would be appreciated.

Czech out my import-dialog branch at git://git.collabora.co.uk/git/user/jonny/empathy.git

I've attached the patch here.

Created attachment 114182 [details]
Review comments of the previous patch

Thanks for you patch, I attached comments in your patch.

(In reply to your comments):
> - When you get the protocol from the pidgin xml you can directly check if we have
>   the corresponding profile. If not you can avoid parsing the rest of settings,
>   if yes you can directly create the account and set settings on it instead of
>   storing in the hash table. I think that way you can avoid lots/most of the
>   complexity.

The reason I added this complexity was to keep the "adding the actual account to MC" part out of "parsing the ~/.purple/accounts.xml file part", so as to make it easier to write importer functions for other applications. For example, to write another application importer function, one would only have to compile a hash table, then submit it to empathy_import_dialog_import_account for actual adding. daf's original bug report mentioned having support for importing from multiple applications.

However, if you think this is just too much on crack, say so. :-)

You may be right, I have to think a bit more to see if it can't be simplified without losing the flexibility. Other comments can still be fixed.

Sure. I've already fixed the other issues raised in your previous comment. I'm now just waiting on the moving of empathy-accounts-dialog from libempathy-gtk, to normal empathy. Only after then will there be somewhere to actually call this dialog from!

*** Bug 540961 has been marked as a duplicate of this bug. ***

Here are my comments for the current state of the git branch:

Import accounts button should have an espacement of 6 with add/remove buttons. the label should be "Import accounts..."
• Code in status icon is not really useful
• Before creating an account, check if there is already one with the same protocol/accountID
• Pidgin also have a display name for accounts, no? Can't we import it?
• I don't like the import dialog UI, I think it should list all accounts from all supported applications and let the user check accounts he want to import.
• once the account is imported, would be great to import its logs.
• EmpathyImportDialog should be a singleton, make the dialog var in empathy_import_dialog_show static and if !=NULL simply call gtk_window_presence on it and return.
• avoid gtk_widget_show_all and call gtk_widget_show on each widget you create instead. This avoid going through all widget contained by the dialog.
• empathy_import_dialog_response_cb: Add an empty line between var declaration and the code.
• empathy_import_dialog_pidgin_import_accounts:  you can replace the first while() loops by for (node=rootnode->children; node; node = node->next){} and the first if in that loop can be changed to continue; to avoid identation of the rest of the code.
• static functions should have the import_* prefix and not empathy_import_*

Buttons should use Title Case, e.g. "Import Accounts...", not "Import accounts...".

(In reply to comment #8)
> Import accounts button should have an espacement of 6 with add/remove buttons.
> the label should be "Import accounts..."

Done.

> • Code in status icon is not really useful

Removed.

> • Before creating an account, check if there is already one with the same
> protocol/accountID

So, what do you think would be the best behaviour here: currently the user sees all Pidgin accounts listed in the importer dialog and the user selects which ones he or she wants to import. Therefore, he or she can easily opt out of importing a certain account manually.

However, it might be fairly trivial to make the default "Import?" checkbox be not active if there is an account existing with the same protocol/username. Do you think this is this the nicest way to achieve this, or would some other way be better?

> • Pidgin also have a display name for accounts, no? Can't we import it?

No, Pidgin uses the username as the display name.

> • I don't like the import dialog UI, I think it should list all accounts from
> all supported applications and let the user check accounts he want to import.

Done.

> • once the account is imported, would be great to import its logs.

This is an account importer. We can do a log importer afterwards..

> • EmpathyImportDialog should be a singleton, make the dialog var in
> empathy_import_dialog_show static and if !=NULL simply call gtk_window_presence
> on it and return.

Done.

> • avoid gtk_widget_show_all and call gtk_widget_show on each widget you
> create instead. This avoid going through all widget contained by the dialog.

Done.

> • empathy_import_dialog_response_cb: Add an empty line between var
> declaration and the code.

Done.

> • empathy_import_dialog_pidgin_import_accounts:  you can replace the first
> while() loops by for (node=rootnode->children; node; node = node->next){} and
> the first if in that loop can be changed to continue; to avoid identation of
> the rest of the code.

Done.

> • static functions should have the import_* prefix and not empathy_import_*

Done.

My import-dialog branch is at git://git.collabora.co.uk/git/user/jonny/empathy.git

(In reply to comment #10)
> However, it might be fairly trivial to make the default "Import?" checkbox be
> not active if there is an account existing with the same protocol/username. Do
> you think this is this the nicest way to achieve this, or would some other way
> be better?

This is what I've now done in my branch.

(In reply to comment #10)
> (In reply to comment #8)

> > • Before creating an account, check if there is already one with the same
> > protocol/accountID
> 
> So, what do you think would be the best behaviour here: currently the user sees
> all Pidgin accounts listed in the importer dialog and the user selects which
> ones he or she wants to import. Therefore, he or she can easily opt out of
> importing a certain account manually.
> 
> However, it might be fairly trivial to make the default "Import?" checkbox be
> not active if there is an account existing with the same protocol/username. Do
> you think this is this the nicest way to achieve this, or would some other way
> be better?

This is perfect IMO.

Comments from the latest branch:

* no_foo is not intuitive for "number of foo". especially no_not_imported which sounds like a double negation. I prefer nb_ or num_

* If import_dialog_set_up_account_list() returns false in the end of the _show function you just hide the window, so it never gets destroyed => leak. To avoid unessesary work, I think you should first load the list of accounts, and directly return NULL if there are no accounts to import. Otherwise you create the dialog's UI to show the list of accounts.

* You parse all accounts 2 times, I think it's avoidable. You should first check the asked gconf key, then call empathy_import_dialog_show() which will return NULL without displaying any UI if there is no account to show. I'm not sure it's useful to first ask the user if he wants to import accounts, he can still click on cancel on the accounts import dialog if he don't want to import them.

* In import_dialog_add_accounts_to_model(): Instead of checking if the lenght of the list is ==0 you can simply check if accounts==NULL. NULL is a GList of len 0.

* In import_dialog_add_accounts_to_model(): Instead of using mc_accounts_filter which seems a bit complicated to me, why don't you simply do somthing like that:

import = !import_dialog_is_account_id_in_list (const gchar *account_id, GList *all_accounts);

* I don't understand why you set a the account_data instide a GValue of type G_TYPE_POINTER. You can directly set the col on the store as G_TYPE_POINTER. And don't forget to call import_dialog_account_data_free() in import_dialog_tree_model_foreach().

* I don't think it's useful to display an import summary if there is no import error. I think you should only count the number of accounts that failed to import and display a warning dialog if that number is >0. Also I think it's nicer to first destroy the import dialog and then show the import summary dialog to avoid stacking too much windows.

* To avoid most case of import fail, I think AccountData should contain the McProfile instead of protocol string. Like that we can directly remote that account from the list of importable accounts if we don't have the corresponding profile. Also, like that we can display mc_profile_get_display_name() instead of the protocol string, which is in some cases more user-friendly I think.

(In reply to comment #12)
> This is perfect IMO.

Good.

> * If import_dialog_set_up_account_list() returns false in the end of the _show
> function you just hide the window, so it never gets destroyed => leak. To avoid
> unessesary work, I think you should first load the list of accounts, and
> directly return NULL if there are no accounts to import. Otherwise you create
> the dialog's UI to show the list of accounts.

I don't actually agree that it never got destroyed, but the code was cryptic so I have changed it. I haven't done exactly as you suggest, but still avoid creating the UI if there are no accounts and other wasteful things.

> * You parse all accounts 2 times, I think it's avoidable. You should first
> check the asked gconf key, then call empathy_import_dialog_show() which will
> return NULL without displaying any UI if there is no account to show. I'm not
> sure it's useful to first ask the user if he wants to import accounts, he can
> still click on cancel on the accounts import dialog if he don't want to import
> them.

Done.

> * In import_dialog_add_accounts_to_model(): Instead of checking if the lenght
> of the list is ==0 you can simply check if accounts==NULL. NULL is a GList of
> len 0.

Using g_list_length is arguably much easier to read, but whatever; done.

> * In import_dialog_add_accounts_to_model(): Instead of using mc_accounts_filter
> which seems a bit complicated to me, why don't you simply do somthing like
> that:
> 
> import = !import_dialog_is_account_id_in_list (const gchar *account_id, GList
> *all_accounts);

I definitely don't see how using mc_accounts_filter is "complicated", but I've done as you asked (unless I misunderstood what you meant).

> * I don't understand why you set a the account_data instide a GValue of type
> G_TYPE_POINTER. You can directly set the col on the store as G_TYPE_POINTER.
> And don't forget to call import_dialog_account_data_free() in
> import_dialog_tree_model_foreach().

I didn't realise that G_TYPE_POINTER was considered a GObject fundamental type as I thought it was just a gpointer, which of course is a pointer to void, which is nothing to do with GObjects. Anyway, I was wrong; I've learned something here!

> * I don't think it's useful to display an import summary if there is no import
> error. I think you should only count the number of accounts that failed to
> import and display a warning dialog if that number is >0. Also I think it's
> nicer to first destroy the import dialog and then show the import summary
> dialog to avoid stacking too much windows.

Done.

> * To avoid most case of import fail, I think AccountData should contain the
> McProfile instead of protocol string. Like that we can directly remote that
> account from the list of importable accounts if we don't have the corresponding
> profile. Also, like that we can display mc_profile_get_display_name() instead
> of the protocol string, which is in some cases more user-friendly I think.

Done.

Branch is at the same place.

I merged the branch with some additional patches. Thanks a lot!