Bug 49505

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

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

Is that still something we want for 1.0 ? We already replaced TpAsv by GVariant in 'next'.

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

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

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?

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

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.

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

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

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

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

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

This looks good for next.

Merged to next.