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.

Additional review on ka-misc:

Can tpl_contact_from_chatroom_id be internalized? (_tpl_contact_from_chatroom_id)

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.

On closer inspection of log-entry-api-review, it seems to be based on an older version of tpl-channel-refactored. Could you sort these branches out and tell me what should be reviewed, please?

I agree with the renaming of TplContact into TplEntity or similar, I realized it when adding TPL_CONTACT_GROUP semantic, but for doing that I really need that most of the important pending branches are done.

This currently might means three branches including the misc-ka.

> Can tpl_contact_from_chatroom_id be internalized?
> (_tpl_contact_from_chatroom_id)

Yep, this was the plan, but since it was introduced in misc-ka, I was waiting to be merged for the renaming. contact-internal.h does not exist "yet".


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

It's that yes. I'll also allow the empty string for a Room name.


Most of the changes suggested by you, which involve renaming of symbols, I'd like to apply on the API review stage.

I have already a list of issues I'd like to rename/change


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


When A TplContact (or maybe-TplEntity) is set to TPL_CONTACT_GROUP (maybe-TPL_ENTITY_ROOM) it means that the TplLogEntry related to the contact/entity has been sent from/to a ROOM.


A TplLogEntry has three important members:
- sender (TpContact in tp-glib)
- receiver (TpContact in tp-glib)
- chat_id (which usually corresponds to the remote identifier or room id but it doesn't need to be, it depends on the TplLogStore implementation)


Before:
When on 1-1
- sender and receiver are set to a proper TplContact
- chat_id = remote id or similar

When on ROOM
- the sender or the receiver is NULL (the remote one), the other one is a TplContact for the local user.
- chat_id = room id or similar

In normal cases, chat_id is the remote/room id.

chat_id is, in practical terms, an identifier for the chats with the couple (local id,remote id) behind a CM (which for TPL translates in a generic name, since pidgin for example calls it "jabber" and not "gabble_jabber" as Empathy/TP do, some other client might not even have a proper name for it).

Empathy: .local/shares/TpLogger/logs/gabble_jabber_foo_40jabber_2eorg0/
Pidgin: .purple/logs/jabber/foo@jabber.org/
InventedClient: .invented_store/logs/jabber/1ab321db/

foo@jabber.org in the first two examples would the the chat_id.
1ab321db would be the chat_id in the third.

The three would represent the chats occurred between the user and a single remote identifier (foo@jabber.org) using jabber/gabble_jabber.
The third one is not informative about the remote name.
 
What does it happen when a logstore has the chat_id != remote_id/room_id?

If TPL assumes that the chat_id and the remote identifier have the same value, an information might be lost.
I need a way to store both a chat_id and a real remote identifier.

The solution I found is change the former semantic of TplContact to something you identified as TplEntity.

In this way, I might have a redundant information since it can be
chat_id == tpl_contact.identifier but at least I do not lose it.

This translates in:

When on 1-1
- sender and receiver are set to a TplContact and TPL_CONTACT_{SELF,CONTACT} is also set.
- chat_id = as before

When on ROOM
- sender and receiver are set to a TplContact, and TPL_CONTACT_{SELF,GROUP} is also set.
- chat_id = as before



The alternative to this way is adding a further member calling room_id, which would be NULL on 1-1, while be set to the real room name/id on TYPE_ROOM with the remote TplContact (representing only a TYPE_CONTACT) set to NULL.

I found the TplEntity way easier, simplifying the what it can happened:
always sender and receiver are set, instead of one being NULL depending on the situation and another member set instead.
Also, it makes possible handling different cases "1-1 vs ROOM" as similar, which actually they are, from a logger point of view.


Answering to your question, I'm not sure how it might translate in TP, I'd say it should be associated to an extended TPL/loggable version of TpHandle:

With a Text channel, I receive a remote handle being TYPE_ROOM when on ROOM or TYPE_CONTACT when on Contact.
It represents a ROOM also, which TpContact does not.

One thing I'm sure of, TplContact has a wrong name, it would be associated to TpContact, which is not and should be renamed ASAP.



> Later
> =====

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

True, I've already taken care of it.
 
> 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.


A TplLogEntryText can understand if it's handling a ROOM or not depending on its internal state.
Checking whether the receiver or the sender is set as TPL_CONTACT_GROUP (using the current naming).

In facts, a ROOM is logged in a different place, so that a identifier clashing would not be a fail.

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

(In reply to comment #6)
> 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

r+ for that branch. It'd be great if you could pick up KA's other branches, too, particularly ones that reduce the public API "surface area" or fix my other design criticisms; I'll leave this bug open, since I made numerous other comments about these branches.

> It'd be great if you could pick up KA's other branches

I agree, I would be great.
I count to get back to TPL soon, I really need to finish it.
Currently I'm quite busy on another task which unfortunately has higher urgency :(

Feel free to commit or ask anything.
As soon as I'll manage to have more time I'll continue from the point you left.

thank you anyway and sorry.

(In reply to comment #7)
> (In reply to comment #6)
> > 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
> 
> r+ for that branch.

Thanks, merged.

> It'd be great if you could pick up KA's other branches,
> too, particularly ones that reduce the public API "surface area" or fix my
> other design criticisms; I'll leave this bug open, since I made numerous other
> comments about these branches.

That's my plan. Any guide about which branch should be fixed/merged and in which order would be welcome. I'm a bit lost tbh.

(In reply to comment #5)
> I agree with the renaming of TplContact into TplEntity or similar, I realized
> it when adding TPL_CONTACT_GROUP semantic, but for doing that I really need
> that most of the important pending branches are done.
> 
> This currently might means three branches including the misc-ka.

Whate are the 2 other branches blocking the work on this bug?

(In reply to comment #5)
> When A TplContact (or maybe-TplEntity) is set to TPL_CONTACT_GROUP
> (maybe-TPL_ENTITY_ROOM) it means that the TplLogEntry related to the
> contact/entity has been sent from/to a ROOM.

Let's move this discussion to Bug #27269.

This branch has been rebased as requested by Cassidy to the current master.

I also internalized _tpl_contact_from_room_id()

Currently this bug is not blocked anymore by 27269, but actually depends on it.

The decision we'll take about TplContact will mean either merging this branch and renaming TplContact or dropping it.

(In reply to comment #12)
> This branch has been rebased as requested by Cassidy to the current master.
> 
> I also internalized _tpl_contact_from_room_id()
> 
> Currently this bug is not blocked anymore by 27269, but actually depends on it.
> 
> The decision we'll take about TplContact will mean either merging this branch
> and renaming TplContact or dropping it.

#27269 is done now, what's next ?

No response no issue, let's assume this is fixed them.