Re: [Tracker] Refactored the src/plugins & (new) Evolution support

[Philip Van Hoof <spam pvanhoof be>]

Fixes for all four change requests are committed and pushed in the git
branch at Codethink's git.

Let me know when/if I can bring this to Subversion tracker/trunk

On Thu, 2009-02-12 at 14:26 +0000, Martyn Russell wrote:
> Martyn Russell wrote:
> > Philip Van Hoof wrote:
> > > ok, fair enough.
> > >
> > > http://git.codethink.co.uk/?p=tracker;a=commitdiff;h=739e33a1a7175d8f481e0fa617cc388b7b7803a7 
> > >
> > >
> > > Was this the last remark before commit approval?
> >
> > I just wanted to look at the files I didn't have, 
> > trackerd/tracker-push.[ch] first. Will get back to you before the end of 
> > the day.
>
> Hi Philip,
>
> #1, instead of doing this:
>
> + static void
> + unload_modules (PushSupportPrivate *private)
> + {
> +     GList *copy = private->modules;
> +
> +     while (copy) {
> +             PushModule *p_module = copy->data;
> +
> +             p_module->shutdown (p_module->registrar);
> +
> +             g_object_unref (p_module->registrar);
> +             g_module_close (p_module->module);
> +             g_slice_free (PushModule, p_module);
> +
> +             copy = g_list_next (copy);
> +     }
> +
> +     g_list_free (private->modules);
> +     private->modules = NULL;
> + }
>
>
> Can I suggest we either use g_list_foreach() or a for() loop here from 
> tracker_push_shutdown().
>
> #2, The load_modules() still has this if(error) goto handle_error. 
> Please do not use goto for single conditions - especially when they just 
> return anyway. Can you change that to just print the error in the 
> condition above and return.
>
> #3, In name_owner_changed_cb(), we have another while() loop for 
> handling a list. Can we use for() there please.
>
> #4, Again, another 2 while() cases where for() is more appropriate in 
> list_names_reply_cb().
>
> --
>
> With those fixed, I think we can commit. I am a bit uneasy about this 
> approach to "push" modules. I would much rather we had the same 
> infrastructure as we do for indexer modules here. It would be expansible 
> and consistent (with how we do it everywhere else already). I think you 
> might find when we come to add more "push" modules that we will have to 
> do something like this later down the line anyway.
>
> Thanks again for the patch Philip :)
-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be