[Bug 24385] Implement media in Haze

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=24385





--- Comment #6 from Will Thompson <will.thompson at collabora.co.uk>  2009-12-04 07:30:50 PST ---
Why not just use the HazeConnectionClass * as the handle for
buddy-caps-changed, rather than making up a new one? But the { static int
handle; return &handle } thing is more purpleish, so feel free to ignore me.

Is HazeMediaBackend really © 2006–2009 Collabora, and © Nokia at all?

+  DEBUG ( "Media.StreamHandler::Error called, error %u (%s) -- emitting
signal",
+      errno, message);
+
+// maybe emit an error here?
+

+      DEBUG ("Unknown media type");
+      // return error?

How would we get a HazeMediaStream with an unknown type? g_assert_not_reached()

+  /* this should also specify the session_id */

Hmm?

+      if (PURPLE_IS_MEDIA (priv->media))
+        g_signal_connect (priv->media, "state-changed",
+            G_CALLBACK (haze_backend_state_changed_cb), backend);

Why would it not be a PurpleMedia? GObject should check that for us, no?

+          3, purple_media_candidate_get_protocol (c) ==
+              PURPLE_MEDIA_NETWORK_PROTOCOL_UDP ? 0 : 1,

What are these magic numbers I see before me?

+  /*
+   * This appears to be called for each pair of components.
+   * I'm not sure how to go about differentiating between the two
+   * components as the ids are the same.
+   */

Maybe you can't. This interface isn't great. :-)

        Typecast the priority to convert between Telepathy and libpurple.

Is this actually all the conversion you need to do to convert between double
and int priority?!

+      tp_svc_media_stream_handler_emit_set_stream_playing (self, TRUE);
+      /* There's likely a much better time to set sending */
+      tp_svc_media_stream_handler_emit_set_stream_sending (self, TRUE);

I think you should only set sending if the local user has explicitly made the
stream have that direction (or it was one of the initial streams in an incoming
call).

+      g_object_add_weak_pointer(G_OBJECT(priv->media),
+          (gpointer*)&priv->media);

I think the strict aliasing warning beast will get upset about this. I'm
surprised it hasn't hit you. Hmm. Is the nano_version in configure.ac set to 1?

        Standardize libpurple includes.

This reminds me about https://bugs.freedesktop.org/show_bug.cgi?id=23061 .
#include <libpurple/foo.h> is actually wrong, because purple.pc does
-I$(prefix)/lib/libpurple, not -I$(prefix)/lib. My plan was to make libpurple
install purple-prefixed.pc (or a better name if you have one) alongside the
existing, sub-ideal .pc files. I never finished that branch. If I finish it,
will you review it and then we can ship it in 2.7? :-)

+  param_spec = g_param_spec_string ("object-path", "D-Bus object path",
+                                    "The D-Bus object path used for this "
+                                    "object on the bus.",
+                                    NULL,
+                                    G_PARAM_READWRITE |
+                                    G_PARAM_STATIC_NAME |
+                                    G_PARAM_STATIC_BLURB);

Use STATIC_STRINGS, and this should be CONSTRUCT_ONLY.

I think strictly speaking HazeMediaBackend::streams' get_property
implementation should ref the GPtrArray. But I don't mind that much.


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