GNOME Bugzilla – Bug 731502
Import mails and contacts from KMail
Last modified: 2015-02-27 12:57:37 UTC
Hi, the patch aimed to import mails from kmail to evolution. (maildir format). I test it in evolution-3.10, it works.
Created attachment 278250 [details] [review] importer maidir format use the camel way to import the mail.
Thanks for the patch! + kmail-libs.h \ Does that mean that Evolution would get a hard dependency on KMail development files?
No, no kmail dependency. The kmail-libs.c/h is just analysing the kmail maildir structure, create folders accordingly and importing the mails. (In reply to comment #2) > Thanks for the patch! > > + kmail-libs.h \ > > Does that mean that Evolution would get a hard dependency on KMail development > files?
Well, what if kmail-libs.h is not available on my computer?
Oh, right, Sorry it is not a complete patch. (In reply to comment #4) > Well, what if kmail-libs.h is not available on my computer?
Oh, no sorry needed - that's why we look at patches, ask questions, and iterate on patches. :)
Review of attachment 278250 [details] [review]: The patch looks fine on a quick reading, the only thing is the description of the operation and the related function names. One thought, what is the difference between doing this importer and creating a new Maildir account which would point directly to the kmail folder, then copy the messages? I mean, except of the complexity of copying 100 folders, of course. I'm setting this to "needs-work", due to missing files in the patch. ::: mail/importers/mail-importer.c @@ +63,3 @@ +import_maildir_desc (struct _import_mbox_msg *m) +{ + return g_strdup (_("Importing maildir")); The name should be "Import KMail mails", just name KMail there, because the imported doesn't seem to be capable of any generic Maildir folder structure. ::: mail/importers/mail-importer.h @@ +64,3 @@ + void (*done)(gpointer data, GError **), + gpointer data); +void mail_importer_import_maildir_sync (EMailSession *session, These two new functions, and those related, should also name 'kmain' in their name.
Created attachment 278750 [details] [review] import kmail & kcontact Hi, thank you for the suggestion, I modifed the patch and attach the new patch now.
Review of attachment 278750 [details] [review]: Thanks for the update. I did it a quick read-review and below are the things which I'd like you to change. Most of the things are about coding style, than any code issues. When it's done, I will test the code and either ask for more changes or commit it. Thanks in advance. ::: evolution/mail/importers/kmail-importer.c @@ +55,3 @@ +#define d(x) + + extra empty line @@ +79,3 @@ +static void +kmail_import_done (gpointer data, + GError **error) parameters should be aligned in one column (similar issue multiple times in the patch) @@ +275,3 @@ + gtk_box_pack_start ((GtkBox *) box, w, FALSE, FALSE, 0); + +/* for now, we don't allow to select a folder */ indent the comment; I would indent the conditional-compiled code as well (those #if/#endif lines) @@ +307,3 @@ + session = e_mail_backend_get_session (E_MAIL_BACKEND (shell_backend)); + + /* TODO: do we validate target? */ Would it be possible to not have any TODO/FIXME in the patch, please? ::: evolution/mail/importers/kmail-libs.c @@ +78,3 @@ + if (!dir) { + /*If we did not find the subdir with 'cur' 'tmp' and 'new', + we don't take it as the kmail box */ missing space at the beginning (right after /*) and wrong comment indentation (the second line). Similar issues are on more places in the patch. @@ +100,3 @@ + +gchar * +kmail_get_base_dir () use 'void', not empty brackets. @@ +115,3 @@ +/home/ktoe/.local/share/local-mail/.inbox.directory/.aaaa.directory + folder://local/inbox/aaaa +*/ indent the comment, please @@ +216,3 @@ +static gchar * +eab_strstrcase (const gchar *haystack, + const gchar *needle) You do not need to duplicate the code, the function is public, thus use it. @@ +240,3 @@ + +static GSList * +eab_contact_list_from_string (const gchar *str) You should change the function name, otherwise it's misleading and one can search for the same name in addressbook, instead of here @@ +247,3 @@ + gchar *p = (gchar *) str; + gchar *q; + if (!p) white-space error ::: evolution/mail/importers/kmail-libs.h @@ +19,3 @@ + * + */ + missing #ifndef/#define .... #endif triple in the header file @@ +33,3 @@ +#include "libemail-engine/e-mail-utils.h" +#include "shell/e-shell.h" +#include "mail/e-mail-backend.h" you do not need all the above includes here, the only one you really need is the camel.h, which you have included below. @@ +37,3 @@ +#include <camel/camel.h> + + extra empty line ::: evolution/mail/importers/mail-importer.c @@ +59,3 @@ +import_kmail_desc (struct _import_mbox_msg *m) +{ + return g_strdup (_("Importing Kmail Mails")); you import also Contacts, right? (the second similar string at the end of kmail-importer.c also mentions only KMail Mails). By the way, they spell it "KMail", not "Kmail" ( http://userbase.kde.org/KMail )).
David: Any chance to update the patch, as per the review in comment 9?
(In reply to comment #10) > David: Any chance to update the patch, as per the review in comment 9? Hi Andre, Sorry for the late, I'll update the patch ASAP, the deadline is this Friday. David
Created attachment 295792 [details] [review] update by fixing format/indent/comment issues Hi, Thanks for the detailed instruction. The updated patch fixes the type/space/name/indent issues. The function 'eab_strstrcase' is replaced by global 'camel_strstrcase' I change one 'FIXME' note to comment. Mails in KMail 'tmp' dir is taken as the deleted mail in my understanding. Two TODOs in the previous patch, one is actually needless, another is a note to remind myself to make a little enhancement ('kcontact_get_list'). I lost my building enviornment now, so just remove the 'TODO' note and post the new patch here.
Thanks for the patch. I made couple changes to it, please compare [1] with the attached version to see what was changed. Some changes were due to coding style issues, some due to build break (pity you use such an old version of evolution). Nonetheless, I committed this, even I understand that you might provide some additional enhancements in the future (preferably in a new bug report). Created commit becf59f in evo master (3.13.90+) [1] [1] https://git.gnome.org/browse/evolution/commit/?id=becf59f
*** Bug 229617 has been marked as a duplicate of this bug. ***