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

[Martyn Russell <martyn imendio com>]

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;

--
Regards,
Martyn