Re: [evolution-patches] [Calendar] ICS-Importer EPlugin

[Tor Lillqvist <tml novell com>]

ti 2006-01-03 klockan 23:07 +0530 skrev Johnny Jacob:

> Please review .

I have no comment about the actual code, presumably it works, just some
humble style opinions...

+ *  Authors: Srinivasa Ragavan <sragavan novell com>
+ *
+ *  Copyright 2005 Novell, Inc. (www.novell.com)

Please do use your name and the real copyright holder here. If much of
the code is cut&pasted so that Srinivasa is still an author and Novell a
copyright holder, just add your own name then as an additional author
and copyright holder.

(If much of the code is cut&pasted, one should investigate whether it
then should really be in just one place in a library and not cut&pasted
into several plugins?)

+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif

IMHO, in new code that's to be included in evolution or e-d-s, no need
to continue with this #ifdef HAVE_CONFIG_H convention. There will always
be a config.h. Just include it unconditionally.

+#include <string.h>
+#include <glib.h>
+#include <gtk/gtk.h>
+#include <libgnome/gnome-i18n.h>
+#include <e-util/e-config.h>
+#include <e-util/e-popup.h>
+#include <mail/em-popup.h>
+#include <mail/em-menu.h>
+#include <mail/mail-ops.h>
+#include <mail/mail-mt.h>
+#include <mail/em-folder-view.h>
+#include <mail/em-format-html-display.h>
+#include "e-attachment-bar.h"
+#include <camel/camel-vee-folder.h>
+#include "e-util/e-error.h"
+#include "e-util/e-icon-factory.h"
+#include <libgnomevfs/gnome-vfs-utils.h>
+#include <libgnomevfs/gnome-vfs-mime.h>
+#include <libgnomevfs/gnome-vfs-mime-utils.h>
+#include <libgnomevfs/gnome-vfs-mime-handlers.h>
+#include <libedataserverui/e-source-selector.h>
+#include <libecal/e-cal.h>
+#include <libical/icalvcal.h>

In new code please don't just copy-paste one of the typical randomly
ordered list of includes, with a random inconsistent mix of "" and <>
style. Does this source really require the libgnomevfs headers? What
about the mail/* headers? I think the most logical order and style is:

#include <config.h> /* actually "config.h" would be more logical I guess */

#include <C standard and POSIX headers>

#include <gtk, libgnomevfs, other GNOME libs and e-d-s headers>

#include "headers from other places in the same source module"

#include "headers from the same directory"

Sure, this is just cosmetics, but hey, readability *is* important.

+       -DEVOLUTION_DATADIR=\""$(datadir)"\"            \
+       -DEVOLUTION_PRIVDATADIR=\""$(privdatadir)"\"    \
+       -DEVOLUTION_GLADEDIR=\""$(gladedir)"\"          \
+       -DEVOLUTION_ETSPECDIR=\""$(etspecdir)"\"        \
+       -DEVOLUTION_ICONSDIR=\""$(imagesdir)"\"         \
+       -DEVOLUTION_IMAGES=\""$(imagesdir)"\"           \
+       -DEVOLUTION_GALVIEWSDIR=\""$(viewsdir)"\"       \
+       -DEVOLUTION_BUTTONSDIR=\""$(buttonsdir)"\"      \
+       -DEVOLUTION_LOCALEDIR=\""$(localedir)"\"        \
+       -DEVOLUTION_UIDIR=\""$(evolutionuidir)"\"       \
+       -DCAMEL_PROVIDERDIR=\""$(camel_providerdir)"\"  \
+       -DPLUGINDIR=\""$(plugindir)"\"                  \

Don't unnecessarily define such pathname macros that aren't used in the source files...

--tml