This is a first review of http://monkey.collabora.co.uk/telepathy-salut.git_tubes/
tubes.txt ========= what's the status of 1-1 D-Bus tubes? We should clarify it before merging. We shouldn't ignore the ack of the SI offer. Please explain the IP policy when a new tube stream is established. Tube closing should have examples. gibber-bytestream-direct ======================== Maybe bytestream-direct-tcp or bytestream-raw-tcp would be a better name? header file: args should be 4 spaces aligned case PROP_PROTOCOL: g_value_set_string (value, (const gchar *)""); we should define and use a name for this priv->stream_init_id doesn't make sense anymore Please wrap > 80 chars lines args of the getter/setter/constructor are miss aligned args of g_signal_new should be 4 spaces aligned static void transport_buffer_empty_cb (GibberTransport *transport, add a space before this function gibber_bytestream_direct_initiate: we should store the SalutContact of the remote peer and use it to get his IP. Do we still need to store the XMPP connection ? gibber-iq-helper ================ Why remove the if (!NULL) check when disposing? We always check that in dispose functions. salut-bytestream-manager.h wasn't removed salut-direct-bytestream-manager =============================== /* guint id -> guint listener_watch * When used by stream tubes, the id is the tube_id */ GHashTable *listeners; That's a lie, this hashtable contains SalutDirectBytestreamManagerListener objects. Humm actually, the key doesn't seem to be a guint either but a tube ptr. I'm not sure this kind of mapping make sense. We just need a way for the bytestream user to stop listening right? Maybe we could pass the port to salut_direct_bytestream_manager_stop_listen and store listeners in a GSList ? Or maybe having a real listener object could make sense? priv->listeners = g_hash_table_new (NULL, NULL); you should_use g_hash_table_new_full and pass a listener_free function as value_destroy_func salut_direct_bytestream_manager_stop_listen: shouldn't remove the listener from the hashtable? They are never removed. You don't seem to need priv->host_name_fqdn anymore. listener_io_in_cb shouldn't it return TRUE to continue listening? listener_io_in_cb you should check if the connection is coming from the right contact salut_direct_bytestream_manager_stop_listen: I think you could just use g_source_remove (id) I'm wondering if, finally, that make sense to have 2 distinct bytestream mgrs. I think the plan is to factor out most of salut-direct-bytestream-manager to a GibberTcpListenner object (or something similar) that could be used by GibberXmppConnection and probably stream tubes too (to watch connections on the local tube socket). Maybe then we could use it directly in a unified bytestream-mgr ? Or maybe if the API of this GibberTcpListenner is easy enough, the direct-bytestream-mgr won't make sense anymore and we could use it directly from the stream tube module? salut-tubes-channel =================== function declarations at the begin of the .c file should have the "static void" on their own line struct _SalutTubesChannelPrivate Please distinct clearly which variables are only used for muc tubes (as the muc_connection) and the one only used for 1-1 tubes. Maybe we should create 2 subclass at some point. _initialise_connection useless double blank lines priv->xmpp_connection = conn; g_object_ref (priv->xmpp_connection); _initialise_connection (self); I'd make _initalise_connection (self, conn) and move the assignment/refing to it tubes_message_received seems this function is misnamed and its only purpose is to create the tube +void tubes_message_close_received (SalutTubesChannel *self, the void should be on its own line DEBUG ("received a tube close message on a non existant tube"); s/existant/existent tubes_muc_message_received, tubes_message_received and tubes_message_close_received should be prefixed with salut_tubes_channel On a style node, I never understood the point to prefix static method with _. They are static and don't have the module prefix so I think that's enough to distinct them from public methods. _send_channel_iq_tube please group declarations in the if block gchar *tube_id_str = g_strdup_printf ("%d", tube_id); I'd separate the assignment from the declaration salut-tubes-manager =================== _close = close_node != NULL; please add ( ) Could be worth to do an early return if (_close) to simplify code tube_type = gibber_xmpp_node_get_attribute (tube_node, "type"); if (g_str_equal (tube_type, "stream")) Is it NULL safe? Why not use tp_strdiff as we always do? DEBUG ("The <iq><tube> does not have a correct type=."); remove the =. and display the bugged type iq_tube_request_cb: TpTubeType tube_type = TP_TUBE_TYPE_DBUS; why this type by default? DEBUG ("received a tube request, tube_id %d", tube_id); s/tube_id/tube id priv->contact_manager and priv->xmpp_connection_manager are never unreffed *Signal callback for when an Tubes channel is closed. Removes the references s/an/a what's the point of salut_tubes_manager_handle_tube_request ? It seems called nowhere. Same question about salut_tubes_manager_handle_si_stream_request. We don't want to use SI for 1-1 tubes. tube-dbus ========= - if (bytestream != NULL) - g_object_set (tube, "bytestream", bytestream, NULL); I don't understand this change, when is set the bytestream now ? + klass->offer_needed = NULL; it should implement this method tube-iface ========== This salut_tube_iface_offer_needed looks very cracky to me. Can't we use the tube's state to determine if we need to send the offer or not? tube-stream =========== + g_assert_not_reached (); node = gibber_xmpp_node_add_child (si_node, "stream"); Please remove the assignation after the assertion as it's pointless now start_stream_direct: early return if salut_contact_manager_get_contact returns NULL priv->xmpp_connection_manager is never unreffed salut_tube_stream_close: what about if salut_xmpp_connection_manager_request_connection doesn't return DONE ? salut_tube_stream_close: DEBUG ("ERROR: '%s'", error->message); make the error message more informative If the CHANNEL property doesn't make sense anymore, remove it. Please test if tube-dbus-muc.py, tube-stream-muc.py and tube-stream-private.py work with your branch. Regards, G. _______________________________________________ Telepathy mailing list Telepathy@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/telepathy