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

[Philip Van Hoof <spam pvanhoof be>]

ok, fair enough.

http://git.codethink.co.uk/?p=tracker;a=commitdiff;h=739e33a1a7175d8f481e0fa617cc388b7b7803a7

Was this the last remark before commit approval?

On Thu, 2009-02-12 at 11:39 +0000, Martyn Russell wrote:
> Philip Van Hoof wrote:
> > On Thu, 2009-02-12 at 10:15 +0000, Martyn Russell wrote:
> > > > > +void
> > > > > +tracker_push_registrar_set_object (TrackerPushRegistrar *registrar,
> > > > > +                                   GObject              *object)
> > > > > +{
> > > > > +        TrackerPushRegistrarPrivate *priv;
> > > > > +
> > > > > +        priv = TRACKER_PUSH_REGISTRAR_GET_PRIVATE (registrar);
> > > > > +
> > > > > +        if (priv->object)
> > > > > +                g_object_unref (priv->object);
> > > > > +
> > > > > +        if (object)
> > > > > +                priv->object = g_object_ref (object);
> > > > > +        else
> > > > > +                priv->object = NULL;
> > > > > +}
> > > > > +
> > > > So, what is the problem here exactly?
> > > Say priv->object = 0xdeadbeef. Say object (passed in as an argument to 
> > > the function) is also 0xdeadbeef, both are the same object. The object 
> > > has 1 reference. We then remove that reference (which finalizes it) and 
> > > then we try to ref it again. It is better in these conditions to always 
> > > ref the new object just in case we are silly enough somewhere to set the 
> > > object to the same thing we already have set.
> >
> > Allowing NULL is being used to reset the object in various places in the
> > code. It would make it complicated if I wouldn't deal with NULL.
> >
> > For example these two in tracker_evolution_push_registrar_enable:
> >
> > tracker_push_registrar_set_object (registrar, NULL);
> > tracker_push_registrar_set_manager (registrar, NULL);
> >
> > The reason is that NameOwnerChanged happens in irregular ways for a new
> > or a being-deleted service. It for example sometimes happens twice per
> > service, in an inconsistent way.
> >
> > Regretfully there's no heuristic that I can apply here. Whether you get
> > NameOwnerChanged twice, three times or just once seems to depend on the
> > moon, Ubuntu version, Fedora version, DBus version, amount of other
> > services, Richard Dawkins's thee-pot between Mars and Earth, etc
> >
> > So each time it happens I need to 'destroy' any existing DBus object
> > being registered for the path, and create a new one. Not just unref a
> > DBus object, but instead being sure that it's destroyed instead. 
> >
> > That's the reason for the resetting (and using NULL is the method to be
> > used for that resetting)
> >
> > Note that the reference count for that reason can't ever be > 1, unless
> > you use the instances wrong (of course). So setting to NULL always
> > destroys (unless you are using the instances wrong, but that's a bug
> > that must just be fixed then).
>
> That doesn't make the change useless though. It is good practise to do 
> it this way especially if someone decides to cut and paste the code - we 
> have all been victims of that one in the past :)
>
> I am not saying it can't be NULL either. It is better to have:
>
>       if (object) {
>               g_object_ref (object);
>       }
>
>       if (priv->object) {
>               g_object_unref (priv->object);
>       }
>
>       priv->object = object;
-- 
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