Bug 27602

http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=shortlog;h=refs/heads/ensure-channel-async

Work-in-Progress

This branch implements the required API, but there is currently no test coverage (and has not yet been tested).

+void tp_connection_ensure_channel_async (TpConnection *self,
+    GHashTable *request, GAsyncReadyCallback callback, gpointer user_data);
+
+TpChannel *tp_connection_ensure_channel_finish (TpConnection *self,
+    GAsyncResult *result, gboolean *yours, GError **error);
+
+void tp_connection_create_channel_async (TpConnection *self,
+    GHashTable *request, GAsyncReadyCallback callback, gpointer user_data);
+
+TpChannel *tp_connection_create_channel_finish (TpConnection *self,
+    GAsyncResult *result, GError **error);

SimpleConnection doesn't implement Chan.Iface.Requests, which makes it unsuitable for use in testing this code.

Isn't this a dup of bug #13422 ?

(In reply to comment #2)
> Isn't this a dup of bug #13422 ?

Almost. More precisely, this is a simple subset of Bug #13422.

Ok. This branch now has test coverage. Review plz.

> Move struct definitions from channel.h to channel-object.h

Hmm, I hadn't realised this would cause a circular reference. I'd much prefer it if you only moved the typedef, either into connection.h or some new central header (types.h or typedefs.h or some such), with the rest staying where it already is.

In other words, in connection.h or types.h or something:

    typedef struct _TpChannel TpChannel;

and in channel.h:

    /* TpChannel typedef is in connection.h */
    typedef struct _TpChannelPrivate TpChannelPrivate;
    typedef struct _TpChannelClass TpChannelClass;

TpProxy and TpDBusDaemon have a similarly circular relationship, with TpDBusDaemon typedef'd in proxy.h to resolve it.

The new API needs adding to docs/reference/*sections.txt, which you'd have spotted if you'd run `make check` with gtk-doc enabled (please do this when writing patches for telepathy-glib). You'd also have spotted that t_c_c_c_f's doc-comment is labelled as if for t_c_e_c_f.

> +  g_return_val_if_fail (g_simple_async_result_is_valid (result,
> +        G_OBJECT (self), tp_connection_create_channel_finish), NULL);

The tag is conventionally the _async function, not the _finish function (I know TpAccount[Manager] uses the finish function in some places).

> ... with the
> + * %TP_CHANNEL_FEATURE_CORE feature ready on it.

Shouldn't these functions take an array of features as an optional argument? I expect it to be extremely common to want GROUP, for a start.

Forgot to review the test:

I'd appreciate it if you used GTest (setup/teardown functions) for new tests like this one.

> +  g_message (G_STRFUNC);

Please get rid of these before merging.

When asserting that error != NULL, please assert the actual error with g_assert_error (error, TP_ERRORS, TP_ERROR_NOT_ENOUGH_CHEESE) or whatever, if it's at all predictable.

When asserting that error == NULL, please use g_assert_no_error (of which test_assert_no_error is a semi-deprecated version - patches to clean up all the uses of that macro in existing code would be welcome, but are out of scope here :-)

(In reply to comment #5)
> > Move struct definitions from channel.h to channel-object.h
> 
> Hmm, I hadn't realised this would cause a circular reference. I'd much prefer
> it if you only moved the typedef, either into connection.h or some new central
> header (types.h or typedefs.h or some such), with the rest staying where it
> already is.
> 
> In other words, in connection.h or types.h or something:
> 
>     typedef struct _TpChannel TpChannel;
> 
> and in channel.h:
> 
>     /* TpChannel typedef is in connection.h */
>     typedef struct _TpChannelPrivate TpChannelPrivate;
>     typedef struct _TpChannelClass TpChannelClass;
> 
> TpProxy and TpDBusDaemon have a similarly circular relationship, with
> TpDBusDaemon typedef'd in proxy.h to resolve it.

Fixed.

> The new API needs adding to docs/reference/*sections.txt, which you'd have
> spotted if you'd run `make check` with gtk-doc enabled (please do this when
> writing patches for telepathy-glib). You'd also have spotted that t_c_c_c_f's
> doc-comment is labelled as if for t_c_e_c_f.

Fixed.

> > +  g_return_val_if_fail (g_simple_async_result_is_valid (result,
> > +        G_OBJECT (self), tp_connection_create_channel_finish), NULL);
> 
> The tag is conventionally the _async function, not the _finish function (I know
> TpAccount[Manager] uses the finish function in some places).

Fixed.

> > ... with the
> > + * %TP_CHANNEL_FEATURE_CORE feature ready on it.
> 
> Shouldn't these functions take an array of features as an optional argument? I
> expect it to be extremely common to want GROUP, for a start.

Fixed.

(In reply to comment #6)
> Forgot to review the test:
> 
> I'd appreciate it if you used GTest (setup/teardown functions) for new tests
> like this one.

Fixed.

> > +  g_message (G_STRFUNC);
> 
> Please get rid of these before merging.

Fixed.

> When asserting that error != NULL, please assert the actual error with
> g_assert_error (error, TP_ERRORS, TP_ERROR_NOT_ENOUGH_CHEESE) or whatever, if
> it's at all predictable.

Fixed. Although in test_ensure_channel_fail, where I think it should return InvalidHandle it returns NotAvailable. I suppose this the CM's fault.

> When asserting that error == NULL, please use g_assert_no_error (of which
> test_assert_no_error is a semi-deprecated version - patches to clean up all the
> uses of that macro in existing code would be welcome, but are out of scope here

Fixed. I'd meant to clean this up in copied code, but forgot.

I added test coverage for requesting features, but it seems that the CHAT_STATES feature is never prepared on the channels (although I can see it being requested in the debug output). Not sure if this a limitation in the CM, or a bug in tp-glib (insufficient debugging output). Looking for advice here.

Now that we have TpAccountChannelRequest and the new ContactList API is coming, do we still need this?
I suspect we don't.

(In reply to comment #9)
> Now that we have TpAccountChannelRequest and the new ContactList API is coming,
> do we still need this?
> I suspect we don't.

I'm inclined to agree.