[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

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |smcv is reviewing
         AssignedTo|telepathy-bugs at lists.freede |cosimo.alfarano at collabora.c
                   |sktop.org                   |o.uk

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-29 06:56:17 PDT ---
This branch seems rather miscellaneous - you're making a lot of relatively
unrelated changes.

You're also altering/augmenting API that wasn't right to start with, so I think
the result of this review, after some fixes, is likely to be "yes, merge it,
but open a new bug for more".

Since log-entry-api-review appears to be based on a prefix of this branch, I've
made a branch smcv/ka-misc which is that prefix, and reviewed only that in this
comment.

Fixable in this branch
======================

> + * @TPL_CONTACT_GROUP: the contact's type represents a chatroom

Do you mean a named chatroom which you can go back to by specifying its name
(IRC/XMPP), or a nameless ad-hoc multi-user conversation with no handle which
you can only join by being invited (MSN/Skype), or both? Please be specific.

To be honest I'd be inclined to define these types in terms of handle types,
and say "a named chatroom (%TP_HANDLE_TYPE_ROOM)" if that's what you mean.

> + * @TPL_CONTACT_SELF: the contact's type represents the owner of the account
> + * whose channel has been logged, as opposite to @TPL_CONTACT_USER which
> + * represents any other user

The English idiom is "as opposed to".

> +tpl_contact_from_chatroom_id (const gchar *chatroom_id)

I'd call this method from_room_id. We have ROOM handles, after all.

> +  g_return_val_if_fail (!TPL_STR_EMPTY (chatroom_id), NULL);

Are you deliberately excluding rooms with an empty name? telepathy-spec doesn't
disallow them.

> +  g_return_if_fail (data != NULL); /* allow zero lenght */

length

> g_store_sqlite_add_message_counter: inizialise to NULL @date, [...]

Don't bother amending the patch unless you're rebasing anyway, but for
reference: "tpl_store_...", "initialize".

------------------------------

Terminology (not fixable immediately)
=====================================

(In reply to comment #0)
> Also, TplContact's type is now set to TPL_CONTACT_GROUP when it represents a
> chatroom instead of a person.

Pre-existing bug, but I don't like this API. A chatroom isn't a type of
contact; it's an orthogonal thing (TP_HANDLE_TYPE_CONTACT vs.
TP_HANDLE_TYPE_ROOM).

If TplContact is meant to encompass all of contact/group/self, then it should
be renamed to something that reflects its multiple purpose: TplEntity?

In particular, a channel can target a TP_HANDLE_TYPE_ROOM handle, but only a
TP_HANDLE_TYPE_CONTACT handle can send us messages.

I suspect this is a case of the API being the wrong shape. We really have two
orthogonal things that contacts can be used for:

* who was the conversation with? (a contact, a room ID, or (for MSN/Skype)
multiple contacts)
* who sent this message? (exactly one contact)

> + * @TPL_CONTACT_USER: the contact's type represents a user (buddy), but not
> + * the the account's owner for which @TPL_CONTACT_SELF is used

In telepathy-spec we call these "contacts", or "the remote contact" when we
need to exclude the local user. The self-handle belongs to "the local user".

> due to a premature disposed object, in some cases chatroom couldn't be logged.
> It's fixed in 2b99424.
> 
> I also refactored TplChannelText in order to keep it simple (28280)
> 
> TplLogEntryText and TplChannelText now do not need to call _set_chatroom(TRUE)
> to be a chatroom channel. They'll understand their status looking whether some
> members are set.
> 
> 
> TPL_CONTACT_SELF has been added to represent the user instead of a generic
> person, this fix a problem with Empathy.

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