Bug 22589 - Implement Immutable_Streams capability flag, and ImmutableStreams channel property
Summary: Implement Immutable_Streams capability flag, and ImmutableStreams channel pro...
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/wj...
Whiteboard:
Keywords: patch
Depends on: 22588
Blocks: 24374
  Show dependency treegraph
 
Reported: 2009-07-01 11:20 UTC by Will Thompson
Modified: 2009-10-07 04:55 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Will Thompson 2009-07-01 11:20:57 UTC
The attached Gabble branch implements the flag and properties introduced in the spec branch at #22588.
Comment 1 Simon McVittie 2009-07-28 09:13:03 UTC
This is good enough to not block merging the spec, but contains mistakes.

+            g_object_get (object, "immutable-streams", &immutable_streams, NULL);
+            tp_asv_set_boolean (props,
+                g_strdup_printf ("%s.%s",
+                    GABBLE_IFACE_CHANNEL_TYPE_STREAMED_MEDIA_FUTURE,
+                    "ImmutableStreams"), immutable_streams);

This works by mistake: it would leak the dup'd string if the hash table was actually tp_asv_new-compatible, but the table returned by tp_dbus_properties_mixin_make_properties_hash expects dup'd strings as keys too. We should document what tp_dbus_properties_mixin_make_properties_hash actually wants, and make this code use g_hash_table_insert explicitly.

Also, this doesn't need to do g_strdup_printf, it can just use g_strdup (G_I_C_T_S_M_F ".ImmutableStreams") thanks to C string concatenation.

   static TpDBusPropertiesMixinPropImpl streamed_media_props[] = {
       { "InitialAudio", "initial-audio", NULL },
       { "InitialVideo", "initial-video", NULL },
+      { "ImmutableStreams", "immutable-streams", NULL },
       { NULL }
   };

This is on the wrong interface (not FUTURE), although that problem will go away when the spec is merged.

+  param_spec = g_param_spec_boolean ("immutable-streams", "ImmutableStreams",
+      "Whether the set of streams on this channel are fixed once requested",
+      FALSE,
+      G_PARAM_READABLE | G_PARAM_STATIC_STRINGS);
+  g_object_class_install_property (object_class, PROP_INITIAL_VIDEO,
+      param_spec);

I think you mean IMMUTABLE_STREAMS, not INITIAL_VIDEO!

+    typeflags |= 32; /* FIXME: use the constant when it's in tp-glib */

Indeed. We shouldn't actually merge this until the spec lands in tp-glib, I think.
Comment 2 Simon McVittie 2009-09-14 12:22:50 UTC
With telepathy-glib 0.7.37 out, this can be picked up again.
Comment 3 Will Thompson 2009-10-05 04:55:54 UTC
I've updated the branch, addressing the review comments:

(In reply to comment #1)
> This is good enough to not block merging the spec, but contains mistakes.
> 
> +            g_object_get (object, "immutable-streams", &immutable_streams,
> NULL);
> +            tp_asv_set_boolean (props,
> +                g_strdup_printf ("%s.%s",
> +                    GABBLE_IFACE_CHANNEL_TYPE_STREAMED_MEDIA_FUTURE,
> +                    "ImmutableStreams"), immutable_streams);
> 
> This works by mistake: it would leak the dup'd string if the hash table was
> actually tp_asv_new-compatible, but the table returned by
> tp_dbus_properties_mixin_make_properties_hash expects dup'd strings as keys
> too. We should document what tp_dbus_properties_mixin_make_properties_hash
> actually wants, and make this code use g_hash_table_insert explicitly.
> 
> Also, this doesn't need to do g_strdup_printf, it can just use g_strdup
> (G_I_C_T_S_M_F ".ImmutableStreams") thanks to C string concatenation.

Deleted all this code!

>    static TpDBusPropertiesMixinPropImpl streamed_media_props[] = {
>        { "InitialAudio", "initial-audio", NULL },
>        { "InitialVideo", "initial-video", NULL },
> +      { "ImmutableStreams", "immutable-streams", NULL },
>        { NULL }
>    };
> 
> This is on the wrong interface (not FUTURE), although that problem will go away
> when the spec is merged.

Those properties were all in the FUTURE when I wrote that branch. But the updated branch adds IS to the non-future, and (if we take the last patch) duplicates IA and IV in the non-future.

> +  param_spec = g_param_spec_boolean ("immutable-streams", "ImmutableStreams",
> +      "Whether the set of streams on this channel are fixed once requested",
> +      FALSE,
> +      G_PARAM_READABLE | G_PARAM_STATIC_STRINGS);
> +  g_object_class_install_property (object_class, PROP_INITIAL_VIDEO,
> +      param_spec);
> 
> I think you mean IMMUTABLE_STREAMS, not INITIAL_VIDEO!

Good catch!

> +    typeflags |= 32; /* FIXME: use the constant when it's in tp-glib */
> 
> Indeed. We shouldn't actually merge this until the spec lands in tp-glib, I
> think.

Fixed.
Comment 4 Simon McVittie 2009-10-06 05:33:29 UTC
Yay! Ship it!

Did we already implement this in 0.9.x? If not, please clone this (I expect it'll need a non-trivial merge).
Comment 5 Will Thompson 2009-10-07 04:52:54 UTC
(In reply to comment #4)
> Yay! Ship it!

Shipped. I'll clone the bug for 0.9, and attach the 0.9 branch.


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.