Bug 40380

Summary: Add tp_dbus_properties_mixin_emit_properties_changed
Product: Telepathy Reporter: Will Thompson <will>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=emit-properties-changed
Whiteboard: review+
i915 platform: i915 features:

Description Will Thompson 2011-08-25 10:19:38 UTC
Here's a branch which adds a method to emit org.freedesktop.DBus.Properties.PropertiesChanged. It's easier than doing it by hand because you don't have to dig out the properties’ new values.

I could be convinced to give it a name shorter than 48 characters.

I decided against having varargs versions because really we shouldn't be writing C anyway. ;)
Comment 1 Jonny Lamb 2011-08-26 03:25:29 UTC
Just a few bikeshedding points:

+ g_critical ("Couldn't fetch '%s' on interface '%s': %s",
+     property, iface, error->message);

I can't remember whether g_critical will include the function name in the log message but perhaps have "Couldn't fetch property '%s..." to make it clearer?

+ 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?

-   g_value_init (value, prop_info->type);
-   iface_impl->getter (self, iface_info->dbus_interface,
-       prop_info->name, value, prop_impl->getter_data);
+   if (value != NULL)
+     {
+       g_value_init (value, prop_info->type);
+       iface_impl->getter (self, iface_info->dbus_interface,
+       prop_info->name, value, prop_impl->getter_data);
+     }

Is this meant to be in the topmost commit?

+ tp_dbus_properties_mixin_emit_properties_changed (
+     GObject *object,
+     const gchar *interface_name,
+     const gchar * const *changed,
+     const gchar * const *invalidated)

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.

+ WARNING ("u mad? %s", error->message);

soz.
Comment 2 Will Thompson 2011-08-26 03:35:56 UTC
(In reply to comment #1)
> Just a few bikeshedding points:
> 
> + g_critical ("Couldn't fetch '%s' on interface '%s': %s",
> +     property, iface, error->message);
> 
> I can't remember whether g_critical will include the function name in the log
> message but perhaps have "Couldn't fetch property '%s..." to make it clearer?

I should be using CRITICAL, which does include the function name

> + 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_.

> -   g_value_init (value, prop_info->type);
> -   iface_impl->getter (self, iface_info->dbus_interface,
> -       prop_info->name, value, prop_impl->getter_data);
> +   if (value != NULL)
> +     {
> +       g_value_init (value, prop_info->type);
> +       iface_impl->getter (self, iface_info->dbus_interface,
> +       prop_info->name, value, prop_impl->getter_data);
> +     }
> 
> Is this meant to be in the topmost commit?

Yes—emit_properties_changed calls _iface_impl_get_property with a NULL GValue if it doesn't actually want the value.

> + tp_dbus_properties_mixin_emit_properties_changed (
> +     GObject *object,
> +     const gchar *interface_name,
> +     const gchar * const *changed,
> +     const gchar * const *invalidated)
> 
> 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”?

> + WARNING ("u mad? %s", error->message);
> 
> soz.

Yeah yeah.
Comment 3 Jonny Lamb 2011-08-26 04:31:24 UTC
(In reply to comment #2)
> I should be using CRITICAL, which does include the function name

You could do both!

> > + 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

> 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.

> > 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.
Comment 4 Will Thompson 2011-08-26 05:51:51 UTC
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.
Comment 5 Jonny Lamb 2011-08-26 08:37:15 UTC
+ * @TP_DBUS_PROPERTIES_MIXIN_FLAG_EMITS_CHANGED: The property's new value is
+ * included in emissions of PropertiesChanged
+ * @TP_DBUS_PROPERTIES_MIXIN_FLAG_EMITS_INVALIDATED: The property is announced
+ * as invalidated, without its value, in emissions of PropertiesChanged

Perhaps include a comment in the docs mentioning that only one of these can be used per property. The assertion later on the code isn't exactly obvious what it's checking for either.

Looks fine otherwise.
Comment 6 Will Thompson 2011-09-07 04:32:09 UTC
Merged; will be in 0.15.6.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.