[Bug 40380] Add tp_dbus_properties_mixin_emit_properties_changed

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=40380

--- Comment #4 from Will Thompson <will.thompson at collabora.co.uk> 2011-08-26 05:51:51 PDT ---
I've pushed some patches that fix your review comments, and also change how
this works a little: now the EmitsChanged annotations from the introspection
XML are included in the generated code, and as a result the function just takes
one array of property names and figures out for itself whether they belong in
'changed' or 'invalidated'.

I could squash some of this stuff down if you think it's clearer or easier to
review? Right now you get to review the delta between my previous iteration of
tp_dbus_properties_mixin_emit_properties_changed and the current one.

(In reply to comment #3)
> (In reply to comment #2)
> > I should be using CRITICAL, which does include the function name
> 
> You could do both!

I switched to using CRITICAL, and changed the message.

> > > + g_return_val_if_fail (self != NULL, FALSE);
> > > + g_return_val_if_fail (G_IS_OBJECT (self), FALSE);
> > > 
> > > Not sure both of these are necessary?
> > 
> > One of the type-checking macros returns TRUE for NULL. I can never remember
> > which one. I *think* it's _IS_.
> 
> no lol

k

> > Yes—emit_properties_changed calls _iface_impl_get_property with a NULL GValue
> > if it doesn't actually want the value.
> 
> Ah okay I missed that.

This got removed in my subsequent changes (and unleaked in the non-NULL case,
which is now the only case).

> > > Not sure about only passing the names of the changed properties. It's only a
> > > bit crap if getting the property is expensive or time-consuming, but I guess
> > > that never happens, right? Yeah, probably not, carry on.
> > 
> > I don't understand the potential complaint here: did you mean “why don't we
> > just always include all the properties in the interface on every signal”?
> 
> No, I was wondering whether you should be giving the actual changed properties'
> values too because if you're calling this method you know what they're new
> value is, and if getting them is expensive it is unnecessary, but getting them
> shouldn't be expensive anyway.

I think it should be cheap.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.