After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 567756 - Add the import the gajim accounts
Add the import the gajim accounts
Status: RESOLVED INCOMPLETE
Product: empathy
Classification: Core
Component: Accounts
2.25.x
Other Linux
: Normal enhancement
: ---
Assigned To: empathy-maint
Depends on:
Blocks:
 
 
Reported: 2009-01-14 15:33 UTC by Stephane Wirtel
Modified: 2012-08-08 20:41 UTC
See Also:
GNOME target: ---
GNOME version: 2.25/2.26



Description Stephane Wirtel 2009-01-14 15:33:43 UTC
With this branch, I propose to add the import of the gajim accounts for Empathy.

git://github.com/matrixise/empathy-with-import-gajim-account.git
Comment 1 Jonny Lamb 2009-01-22 23:35:55 UTC
Hi. Thanks for your patch. Here are a few comments I found from a quick look:

 * empathy_import_dialog_show: You removed the dialog showing if there are no
   accounts to import. However, this dialog is only shown if the user clicks
   on the "Import accounts" button. I don't think you should just ignore this
   silently, but I agree that the message is out-of-date. Just make it more
   generic, like "There are no accounts to import".

 * typedef struct _Foo { ... } Foo; really doesn't require a struct name.
   (i.e. omit "_Foo").

 * account_info->port = 5222: I don't think this is necessary as connection
   managers have these kind of defaults already.

 * empathy_import_account_data_new_from_gajim_account_info: Don't give static
   functions the prefix "empathy_". Also, this function name is rather
   verbose. :-)

 * The g_asserts are probably unnecessary because 

 * empathy_import_gajim_load: g_file_load_contents is blocking. If the gajim
   config is particularly large, then the UI will be frozen.

 * empathy_import_gajim_load: `file` is not unrefed if `goto FILENAME` is
   executed.

 * You shouldn't assign variables on the same line as the declaration, and you
   should declare variables at the top of a function.

 * g_strv_length (info) is called just after g_strfreev (info).

 * g_strdup (g_strjoinv (...)) is bad because g_strjoinv returns a
   newly-allocated string so the strdup is unnecessary and leaks the string.

 * Just a suggestion and feel free to ignore it, but when looking at the
   key of the config item, using GQuarks and a switch case statement would
   probably be neater.

 * account_info->use_ssl = (!tp_strdiff (value, "True") ? TRUE : FALSE);

   can be written simply as:

   account_info->use_ssl = !tp_strdiff (value, "True");

   as tp_strdiff returns a gboolean.

 * item = g_list_first (gajim_accounts); while (item != NULL) is probably nicer
   as a for loop:

     GList *l;
     for (l = gajim_accounts; l; l = l->next)

   This is also a bit nicer because it's the way list iterations are done
   throughout Empathy.

 * empathy-import-gajim.c: contains a few lines with trailing whitespace.
   `git diff --check` is useful to detect these kind of things.

Thanks again for your patch!
Comment 2 Stephane Wirtel 2009-01-23 01:40:50 UTC
Thank you for your review.

I fixed some points, you will find a new revision on the same branch.

three points have not been fixed.

* empathy_import_gajim_load: g_file_load_contents is blocking. If the gajim
  config is particularly large, then the UI will be frozen.
Sorry, I don't know how to do it. But the config file isn't large, some lines.

* Just a suggestion and feel free to ignore it, but when looking at the
  key of the config item, using GQuarks and a switch case statement would
  probably be neater.
I don't know how to use the GQuarks, 

* g_strv_length (info) is called just after g_strfreev (info)
info has been assigned with g_strsplit, you can check it.

Can you review the new revision ? 

Thank you
Comment 3 Dafydd Harries 2009-01-23 09:05:12 UTC
Meta-review. :)

 * The g_asserts are probably unnecessary because 

Because what?

 * empathy_import_gajim_load: g_file_load_contents is blocking. If the gajim
   config is particularly large, then the UI will be frozen.

Even if the file is small, IO is slow. What's the appropriate async API to use here?

 * You shouldn't assign variables on the same line as the declaration, and you
   should declare variables at the top of a function.

I think that trivial initializations are fine; complex ones should be deferred.
Comment 4 Cosimo Cecchi 2009-01-23 09:24:25 UTC
(In reply to comment #3)

>  * empathy_import_gajim_load: g_file_load_contents is blocking. If the gajim
>    config is particularly large, then the UI will be frozen.
> 
> Even if the file is small, IO is slow. What's the appropriate async API to use
> here?

You should use g_file_load_contents_async () to avoid freezing the UI. Doing sync I/O is always evil and not recommended even if the file is usually slow (for instance, your home folder could be on NFS).
Comment 5 Jonny Lamb 2009-01-23 12:22:32 UTC
(In reply to comment #3)
>  * The g_asserts are probably unnecessary because 
> 
> Because what?

Great quoting Dafydd. I think argument sanity checking is usually more necessary further up in non-static functions. I suppose, thinking about it now though, when reading from a file like this, there is a more point in sanity checking.

Okay, you win, I agree there should be some checking, but I'm not sure g_assert is the best thing. It'll just display a cryptic warning.

>  * empathy_import_gajim_load: g_file_load_contents is blocking. If the gajim
>    config is particularly large, then the UI will be frozen.
> 
> Even if the file is small, IO is slow. What's the appropriate async API to use
> here?

As Cosimo suggests, g_file_load_contents_async.

>  * You shouldn't assign variables on the same line as the declaration, and you
>    should declare variables at the top of a function.
> 
> I think that trivial initializations are fine; complex ones should be deferred.

Hmm, I'm not so sure moving away from the style that Empathy uses literally everwhere is such a great idea.. Thoughts Xavier?
Comment 6 Xavier Claessens 2009-01-23 12:42:10 UTC
I didn't read the code, but just in case: Never assert/warning if something is wrong in the input. What ever crap you find in the accounts storage, it should never generate warnings. Warnings are for progammation errors, if the input is wrong, you can print a debug message or warn the user, but no g_assert/g_warning.

For initializations, I just find it more clear to read the code, if it is still clear I don't mind... When I code myself I prefer to never call functions in the initialization.
Comment 7 Jonny Lamb 2009-01-30 23:10:06 UTC
(In reply to comment #2)
> * empathy_import_gajim_load: g_file_load_contents is blocking. If the gajim
>   config is particularly large, then the UI will be frozen.
> Sorry, I don't know how to do it. But the config file isn't large, some lines.

Use g_file_load_contents_async.

> I don't know how to use the GQuarks,

Okay, don't worry. It might be worth looking into it what they are though. Start here: http://www.google.co.uk/search?q=gquark

> * g_strv_length (info) is called just after g_strfreev (info)
> info has been assigned with g_strsplit, you can check it.

Good point. I missed that.

> Can you review the new revision ? 

You appear to have fixed everything else on my review. Thanks! I'll give the branch a test once the async version of g_file_load_contents is in use.

Thanks again for your patch.
Comment 8 Stephane Wirtel 2009-02-08 15:18:34 UTC
How can I use the g_file_load_contents_async function if the API is not async-ready ?

Currently, the import function must return a GList *, which can't be made if the file is loaded async.

I asked to Xavier to help to write my asynchronous code but he tells me it is not possible without changing the API.

Thanks
Comment 9 Jonny Lamb 2009-08-27 15:36:28 UTC
Maybe we should suck it up and get this merged and make it async later?
Comment 10 Guillaume Desmottes 2009-08-27 15:51:52 UTC
If you think that's good enough then go for it. It's shame to let useful contributions rotting on the bugzilla.
Comment 11 Cosimo Cecchi 2009-08-28 08:29:34 UTC
Btw, the patch would have to be updated a bit, as the import system has been reworked a bit recently with the transition to MC5 and the account assistant.
Comment 12 Stephane Wirtel 2009-08-28 08:32:58 UTC
(In reply to comment #11)
> Btw, the patch would have to be updated a bit, as the import system has been
> reworked a bit recently with the transition to MC5 and the account assistant.

I will upgrade the patch asap, because I'm very busy on a project for the job and I don't have the time to update the patch.

I will try to provide a new patch in one month.

Stephane
Comment 13 Stephane Wirtel 2009-11-12 12:43:25 UTC
Hi all, 

I will rework on this patch during the next week.

Regards,

Stephane
Comment 14 Jean-François Fortin Tam 2012-08-08 20:41:04 UTC
This patch has never been completed. Closing, unless Jonny wishes to do some bit of archeology and forward-port that patch to the current codebase...