Bug 24385

+    haze_connection_capabilities_class_init(object_class);

Coding style: space before (. By the way, thanks for following the
Telepathy style in new files, and the Haze style (which I regret being
different ;-)) in existing files.

tp_flags_to_purple_caps and purple_caps_to_tp_flags:

+  caps |= (flags & TP_CHANNEL_MEDIA_CAPABILITY_AUDIO) != 0 ?
+      PURPLE_MEDIA_CAPS_AUDIO : 0;

What's wrong with:

    if (flags & TP_CHANNEL_MEDIA_CAPABILITY_AUDIO)
      caps |= PURPLE_MEDIA_CAPS_AUDIO;

?

haze_connection_advertise_capabilities: please don't copy-paste
incorrect docstrings. :)

+    }
+  for (i = 0; NULL != del[i]; i++)

Style pedantry: please leave a blank line after the end of a block
before the next statement (if it's not an 'else'). I think it makes the
code easier to follow.

+  g_timeout_add (10000, caps_received_cb_cb, crd);

As a general point, it's better to use g_timeout_add_seconds() where
possible to avoid wakeups. But this is just a hack so I'm not too
bothered. :)

But: if the PurpleConnection is disconnected before the 10 seconds are
up, we'll crash when the callback is called.

You never disconnect from PurpleMediaManager:init-media.

I'm inclined to say that haze_media_manager_channel_class (); should
have a static hash table containing the channel class, but I think this
might just be a note from the department of premature optimization. :-)

+  value = tp_g_value_slice_new (G_TYPE_STRING);
+  g_value_set_static_string (value, TP_IFACE_CHANNEL_TYPE_STREAMED_MEDIA);
+  g_hash_table_insert (table, TP_IFACE_CHANNEL ".ChannelType", value);

could just become

    tp_asv_set_static_string (table, TP_IFACE_CHANNEL ".ChannelType",
        TP_IFACE_CHANNEL_TYPE_STREAMED_MEDIA);

In fact, the whole function could become:

    return tp_asv_new (
        TP_IFACE_CHANNEL ".ChannelType", G_TYPE_STRING,
            TP_IFACE_CHANNEL_TYPE_STREAMED_MEDIA,
        TP_IFACE_CHANNEL ".TargetHandleType", G_TYPE_UINT,
            TP_HANDLE_TYPE_CONTACT,
        NULL);

:-)

(The code in Gabble's channel managers pre-dates tp_asv_new() and
tp_asv_set_*().)

TBH. it's probably worth copying gabble_g_ptr_array_copy () into a new
util.c in Haze (or indeed putting it in tp-glib, which already contains
tp_g_ptr_array_contains ()).

Retitling the bug since it's kind of an umbrella for this branch.

Up to f0aa818 and 1b75416 (media-buddy-caps-changed-signal and media-ui-changed-signal):

Gabble does the:

  unsigned ready: 1;

crap for bitfielding booleans, but I really don't think it's worth bitfielding
these variables: the extra few words used by the channel and manager's private
structs are the least of your worries when making a/v calls. :)

(If you were feeling keen I guess you could write a patch for Gabble too :P)

I don't think Haze needs to support the RequestChannel() methods of creating
streamed media channels at all. This would simplify a bunch of code, because
you can require a handle, and not have to have the peer_in_rp stuff, and
simplify the property getters, etc.

In theory we've got D-Bus properties that replace all the Tp properties on
MediaSignalling, so Haze never needs to implement them. (Which simplifies the
code.

+          /*
+           * This isn't always true. We might need to have this happen on
+           * REJECT/HANGUP signals instead
+           */

That sounds like something that needs fixing :P.

+          "Streams can't be removed in this Haze protocol's calls" };

Would be nicer to include the protocol's name. This is just a debugging string, but...

Why does haze_media_channel_remove_streams() dup a list of IDs before checking
if the method's even supported?

+          g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+              "given media type %u is invalid", media_type);
+          return FALSE;

Technically this is the wrong error, because someone could add
Media_Type_Smell. From RequestStreams() in the spec:

    # Not Implemented

    A stream type given is not implemented by the connection manager. Since
    0.17.23, connection managers SHOULD raise this error in preference to
    InvalidArgument.

    (Connection managers can't know whether an unknown number is a valid stream
    type that was introduced in a later spec version.)

MediaStream's D-Bus properties are undrafted now, so Haze (and Gabble) should
just use the D-Bus properties mixin rather than manually implementing Get and
GetAll.

media_error_cb:

+  HazeMediaStream *stream = g_ptr_array_index (chan->priv->streams, 0);

Why're you picking the first one at random? And, if there are no streams this
will crash.

A bunch of patches add trailing whitespace.

Review of 'media' up to f2fe99be4:

+      if (type == PURPLE_MEDIA_INFO_REJECT)
+        reason = "Media session rejected";
+      else
+        reason = "Media session hang up";
+

This is meant to be a message from the peer, like a part message on IRC, not a string added by the (untranslated) CM. Jingle does actually support these; just leave it empty if libpurple doesn't expose it (I'm reasonably sure no UI will ever expose it).

+      if (name != NULL)
+          /* Remove participant */
+          tp_intset_add (set, priv->initial_peer);
+      else
+          /* Remove us */
+          tp_intset_add (set, mixin->self_handle);

If the call's ending, remove both of us from the call. This is vaguely documented in the group change reasons in the spec, but not documented anywhere else. I don't think any UI will actually care but it's better to be consistent with Gabble.

Maybe you could merge the two mini-branches for 2.7devel functionality (buddy-caps-changed-signal and ui-changed-signal) into the 2.7 branch?

I've updated the branch with the refactorings I had been wanting to do. Review away :)

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.

Regarding the copyright stuff at the top of several of the files, I did copy stuff from the gabble media files, although several times that was more boilerplate (GObject class type stuff). I'll get to the remaining fixes shortly.

I finished making the review changes on the branch and wrapping the media stuff with ifdefs like we discussed earlier. Everything is pushed and ready for your perusal.

This has been merged.