Bug 33901

Summary: add tp_capabilities_supports_room_list()
Product: Telepathy Reporter: Guillaume Desmottes <guillaume.desmottes>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/support-room-list-33901
Whiteboard: review+ with typo fixes.
i915 platform: i915 features:

Description Guillaume Desmottes 2011-02-04 05:19:29 UTC
It would be handy to have API to easily check if RoomList is supported or not.
Comment 2 Will Thompson 2011-02-09 04:28:23 UTC
You should also return a list of known servers, by scanning all the channel classes for requests with Server in Fixed. I don't know if Gabble implements it, but it sure should.

Assuming Gabble does this, then we can say something like “if with_server is set to %TRUE, but known_servers is empty, requests which do not specify Server can be expected to fail”. This is only applicable when calling this function on the caps from a running connection in Gabble's case I guess.

+      for (j = 0; allowed_properties[j] != NULL; j++)
+        {
+          if (!tp_strdiff (allowed_properties[j],
+                TP_PROP_CHANNEL_TYPE_ROOM_LIST_SERVER))
+            server = TRUE;
+        }

This is secret code for:

       server = tp_strv_contains (allowed_properties, TP_PROP_CHANNEL_TYPE_ROOM_LIST_SERVER);

+ * Return whether this protocol or connection can perform rooms
+ * listing.

“Returns whether or not this protocol or connection supports listing rooms.”


+ * Return whether this protocol or connection can perform rooms
+ * listing.
+ *
+ * Returns: %TRUE if a channel request containing RoomList as ChannelType,
+ * HandleTypeNone as TargetHandleType can be expected to work,
+ * %FALSE otherwise.

I actually think these should be swapped around. The body should say something like:

  Discovers whether this protocol or connection supports listing rooms. Specifically, if this function returns %TRUE, a room list channel can be requested as follows:

  |[ ... code for requesting a channel ... ]|

  If with_server is set to %TRUE, a list of rooms on a particular server can be requested as follows:

  |[ ... ]|
Comment 3 Will Thompson 2011-02-09 04:33:06 UTC
Or, thinking aloud, maybe we should have:

enum {
  ROOM_LISTING_SUPPORTED = 1,
  ROOM_LISTING_SERVER_SUPPORTED = 2,
  ROOM_LISTING_SERVER_REQUIRED = 4
} RoomListingSupportFlags;

as the return value, rather than a proliferation of non-self-documenting booleans? I'm not sure this helps much.
Comment 4 Guillaume Desmottes 2011-02-09 06:05:52 UTC
(In reply to comment #2)
> You should also return a list of known servers, by scanning all the channel
> classes for requests with Server in Fixed. I don't know if Gabble implements
> it, but it sure should.

I tried implementing this in Gabble (bug #34071) but I'm not sure that's
actually possible. :\

> +      for (j = 0; allowed_properties[j] != NULL; j++)
> +        {
> +          if (!tp_strdiff (allowed_properties[j],
> +                TP_PROP_CHANNEL_TYPE_ROOM_LIST_SERVER))
> +            server = TRUE;
> +        }
> 
> This is secret code for:
> 
>        server = tp_strv_contains (allowed_properties,
> TP_PROP_CHANNEL_TYPE_ROOM_LIST_SERVER);

Oh nice, changed.

> + * Return whether this protocol or connection can perform rooms
> + * listing.
> 
> “Returns whether or not this protocol or connection supports listing rooms.”

fixed.

> 
> + * Return whether this protocol or connection can perform rooms
> + * listing.
> + *
> + * Returns: %TRUE if a channel request containing RoomList as ChannelType,
> + * HandleTypeNone as TargetHandleType can be expected to work,
> + * %FALSE otherwise.
> 
> I actually think these should be swapped around. The body should say something
> like:
> 
>   Discovers whether this protocol or connection supports listing rooms.
> Specifically, if this function returns %TRUE, a room list channel can be
> requested as follows:
> 
>   |[ ... code for requesting a channel ... ]|
> 
>   If with_server is set to %TRUE, a list of rooms on a particular server can be
> requested as follows:
> 
>   |[ ... ]|

done.

(In reply to comment #3)
> Or, thinking aloud, maybe we should have:
> 
> enum {
>   ROOM_LISTING_SUPPORTED = 1,
>   ROOM_LISTING_SERVER_SUPPORTED = 2,
>   ROOM_LISTING_SERVER_REQUIRED = 4
> } RoomListingSupportFlags;
> 
> as the return value, rather than a proliferation of non-self-documenting
> booleans? I'm not sure this helps much.

This could be nice, but shouldn't ROOM_LISTING_SUPPORTED be "included" in
ROOM_LISTING_SERVER_SUPPORTED and ROOM_LISTING_SERVER_REQUIRED?
Comment 5 Guillaume Desmottes 2011-02-14 04:04:37 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > You should also return a list of known servers, by scanning all the channel
> > classes for requests with Server in Fixed. I don't know if Gabble implements
> > it, but it sure should.
> 
> I tried implementing this in Gabble (bug #34071) but I'm not sure that's
> actually possible. :\

According the discussion on bug #34071 I suspect this should be done as a separated API.
Comment 6 Will Thompson 2011-02-22 02:54:32 UTC
+ *     TP_PROP_CHANNEL_TYPE_ROOM_LIST_SERVER: G_TYPE_STRING,

Example no compile.

+ * /\* Same code as above but with request defined using: *\/

Does that work?

+ * tp_account_channel_request_create_channel_async (req, NULL, NULL,

Shouldn't the example use create_and_handle_channel probably? This is a nit-pick though.

+ * If with_server is set to %TRUE, a list of rooms on a particular server can be

@with_server.
Comment 7 Guillaume Desmottes 2011-02-22 03:09:29 UTC
(In reply to comment #6)
> + *     TP_PROP_CHANNEL_TYPE_ROOM_LIST_SERVER: G_TYPE_STRING,
> 
> Example no compile.

Oops fixed (amend)

> + * /\* Same code as above but with request defined using: *\/
> 
> Does that work?

Well, the conference Server doesn't exist (I took it from the XEP); I can put
a real one if you prefer.

> + * tp_account_channel_request_create_channel_async (req, NULL, NULL,
> 
> Shouldn't the example use create_and_handle_channel probably? This is a
> nit-pick though.

You're right; fixed.

> + * If with_server is set to %TRUE, a list of rooms on a particular server can
> be
> 
> @with_server.

fixed.
Comment 8 Will Thompson 2011-02-22 03:30:02 UTC
gogogo.
Comment 9 Guillaume Desmottes 2011-02-22 03:53:26 UTC
Merged to master; will be in 0.13.14

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.