Bug 49505

Summary: somewhat high-level API to add client channel filters
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-channel-filter-49505
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 30422    
Attachments: proposed patch

Description Simon McVittie 2012-05-04 11:24:37 UTC
Created attachment 61042 [details] [review]
proposed patch

Continuing our campaign against GHashTables masquerading as high-level API, here is something that's not so good:

-   * |[
-   * tp_base_client_take_handler_filter (handler, tp_asv_new (
-   *               TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING, TP_IFACE_CHANNEL_TYPE_FILE_TRANSFER,
-   *               TP_PROP_CHANNEL_TARGET_HANDLE_TYPE, G_TYPE_UINT, TP_HANDLE_TYPE_CONTACT,
-   *               TP_PROP_CHANNEL_REQUESTED, G_TYPE_BOOLEAN, FALSE,
-   *               TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA_SERVICE_NAME, G_TYPE_STRING, "service.name",
-   *               NULL));

and my proposal to replace it:

+ * filter = tp_channel_filter_new_for_file_transfer ("service.name");
+ * tp_channel_filter_require_locally_requested (filter, FALSE);
+ * tp_base_client_take_handler_filter_object (client, filter);

TpChannelFilter is a GObject which encapsulates an a{sv} of properties, resembling Jonny's TpFutureAccount on Bug #47100, and my new convenience API on TpAccountChannelRequest on Bug #48780.
Comment 1 Simon McVittie 2012-05-09 09:37:07 UTC
Comment on attachment 61042 [details] [review]
proposed patch

Review of attachment 61042 [details] [review]:
-----------------------------------------------------------------

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".
Comment 2 Simon McVittie 2013-11-11 16:45:02 UTC
(In reply to comment #1)
> 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.

Anyone?
Comment 3 Guillaume Desmottes 2013-12-27 14:26:53 UTC
Comment on attachment 61042 [details] [review]
proposed patch

Review of attachment 61042 [details] [review]:
-----------------------------------------------------------------

::: 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

agreed.

::: telepathy-glib/channel-filter.c
@@ +331,5 @@
> + * been passed to a #TpBaseClient.
> + */
> +void
> +tp_channel_filter_require_channel_type (TpChannelFilter *self,
> +    const gchar *channel_type)

Wouldn't it be enough to drop this from the public API and enforce users to create the specialised version of _new_*()? At the moment we could so something like:
  f = tp_channel_filter_new_for_text_chatrooms ();
  tp_channel_filter_require_channel_type (f, TP_IFACE_CHANNEL_TYPE_CALL);
which is pretty weird.

If we go this way, tp_channel_filter_new () could be renamed new_for_all() or something.

Another option could be to return_if_fail () if there is already a channel type on the filter.
Comment 4 Guillaume Desmottes 2014-03-03 14:00:24 UTC
Is that still something we want for 1.0 ? We already replaced TpAsv by GVariant in 'next'.
Comment 5 Simon McVittie 2014-03-03 16:32:48 UTC
(In reply to comment #4)
> Is that still something we want for 1.0 ? We already replaced TpAsv by
> GVariant in 'next'.

Well, there are two parts to what I originally wrote:

> Continuing our campaign against GHashTables masquerading as high-level API,
> here is something that's not so good:
> 
> -   * |[
> -   * tp_base_client_take_handler_filter (handler, tp_asv_new (
> -   *               TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING,
> TP_IFACE_CHANNEL_TYPE_FILE_TRANSFER,
> -   *               TP_PROP_CHANNEL_TARGET_HANDLE_TYPE, G_TYPE_UINT,
> TP_HANDLE_TYPE_CONTACT,
> -   *               TP_PROP_CHANNEL_REQUESTED, G_TYPE_BOOLEAN, FALSE,
> -   *              
> TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA_SERVICE_NAME,
> G_TYPE_STRING, "service.name",
> -   *               NULL));
> 
> and my proposal to replace it:
> 
> + * filter = tp_channel_filter_new_for_file_transfer ("service.name");
> + * tp_channel_filter_require_locally_requested (filter, FALSE);
> + * tp_base_client_take_handler_filter_object (client, filter);

One is to use GVariant instead of GHashTable for the low-level API that requires you to speak fluent D-Bus. Yes, done.

The other is to be able to say "I handle file transfers whose service name is service.name" without using telepathy-glib-dbus.h and coupling your source code to the specifics of the D-Bus API. We don't have that.

In practice, this means nobody can write a Telepathy Handler (or Observer or Approver) without having our D-Bus API in their source code (and keeping up with, e.g., moving from FileTransfer1 to FileTransfer2 if we redo how hashing works post-1.0).
Comment 6 Simon McVittie 2014-03-03 16:34:35 UTC
(In reply to comment #3)
> At the moment we could so
> something like:
>   f = tp_channel_filter_new_for_text_chatrooms ();
>   tp_channel_filter_require_channel_type (f, TP_IFACE_CHANNEL_TYPE_CALL);
> which is pretty weird.
> 
> If we go this way, tp_channel_filter_new () could be renamed new_for_all()
> or something.

new_for_all_types() perhaps, yeah.

> Another option could be to return_if_fail () if there is already a channel
> type on the filter.

Also a plausible route to take. I think I slightly prefer it this way.
Comment 7 Guillaume Desmottes 2014-03-06 11:38:01 UTC
Hum actually this is going to be pretty similar to TpAccountChannelRequest's API: high level API to describe channel classes and their properties.

I'm wondering if there is room for some API factorisation here. In both cases, we want to be able to 'describe' any channel type and, depending on the case:

- TpAccountChannelRequest: need (potentially) API for any requestable property.
- TpBaseClient: need API for any immutable property

Do you think we could have a common object used by both or that would be crack?
Comment 8 Simon McVittie 2014-03-06 12:33:23 UTC
(In reply to comment #7)
> Do you think we could have a common object used by both or that would be
> crack?

The syntax is the same (map from property to value), but I think the uses/context/semantics are sufficiently different that they shouldn't be the same C type.

Concretely, client channel filters are rather broad and vague (matching individual contacts makes very little sense), whereas channel requests usually do need to match an individual contact.
Comment 9 Guillaume Desmottes 2014-03-12 11:54:41 UTC
I rebased and refreshed your patch: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-channel-filter-49505

(In reply to comment #1)
> ::: 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.

But we now have:

void tp_base_client_add_approver_filter (TpBaseClient *self,
    GVariant *filter);

Maybe this one could be renamed add_approver_filter_variant() so {add,take}_filter() can be used for the higher level API.

> ::: 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).

Fixed using tp_channel_filter_new_for_text_chats().

> @@ +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.

Fixed using tp_channel_filter_new_for_file_transfer().

> ::: 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.

Fixed.

> ::: 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.

No strong opinion here.
 
> @@ +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 :-(

Fixed.

> @@ +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.

I think it makes sense to pass the EntityType as argument here.

> @@ +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.

Agreed; removed.

> @@ +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".

I think that's fine.
Comment 10 Guillaume Desmottes 2014-03-12 11:59:59 UTC
(In reply to comment #6)
> (In reply to comment #3)
> > At the moment we could so
> > something like:
> >   f = tp_channel_filter_new_for_text_chatrooms ();
> >   tp_channel_filter_require_channel_type (f, TP_IFACE_CHANNEL_TYPE_CALL);
> > which is pretty weird.
> > 
> > If we go this way, tp_channel_filter_new () could be renamed new_for_all()
> > or something.
> 
> new_for_all_types() perhaps, yeah.
> 
> > Another option could be to return_if_fail () if there is already a channel
> > type on the filter.
> 
> Also a plausible route to take. I think I slightly prefer it this way.

I think I'd prefer to have tp_channel_filter_new_for_all_types() and remove tp_channel_filter_require_channel_type() completely.

Rationale: this a very high level API and the channel type is the key part of this API's naming scheme. If you have to use another channel type then either it's common enough to be added to the high level API or you should use the GVariant based lower API.
And that makes clear that the channel filter is bound to a channel type as there is no way to change it once created.
Comment 11 Simon McVittie 2014-03-12 13:02:57 UTC
(In reply to comment #9)
> I rebased and refreshed your patch:
> http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-channel-filter-49505

(Not reviewed)

> > This naming is obviously terrible; in next we should s/_object// and get rid
> > of the hash-table-based versions.
> 
> But we now have:
> 
> void tp_base_client_add_approver_filter (TpBaseClient *self,
>     GVariant *filter);
> 
> Maybe this one could be renamed add_approver_filter_variant() so
> {add,take}_filter() can be used for the higher level API.

Yes, makes sense!

> > > + * 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.
> 
> Agreed; removed.

Oh, I'd intended "there's no point in being speculatively general" as rationale for having that text instead of taking a mostly-pointless TpEntityType parameter :-)
Comment 12 Simon McVittie 2014-03-12 13:03:58 UTC
(In reply to comment #10)
> Rationale: this a very high level API and the channel type is the key part
> of this API's naming scheme. If you have to use another channel type then
> either it's common enough to be added to the high level API or you should
> use the GVariant based lower API.

You've convinced me. Please do it the way you said.
Comment 13 Guillaume Desmottes 2014-03-12 15:34:18 UTC
(In reply to comment #11)
> > > This naming is obviously terrible; in next we should s/_object// and get rid
> > > of the hash-table-based versions.
> > 
> > But we now have:
> > 
> > void tp_base_client_add_approver_filter (TpBaseClient *self,
> >     GVariant *filter);
> > 
> > Maybe this one could be renamed add_approver_filter_variant() so
> > {add,take}_filter() can be used for the higher level API.
> 
> Yes, makes sense!

done.

> > > > + * 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.
> > 
> > Agreed; removed.
> 
> Oh, I'd intended "there's no point in being speculatively general" as
> rationale for having that text instead of taking a mostly-pointless
> TpEntityType parameter :-)

Ah ok; I brought it back. :)
Comment 14 Guillaume Desmottes 2014-03-12 16:32:12 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > Rationale: this a very high level API and the channel type is the key part
> > of this API's naming scheme. If you have to use another channel type then
> > either it's common enough to be added to the high level API or you should
> > use the GVariant based lower API.
> 
> You've convinced me. Please do it the way you said.

Done.

The branch has been updated.
Comment 15 Simon McVittie 2014-03-18 11:51:47 UTC
This looks good for next.
Comment 16 Guillaume Desmottes 2014-03-18 13:32:02 UTC
Merged to next.

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.