Bug 50563

Comment on attachment 62351 [details] [review]
Telepathy observer patch

Review of attachment 62351 [details] [review]:
-----------------------------------------------------------------

First of all, great work, I'm looking forward to seeing the results of this!

Now some comments:

 - Urgh. No URI for messages.
 - When the conversation is created, do you already have a message? If not, don't give Open/Close events a message, just the contact (note that IMMessage represents a single message, not a conversation).
 - Some documentation wouldn't hurt (like eg. in create_text_event explaining how it initializes the event manifestation).
 - Multi-user text conversations?

 - All the above for audio calls.
 - Why does the NFO_AUDIO object get named after the first contact?
 - observe_call_channel(): please use constants TP_CALL_STATE_*

 - "event_template.set_interpretation (ZG_CREATE_EVENT); if (channel.requested == false) event_template.set_manifestation (ZG_WORLD_ACTIVITY);" this is duplicated in create_call_event and observe_call_channel (plus, if/else so there's always a single assignment would be nicer).

- This comment "//TODO: Add payloads" seems old (no longer applicable)?
- I thought we'd decided to use namespaces in JSON payloads?
- Why is the list of call participants duplicated as subjects and in the JSON?
- «details_obj.set_double_member ("duation", duration);» <- typo "duation"
- If it isn't already, please document the JSON format on the wiki

- observe_ft_channel(): please use TP_FILE_TRANSFER_STATE_* instead of magic numbers.
- query_info() will block all of datahub if it's slow for some reason; please use query_info_async (you can use yield)
- "//TODO: create if else" - another obsolete comment?

(In reply to comment #1)
> Comment on attachment 62351 [details] [review] [review]
> Telepathy observer patch
> 
> Review of attachment 62351 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> First of all, great work, I'm looking forward to seeing the results of this!
> 
> Now some comments:
> 
>  - Urgh. No URI for messages.

Yeah I have no idea what URI I could use there. There is no permanent URI for the message. I prefer linking to the existing xml files maybe. Same goes for calls, I have no clue what URI to use.

>  - When the conversation is created, do you already have a message? If not,
> don't give Open/Close events a message, just the contact (note that IMMessage
> represents a single message, not a conversation).

As explained i need the first subject to be a message to be able to distinguish between send and receives

>  - Some documentation wouldn't hurt (like eg. in create_text_event explaining
> how it initializes the event manifestation).

On it.

>  - Multi-user text conversations?

Will not be logged since it is a pain and causes too much noise

>  - All the above for audio calls.
>  - Why does the NFO_AUDIO object get named after the first contact?

Do you have any better suggestions how we can call it? Leave it empty?

>  - observe_call_channel(): please use constants TP_CALL_STATE_*

Got it.

>  - "event_template.set_interpretation (ZG_CREATE_EVENT); if (channel.requested
> == false) event_template.set_manifestation (ZG_WORLD_ACTIVITY);" this is
> duplicated in create_call_event and observe_call_channel (plus, if/else so
> there's always a single assignment would be nicer).

Got it.

> - This comment "//TODO: Add payloads" seems old (no longer applicable)?

Sorry

> - I thought we'd decided to use namespaces in JSON payloads?

I use them, the namespaces are defined on the top of the file

> - Why is the list of call participants duplicated as subjects and in the JSON?

Just for convenience. Is this a problem?

> - «details_obj.set_double_member ("duation", duration);» <- typo "duation"

On it.

> - If it isn't already, please document the JSON format on the wiki

Good idea.

> - observe_ft_channel(): please use TP_FILE_TRANSFER_STATE_* instead of magic
> numbers.

Got it.

> - query_info() will block all of datahub if it's slow for some reason; please
> use query_info_async (you can use yield)

Sorry for that. Will do

> - "//TODO: create if else" - another obsolete comment?

Yep

Created attachment 62594 [details] [review]
Telepathy observer patch

I did the changes you requested. Please review

Comment on attachment 62594 [details] [review]
Telepathy observer patch

Review of attachment 62594 [details] [review]:
-----------------------------------------------------------------

>>  - Multi-user text conversations?
> Will not be logged since it is a pain and causes too much noise

How are you handling them, though? Shouldn't be a check somewhere so they are ignored?


>>  - Why does the NFO_AUDIO object get named after the first contact?
> Do you have any better suggestions how we can call it? Leave it empty?

Anything that isn't misleading. "Call with X" for one person and "Call with N others" for >1 people would be a simple option (maybe event something like "Call with John, Mark and 3 others"). Or you can just hardcode it "Voice call" or leave it empty; I'm not sure there's much point in using this string in user visible interfaces anyway (they'll probably want to take the contact information and build something themselves).


>>  - "event_template.set_interpretation (ZG_CREATE_EVENT); if (channel.requested
>> == false) event_template.set_manifestation (ZG_WORLD_ACTIVITY);" this is
>> duplicated in create_call_event and observe_call_channel (plus, if/else so
>> there's always a single assignment would be nicer).

This hasn't changed.


>> - I thought we'd decided to use namespaces in JSON payloads?
>I use them, the namespaces are defined on the top of the file

Right, sorry.


>> - Why is the list of call participants duplicated as subjects and in the JSON?
> Just for convenience. Is this a problem?

There isn't much point in doing so (and it increases the size of the DB; keep in mind that we can't even reuse the name strings, like happens with the subjects).

Created attachment 62821 [details] [review]
Fix bloated payloads and redundant if/else

Created attachment 62933 [details] [review]
Overall patch with all changes so far (including mine)

Created attachment 62934 [details] [review]
Some fixes (included in patch below)

I've attached a patch with some changes. Let me know what you think of them.

I think this is pretty close now. Still not happy about the missing URI, but we can ignore this for now. Also wondering if we couldn't log the messages in the payload if we're already logging all that stuff.

Comment on attachment 62934 [details] [review]
Some fixes (included in patch below)

Review of attachment 62934 [details] [review]:
-----------------------------------------------------------------

::: src/telepathy-observer.vala
@@ +89,5 @@
>                                ZG_ACCESS_EVENT,
>                                "",
>                                this.actor,
>                                null,
>                                null);

I think we should try to extract the timestamp from the channel or the telepathy event. This could be useful if the event is based on a pending message from lets say an hour ago. Currently it seems like we would be pushing it with a new timestamp which is kind of a lie. :P

@@ -112,5 @@
>                                "",
>                                this.actor,
>                                null,
>                                null);
> -    event_template.set_origin (obj_path);

Why did you remove the event_template origin from here, cant we set in during construction of the event_template.

@@ +160,4 @@
>            event_template.set_manifestation (ZG_WORLD_ACTIVITY);
>            this.push_event (event_template);
>          }
> +        // FIXME: what about sent messages? what happens with them?

I don't think pending messages contain sent messages. Ask the telepathy people.

@@ +210,5 @@
>                                ZG_ACCESS_EVENT,
>                                ZG_USER_ACTIVITY,
>                                this.actor,
>                                null,
> +                              null);

why remove the event_origin? I know its duplicated in the subject, but I think that is ok, since the origin kinda of applies for both.

@@ -390,4 @@
>            event_template.set_manifestation (ZG_WORLD_ACTIVITY);
>          }
>          event_template.set_actor (this.actor);
> -        event_template.set_origin (obj_path);

Y U NO LET THE ORIGIN BE?

Ok this looks good. Can we make it disableable per commandline

Can you put back the event_origin back as it was for one release only please. Since the current folks released depends on it.