[Bug 27642] review for some refactoring plus chatroom logging fix in TPL

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-29 07:15:44 PDT ---
Reviewing the part of tpl-channel-refactored that comes after smcv/ka-misc:

For this branch
===============

> +  g_assert ((TPL_IS_CONTACT (remote_contact) && TPL_STR_EMPTY (chatroom_id)) ||
> +      (!TPL_IS_CONTACT (remote_contact) && !TPL_STR_EMPTY (chatroom_id)));
...
> +  /* if chatroom_id is empty/NULL, it's not a chatroom */
> +  return !TPL_STR_EMPTY (chatroom_id);

If chatroom_id is a TargetID of type TP_HANDLE_TYPE_ROOM, then I'd prefer NULL
to be the value for "not a chatroom", with "" a legal value for a chatroom ID
(telepathy-spec doesn't forbid it).

> +  return (tpl_contact_get_contact_type (sender) == TPL_CONTACT_GROUP ||
> +      tpl_contact_get_contact_type (receiver) == TPL_CONTACT_GROUP);

What would it mean to have a message sent by a group? How would that be
represented in Telepathy? Unless the API is really screwed up, I don't think
this can happen?

> -        tpl_channel_text_set_my_contact (tpl_text, contacts[0]);
> +        tpl_channel_text_set_my_contact (tpl_text,
> +            tpl_contact_from_tp_contact (contacts[0]));

Doesn't this leak a TplContact?

> TplChannelText: make static most private methods

Yes please! Please cherry-pick this if the rest of the branch takes a while.

> +      /* 1-1 chat, the user is the reiceiver */

"receiver"

Later
=====

>    if (!tpl_channel_text_is_chatroom (tpl_text))
>      tpl_log_entry_text_set_chat_id (log, tpl_contact_get_identifier (
>            tpl_contact_sender));
>    else
> +    tpl_log_entry_text_set_chat_id (log,
> +        tpl_channel_text_get_chatroom_id (tpl_text));

It concerns me a bit that TplLogEntryText is throwing away a bit (literally) of
information - whether the "chat ID" is a contact or a room. 

Contact IDs and room IDs are not necessarily distinguishable, or even
different. In XMPP, they have the same syntax (they're just JIDs).

I would like the logger to not fail if/when we discover a protocol where
usernames and chatroom names can overlap (e.g. a user "smcv" can be in a
chatroom also called "smcv"). I'm fairly sure such a protocol exists, somewhere
in the world.

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