Re: [evolution-patches] Re: New hook for junk plugin (improved)

[Vivek Jain <jvivek novell com>]

hi, 

I have removed the NULL checks from the camel-junk-plugin.c. Instead I
make sure its not NULL, before calling junk-plugin methods from
camel-folder.c. Attached patch to camel does that job.

I have incorporated all the comments in my file, em-junk-hook.c.

1. Return "None" name when there is no plugin
2. Pass only camel-mime-message in target
3. have removed the unneeded init call
4. have removed unnecessary null checks before g_free
5. Am assigning the plugin to the session only when the item is
constructed.
6. Handled the case of loading multiple plugins. I refuse to load the
second one and give a warning, that eventually gives one more warning
from e-utils, citing the plugin name which failed to load.

I am attaching the modified file "em-junk-plugin.c" and a patch to
camel.
Please let me know if they look ok.

Thanks,
Vivek Jain



On Tue, 2005-07-05 at 13:32 +0800, Not Zed wrote:
> Style has a few niggles.  K&r style braces!!
> 
> All of the checks in the junk plugin methods for a NULL junk plugin
> pointer - i dont like those.  It shouldn't be being called if it isn't
> set.  None of the junk processing should occur if there is no plugin
> assigned.
> 
> There's absolutely no reason to call junk_plugin_init in the hook loader
> - you know it does nothing because all its doing is calling your null
> init callback!
> 
> I think you should return NULL for the name if the plugin is disabled.
> Or find another string like "None" which already exists and use that,
> the extra string to translate is redundant.
> 
> The target shouldn't have the item in it.  If the junk callbacks need
> the item - which they dont anyway, it only contains internal data to the
> hook that the plugin has no use of - that, and the target should be
> wrapped in a hookdata struct.  I think you can just pass the target
> directly with just the message in it, at least.
> 
> You dont need to check pointers for NULL when calling g_free, it already
> adds that overhead for you.
> 
> The code shouldn't assign the junk plugin to the session - THEN validate
> if it was ok - and free the plugin from under the session if it wasn't!
> 
> It doesn't handle having multiple junk plugins very well.  i.e. it just
> overwrites the last one with a new one, leaking.
> 
> I'm not sure the best way here.  One is to just setup a single hook to
> the session, but load all plugins into a list, and just run the first
> enabled plugin for the callbacks, or just spit a warning if you already
> have one loaded and another one comes along.  It has to do something
> better than just leaking at least.
> 
> 
> On Mon, 2005-07-04 at 16:26 +0530, Vivek Jain wrote:
> > hi,
> > 
> > I am attaching the modified "em-junk-hook.c" and  patches to
> > mail-session.c and camel-junk-plugin.c
> > 
> > Now I am assigning NULL to junk_plugin member of session while init.
> > The plugin is supplied in the "construct" of the "item". If "item" is
> > not constructed, there is no plugin to load.
> > 
> > file camel-junk-plugin.c is modified to handle the cases when there is
> > no plugin at all.
> > Please let me know if it looks ok.
> > 
> > Thanks,
> > Vivek Jain
> > 
> > 
> > 
> > On Fri, 2005-06-24 at 14:33 +0530, Vivek Jain wrote:
> > > Hi,
> > > 
> > > Now I am passing the CamelJunkPlugin object to the callbacks and
> > > subclassing it into the EMJunkHookItem, so that I can get the "item"
> > > when methods are invoked.
> > > 
> > > The camel/camel-junk-plugin.[ch] files have to be patched for this.
> > > I have attached the improved em-junk-hook.[ch] files and a patch to
> > > camel.
> > > 
> > > Please let me know if this looks ok.
> > > 
> > > Thanks,
> > > Vivek Jain
> > > 
> > > 
> > > On Thu, 2005-06-23 at 21:11 +0530, Vivek Jain wrote:
> > > > hi,
> > > > 
> > > > I have created a hook that wraps up camelJunkPlugin and allows its
> > > > methods to be exposed to the plugins. Using this hook one can write a
> > > > plugin to use its own method for spam handling. As an example I have
> > > > moved the spamassasin junk mail handling to a plugin which used this new
> > > > hook.
> > > > 
> > > > I have added two files em-format-hook.c em-format-hook.h in the /mail
> > > > and removed em-junk-filter.[ch] from here. I have moved em-junk-filter.c
> > > > to the newly written sa-junk-plugin.
> > > > 
> > > > I am attaching the following:
> > > > 
> > > > 1. Patch for mail/
> > > > 2. two new files
> > > > 3. sa-junk-plugin
> > > > 
> > > > 
> > > > Thanks,
> > > > Vivek Jain
> > > > 
> > _______________________________________________
> > evolution-patches mailing list
> > evolution-patches lists ximian com
> > http://lists.ximian.com/mailman/listinfo/evolution-patches
> 
> _______________________________________________
> evolution-patches mailing list
> evolution-patches lists ximian com
> http://lists.ximian.com/mailman/listinfo/evolution-patches

Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution-data-server/camel/ChangeLog,v
retrieving revision 1.2451
diff -u -p -r1.2451 ChangeLog
--- ChangeLog	1 Jul 2005 03:30:35 -0000	1.2451
+++ ChangeLog	5 Jul 2005 10:13:31 -0000
@@ -1,3 +1,8 @@
+2005-07-05 Vivek Jain <jvivek novell com>
+
+	* camel-folder.c: (filter_filter) check if junk_plugin is
+	NULL before using it.
+	
 2005-06-24	Matt Brown <matt mattb net nz>
 
 	* camel-gpg-context.c:	Extend verify and decrypt functions to support
Index: camel-folder.c
===================================================================
RCS file: /cvs/gnome/evolution-data-server/camel/camel-folder.c,v
retrieving revision 1.209
diff -u -p -r1.209 camel-folder.c
--- camel-folder.c	10 May 2005 19:29:35 -0000	1.209
+++ camel-folder.c	5 Jul 2005 10:13:31 -0000
@@ -1649,41 +1649,43 @@ filter_filter(CamelSession *session, Cam
 	CamelException ex;
 	CamelJunkPlugin *csp = ((CamelService *)m->folder->parent_store)->session->junk_plugin;
 
-	if (m->junk) {
-		camel_operation_start (NULL, _("Learning junk"));
-
-		for (i = 0; i < m->junk->len; i ++) {
-			CamelMimeMessage *msg = camel_folder_get_message(m->folder, m->junk->pdata[i], NULL);
-			int pc = 100 * i / m->junk->len;
-			
-			camel_operation_progress(NULL, pc);
-
-			if (msg) {
-				camel_junk_plugin_report_junk (csp, msg);
-				camel_object_unref (msg);
+	if (csp != NULL) {
+		if (m->junk) {
+			camel_operation_start (NULL, _("Learning junk"));
+
+			for (i = 0; i < m->junk->len; i ++) {
+				CamelMimeMessage *msg = camel_folder_get_message(m->folder, m->junk->pdata[i], NULL);
+				int pc = 100 * i / m->junk->len;
+
+				camel_operation_progress(NULL, pc);
+
+				if (msg) {
+					camel_junk_plugin_report_junk (csp, msg);
+					camel_object_unref (msg);
+				}
 			}
+			camel_operation_end (NULL);
 		}
-		camel_operation_end (NULL);
-	}
 
-	if (m->notjunk) {
-		camel_operation_start (NULL, _("Learning non-junk"));
-		for (i = 0; i < m->notjunk->len; i ++) {
-			CamelMimeMessage *msg = camel_folder_get_message(m->folder, m->notjunk->pdata[i], NULL);
-			int pc = 100 * i / m->notjunk->len;
-
-			camel_operation_progress(NULL, pc);
-
-			if (msg) {
-				camel_junk_plugin_report_notjunk (csp, msg);
-				camel_object_unref (msg);
+		if (m->notjunk) {
+			camel_operation_start (NULL, _("Learning non-junk"));
+			for (i = 0; i < m->notjunk->len; i ++) {
+				CamelMimeMessage *msg = camel_folder_get_message(m->folder, m->notjunk->pdata[i], NULL);
+				int pc = 100 * i / m->notjunk->len;
+
+				camel_operation_progress(NULL, pc);
+
+				if (msg) {
+					camel_junk_plugin_report_notjunk (csp, msg);
+					camel_object_unref (msg);
+				}
 			}
+			camel_operation_end (NULL);
 		}
-		camel_operation_end (NULL);
-	}
 
-	if (m->junk || m->notjunk)
-		camel_junk_plugin_commit_reports (csp);
+		if (m->junk || m->notjunk)
+			camel_junk_plugin_commit_reports (csp);
+	}
 
 	if (m->driver && m->recents) {
 		camel_operation_start(NULL, _("Filtering new message(s)"));

/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
/*
 *  Authors: Vivek Jain <jvivek novell com>
 *
 *  Copyright 2004 Novell, Inc. (www.novell.com)
 *
 *  This program is free software; you can redistribute it and/or modify
 *  it under the terms of the GNU General Public License as published by
 *  the Free Software Foundation; either version 2 of the License, or
 *  (at your option) any later version.
 *
 *  This program is distributed in the hope that it will be useful,
 *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *  GNU General Public License for more details.
 *
 *  You should have received a copy of the GNU General Public License
 *  along with this program; if not, write to the Free Software
 *  Foundation, Inc., 59 Temple Street #330, Boston, MA 02111-1307, USA.
 *
 */

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

#include <string.h>
#include <stdlib.h>
#include <glib.h>
#include "em-junk-hook.h"
#include "mail-session.h"
#include <e-util/e-icon-factory.h>
#include <camel/camel-junk-plugin.h>
#include <glib/gi18n.h>

static GHashTable *emjh_types;
static GObjectClass *parent_class = NULL;

static void *emjh_parent_class;
static GObjectClass *emj_parent;
#define emjh ((EMJunkHook *)eph)

#define d(x)

static const EPluginHookTargetKey emjh_flag_map[] = {
	{ 0 }
};

/* ********************************************************************** */

/* Mail junk plugin */

/*
  <hook class="org.gnome.evolution.mail.junk:1.0">
  <group id="EMJunk">
     <item check_junk="sa_check_junk"
     	   report_junk="sa_report_junk"
	   report_non_junk="sa_report_non_junk"
	   commit_reports = "sa_commit_reports"/>
  </group>
  </hook>

*/

static void 
em_junk_init(CamelJunkPlugin *csp)
{
}

static const char *
em_junk_get_name (CamelJunkPlugin *csp)
{
	struct _EMJunkHookItem *item = (EMJunkHookItem *)csp;

	if (item->hook && item->hook->hook.plugin->enabled) {
		return (item->hook->hook.plugin->name);
	} else
		return 	_("None");

}

static gboolean 
em_junk_check_junk(CamelJunkPlugin *csp, CamelMimeMessage *m)
{
	struct _EMJunkHookItem *item = (EMJunkHookItem *)csp;

	if (item->hook && item->hook->hook.plugin->enabled) {
		EMJunkHookTarget target = {
			  m
		};

		return (gboolean)(e_plugin_invoke(item->hook->hook.plugin, item->check_junk, &target));
	}

	return FALSE;
}

static void 
em_junk_report_junk(CamelJunkPlugin *csp, CamelMimeMessage *m)
{
	struct _EMJunkHookItem *item = (EMJunkHookItem *)csp;

	if (item->hook && item->hook->hook.plugin->enabled) {
		EMJunkHookTarget target = {
			  m
		};

		e_plugin_invoke(item->hook->hook.plugin, item->report_junk, &target);
	}
}

static void 
em_junk_report_non_junk(CamelJunkPlugin *csp, CamelMimeMessage *m)
{
	struct _EMJunkHookItem *item = (EMJunkHookItem *)csp;

	if (item->hook && item->hook->hook.plugin->enabled) {
		EMJunkHookTarget target = {
			 m
		};
		e_plugin_invoke(item->hook->hook.plugin, item->report_non_junk, &target);
	}
}

static void 
em_junk_commit_reports(CamelJunkPlugin *csp)
{
	struct _EMJunkHookItem *item = (EMJunkHookItem *)csp;

	if (item->hook && item->hook->hook.plugin->enabled) 
		e_plugin_invoke(item->hook->hook.plugin, item->commit_reports, NULL);

}

static void 
emj_dispose (GObject *object)
{
	if (parent_class->dispose)
		parent_class->dispose (object);
}

static void 
emj_finalize (GObject *object)
{
	if (parent_class->finalize)
		parent_class->finalize (object);
}

static void
emjh_free_item(EMJunkHookItem *item)
{
	g_free (item->check_junk);
	g_free (item->report_junk);
	g_free (item->report_non_junk);
	g_free (item->commit_reports);
	g_free(item);
}

static void
emjh_free_group(EMJunkHookGroup *group)
{
	g_slist_foreach(group->items, (GFunc)emjh_free_item, NULL);
	g_slist_free(group->items);

	g_free(group->id);
	g_free(group);
}

static struct _EMJunkHookItem *
emjh_construct_item(EPluginHook *eph, EMJunkHookGroup *group, xmlNodePtr root)
{
	struct _EMJunkHookItem *item; 
	EMJunk junk_plugin = {
		{
			em_junk_get_name,
			1,
			em_junk_check_junk,
			em_junk_report_junk,
			em_junk_report_non_junk,
			em_junk_commit_reports,
			em_junk_init,
		}
	};

	d(printf("  loading group item\n"));
	item = g_malloc0(sizeof(*item));
	item->csp =  junk_plugin.csp;
	item->check_junk = e_plugin_xml_prop(root, "check_junk");
	item->report_junk = e_plugin_xml_prop(root, "report_junk");
	item->report_non_junk = e_plugin_xml_prop(root, "report_non_junk");
	item->commit_reports = e_plugin_xml_prop(root, "commit_reports");
	item->hook = emjh;
	
	if (item->check_junk == NULL || item->report_junk == NULL || item->report_non_junk == NULL || item->commit_reports == NULL)
		goto error;
	
	/* assign the plugin to the session*/
	session->junk_plugin = CAMEL_JUNK_PLUGIN (&(item->csp));
	
	return item;
error:
	printf ("ERROR");
	emjh_free_item (item);
	return NULL;
}

static struct _EMJunkHookGroup *
emjh_construct_group(EPluginHook *eph, xmlNodePtr root)
{
	struct _EMJunkHookGroup *group;
	xmlNodePtr node;

	d(printf(" loading group\n"));
	group = g_malloc0(sizeof(*group));

	group->id = e_plugin_xml_prop(root, "id");
	if (group->id == NULL)
		goto error;

	node = root->children;
	
	/* We'll processs only  the first item from xml file*/
	while (node) {
		if (0 == strcmp(node->name, "item")) {
			struct _EMJunkHookItem *item;

			item = emjh_construct_item(eph, group, node);
			if (item)
				group->items = g_slist_append(group->items, item);
			break;
		}
		
		node = node->next;
	}

	return group;
error:
	emjh_free_group(group);
	
	return NULL;
}

static int
emjh_construct(EPluginHook *eph, EPlugin *ep, xmlNodePtr root)
{
	xmlNodePtr node;
	static gboolean loaded = FALSE;
		
	d(printf("loading junk hook\n"));

	if (((EPluginHookClass *)emjh_parent_class)->construct(eph, ep, root) == -1)
		return -1;

	if (loaded) {
		g_warning ("Can't load multiple plugins to this hook:ignored");
		return -1;
	}

	node = root->children;
	while (node) {
		if (strcmp(node->name, "group") == 0) {
			struct _EMJunkHookGroup *group;

			group = emjh_construct_group(eph, node);
			if (group) {
				emjh->groups = g_slist_append(emjh->groups, group);
			}
		}
		node = node->next;
	}
	
	eph->plugin = ep;
	loaded = TRUE;

	return 0;
}

/*XXX: don't think we need here*/
static void
emjh_enable(EPluginHook *eph, int state)
{
	GSList *g;
	
	g = emjh->groups;
	if (emjh_types == NULL)
		return;

}

static void
emjh_finalise(GObject *o)
{
	EPluginHook *eph = (EPluginHook *)o;

	g_slist_foreach(emjh->groups, (GFunc)emjh_free_group, NULL);
	g_slist_free(emjh->groups);

	((GObjectClass *)emjh_parent_class)->finalize(o);
}

static void
emjh_class_init(EPluginHookClass *klass)
{
	((GObjectClass *)klass)->finalize = emjh_finalise;
	klass->construct = emjh_construct;
	klass->enable = emjh_enable;
	klass->id = "org.gnome.evolution.mail.junk:1.0";
}

GType
em_junk_hook_get_type(void)
{
	static GType type = 0;
	
	if (!type) {
		static const GTypeInfo info = {
			sizeof(EMJunkHookClass), NULL, NULL, (GClassInitFunc) emjh_class_init, NULL, NULL,
			sizeof(EMJunkHook), 0, (GInstanceInitFunc) NULL,
		};

		emjh_parent_class = g_type_class_ref(e_plugin_hook_get_type());
		type = g_type_register_static(e_plugin_hook_get_type(), "EMJunkHook", &info, 0);
	}
	
	return type;
}

static void
emj_class_init (EMJunkClass *klass)
{
	GObjectClass *object_class = G_OBJECT_CLASS (klass);
	parent_class = g_type_class_peek_parent (klass);
	object_class->dispose = emj_dispose;
	object_class->finalize = emj_finalize;
}

GType
emj_get_type(void)
{
	static GType type = 0;
	
	if (!type) {
		static const GTypeInfo info = {
			sizeof(EMJunkClass), NULL, NULL, (GClassInitFunc) emj_class_init, NULL, NULL,
			sizeof(EMJunk), 0, (GInstanceInitFunc) NULL,
		};

		emj_parent = g_type_class_ref(G_TYPE_OBJECT);
		type = g_type_register_static(G_TYPE_OBJECT, "EMJunk", &info, 0);
	}
	
	return type;
}

void em_junk_hook_register_type(GType type)
{
	EMJunk *klass;

	if (emjh_types == NULL)
		emjh_types = g_hash_table_new(g_str_hash, g_str_equal);

	d(printf("registering junk plugin type '%s'\n", g_type_name(type)));

	klass = g_type_class_ref(type);
	g_hash_table_insert(emjh_types, (void *)g_type_name(type), klass);
}