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

[Not Zed <notzed ximian com>]

The camel patch doesn't need to be so big, and you can save some
processing too.

In camel-folder.c:folder_changed, you can just make this one line
change:

	CAMEL_FOLDER_UNLOCK(folder, change_lock);

->	if (changed->uid_changed->len) {
		guint32 flags;

		for (i = 0; i < changed->uid_changed->len; i ++) {
			flags = camel_folder_get_message_flags (folder,
changed->uid_changed->pdata [i]);
			if (flags & CAMEL_MESSAGE_JUNK_LEARN) {


Change the line with -> on it to:
	if (session->junk_plugin && changed->uid_changed->len) {

This will prevent the junk data from ever being setup in the first place
and shortcut all of the subsequent processing (that whole block is only
doing checks for junk processing).

The rest looks ready, but its 2005, not 2004 anymore :).

Please commit once the above change is made to the camel patch.

Thanks,
 Michael

On Tue, 2005-07-05 at 16:21 +0530, Vivek Jain wrote:
> 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