Bug 40380

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.

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

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

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.

+ * @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.

Merged; will be in 0.15.6.