Bug 30338 (TpRoomList)

Proposed API:

TpRoomList * tp_room_list_new (TpConnection *conn,
    const gchar *object_path,
    const GHashTable *immutable_properties,
    GError **error);

accessors
---------

const gchar * tp_room_list_get_server (TpRoomList *self);
gboolean tp_room_list_get_listing (TpRoomList *self);

methods
-------

void tp_room_list_start_listing_async (TpRoomList *self,
    GAsyncReadyCallback callback,
    gpointer user_data);

gboolean tp_room_list_start_listing_finish (TpRoomList *self,
    GAsyncResult *result,
    GError **error);

void tp_room_list_stop_listing_async (TpRoomList *self,
    GAsyncReadyCallback callback,
    gpointer user_data);

gboolean tp_room_list_stop_listing_finish (TpRoomList *self,
    GAsyncResult *result,
    GError **error);

Signals
-------

'listing' changes are announced using "notify::listing"

got-rooms (GPtrArray *rooms) with rooms containing TP_STRUCT_TYPE_ROOM_INFO
Or maybe we should have a small TpRoomInfo object?

Hum, we'll probably need a TP_ROOM_LIST_FEATURE_LISTING feature to fetch the Listing property.

I'd personally give this low priority: if you want to do a straightforward port from Empathy you're welcome to do so, but text and tubes channels are more important IMO.

(In reply to comment #1)
> TpRoomList * tp_room_list_new (TpConnection *conn,

Perhaps TpRoomListChannel. I'm not sure whether we should enforce a "Channel" suffix for all tp-glib TpChannel subclasses.

(Precedent: in telepathy-qt4, I initially only added Channel where the channel type didn't make sense as a noun - so we had Tp::TextChannel and Tp::StreamedMediaChannel, but Tp::RoomList and Tp::StreamTube - but Andre decided it was better to add Channel to all of them, consistently, and that's now the convention.)

> gboolean tp_room_list_get_listing (TpRoomList *self);

This is mutable, so it'll need a Feature if you want it.

> void tp_room_list_start_listing_async (TpRoomList *self,
>     GAsyncReadyCallback callback,
>     gpointer user_data);
> 
> gboolean tp_room_list_start_listing_finish (TpRoomList *self,
>     GAsyncResult *result,
>     GError **error);

We should document in the spec whether ListRooms raises an error if this channel is already in the middle of listing rooms. I think we should define Create semantics as the only ones that make sense and delete the possibility of the channel being a per-connection singleton, tbh (i.e. make it look like ContactSearch) - does Empathy actually cope with being given a room list that's already listing?

> got-rooms (GPtrArray *rooms) with rooms containing TP_STRUCT_TYPE_ROOM_INFO
> Or maybe we should have a small TpRoomInfo object?

A TpRoomInfo struct or small-GObject, please:

    TpRoomInfo *tp_room_info_new (GValueArray *dbus_glib_struct);
or perhaps
    TpRoomInfo *tp_room_info_new (TpHandle handle, const gchar *ctype, GHashTable *asv);

and in either case

    TpHandle tp_room_info_get_handle (TpRoomInfo *self);
    const gchar *tp_room_info_get_channel_type (TpRoomInfo *self);
    const gchar *tp_room_info_get_handle_name (TpRoomInfo *self);
    const gchar *tp_room_info_get_name (TpRoomInfo *self);
    const gchar *tp_room_info_get_description (TpRoomInfo *self);
    const gchar *tp_room_info_get_subject (TpRoomInfo *self);
    guint tp_room_info_get_members (TpRoomInfo *self, gboolean *known);
    /* "password" on D-Bus */
    gboolean tp_room_info_get_requires_password (TpRoomInfo *self, gboolean *known);
    gboolean tp_room_info_get_invite_only (TpRoomInfo *self, gboolean *known);
    const gchar *tp_room_info_get_room_id (TpRoomInfo *self);
    const gchar *tp_room_info_get_server (TpRoomInfo *self);

All the string accessors would have tp_asv_get_string() semantics. The int and boolean accessors would have tp_asv_get_uint32 semantics, with valid mapped to known.

(In reply to comment #3)
> I'd personally give this low priority: if you want to do a straightforward port
> from Empathy you're welcome to do so, but text and tubes channels are more
> important IMO.

Sure, I mostly opened the bug for completion purpose.
Oth, I'd really like to move forward on bug #29973 and we can't really merge it before we have a least on TpChannel subclass to proof use it.
As TpStreamTube and TpTextChannel are not that trivial maybe it could be this one which is less controversial? Oth, that's not true anymore if we want to change the spec first.

> (In reply to comment #1)
> > TpRoomList * tp_room_list_new (TpConnection *conn,
> 
> Perhaps TpRoomListChannel. I'm not sure whether we should enforce a "Channel"
> suffix for all tp-glib TpChannel subclasses.
> 
> (Precedent: in telepathy-qt4, I initially only added Channel where the channel
> type didn't make sense as a noun - so we had Tp::TextChannel and
> Tp::StreamedMediaChannel, but Tp::RoomList and Tp::StreamTube - but Andre
> decided it was better to add Channel to all of them, consistently, and that's
> now the convention.)

I don't really care but staying coherent and "sync" with tp-qt4 could be good enough arguments to always have "Channel" in the name.

> > gboolean tp_room_list_get_listing (TpRoomList *self);
> 
> This is mutable, so it'll need a Feature if you want it.

Yep, see the other comment :)

> > void tp_room_list_start_listing_async (TpRoomList *self,
> >     GAsyncReadyCallback callback,
> >     gpointer user_data);
> > 
> > gboolean tp_room_list_start_listing_finish (TpRoomList *self,
> >     GAsyncResult *result,
> >     GError **error);
> 
> We should document in the spec whether ListRooms raises an error if this
> channel is already in the middle of listing rooms. I think we should define
> Create semantics as the only ones that make sense and delete the possibility of
> the channel being a per-connection singleton, tbh (i.e. make it look like
> ContactSearch) - does Empathy actually cope with being given a room list that's
> already listing?

Yeah that makes sense. Empathy does check if the channel is already listing but I think this change would be sane.

> > got-rooms (GPtrArray *rooms) with rooms containing TP_STRUCT_TYPE_ROOM_INFO
> > Or maybe we should have a small TpRoomInfo object?
> 
> A TpRoomInfo struct or small-GObject, please:
> 
>     TpRoomInfo *tp_room_info_new (GValueArray *dbus_glib_struct);
> or perhaps
>     TpRoomInfo *tp_room_info_new (TpHandle handle, const gchar *ctype,
> GHashTable *asv);
> 
> and in either case
> 
>     TpHandle tp_room_info_get_handle (TpRoomInfo *self);
>     const gchar *tp_room_info_get_channel_type (TpRoomInfo *self);
>     const gchar *tp_room_info_get_handle_name (TpRoomInfo *self);
>     const gchar *tp_room_info_get_name (TpRoomInfo *self);
>     const gchar *tp_room_info_get_description (TpRoomInfo *self);
>     const gchar *tp_room_info_get_subject (TpRoomInfo *self);
>     guint tp_room_info_get_members (TpRoomInfo *self, gboolean *known);
>     /* "password" on D-Bus */
>     gboolean tp_room_info_get_requires_password (TpRoomInfo *self, gboolean
> *known);
>     gboolean tp_room_info_get_invite_only (TpRoomInfo *self, gboolean *known);
>     const gchar *tp_room_info_get_room_id (TpRoomInfo *self);
>     const gchar *tp_room_info_get_server (TpRoomInfo *self);
> 
> All the string accessors would have tp_asv_get_string() semantics. The int and
> boolean accessors would have tp_asv_get_uint32 semantics, with valid mapped to
> known.

I'd vote for a small object because from my experience each time we have this kind of thing as a struct we end up at some point saying "shit it would easier to use if it was a proper object".

http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=tp-room-list-30338

Two open questions:

a) I didn't include any API to stop the listing. What about calling StopListing() automatically when the proxy is invalidated? The requester should close the channel anyway once it's done with it.

b) Should start_listing_async() be renamed to list_async() or something, complete when the list operation is done (ListingRooms goes back to FALSE) and return the  list of TpRoomInfo found. User will still have the got-rooms signal to display rooms while they are found, but having a list API to returning any result looks a bit weird to me.

(In reply to comment #5)
> http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=tp-room-list-30338

A loooot of new code late on a Friday makes me think I might've missed something but it seems to look fine from here...

> a) I didn't include any API to stop the listing. What about calling
> StopListing() automatically when the proxy is invalidated? The requester should
> close the channel anyway once it's done with it.

That seems reasonable to me.

> b) Should start_listing_async() be renamed to list_async() or something,
> complete when the list operation is done (ListingRooms goes back to FALSE) and
> return the  list of TpRoomInfo found. User will still have the got-rooms signal
> to display rooms while they are found, but having a list API to returning any
> result looks a bit weird to me.

Good point. I would also be fine if you did this list_async() rename.

Any further preferences, Simon?

(In reply to comment #6)
> > b) Should start_listing_async() be renamed to list_async() or something,
> > complete when the list operation is done (ListingRooms goes back to FALSE) and
> > return the  list of TpRoomInfo found. User will still have the got-rooms signal
> > to display rooms while they are found, but having a list API to returning any
> > result looks a bit weird to me.
> 
> Good point. I would also be fine if you did this list_async() rename.

I'm vaguely against this; the reason why the RoomList API is so odd is that the list can be pretty huge (IRC, or XMPP on a big server that is everyone's default chatroom host), and having list_async() return the complete list means we have to hold everything in a temporary list (GQueue?) inside telepathy-glib for the duration of the operation - even if, for instance, the caller is performing client-side filtering on got-rooms and throwing away most of them.

I agree that start_listing_async() is relatively useless (it returning successfully is basically meaningless), but renaming to list_async() without returning the GList<TpRoomInfo> would indeed seem pretty strange.

Having two ways to access exactly the same information with different timing also seems odd to me. At the moment, got-rooms is the only way, which seems good.

On the other hand, "most" API users will just be turning each room into a row in a GtkListModel as it arrives...

I wonder whether we could do this differently by having the methods pretend to be synchronous? Something like this (assume the usual C boilerplate and TpRoomList* first argument):

    void start()
    signal room-list-failed(GError)  /* or even use invalidated? */
    signal got-rooms(...)
    void stop()

stop being void is a bit odd, but on the other hand, why would you ever want to know how long it took? And, if you fail to stop it, what will you ever do about it? (Destroy() falling back to Close() if unimplemented? ... but stop() could do that automatically, perhaps.)

(In reply to comment #7)
> I'm vaguely against this; the reason why the RoomList API is so odd is that the
> list can be pretty huge (IRC, or XMPP on a big server that is everyone's
> default chatroom host), and having list_async() return the complete list means
> we have to hold everything in a temporary list (GQueue?) inside telepathy-glib
> for the duration of the operation - even if, for instance, the caller is
> performing client-side filtering on got-rooms and throwing away most of them.

Fair enough.

> I wonder whether we could do this differently by having the methods pretend to
> be synchronous? Something like this (assume the usual C boilerplate and
> TpRoomList* first argument):
> 
>     void start()
>     signal room-list-failed(GError)  /* or even use invalidated? */
>     signal got-rooms(...)
>     void stop()
> 
> stop being void is a bit odd, but on the other hand, why would you ever want to
> know how long it took? And, if you fail to stop it, what will you ever do about
> it? (Destroy() falling back to Close() if unimplemented? ... but stop() could
> do that automatically, perhaps.)

I like this approach. I think I'd make call Destroy/Close if something go wrong (start/stop failing as there is no much you can do in that case anyway) so invalidated will be fired.

But do we really need stop()? User can just close/destroy the channel once he's the done with it.

I've been looking at channel creation APIs a bit. I think it makes sense for TpRoomList to have a tp_room_list_channel_new_async(), similar to the one in TpContactSearch, rather than going via TpAccountChannelRequest - the two channel types are pretty similar.

(In reply to comment #1)
> TpRoomList * tp_room_list_new (TpConnection *conn,
>     const gchar *object_path,
>     const GHashTable *immutable_properties,
>     GError **error);

Does this need to be public at all?

(In reply to comment #9)
> I've been looking at channel creation APIs a bit. I think it makes sense for
> TpRoomList to have a tp_room_list_channel_new_async(), similar to the one in
> TpContactSearch, rather than going via TpAccountChannelRequest - the two
> channel types are pretty similar.

You're right; I totally forgot about this contact search API. I'll do that.

(In reply to comment #10)
> (In reply to comment #1)
> > TpRoomList * tp_room_list_new (TpConnection *conn,
> >     const gchar *object_path,
> >     const GHashTable *immutable_properties,
> >     GError **error);
> 
> Does this need to be public at all?

No; I meant to remove it but got distracted by a bee and forgot.

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #1)
> > > TpRoomList * tp_room_list_new (TpConnection *conn,
> > >     const gchar *object_path,
> > >     const GHashTable *immutable_properties,
> > >     GError **error);
> > 
> > Does this need to be public at all?
> 
> No; I meant to remove it but got distracted by a bee and forgot.

Oh actually I did in the implementation; see http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=tp-room-list-30338

(In reply to comment #9)
> I've been looking at channel creation APIs a bit. I think it makes sense for
> TpRoomList to have a tp_room_list_channel_new_async(), similar to the one in
> TpContactSearch, rather than going via TpAccountChannelRequest - the two
> channel types are pretty similar.

I think it doesn't make sense to hook TpRoomListChannel into the client factory code then.

Here we go: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=tp-room-list-30338

I implemented the 'sync' API mentioned above and the object is no longer a TpChannel.

I didn't include stop() as user can just destroy TpRoomList once it's done with it.

TpRoomInfo:

 - SECTION doc refers to TpRoomListChannel which does not exist

 - I know this is common, but GObjectClass guarantees that finalize/dispose/etc are non-NULL, they implement them explicitly to not having to check in every subclass.
+  void (*chain_up) (GObject *) =
+      ((GObjectClass *) tp_room_info_parent_class)->finalize;

 - tp_room_info_get_members() I was surprised it return a number, I was expecting getting a list of TpContact from the func name... maybe _get_n_members()? or _get_members_count()?

 - Shouldn't it expose each getter as a property as well?

 - I'm not fan of self->priv->hash name, it does not tell what it contains. 'asv' would already describe it better... or "parameters" maybe?



TpRoomList:

 - SECTION doc says it's a TpChannel subclass... it's not.

 - "got-rooms" signal emits only one room at a time, right? so no plurial needed.

 - tp_room_list_get_listing() --> _is_listing() ?

 - I think it should "keep its state". I mean that if you miss signals, you can't know the rooms listed, or the fail error... I would like a getter for those 2.




simple-conn.c:

 - chan = g_hash_table_lookup (self->priv->channels, GUINT_TO_POINTER (0));
WTF using 0 as key?? is that really on purpose?

(In reply to comment #15)
> TpRoomInfo:
> 
>  - SECTION doc refers to TpRoomListChannel which does not exist

fixed.

>  - I know this is common, but GObjectClass guarantees that finalize/dispose/etc
> are non-NULL, they implement them explicitly to not having to check in every
> subclass.
> +  void (*chain_up) (GObject *) =
> +      ((GObjectClass *) tp_room_info_parent_class)->finalize;

IIRC was advocating for some reason but I don't remember the details.

>  - tp_room_info_get_members() I was surprised it return a number, I was
> expecting getting a list of TpContact from the func name... maybe
> _get_n_members()? or _get_members_count()?

Renamed to get_members_count().

>  - Shouldn't it expose each getter as a property as well?

Not sure it's worth it as they are not 'construct'. The only advantages I see is to get all the 'props' in one g_object_get() call but I hate this as it's not safe at all.
 
>  - I'm not fan of self->priv->hash name, it does not tell what it contains.
> 'asv' would already describe it better... or "parameters" maybe?

Agreed. I renamed it 'info' as that's the name used in the spec. 
 
> TpRoomList:
> 
>  - SECTION doc says it's a TpChannel subclass... it's not.

fixed.

>  - "got-rooms" signal emits only one room at a time, right? so no plurial
> needed.

Good point; renamed.

>  - tp_room_list_get_listing() --> _is_listing() ?

I meant to change that but forgot; fixed.
 
>  - I think it should "keep its state". I mean that if you miss signals, you
> can't know the rooms listed, or the fail error... I would like a getter for
> those 2.

How can you miss it? They won't be fired until _star() is called so you are supposed to connect the signals before.

> simple-conn.c:
> 
>  - chan = g_hash_table_lookup (self->priv->channels, GUINT_TO_POINTER (0));
> WTF using 0 as key?? is that really on purpose?

Yeah, room list channel has None as HandleType so no Handle either.

(In reply to comment #16)
> (In reply to comment #15)
> >  - I think it should "keep its state". I mean that if you miss signals, you
> > can't know the rooms listed, or the fail error... I would like a getter for
> > those 2.
> 
> How can you miss it? They won't be fired until _star() is called so you are
> supposed to connect the signals before.

Was suggesting this for completeness of the API, but not really important indeed. And TpContactSearch does not do that neither. Ok forget me :)

> > simple-conn.c:
> > 
> >  - chan = g_hash_table_lookup (self->priv->channels, GUINT_TO_POINTER (0));
> > WTF using 0 as key?? is that really on purpose?
> 
> Yeah, room list channel has None as HandleType so no Handle either.

Hm, tbh I don't like this, it means it will fail badly if we add other channels with handle==0 like MSN-like MUCs, etc... Also the comment in the priv struct says /* TpHandle => reffed TpTestsTextChannelNull */ which is wrong now... tbh I would keep a separate TpTestsRoomListChan* field in priv struct and maybe rename channels to text_channels?

(In reply to comment #17)
> > > simple-conn.c:
> > > 
> > >  - chan = g_hash_table_lookup (self->priv->channels, GUINT_TO_POINTER (0));
> > > WTF using 0 as key?? is that really on purpose?
> > 
> > Yeah, room list channel has None as HandleType so no Handle either.
> 
> Hm, tbh I don't like this, it means it will fail badly if we add other channels
> with handle==0 like MSN-like MUCs, etc... Also the comment in the priv struct
> says /* TpHandle => reffed TpTestsTextChannelNull */ which is wrong now... tbh
> I would keep a separate TpTestsRoomListChan* field in priv struct and maybe
> rename channels to text_channels?

done.

Looks good now, +1

Thanks merged to master; will be in 0.19.0