In preparation for potentially moving the non-Telepathy-specific Jingle code down into Wocky in the future, I have prepared an absurdly large branch to remove uses of telepathy-glib (and, for that matter, Gabble API) from pretty much all of the Jingle-flavoured classes. I'm sorry it's so big, and I'm also sorry that it doesn't end with a commit which actually moves the code to Wocky, but there we go.
(In reply to comment #0) > I'm sorry it's so big, and I'm also sorry that it doesn't end with a commit > which actually moves the code to Wocky, but there we go. Haha, I was like "okay I'll take a look" ... http://oi41.tinypic.com/qxjapx.jpg
> @@ -2374,14 +2361,14 @@ session_terminated_cb (GabbleJingleSession *session, > TpGroupMixin *mixin = TP_GROUP_MIXIN (channel); > guint terminator; > JingleState state; > - TpHandle peer; > + TpHandle peer = gabble_jingle_session_get_peer_handle (priv->session); > TpIntSet *set; > > DEBUG ("called"); > > + peer = gabble_jingle_session_get_peer_handle (priv->session); Duplicate call to gabble_jingle_session_get_peer_handle() (in "MediaChannel: use gabble_jingle_session_get_peer_handle()", d884caf42e3) Everything else up to and including that commit looks good. There might be a million patches but at least they're easy to review :-)
"Send session-{initiate,accept} IQs using porter directly.", 72ea73343252: I think you're leaking sess in the callbacks, except in the early-return cases where the state has become inappropriate? Everything else up to and including that point looks good, except as already reviewed.
> +static void > +gabble_jingle_info_dispose (GObject *object) > +{ > + GabbleJingleInfo *self = GABBLE_JINGLE_INFO (object); > + GabbleJingleInfoPrivate *priv = self->priv; > + GObjectClass *parent_class = gabble_jingle_info_parent_class; > + > + if (parent_class->dispose != NULL) > + parent_class->dispose (object); Surely dispose should chain up at the end, not the beginning? I see dispose and finalize as progressively breaking the functionality of the object's classes, most-derived first. jingle_info_reply_cb looked crashy, but you fixed that in the next commit. Everything else up to 39b90872eb "JingleContent: don't store a GabbleConnection" looks good except as noted already.
> + /* Only emitted for new incoming sessions, mainly for legacy reasons */ > + signals[NEW_SESSION] = g_signal_new ("new-session", Can't the JingleMint's new signal be "incoming-session" or something? In da9f352766e "Represent Jingle relays by a struct, not an aa{sv}": > + GPtrArray *tp_relays = gabble_build_tp_relay_info (relays); > + tp_base_media_call_stream_set_relay_info (stream, tp_relays); Nitpicking: I'd prefer a blank line between initialized locals and other code. All reviewed now, looks good apart from the things already noted.
(In reply to comment #2) > > @@ -2374,14 +2361,14 @@ session_terminated_cb (GabbleJingleSession *session, > > TpGroupMixin *mixin = TP_GROUP_MIXIN (channel); > > guint terminator; > > JingleState state; > > - TpHandle peer; > > + TpHandle peer = gabble_jingle_session_get_peer_handle (priv->session); > > TpIntSet *set; > > > > DEBUG ("called"); > > > > + peer = gabble_jingle_session_get_peer_handle (priv->session); > > Duplicate call to gabble_jingle_session_get_peer_handle() (in "MediaChannel: > use gabble_jingle_session_get_peer_handle()", d884caf42e3) This local gets removed entirely in a later patch, because the MediaChannel tracks its own peer. > Everything else up to and including that commit looks good. There might be a > million patches but at least they're easy to review :-) I try! (In reply to comment #3) > "Send session-{initiate,accept} IQs using porter directly.", 72ea73343252: I > think you're leaking sess in the callbacks, except in the early-return cases > where the state has become inappropriate? Aye. Patch fixing this pushed. (In reply to comment #4) > > +static void > > +gabble_jingle_info_dispose (GObject *object) > > +{ > > + GabbleJingleInfo *self = GABBLE_JINGLE_INFO (object); > > + GabbleJingleInfoPrivate *priv = self->priv; > > + GObjectClass *parent_class = gabble_jingle_info_parent_class; > > + > > + if (parent_class->dispose != NULL) > > + parent_class->dispose (object); > > Surely dispose should chain up at the end, not the beginning? I see dispose and > finalize as progressively breaking the functionality of the object's classes, > most-derived first. Yup. I even called someone out on this the other day. Fixed.
(In reply to comment #5) > > + /* Only emitted for new incoming sessions, mainly for legacy reasons */ > > + signals[NEW_SESSION] = g_signal_new ("new-session", > > Can't the JingleMint's new signal be "incoming-session" or something? Good idea. Renamed. > In da9f352766e "Represent Jingle relays by a struct, not an aa{sv}": > > > + GPtrArray *tp_relays = gabble_build_tp_relay_info (relays); > > + tp_base_media_call_stream_set_relay_info (stream, tp_relays); > > Nitpicking: I'd prefer a blank line between initialized locals and other code. Fixed.
This looks good now.
It is merged! http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/?id=0a7c66f48e2a4f5f1b80237def4063fa952ccc71 A disappointing diffstat in the end: 53 files changed, 2177 insertions(+), 1205 deletions(-) Oh well.
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.