[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 #9 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> ---
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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.