[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

Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://git.collabora.co.uk/ |http://git.collabora.co.uk/
                   |?p=user/kalfa/telepathy-log |?p=user/cassidy/telepathy-l
                   |ger.git;a=shortlog;h=refs/h |ogger;a=shortlog;h=refs/hea
                   |eads/tpl-channel-refactored |ds/ka-misc-27642
  Status Whiteboard|review-                     |

--- Comment #6 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-05-21 06:09:28 PDT ---
(In reply to comment #1)
> 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.

Let's fix this mess. I created a branch based on smc/ka-misc and fixed your
comments.
http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/ka-misc-27642

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

done.

> > + * @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".

fixed.

> > +tpl_contact_from_chatroom_id (const gchar *chatroom_id)
> 
> I'd call this method from_room_id. We have ROOM handles, after all.

done.

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

fixed.

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

fixed.

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