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 _______________________________________________ tracker-list mailing list tracker-list@gnome.org http://mail.gnome.org/mailman/listinfo/tracker-list
