Bug 37358

Created attachment 46940 [details] [review]
ExampleEcho2Channel: implement SMS interface

Created attachment 46941 [details] [review]
TpTextChannel: add TP_TEXT_CHANNEL_FEATURE_SMS (#37358)

Created attachment 46942 [details] [review]
ExampleEcho2Channel: add a simple implementation of SMS.GetSMSLength()

Created attachment 46943 [details] [review]
add tp_text_channel_get_sms_length_async()

-      destroyable_iface_init)
+      destroyable_iface_init);
+    G_IMPLEMENT_INTERFACE (TP_TYPE_SVC_CHANNEL_INTERFACE_SMS, sms_iface_init);

These macros shouldn't end with a ; because it breaks some compilers (ask smcv for details).

+set_property (GObject *object,
+              guint property_id,
+              const GValue *value,
+              GParamSpec *pspec)

Not in Telepathy style.

+      case PROP_SMS:
+        self->priv->sms = g_value_get_boolean (value);
+        break;
+    default:
+      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
+      break;

Weird indenting.

Similarly both for get_property()

+  static TpDBusPropertiesMixinIfaceImpl prop_interfaces[] = {
+      { TP_IFACE_CHANNEL_INTERFACE_SMS,
+        tp_dbus_properties_mixin_getter_gobject_properties,
+        NULL,
+        sms_props,
+      },
+      { NULL }
+  };

+  klass->dbus_properties_class.interfaces = prop_interfaces;
+  tp_dbus_properties_mixin_class_init (object_class,
+      G_STRUCT_OFFSET (ExampleEcho2ChannelClass, dbus_properties_class));

TpBaseChannel already implements TpSvcDBusProperties, you can use tp_dbus_properties_mixin_implement_interface()

Personally, I don't think SMS needs to be a separate feature, that it's worth just having in CORE.

+  param_spec = g_param_spec_boolean ("is-sms",

is-sms-channel ?

+  if (error != NULL)
+    {
+      DEBUG ("Failed to get SMS length: %s", error->message);
+
+      g_simple_async_result_set_from_error (result, error);
+    }

missing goto finally; ?

(In reply to comment #5)
> -      destroyable_iface_init)
> +      destroyable_iface_init);
> +    G_IMPLEMENT_INTERFACE (TP_TYPE_SVC_CHANNEL_INTERFACE_SMS, sms_iface_init);
> 
> These macros shouldn't end with a ; because it breaks some compilers (ask smcv
> for details).

Right, I already experienced that as well; removed.

> +set_property (GObject *object,
> +              guint property_id,
> +              const GValue *value,
> +              GParamSpec *pspec)
> 
> Not in Telepathy style.

fixed.

> +      case PROP_SMS:
> +        self->priv->sms = g_value_get_boolean (value);
> +        break;
> +    default:
> +      G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
> +      break;
> 
> Weird indenting.
> 
> Similarly both for get_property()

fixed.

> +  static TpDBusPropertiesMixinIfaceImpl prop_interfaces[] = {
> +      { TP_IFACE_CHANNEL_INTERFACE_SMS,
> +        tp_dbus_properties_mixin_getter_gobject_properties,
> +        NULL,
> +        sms_props,
> +      },
> +      { NULL }
> +  };
> 
> +  klass->dbus_properties_class.interfaces = prop_interfaces;
> +  tp_dbus_properties_mixin_class_init (object_class,
> +      G_STRUCT_OFFSET (ExampleEcho2ChannelClass, dbus_properties_class));
> 
> TpBaseChannel already implements TpSvcDBusProperties, you can use
> tp_dbus_properties_mixin_implement_interface()

Oh good point; done.

> Personally, I don't think SMS needs to be a separate feature, that it's worth
> just having in CORE.

The general philosophy is to restrict CORE as much as possible. Users won't
have to care about the SMS feature as most of them will use the default
factory preparing this iface (actually I forgot to add it but I did now. :)

> +  param_spec = g_param_spec_boolean ("is-sms",
> 
> is-sms-channel ?

renamed.

> +  if (error != NULL)
> +    {
> +      DEBUG ("Failed to get SMS length: %s", error->message);
> +
> +      g_simple_async_result_set_from_error (result, error);
> +    }
> 
> missing goto finally; ?

Ooops; fixed.

http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=sms-37358
updated

++

Consider squashing before merge.

The comments refer to properties added in #36334. We should get that merged too.

Rebased, I'll push once bug #36334 is fixed.

Fixed and merged to master; will be in 0.15.1