[Bug 49505] somewhat high-level API to add client channel filters

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=49505

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-05-09 09:37:07 PDT ---
Comment on attachment 61042
  --> https://bugs.freedesktop.org/attachment.cgi?id=61042
proposed patch

Review of attachment 61042:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=49505&attachment=61042)
-----------------------------------------------------------------

There are some trivia here that I spotted while looking at it again, but I'd
like to hear someone else's opinion on the API before I go revising anything.

::: docs/reference/telepathy-glib-sections.txt
@@ +5629,5 @@
>  <TITLE>base-client</TITLE>
>  TpBaseClient
>  TpBaseClientClass
> +tp_base_client_add_observer_filter_object
> +tp_base_client_take_observer_filter_object

This naming is obviously terrible; in next we should s/_object// and get rid of
the hash-table-based versions.

::: telepathy-glib/base-client.c
@@ +387,5 @@
> + * For instance, this Client:
> + *
> + * |[
> + * filter = tp_channel_filter_new ();
> + * tp_channel_filter_require_type_is_text (filter);

This method doesn't actually exist any more - I should revise this example
(perhaps just to use require_channel_type, even though that's approximately
never what you want).

@@ +393,5 @@
> + * g_object_unref (filter);
> + *
> + * filter = tp_channel_filter_new ();
> + * tp_channel_filter_require_target_is_contact (filter);
> + * tp_channel_filter_require_type_is_call (filter);

Neither does this.

::: telepathy-glib/base-client.h
@@ +109,4 @@
>  void tp_base_client_add_observer_filter (TpBaseClient *self,
>      GHashTable *filter);
>  
> +_TP_AVAILABLE_IN_0_20

These should say _IN_UNRELEASED, but that didn't exist when I started this
branch.

::: telepathy-glib/channel-filter.c
@@ +167,5 @@
> +      NULL);
> +}
> +
> +/**
> + * tp_channel_filter_new_for_text_chats:

This is an odd asymmetry with new_for_calls (which takes a handle type), but
perhaps Text is sufficiently common that it deserves this shortcut? I'm not
sure.

@@ +173,5 @@
> + * Return a channel filter that matches 1-1 text chats,
> + * such as #TpTextChannels carrying private messages or SMSs.
> + *
> + * It is not necessary to call tp_channel_filter_require_target_is_room()
> + * on the returned filter.

This should say require_target_is_contact :-(

@@ +259,5 @@
> + * Narrow @self to require that the channel communicates with an
> + * ad-hoc, unnamed group of contacts.
> + *
> + * For instance, among other things, this filter would match #TpCallChannels
> + * for conference calls in cellular telephony.

... I think it would? (If it doesn't it should, anyway.)

@@ +360,5 @@
> + * tp_channel_filter_require_locally_requested (filter, FALSE);
> + * ]|
> + */
> +TpChannelFilter *
> +tp_channel_filter_new_for_stream_tubes (const gchar *service)

I was vaguely going for "perhaps it makes sense to match RFB tubes both in
rooms and from contacts?" but I'm not so sure - maybe this should just take a
TpHandleType like new_for_calls does. Opinions? Should you be able to match all
Tubes of a particular service with a single filter, or does encouraging people
to add handle-type-specific filters make more sense anyway?

D-Bus tubes have the same issue.

@@ +418,5 @@
> + * a #TpContact, as used by #TpFileTransferChannel.
> + *
> + * At the time of writing, file transfers with other types of target
> + * (like chatrooms) have not been implemented. If they are, they will
> + * use a different filter.

I'm not sure that FTs for anyone other than contacts make sense anyway, so
there's no point in being speculatively general here.

@@ +422,5 @@
> + * use a different filter.
> + *
> + * Using this method will match both incoming and outgoing file transfers.
> + * If you only want to match one direction, use
> + * tp_channel_filter_require_locally_requested() to select it.

I'd have had a boolean parameter for "locally/remotely initiated", but that
would have ruled out "I want to handle|approve|observe every file transfer".

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.