I started clean up useless methods and privatize something waiting for the official review.

tpl-channel-refactored branch has some changes to the TplContact API, so it should be reviewed before reviewing TplContact API.

Please merge these two commits, and drop the other two:

> bcbd2ed TplContact: remove TpContact reference
> 1e21d30 TplContact: internalize Class, tpl_contact_new and tpl_contact_from_tp_contact

hRationale below.

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

> -  PROP0,
> -  PROP_IDENTIFIER,
> +  PROP_IDENTIFIER = 0,

No, properties must have a nonzero ID (which doesn't appear to be documented, but GObject would g_critical if you ran this code).

There are two equally correct idioms:

enum {
  PROP0,       /* dummy */
  PROP_FIRST,
  PROP_SECOND
};

or

enum {
  PROP_FIRST = 1,
  PROP_SECOND
};

> -  TPL_CONTACT_GROUP
> +  TPL_CONTACT_CHATROOM

See my comments on Bug #27642. I'd suggest just not making this change for now, while we decide whether TplContact is a genuine contact, or a thing-that-receives-messages.

(In reply to comment #2)
> Please merge these two commits, and drop the other two:

done.

I moved all the setters as internal API. User should never use those.

r+ for the branch. Leaving this open for the moment since the class still needs API review.

merged.

For now we should focus on reviewing the API used by Empathy and make the rest private.
Empathy uses the following contact API:

tpl_contact_get_alias ()
tpl_contact_get_avatar_token ()
tpl_contact_get_contact_type ()
tpl_contact_get_identifier ()

Any comment on this API?

Here is a branch keeping only this API: http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/contact-api-27269

(In reply to comment #7)
> For now we should focus on reviewing the API used by Empathy and make the rest
> private.
> Empathy uses the following contact API:
> 
> tpl_contact_get_alias ()
> tpl_contact_get_avatar_token ()
> tpl_contact_get_contact_type ()
> tpl_contact_get_identifier ()
> 
> Any comment on this API?

My major objection is that TplContact doesn't actually seem to be a contact!

  36 /* TplContactType:
  37  *
  38  * @TPL_CONTACT_UNKNOWN: the current contact's type is unknown
  39  * @TPL_CONTACT_USER: the contact's type represents a user (buddy), but not
  40  * the the account's owner for which @TPL_CONTACT_SELF is used
  41  * @TPL_CONTACT_GROUP: a named chatroom (#TP_HANDLE_TYPE_ROOM)
  42  * @TPL_CONTACT_SELF: the contact's type represents the owner of the account
  43  * whose channel has been logged, as opposed to @TPL_CONTACT_USER which
  44  * represents any other user
  45  */

Either it should be renamed to something that reflects its real purpose (which KA explained in <https://bugs.freedesktop.org/show_bug.cgi?id=27642#c5>, although I didn't fully understand the explanation), or it should only be used for genuine contacts.

In Bug #27642, KA wrote:
> 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 #9)
> > 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.

Let's have some concrete examples: "smcv" is the self-contact, "fred" is another contact, and "#telepathy" is a room.

In #telepathy:
<smcv> fred: ping?
<fred> smcv: pong

In a private chat with Fred:
<smcv> hello?
<fred> o hai

Am I right in thinking those four log entries are as follows?

(chat_id="#telepathy", sender=<SELF, smcv>, receiver=<ROOM, #telepathy>, text="fred: ping?")
(chat_id="#telepathy", sender=<CONTACT, fred>, receiver=<ROOM, #telepathy>, text="smcv: pong")
(chat_id="fred", sender=<SELF, smcv>, receiver=<CONTACT, fred>, text="hello?")
(chat_id="fred", sender=<CONTACT, fred>, receiver=<SELF, smcv>, text="o hai")

That's how I understood it.
Are you ok with this API? Should I renamed TpContact and make it happen?

TplContact is tied up in the TplLogEntry API, so to an extent they need to be considered together.

Senders are always genuine contacts (in Telepathy terminology), either TPL_CONTACT_USER or TPL_CONTACT_SELF.

The receiver isn't really a property of the log entry: I'd say that conceptually, it's more like a property of the conversation. A given conversation is directed to the TargetID of its channel, which can either be a contact (self or remote), a named chatroom (as seen in XMPP/IRC), an ad-hoc chatroom (as seen in MSN/Skype), or some other thing that hasn't been defined yet.

It seems to me that there are basically three ways we could represent messages' receivers:

1) Rename TplContact to something like TplEntity or TplTarget. A log entry has a sender, a TplEntity whose type is SELF or USER, and a receiver, a TplEntity of any type.

2) Get rid of non-contact functionality from TplContact, and turn the type into a simple boolean, is_self. 1-1 messages have a non-NULL receiver contact. Other messages have a NULL receiver contact; the receiving chatroom can be inferred in some way from the message (via the chat ID if its type becomes better-defined, perhaps).

3) Get rid of non-contact functionality from TplContact, and turn the type into a simple boolean, is_self. Get rid of the receiver property too, and relate the message to a first-class Conversation object, which either has a pointer to a TplContact (called "target_contact" or something), or a way to get the chatroom name.

(1) is the easiest, but is the one I like least. What would get_alias(), get_avatar_token() mean for a chatroom?

Another question this API gives me is: what is the scope of a TplContact? It's not clear to me which of these would typically be true:

* All my conversations with Cosimo represent him as the same TplContact. get_alias(), get_avatar_token() return his most recent details; older details are ignored.

* Each conversation with Cosimo represents him as a different TplContact. get_alias(), get_avatar_token() return the most recent details from that conversation. I have to use get_identifier() to relate the contacts to each other.

* Each visually distinct version of Cosimo represents him as a different TplContact, i.e. whenever get_alias() or get_avatar_token() changes from one message to the next, the TplContact changes. I have to use get_identifier() to relate the contacts to each other.

Why are the alias and the avatar token in the log? Is it basically a tiny version of persistent contact storage (analogous to libfolks or osso-abook), to be dropped in some future version?

The avatar token doesn't seem very useful when you can only get a cached avatar via a TpContact - we'll probably have to add API to telepathy-glib to see whether a cached avatar is still available, and if so, return them. This will require the connection manager name and the protocol name (both derivable from the TpAccount name), and can't be done for non-Telepathy avatars.

Perhaps a better way to do the avatar would be to keep the avatar token, CM name and protocol name behind the scenes, and have:

    GFile *tpl_contact_get_avatar (TplContact *contact);

which can return NULL, and would be implemented in terms of new telepathy-glib API, something like:

    GFile *tp_account_get_cached_avatar (TpAccount *account,
        const gchar *avatar_token);

> TplContact *tpl_contact_from_room_id (const gchar *chatroom_id);
> TplContact *tpl_contact_from_tp_contact (TpContact *contact);

Shouldn't these be internal?

Simon, the example you did for the log entry is right.

Simon said:
> The receiver isn't really a property of the log entry: I'd say that
> conceptually, it's more like a property of the conversation. A given
> conversation is directed to the TargetID of its channel, which can either be a
> contact (self or remote), a named chatroom (as seen in XMPP/IRC), an ad-hoc
> chatroom (as seen in MSN/Skype), or some other thing that hasn't been defined
> yet.

The receiver for TPL needs to be a bit fat.
It needs to store Alias and AvatarTokens for each log entry.

This makes things a bit harder/huglier, but helps remembering historical context (see a comment later)

> 2) Get rid of non-contact functionality from TplContact, and turn the type into
> a simple boolean, is_self. 1-1 messages have a non-NULL receiver contact. Other
> messages have a NULL receiver contact; the receiving chatroom can be inferred
> in some way from the message (via the chat ID if its type becomes
> better-defined, perhaps).

> 3) Get rid of non-contact functionality from TplContact, and turn the type into
> a simple boolean, is_self. Get rid of the receiver property too, and relate the
> message to a first-class Conversation object, which either has a pointer to a
> TplContact (called "target_contact" or something), or a way to get the chatroom
> name.


The issue with this approach 2 or 3 is that it might have problems with non Telepathic LogStores, as far as I can see.
3 does not remember historical contexts as well.

Consider the logger importing and browsing logs which are not from Empathy/TP, but from any other client from which TPL cannot infer the receiver from the chat_id.

As I wrote elsewhere TPL stores data in a tree like:

* <cm>
** <proto>
*** <chat_id> (the remote room/buddy id)
**** <date>
***** <log entries>

Pidgin does something similar, but TP cannot infer the CM, since in libpurple there is no such concept.

What does it happens if a client stores its logs using a hash for <chat_id>?
Most likely, I cannot infer the receiver or the room id from the hash.
I need to obtain it explicitly.

TPL plays on a wider field than TP, unless we decide to limit its potentialities.

I'd rather to have a bit more properties, which IMHO do not create complexity in the API, than denying the possibility for TPL to have a TplLogStore able to understand a client storing logs as the example above.

Which means, I'd be for proposal 1, the one you like the least:
Renaming TplContact and using SELF, CONTACT, ROOM as contact types.

Another advantage of it is that allows to have all the info E. stored before TPL, including historical contexts.


> (1) is the easiest, but is the one I like least. What would get_alias(),
> get_avatar_token() mean for a chatroom?

true.

For it I don't really have a solution. Either:

* add a GInterface for basic methods, considering Alias and Avatar "extra" methods just for CONTACTS (see my comment below about historical contexts for a use case, in any other case I don't see their utility).
* remove such methods from TplContact, dropping the historical context case below.
* accept them to be NULL


> Another question this API gives me is: what is the scope of a TplContact? It's
> not clear to me which of these would typically be true:

Shortly:

Empathy log store stores the full name of the contact in the XML, along with the id and the avatar id. In each log entry.

This means that each entry might have a different value for Alias and AvatarTok and is a different TplContact.

> * Each conversation with Cosimo represents him as a different TplContact.
> get_alias(), get_avatar_token() return the most recent details from that
> conversation. I have to use get_identifier() to relate the contacts to each
> other.
> 
> * Each visually distinct version of Cosimo represents him as a different
> TplContact, i.e. whenever get_alias() or get_avatar_token() changes from one
> message to the next, the TplContact changes. I have to use get_identifier() to
> relate the contacts to each other.

If I understood correctly what you mean, it's the last.

Also, two objects having the same data are actually different instances. But this can be changed.

The TplContact data is retrieved from each LogEntry, which holds such info in its XML (assuming LogStoreXML, which will change eventually in a compatible way).

This results in the E. log viewer (with the right theme) to have N entries from Cosimo being actually N different TplContact and having a different "mutable" property (avatar, alias).

The id is the only property which can be used to understand the relation.

> Why are the alias and the avatar token in the log? Is it basically a tiny
> version of persistent contact storage (analogous to libfolks or osso-abook), to
> be dropped in some future version?

Empathy log reminiscence, for backward compatibility, but it makes sense to me.

> The avatar token doesn't seem very useful when you can only get a cached avatar
> via a TpContact - we'll probably have to add API to telepathy-glib to see
> whether a cached avatar is still available, and if so, return them. This will
> require the connection manager name and the protocol name (both derivable from
> the TpAccount name), and can't be done for non-Telepathy avatars.


Originally I was thinking of caching avatar pics also, it's still an idea, since it's not the scope of libfolks caching old information.
If I am not mistaken, currently empathy do some caching.

For non-telepathy LogStores, probably it won't be possible to have historical contexts, depending on the way the log storing is done.

Why keeping avatar and alias in the log entries?
What does it happen if Cosimo changes his avatar with a funny pic and comments with simon his new avatar, then changes it back?
The context of the log is not just the body, sender and receiver, but also the avatar or alias. That's what I call historical context.

There is a (small, being really corner case) needing to record the context, especially when it comes for free and does not really need much storage space.

Currently though, the avatar is not stored, only the avatar id, relying on Empathy to remember it.

> Perhaps a better way to do the avatar would be to keep the avatar token, CM
> name and protocol name behind the scenes, and have:
> 
>     GFile *tpl_contact_get_avatar (TplContact *contact);
> 
> which can return NULL, and would be implemented in terms of new telepathy-glib
> API, something like:
> 
>     GFile *tp_account_get_cached_avatar (TpAccount *account,
>         const gchar *avatar_token);

That's OK.
I'd store it anyway, though, so that everything is self contained in the log store dir (copying $XDG_DATA/logs/TpLogger somewhere else would remember the avatars).

> > TplContact *tpl_contact_from_room_id (const gchar *chatroom_id);
> > TplContact *tpl_contact_from_tp_contact (TpContact *contact);
> 
> Shouldn't these be internal?

internalized in my tpl-channel-refactored branch, which now is rebased agaist current master.

(In reply to comment #8)
> Here is a branch keeping only this API:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/contact-api-27269

Most of this branch is now obsolete. Only one commit left:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/contact-redo

(In reply to comment #14)
> Most of this branch is now obsolete. Only one commit left:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/contact-redo

review+

(In reply to comment #13)
> Empathy log store stores the full name of the contact in the XML, along with
> the id and the avatar id. In each log entry.
> 
> This means that each entry might have a different value for Alias and AvatarTok
> and is a different TplContact.

In that case, another way to model this would be to give each Entry more "properties" (getters) in its C API, and discard the TplContact object entirely:

* sender ID (always a contact ID)
* sender alias
* sender avatar token
* receiver ID (contact, or chatroom, or "" if MSN)
* receiver alias (or "" if not a contact?)
* receiver avatar token (or "" if not a contact)

If the IDs are the thing you have to use to understand the relation between entries, then it seems odd to introduce another object that doesn't have the right many:1 relationship.

> Originally I was thinking of caching avatar pics also, it's still an idea,
> since it's not the scope of libfolks caching old information.
> If I am not mistaken, currently empathy do some caching.

At the moment, neither Empathy nor telepathy-glib ever expires old avatars from its cache, so they'll stay in ~/.cache/telepathy/avatars forever (or at least, until deleted).

> Why keeping avatar and alias in the log entries?
> What does it happen if Cosimo changes his avatar with a funny pic and comments
> with simon his new avatar, then changes it back?
> The context of the log is not just the body, sender and receiver, but also the
> avatar or alias. That's what I call historical context.

Right, I can see the reasoning for doing this.

Note that in MSN, you can attach a nickname to a message that differs from the nickname you want in contacts' contact lists. We now expose this in the Messages interface as a header field, "sender-nickname".

In the long term, I think Tpl's log format should be (a serialization of) the MessagePart[] aa{sv} from the Messages interface, and we should insert the sender/receiver alias and avatar token into the header part if we want to keep them (we can reserve some keys for that purpose).

I renamed TplContact to TplEntity:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/entity

(In reply to comment #17)
> I renamed TplContact to TplEntity:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/entity

Looks good, but I'd like a couple more changes:

> +  tpl_entity_sender = _tpl_entity_from_tp_contact (me);
> +  _tpl_entity_set_entity_type (tpl_entity_sender, TPL_ENTITY_USER);

Pre-existing bug: in both on_received_signal_cb and on_sent_signal_cb, shouldn't the entity created from @me be of type TPL_ENTITY_SELF?

>    /* contact is a user (buddy) */
> -  TPL_CONTACT_USER,
> +  TPL_ENTITY_USER,

Now that the word "contact" isn't in the API, I think TPL_ENTITY_USER should be renamed to TPL_ENTITY_CONTACT. Remote contacts aren't necessarily users of our software, whereas the local user is: so in telepathy-spec we frequently say "the local user" to mean the equivalent of TPL_ENTITY_SELF, and "the remote contact" to mean the equivalent of TPL_ENTITY_USER.

(In reply to comment #18)
> > +  tpl_entity_sender = _tpl_entity_from_tp_contact (me);
> > +  _tpl_entity_set_entity_type (tpl_entity_sender, TPL_ENTITY_USER);
> 
> Pre-existing bug: in both on_received_signal_cb and on_sent_signal_cb,
> shouldn't the entity created from @me be of type TPL_ENTITY_SELF?

Make sense to me.
KA: do you confirm?

> >    /* contact is a user (buddy) */
> > -  TPL_CONTACT_USER,
> > +  TPL_ENTITY_USER,
> 
> Now that the word "contact" isn't in the API, I think TPL_ENTITY_USER should be
> renamed to TPL_ENTITY_CONTACT. Remote contacts aren't necessarily users of our
> software, whereas the local user is: so in telepathy-spec we frequently say
> "the local user" to mean the equivalent of TPL_ENTITY_SELF, and "the remote
> contact" to mean the equivalent of TPL_ENTITY_USER.

Fair enough. I renamed it.

I confirm, it should be SELF.

There is a branch of mine in which I fixed it, but it's probably out of date.

Thanks. I fixed that.

r+ for the branch, then.

(In reply to comment #16)
> In that case, another way to model this would be to give each Entry more
> "properties" (getters) in its C API, and discard the TplContact object
> entirely:
> 
> * sender ID (always a contact ID)
> * sender alias
> * sender avatar token
> * receiver ID (contact, or chatroom, or "" if MSN)
> * receiver alias (or "" if not a contact?)
> * receiver avatar token (or "" if not a contact)

I think doing this, and discarding TplEntity, may be the right long-term answer.

>> In that case, another way to model this would be to give each Entry more
>> "properties" (getters) in its C API, and discard the TplContact object
>> entirely:
>> 
>> * sender ID (always a contact ID)
>> * sender alias
>> * sender avatar token
>> * receiver ID (contact, or chatroom, or "" if MSN)
>> * receiver alias (or "" if not a contact?)
>> * receiver avatar token (or "" if not a contact)
>
> I think doing this, and discarding TplEntity, may be the right long-term
> answer.

Missed it. I agree.

Merged to master.

As this is all merged to master ...