GNOME Bugzilla – Bug 672709
Add new interaction details properties to individuals
Last modified: 2012-09-27 10:53:07 UTC
A feature that I think would enrich the developer story for folks is if folks could actually provide developers with some interaction details for each individual. Thus I added 4 new properties and a new signal to individual. - im_interaction_count: a counter describing the amount of IMs sent to or received from the individual - call_interaction_count: a counter describing the amount of successful "Calls" with the user the individual - last_im_interaction_timestamp: the timestamp of the last IM interaction - last_call_interaction_timestamp: the timestamp of the last Call interaction - interaction_changed: a signal emitted once one or more of the properties above are changed I pushed my code to github for review since it will allow more interactive comments. Also the test i have is kind of a standalone process that monitors the actual interaction. Currently the only backend that reports interaction changes is the telepathy backend with the support of zeitgeist.zeitgeist-telepathy-logger is required to be installed for that. I will propose to be shipped with Telepathy.
https://github.com/seiflotfy/folks-zeitgeist/pull/1/files
It would be best to keep all the patch review/discussion on Bugzilla now that you've created the bug report, so could you please attach your branch as a couple of patches to this bug report? Thanks. Some initial comments: • Make sure to split the core and Telepathy backend changes into two separate commits to make reviewing easier. • What's the current state of Zeitgeist as a GNOME dependency? From a quick look at jhbuild, it's not currently in the core modulesets, which (iirc) means we can't have it as a hard dependency. Unless I'm mistaken, that means the changes to the Telepathy backend have to be optional at configure time. The core changes (InteractionDetails interface and changes to Individual) are fine though. • What's the use case for this? Is it going to be used in Empathy, or some other client? • From [1], it looks like Zeitgeist depends on Vala 0.14 (please correct me if I'm wrong). folks depends on Vala 0.16, which could cause conflicts. Are there any plans to port Zeitgeist to Vala 0.16? [1]: http://bazaar.launchpad.net/~zeitgeist/zeitgeist/bluebird/view/head:/configure.ac
(In reply to comment #3) > It would be best to keep all the patch review/discussion on Bugzilla now that > you've created the bug report, so could you please attach your branch as a > couple of patches to this bug report? Thanks. > > Some initial comments: > • Make sure to split the core and Telepathy backend changes into two separate > commits to make reviewing easier. On it tomorrow morning > • What's the current state of Zeitgeist as a GNOME dependency? From a quick > look at jhbuild, it's not currently in the core modulesets, which (iirc) means > we can't have it as a hard dependency. Unless I'm mistaken, that means the > changes to the Telepathy backend have to be optional at configure time. The > core changes (InteractionDetails interface and changes to Individual) are fine > though. Zeitgeist is not a GNOME dependency yet. Currently we are struggling with a chicken and egg problem. On the one hand we are told we need a module to depend on us to become a dependency and on the other hand modules don't want to depend on us until we are a dependency. I talked to someone on the release team and I was told it is OK for folks to depend on Zeitgeist, and that I should propose Zeitgeist for GNOME inclusion. > • What's the use case for this? Is it going to be used in Empathy, or some > other client? Empathy should be able to populate its list according to recent or a mixture between frequent and recent by ranking using a modification of (interaction count)/(time since last interaction) Also it should be used for contacts to expose the most common mean of communication for an individual > • From [1], it looks like Zeitgeist depends on Vala 0.14 (please correct me if > I'm wrong). folks depends on Vala 0.16, which could cause conflicts. Are there > any plans to port Zeitgeist to Vala 0.16? > > [1]: > The next Zeitgeist will depend on Vala 0.16 I already did the necessary updates for that. http://bazaar.launchpad.net/~zeitgeist/zeitgeist/bluebird/view/head:/configure.ac
Here is the new splitted commit you asked for https://github.com/seiflotfy/folks-zeitgeist/commits/new-properties
(In reply to comment #4) > Zeitgeist is not a GNOME dependency yet. Currently we are struggling with a > chicken and egg problem. On the one hand we are told we need a module to depend > on us to become a dependency and on the other hand modules don't want to depend > on us until we are a dependency. I talked to someone on the release team and I > was told it is OK for folks to depend on Zeitgeist, and that I should propose > Zeitgeist for GNOME inclusion. OK. I think I'd prefer Zeitgeist to be a soft dependency (as fredp suggested in the conversation log you e-mailed me), but I guess a hard dependency would be OK. Travis, what do you think? > Empathy should be able to populate its list according to recent or a mixture > between frequent and recent by ranking using a modification of (interaction > count)/(time since last interaction) > > Also it should be used for contacts to expose the most common mean of > communication for an individual Makes sense. :-) > The next Zeitgeist will depend on Vala 0.16 I already did the necessary updates > for that. Great! (In reply to comment #5) > Here is the new splitted commit you asked for > https://github.com/seiflotfy/folks-zeitgeist/commits/new-properties Can you attach the patches here for review please?
Created attachment 210540 [details] [review] Add new InteractionDetails properties to Individual
Created attachment 210541 [details] [review] Modified Telepathy backend to implement new interaction Details
Created attachment 210542 [details] [review] Add dummy test This test is more or less a real life test... It is hard to implement an automated test since this involves using Zeitgeist via dbus and Telepathy. If you have hints how to write one please tell me. To test you will need to run the following script http://bazaar.launchpad.net/~zeitgeist-dataproviders/zeitgeist-datasources/trunk/view/head:/telepathy/zeitgeist-telepathy-observer and also run the individual test. And watch how it prints out counts and timestamps while you interact with users via telepathy/empathy
(In reply to comment #6) > (In reply to comment #4) > > Zeitgeist is not a GNOME dependency yet. Currently we are struggling with a > > chicken and egg problem. On the one hand we are told we need a module to depend > > on us to become a dependency and on the other hand modules don't want to depend > > on us until we are a dependency. I talked to someone on the release team and I > > was told it is OK for folks to depend on Zeitgeist, and that I should propose > > Zeitgeist for GNOME inclusion. > > OK. I think I'd prefer Zeitgeist to be a soft dependency (as fredp suggested in > the conversation log you e-mailed me), but I guess a hard dependency would be > OK. > > Travis, what do you think? Yeah, now that I think about it, we could manage a soft dependency by adding the fields but only populating them with non-default values in the case that Zeitgeist support was built. It wouldn't be too much trouble and it would remove any concerns about adding it as a hard dependency.
Review of attachment 210540 [details] [review]: ::: folks/individual.vala @@ +786,3 @@ + } + + private uint _get_im_interaction_count () It'll be a bit clearer to move _get_im_interaction_count() into the definition of im_interaction_count.get, seeing as _get_im_interaction_count() isn't called from anywhere else. Same for _get_last_im_interaction_timestamp() and _get_call_interaction_count(). @@ +2030,3 @@ } + + /* Subscribe to the interactions signal for the persona */ This block of code should be in _connect_to_persona(). A corresponding block which disconnects the signal handler should be in _disconnect_from_persona(). @@ +2035,3 @@ + { + p_interaction_details.interaction_changed.connect(() => + { this.interaction_changed(); }); Need spaces between method names and the opening brackets of their parameter lists. ::: folks/interaction-details.vala @@ +21,3 @@ +using GLib; + +public interface Folks.InteractionDetails : Object This interface needs a doc comment. @@ +25,3 @@ + /** + * The IM interaction associated with a Persona + */ All these doc comments need ‘@since UNRELEASED’ lines added (the ‘UNRELEASED’ gets replaced by the version name at release time). @@ +34,3 @@ + * The latest IM interaction timestamp associated with a Persona + */ + public abstract uint last_im_interaction_timestamp This should be a DateTime, rather than a uint. We're not in the 80s: we can have more structured representations of time than seconds since the epoch. :-) @@ +45,3 @@ + { + get; + } Are there likely to be more types of interactions added later? I can imagine this interface getting quite big if it has to have properties for lots of different types of interactions. If so, it might make sense to make the interface a bit more generic — though I have no idea what would be better at the moment. @@ +61,3 @@ + */ + public async signal void interaction_changed (); + Stray blank line. ::: folks/persona-store.vala @@ +270,3 @@ + + INTERACTION_COUNT, + LAST_INTERACTION_TIMESTAMP These need documentation comments.
Review of attachment 210540 [details] [review]: ::: folks/interaction-details.vala @@ +60,3 @@ + * This is emitted if the contact is interacted with in the form of IM or calls + */ + public async signal void interaction_changed (); Why is this async? Also, why is a separate signal used instead of GObject’s ‘notify’ signal?
Review of attachment 210541 [details] [review]: This is looking like a good first attempt. Most of the comments below are trivial things, but there are a couple of big questions in there which need answering. ::: backends/telepathy/lib/tpf-persona-store.vala @@ +71,3 @@ + private Zeitgeist.Log log; + private Zeitgeist.Monitor monitor; Private variables should be prefixed by ‘_’. @@ +80,2 @@ private HashMap<string, Persona> _personas; + private HashMap<string, Persona> _uid_personas; It should be possible to just use this._personas by transforming UIDs into IIDs (the code which builds them both is in the constructor for Tpf.Persona, and should be easy to reverse-engineer). Adding a whole new map should be unnecessary. @@ +117,3 @@ private Cancellable? _load_cache_cancellable = null; private bool _cached = false; + private bool _is_counter_populated = false; Instead of this boolean, you could check if (this._monitor != null) instead. @@ +229,3 @@ { + if (this._is_counter_populated == false) + this._populate_counters (); This call to _populate_counters() should be moved into prepare() and then the check for _is_counter_populated should be moved into the outer if statement. @@ +690,3 @@ + var new_origin = origin.replace ("x-telepathy-account-path:", ""); + return "telepathy:%s%s:%s".printf (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE, + new_origin, new_uri); For the benefit of those who aren't Zeitgeist experts (me!), could you add a comment describing the ID format, or a link to some external documentation about it please? @@ +693,3 @@ + } + + private bool _increase_persona_counter (string? id, string? interaction_type, Event event) It should be possible to refactor the code so that id and interaction_type are never null when passed into this function. That'll simplify the code in here a bit (especially when building with --enable-experimental-non-null). @@ +709,3 @@ + interpretation == Zeitgeist.ZG_RECEIVE_EVENT)) + { + persona._im_interaction_count = persona._im_interaction_count + 1; Could just use ‘++’. @@ +722,3 @@ + persona._call_interaction_count = persona._call_interaction_count + 1; + if (persona._last_call_interaction_timestamp < converted_timestamp ) + persona._last_call_interaction_timestamp = converted_timestamp ; There are some stray spaces in the above two lines. Also, please put braces around if blocks, even if they're only single lines. @@ +749,3 @@ + if (changed_personas.size > 0) + foreach (var persona in changed_personas) + persona.interaction_changed (); The if statement should be unnecessary, I think. Please put braces around the foreach block. Why are the calls to interaction_changed() done in a separate loop after the first one? Couldn't you just move the persona.interaction_changed() call in place of changed_personas.add(…)? @@ +755,3 @@ + { + var origin = this.id.replace (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE, + "x-telepathy-account-path:"); Please add some comments explaining this code. @@ +776,3 @@ + * the counters of the personas */ + PtrArray events = this._get_zeitgeist_event_templates(); + log = new Zeitgeist.Log(); Use ‘this.’ when referring to object-level variables. There are several places where this needs changing. @@ +784,3 @@ + 0, + ResultType.MOST_RECENT_EVENTS, + null); Please indent these 4 spaces past the indentation of the first line (“var results…”) as in the rest of the code. @@ +788,3 @@ + { + var interaction_type = e.get_subject (0).get_interpretation (); + for (var i=1; i < e.num_subjects (); i++) Spaces around the equals sign. @@ +791,3 @@ + { + var id = this._get_id_from_event_metadata (e.get_subject (i).get_uri (), + e.get_origin()); Space before ‘()’. @@ +796,3 @@ + } + } + catch {} This should call warning("Some suitable message.") rather than just ignoring all errors. ::: backends/telepathy/lib/tpf-persona.vala @@ +397,3 @@ + * A counter for IM interactions (send/receive message) with the persona. + * + * See {@link Folks.InteractionDetails.im_interaction_count} Need ‘@since UNRELEASED’ lines in all these comments. @@ +399,3 @@ + * See {@link Folks.InteractionDetails.im_interaction_count} + */ + [CCode (notify = false)] Why aren't notifications made of changes to these properties (either implicitly or explicitly)? ::: configure.ac @@ +181,2 @@ PKG_CHECK_MODULES([TP_GLIB], [telepathy-glib >= $TP_GLIB_REQUIRED]) +PKG_CHECK_MODULES([ZEITGEIST], [zeitgeist-1.0 >= $ZEITGEIST_REQUIRED]) As Travis said, it would be best if this was made into a soft dependency.
(In reply to comment #9) > Created an attachment (id=210542) [details] [review] > Add dummy test > > This test is more or less a real life test... It is hard to implement an > automated test since this involves using Zeitgeist via dbus and Telepathy. > If you have hints how to write one please tell me. > > To test you will need to run the following script > http://bazaar.launchpad.net/~zeitgeist-dataproviders/zeitgeist-datasources/trunk/view/head:/telepathy/zeitgeist-telepathy-observer > > and also run the individual test. And watch how it prints out counts and > timestamps while you interact with users via telepathy/empathy I haven't had a chance to run your patches yet (sorry). I also haven't reviewed the test patch — might as well leave it until the main patches are nearer being committed. The most flexible way to test this would be to write a mock-up Zeitgeist D-Bus server, which exposes itself on a private bus and is controlled by the unit tests themselves in order to test folks as a D-Bus client. This is what's done (in a limited form) to test folks’ Telepathy and libsocialweb backends. However, this is a lot of work, especially if Zeitgeist’s D-Bus API is at all stateful. I’ve written Bendy Bus[1] to try and fix the problem of testing D-Bus clients by simulating the D-Bus server, but it’s probably not quite ready for use in real software yet. (Unless you have a bit of time to put into it. :-)) [1]: http://tecnocode.co.uk/2012/01/19/all-aboard-the-bendy-bus/
(In reply to comment #12) > Review of attachment 210540 [details] [review]: > > ::: folks/interaction-details.vala > @@ +60,3 @@ > + * This is emitted if the contact is interacted with in the form of IM or > calls > + */ > + public async signal void interaction_changed (); > > Why is this async? Also, why is a separate signal used instead of GObject’s > ‘notify’ signal? How would the notify signal look like. Keep in mind that all the properties are pull properties and not push.
(In reply to comment #15) > (In reply to comment #12) > > Review of attachment 210540 [details] [review] [details]: > > > > ::: folks/interaction-details.vala > > @@ +60,3 @@ > > + * This is emitted if the contact is interacted with in the form of IM or > > calls > > + */ > > + public async signal void interaction_changed (); > > > > Why is this async? Also, why is a separate signal used instead of GObject’s > > ‘notify’ signal? > > How would the notify signal look like. Keep in mind that all the properties are > pull properties and not push. Well, in individual.vala where you're currently connecting to the interaction-changed signals of all the individual's personas, just connect to the personas' notify::my-counter signals instead. In tpf-persona-store.vala's _handle_new_interaction(), remove the code to emit interaction-changed, and instead add code to _increase_persona_counter to emit the appropriate notify::my-counter signal on the persona. It might make things a bit tidier to also move most of _increase_persona_counter from tpf-persona-store.vala to tpf-persona.vala as an internal method. Just keep the code which looks up Tpf.Personas by their IDs in tpf-persona-store.vala.
Created attachment 211083 [details] [review] Add new InteractionDetails properties to Individual
Created attachment 211084 [details] [review] Modified Telepathy backend to implement new interaction Details
Created attachment 211085 [details] [review] Add dummy test
So I updated the patches according to your reviews. The only thing I did not take a stab at is the "soft-dependency" which I intend to do once the review of the patches passes.
Review of attachment 211083 [details] [review]: Looking better, but still a few things to tidy up. Also, you didn’t answer this question from before (about InteractionDetails): > Are there likely to be more types of interactions added later? I can imagine > this interface getting quite big if it has to have properties for lots of > different types of interactions. If so, it might make sense to make the > interface a bit more generic — though I have no idea what would be better at > the moment. ::: folks/individual.vala @@ +780,3 @@ + * {@inheritDoc} + */ + [CCode (notify = false)] I think all these (notify = false) annotations are actually redundant — they only apply to auto-generated setter methods (which these properties don’t have). @@ +804,3 @@ + + [CCode (notify = false)] + public DateTime last_im_interaction_datetime I think this should probably be nullable (e.g. ‘DateTime?’) to deal with the case where no personas implement InteractionDetails. @@ +818,3 @@ + DateTime interaction_datetime = my_interaction_details.last_im_interaction_datetime; + if (this._last_im_interaction_datetime == null || + interaction_datetime.to_unix () > this._last_im_interaction_datetime.to_unix ()) Would be simpler to use interaction_datetime.compare (this._last_im_interaction_datetime) > 1 here. @@ +856,3 @@ + + [CCode (notify = false)] + public DateTime last_call_interaction_datetime Similarly this, and the declaration above, should have type ‘DateTime?’. @@ +870,3 @@ + var interaction_datetime = my_interaction_details.last_call_interaction_datetime; + if (this._last_call_interaction_datetime == null || + interaction_datetime.to_unix () > this._last_call_interaction_datetime.to_unix ()) Use DateTime.compare() here too. @@ +1042,3 @@ + { + this.notify_property ("im-interaction-count"); + } These functions look good, but it might be helpful to add a comment above them explaining that the properties are pull rather than push, and that these functions are called in response to personas emitting similar notifications. @@ +1603,3 @@ this._persona_group_changed_cb); } + /* Subscribe to the interactions signal for the persona */ You need to add a corresponding block of disconnect()s in _disconnect_from_persona(), or we’ll leak the signal handlers (sort of). @@ +1610,3 @@ + persona.notify["call-interaction-count"].connect (this._notify_call_interaction_count_cb); + persona.notify["last-im-interaction-datetime"].connect (this._notify_last_im_interaction_datetime_cb); + persona.notify["last-call-interaction-datetime"].connect(this._notify_last_call_interaction_datetime_cb); Missing space before the ‘(’. @@ +1612,3 @@ + persona.notify["last-call-interaction-datetime"].connect(this._notify_last_call_interaction_datetime_cb); + //p_interaction_details.interaction_changed.connect(() => + // { this.interaction_changed (); }); These lines can be removed. ::: folks/interaction-details.vala @@ +22,3 @@ + +/** + * Object representing interaction details for an Individual or Persona Might be helpful to briefly explain what “interaction details” are. @@ +24,3 @@ + * Object representing interaction details for an Individual or Persona + * + * See {@link Folks.AbstractFieldDetails}. Eh? This has got nothing to do with AbstractFieldDetails! @@ +26,3 @@ + * See {@link Folks.AbstractFieldDetails}. + * + * @since 0.6.0 Should be ‘@since UNRELEASED’. @@ +45,3 @@ + * @since UNRELEASED + */ + public abstract DateTime last_im_interaction_datetime As mentioned in Individual, this should have type ‘DateTime?’. Same for last_call_interaction_datetime. ::: folks/persona-store.vala @@ +272,3 @@ + * Field for {@link InteractionDetails.im_interaction_count}. + * + * @since 0.6.2 These should all be ‘@since UNRELEASED’.
Review of attachment 211084 [details] [review]: A couple of things were forgotten to be fixed from my first review. Apart from that, it’s looking better. :-) ::: backends/telepathy/lib/tpf-persona-store.vala @@ +71,3 @@ + private Zeitgeist.Log _log; + private Zeitgeist.Monitor _monitor; These should both be nullable, and have default values of null. Please add comments above/after them saying that they’re null only before _populate_counters() has been called. @@ +80,2 @@ private HashMap<string, Persona> _personas; + private HashMap<string, Persona> _uid_personas; Why is this still here? As I said before, you should map between IIDs and UIDs on demand, and just use this._personas as the canonical collection of personas. @@ +239,3 @@ + if (this._monitor == null) + { + this._populate_counters (); Why is this still here? As I said before, it should be moved into prepare(). @@ +741,3 @@ + var new_origin = origin.replace ("x-telepathy-account-path:", ""); + return "telepathy:%s%s:%s".printf (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE, + new_origin, new_uri); In order to remove this._uid_personas, change this printf() to build an _IID_ (see Tpf.Persona’s constructor for the format) and return that instead. Rename the method to “_get_iid_from_event_metadata”. @@ +747,3 @@ + { + /* Check if the persona id and interaction is valid. If so increase the + * appropriate interacton counter and return true, to signify that an “and return true” is outdated. @@ +758,3 @@ + private void _handle_new_interaction (TimeRange timerange, ResultSet events) + { + /* Store an array of personas effected by change */ Comment’s outdated. @@ +761,3 @@ + foreach (var e in events) + { + for (var i=1; i < e.num_subjects(); i++) Missing spaces around the ‘=’ and before the ‘()’ here. @@ +830,3 @@ + this._monitor.events_inserted.connect (this._handle_new_interaction); + this._log.install_monitor (this._monitor); + } This if block isn’t indented correctly. ::: backends/telepathy/lib/tpf-persona.vala @@ +394,3 @@ + + internal uint _im_interaction_count = 0; This (and the others below) can be private, rather than internal, now that they’re only modified by _increase_counter(). @@ +403,3 @@ + * See {@link Folks.InteractionDetails.im_interaction_count} + */ + [CCode (notify = false)] As with Individual, these (notify = false) annotations are redundant since the property doesn’t have an auto-generated setter. @@ +409,3 @@ + } + + internal DateTime _last_im_interaction_datetime = null; Should have type ‘DateTime?’. Same for last_call_interaction_datetime below. @@ +415,3 @@ + * persona. + * + * See {@link Folks.InteractionDetails.last_im_interaction_datetime} Missing a ‘@since’ line. @@ +430,3 @@ + * @since UNRELEASED + * + * See {@link Folks.InteractionDetails.call_interaction_count} With all these properties, please move the ‘@since’ line below the ‘See’ line. @@ +1161,3 @@ } + + internal void _increase_counter (string? id, string? interaction_type, Event event) This line’s indented 2 spaces too far. id and interaction_type can’t be null at this point, so should just have type ‘string’. @@ +1172,3 @@ + { + this._im_interaction_count++; + this.notify_property("im-interaction-count"); Missing space before the ‘(’. Also in a few other places in this method. @@ +1176,3 @@ + this._last_im_interaction_datetime.to_unix () < converted_timestamp) + { + this._last_im_interaction_datetime = new DateTime.from_unix_utc (converted_timestamp); It would simplify the code a bit to move the ‘new DateTime’ call up to the top of the method, where converted_timestamp is calculated at the moment. You can then use DateTime.compare() in the if statements, and just copy the new DateTime to this._last_im_interaction_datetime (etc.) here. @@ +1180,3 @@ + } + debug ("Persona %s IM interaction details changed:\n - count: %u \n - timestamp: %lld\n", + id, this._im_interaction_count, this._last_im_interaction_datetime.to_unix ()); For debug purposes, it would be a lot clearer to print out a human-readable version of this._last_im_interaction_datetime, rather than a Unix timestamp. :-) @@ +1191,3 @@ + this._last_call_interaction_datetime.to_unix () < converted_timestamp) + { + this._last_call_interaction_datetime = new DateTime.from_unix_utc (converted_timestamp); Same as above with this DateTime.
Created attachment 211505 [details] [review] Add new InteractionDetails properties to Individual and Persona
Created attachment 211506 [details] [review] Modified Telepathy backend to implement new interaction Details
I commented on things i did not manage to change. Please have a look at the previous reviews
Review of attachment 211505 [details] [review]: Pretty much there now. ::: folks/individual.vala @@ +800,3 @@ + * {@inheritDoc} + */ + private DateTime _last_im_interaction_datetime; This needs to be nullable and be set to null by default. (If you keep it: see below for a reason why you might remove it altogether.) @@ +806,3 @@ + get + { + this._last_im_interaction_datetime = null; There’s no point in re-calculating the last IM interaction date/time every time the getter is called if you then store it in an object-level variable. Either return the variable if it’s set (and nullify it in _notify_last_im_interaction_datetime_cb()), or just use a local variable here and get rid of the object-level _last_im_interaction_datetime. @@ +850,3 @@ + * {@inheritDoc} + */ + private DateTime _last_call_interaction_datetime = null; Needs to be nullable. @@ +866,3 @@ + var interaction_datetime = my_interaction_details.last_call_interaction_datetime; + if (this._last_call_interaction_datetime == null || + interaction_datetime.compare (this._last_im_interaction_datetime) > 1) s/last_im_interaction/last_call_interaction/ @@ +876,3 @@ + } + + /** Extraneous open-comment delimiter. @@ +1756,3 @@ } + /* Subscribe to the interactions signal for the persona */ s/Subscribe to/Unsubscribe from/
Review of attachment 211506 [details] [review]: (In reply to comment #25) > I commented on things i did not manage to change. Please have a look at the > previous reviews Sorry, but I can’t find any comments about the two points below which I raised before. These are fairly major points, so there’s little point in me reviewing attachment #211506 [details] until they’re addressed. If I’ve missed your comments about them, please direct me to them more obviously. :-) (In reply to comment #22) > ::: backends/telepathy/lib/tpf-persona-store.vala > @@ +80,2 @@ > private HashMap<string, Persona> _personas; > + private HashMap<string, Persona> _uid_personas; > > Why is this still here? As I said before, you should map between IIDs and UIDs > on demand, and just use this._personas as the canonical collection of personas. > > @@ +239,3 @@ > + if (this._monitor == null) > + { > + this._populate_counters (); > > Why is this still here? As I said before, it should be moved into prepare().
(In reply to comment #27) > Review of attachment 211506 [details] [review]: > > (In reply to comment #25) > > I commented on things i did not manage to change. Please have a look at the > > previous reviews > > Sorry, but I can’t find any comments about the two points below which I raised > before. These are fairly major points, so there’s little point in me reviewing > attachment #211506 [details] until they’re addressed. If I’ve missed your comments about > them, please direct me to them more obviously. :-) > > (In reply to comment #22) > > ::: backends/telepathy/lib/tpf-persona-store.vala > > @@ +80,2 @@ > > private HashMap<string, Persona> _personas; > > + private HashMap<string, Persona> _uid_personas; > > > > Why is this still here? As I said before, you should map between IIDs and UIDs > > on demand, and just use this._personas as the canonical collection of personas. > > > > @@ +239,3 @@ > > + if (this._monitor == null) > > + { > > + this._populate_counters (); > > > > Why is this still here? As I said before, it should be moved into prepare(). I think the link for the review is https://bugzilla.gnome.org/review?bug=672709&attachment=211084
Review of attachment 211084 [details] [review]: ::: backends/telepathy/lib/tpf-persona-store.vala @@ +239,3 @@ + if (this._monitor == null) + { + this._populate_counters (); Can you tell me where is it best to put it in prepare? The problem is I need the contact list to be ready before i start populating the counters. @@ +741,3 @@ + var new_origin = origin.replace ("x-telepathy-account-path:", ""); + return "telepathy:%s%s:%s".printf (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE, + * x-telepathy-account-path identifies the account used with the contact the problem here is that i cant not extract the iid out of the event. since the iid looks something like jabber:foo@bar.org ==> I am missing the jabber part in the logging
(In reply to comment #29) > Review of attachment 211084 [details] [review]: > > ::: backends/telepathy/lib/tpf-persona-store.vala > @@ +239,3 @@ > + if (this._monitor == null) > + { > + this._populate_counters (); > > Can you tell me where is it best to put it in prepare? The problem is I need > the contact list to be ready before i start populating the counters. Ah, I see. Then it should probably be called wherever this._got_stored_channel_members is set to true, which is basically equivalent to what you’ve got already, but doesn’t mess up the semantics of notify_if_is_quiescent(). However, bug #630822 will change (and simplify) the way Tpf.PersonaStore is prepare()d so I think it might be best to wait until that’s committed, then rebase on it. You can then wrap the new version of _notify_if_is_quiescent() with a method which calls _populate_counters() first, or something. (I’d rather commit bug #630822 before this patch, because this patch should be a lot easier to rebase.) > @@ +741,3 @@ > + var new_origin = origin.replace ("x-telepathy-account-path:", ""); > + return "telepathy:%s%s:%s".printf > (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE, > + * x-telepathy-account-path identifies the account used with the contact > > the problem here is that i cant not extract the iid out of the event. since the > iid looks something like jabber:foo@bar.org ==> I am missing the jabber part in > the logging In the worst case, you could use tp_account_parse_object_path() on (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE + new_origin). However, I think it would be best to just use: Persona.build_uid (store.type_id, store.id, new_uri); and make sure that the Zeitgeist Log only ever returns events for the relevant Tpf.PersonaStore, which I think is already the case due to the code in _get_zeitgeist_event_templates().
(In reply to comment #30) > (In reply to comment #29) > > Review of attachment 211084 [details] [review] [details]: > > > > ::: backends/telepathy/lib/tpf-persona-store.vala > > @@ +239,3 @@ > > + if (this._monitor == null) > > + { > > + this._populate_counters (); > > > > Can you tell me where is it best to put it in prepare? The problem is I need > > the contact list to be ready before i start populating the counters. > > Ah, I see. Then it should probably be called wherever > this._got_stored_channel_members is set to true, which is basically equivalent > to what you’ve got already, but doesn’t mess up the semantics of > notify_if_is_quiescent(). > > However, bug #630822 will change (and simplify) the way Tpf.PersonaStore is > prepare()d so I think it might be best to wait until that’s committed, then > rebase on it. You can then wrap the new version of _notify_if_is_quiescent() > with a method which calls _populate_counters() first, or something. So I should wait? > (I’d rather commit bug #630822 before this patch, because this patch should be > a lot easier to rebase.) > > > @@ +741,3 @@ > > + var new_origin = origin.replace ("x-telepathy-account-path:", ""); > > + return "telepathy:%s%s:%s".printf > > (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE, > > + * x-telepathy-account-path identifies the account used with the contact > > > > the problem here is that i cant not extract the iid out of the event. since the > > iid looks something like jabber:foo@bar.org ==> I am missing the jabber part in > > the logging > > In the worst case, you could use tp_account_parse_object_path() on > (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE + new_origin). However, I think it > would be best to just use: > Persona.build_uid (store.type_id, store.id, new_uri); > and make sure that the Zeitgeist Log only ever returns events for the relevant > Tpf.PersonaStore, which I think is already the case due to the code in > _get_zeitgeist_event_templates(). Your suggestion implied the persona is already there and I am building an ID for it. I need the id to be able to get the persona. build_uid is not a static method.
Created attachment 211847 [details] [review] Add new InteractionDetails properties to Individual and Persona
(In reply to comment #31) > So I should wait? As discussed on IRC, you can either start rebasing now (given that the Tpf.PersonaStore stuff probably won’t change much now) or wait. Up to you. > Your suggestion implied the persona is already there and I am building an ID > for it. I need the id to be able to get the persona. build_uid is not a static > method. Folks.Persona.build_uid() is public and static, so unless I’ve missed something it should work OK. :-)
Created attachment 211863 [details] [review] Modified Telepathy backend to implement new interaction Details
I see that the pending merge of bug #620833 is done. How do I proceed?
(In reply to comment #35) > I see that the pending merge of bug #620833 is done. How do I proceed? Rebase and fix the comments in #30. It would be nice if you could get the test case we discussed done, but I don’t think that should be necessary for the initial merge.
I fixed everything from #30 except for the this._populate_counters placement. I am still not sure where to place it exactly :(
Created attachment 212325 [details] [review] Add new InteractionDetails properties to Individual
Created attachment 212326 [details] [review] Modified Telepathy backend to implement new interaction Details
I reverted all changes to _notify_if_is_quiescent and added the the populate counter exactly after _got_initial_members is set to true (not forced)
Review of attachment 212325 [details] [review]: This is ready to be merged, save for one indentation problem. Once the other patch is ready, and this indentation problem is fixed, this can be merged. :-) ::: folks/individual.vala @@ +789,3 @@ + { + var my_interaction_details = persona as InteractionDetails; + if (my_interaction_details != null) { Brace should be indented on the next line.
Review of attachment 212326 [details] [review]: Still to do: • Convert the Zeitgeist dependency into a soft-dependency. • Write the test case with a mock Zeitgeist D-Bus server (not a blocker for merging). • Remove _uid_personas and fix the issues below. ::: backends/telepathy/lib/tpf-persona-store.vala @@ +47,3 @@ * This is the roster + self_contact */ private HashMap<string, Persona> _personas; + private HashMap<string, Persona> _uid_personas; This still needs to be removed. @@ +82,3 @@ + private Zeitgeist.Log _log= null; + private Zeitgeist.Monitor _monitor = null; These still need to be nullable. @@ +1029,3 @@ + { + this._populate_counters (); + } This approach looks good to me, but the indentation’s wrong. I think you could also add this code to _force_quiescent(). _force_quiescent() is called when the connection is offline, so the contact list is loaded from its on-disk cache instead. Zeitgeist should still work in this case (I think), so we might as well still populate the counters. ::: backends/telepathy/lib/tpf-persona.vala @@ +405,3 @@ + } + + internal DateTime _last_im_interaction_datetime = null; This can be private, and needs to be nullable. @@ +434,3 @@ + } + + internal DateTime _last_call_interaction_datetime = null; This can also be private, and also needs to be nullable. @@ +1187,3 @@ } + + internal void _increase_counter (string? id, string? interaction_type, Event event) id and interaction_type don’t need to be nullable. @@ +1206,3 @@ + this.notify_property ("last-im-interaction-datetime"); + } + debug ("Persona %s IM interaction details changed:\n - count: %u \n - timestamp: %lld\n", Is the format placeholder ‘%lld’ really correct for a string-formatted date-time? @@ +1221,3 @@ + this.notify_property ("last-call-interaction-datetime"); + } + debug ("Persona %s Call interaction details changed:\n - count: %u \n - timestamp: %lld\n", Same format placeholder problem as above.
Review of attachment 212326 [details] [review]: ::: backends/telepathy/lib/tpf-persona-store.vala @@ +47,3 @@ * This is the roster + self_contact */ private HashMap<string, Persona> _personas; + private HashMap<string, Persona> _uid_personas; Oh. Didn't we discuss on IRC that it is a bit difficult. As discussed before: > the problem here is that i cant not extract the iid out of the event. since the > iid looks something like jabber:foo@bar.org ==> I am missing the jabber part in > the logging In the worst case, you could use tp_account_parse_object_path() on (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE + new_origin). However, I think it would be best to just use: Persona.build_uid (store.type_id, store.id, new_uri); and make sure that the Zeitgeist Log only ever returns events for the relevant Tpf.PersonaStore, which I think is already the case due to the code in _get_zeitgeist_event_templates().
(In reply to comment #43) > > the problem here is that i cant not extract the iid out of the event. since the > > iid looks something like jabber:foo@bar.org ==> I am missing the jabber part in > > the logging > > In the worst case, you could use tp_account_parse_object_path() on > (TelepathyGLib.ACCOUNT_OBJECT_PATH_BASE + new_origin). However, I think it > would be best to just use: > Persona.build_uid (store.type_id, store.id, new_uri); > and make sure that the Zeitgeist Log only ever returns events for the relevant > Tpf.PersonaStore, which I think is already the case due to the code in > _get_zeitgeist_event_templates(). Sorry, I got that wrong. You should be able to use: this.account.protocol + ":" + new_uri to build the IID. I was giving instructions for building the UID before which was, obviously, wrong. However, as I said before, this depends on the Zeitgeist Log only ever returning events for this Tpf.PersonaStore, which I think is the case already.
OK now I am totally confused. This works nicely. What is it I should change. The logs from zeitgeist need the UID since this is what is stored. I cant get data from zeitgeist with the IID. The entries in zeitgeist are missing the this.account.protocol part in the ids. I need the full telepathy account path since this is what is stored in zeitgeist :(
(In reply to comment #45) > OK now I am totally confused. This works nicely. What is it I should change. > The logs from zeitgeist need the UID since this is what is stored. I cant get > data from zeitgeist with the IID. > > The entries in zeitgeist are missing the this.account.protocol part in the > ids. > I need the full telepathy account path since this is what is stored in > zeitgeist :( Untested, but I think this should work. + private string? _get_iid_from_event_metadata (string? uri, string? origin) + { + /* Format a proper IID represting a persona in the store. + * Zeitgeist uses x-telepathy-identifier as a prefix for telepathy, which + * which is stored as the uri of a subject of an event. While + * x-telepathy-account-path identifies the account used with the contact + * and is stored in the event.origin. */ + if (uri == null || origin == null) + { + return null; + } + + var new_uri = uri.replace ("x-telepathy-identifier:", ""); + return this.account.protocol + ":" + new_uri; + }
Created attachment 213658 [details] [review] Modified Telepathy backend to implement new interaction Details I started the soft dependency but for some reason (I can't figure out why) it is not working :( Can you help me find out what the problem is. You can see the changes in the patch.
Review of attachment 213658 [details] [review]: The main problem I can think of here is that you haven’t modified build-conf.vapi in order to expose HAVE_ZEITGEIST to Vala. Without knowing what compiler errors you’re getting, though, this is just a guess. (Please provide compiler errors in future.) You also need to add a VALA_CHECK_PACKAGES() macro call in configure.ac for the Zeitgeist Vala bindings (see how $enable_tracker_backend does it). ::: backends/telepathy/lib/Makefile.am @@ +123,3 @@ --pkg gee-1.0 \ --pkg telepathy-glib \ + --pkg zeitgeist-1.0 \ This needs to be made conditional. e.g.: if HAVE_ZEITGEIST libfolks_telepathy_la_VALAFLAGS += --pkg zeitgeist-1.0 endif ::: configure.ac @@ +102,3 @@ + +AC_ARG_ENABLE(zeitgeist-backend, This should probably be named “zeitgeist” rather than “zeitgeist-backend”, since it’s technically not building a separate backend. @@ +194,2 @@ PKG_CHECK_MODULES([TP_GLIB], [telepathy-glib >= $TP_GLIB_REQUIRED]) +PKG_CHECK_MODULES([ZEITGEIST], [zeitgeist-1.0 >= $ZEITGEIST_REQUIRED]) This needs to be inside an “if test x$enable_zeitgeist = xyes; then” block.
Created attachment 214200 [details] [review] Modified Telepathy backend to implement new interaction Details So here I cleaned it up. However I don't get an error. But rather #if HAVE_ZEITGEIST is never recognized. So I am stuck
(In reply to comment #49) > Created an attachment (id=214200) [details] [review] > Modified Telepathy backend to implement new interaction Details > > So here I cleaned it up. However I don't get an error. But rather > #if HAVE_ZEITGEIST > is never recognized. So I am stuck You need to (unconditionally) add “using BuildConf” to the top of any file which uses “#if HAVE_ZEITGEIST”.
Yeah, but isn't BuildConf part of the Folks namespace? why isn't "using BuildConf;" working then?
(In reply to comment #51) > Yeah, but isn't BuildConf part of the Folks namespace? why isn't "using > BuildConf;" working then? This doesn't work because the Telepathy backend doesn't include build-conf amongst its packages (compare folks/Makefile.am and backends/telepathy/lib/Makefile.am). I initially intended to make the backends not depend upon build-conf, since, eg, if the Telepathy backend cares about whether the Libsocialweb backend is built, that starts setting up some hairy inter-dependencies. On the other hand, we'd have to do something similar to support the soft dependency, so I suppose this isn't too big of a deal. Equivalent setups are fairly common in other projects.
Created attachment 214764 [details] [review] Modified Telepathy backend to implement new interaction Details
Created attachment 214778 [details] [review] Added new interaction properties to Individual and Persona
Created attachment 214779 [details] [review] Modified Telepathy backend to implement new interaction Details
So this is done now. Please review. I will add test cases later next week. Right now we are making zeitgeist-datahub do all the observing logging by default. This way Telepathy needs not to know about Zeitgeist :D
Review of attachment 214778 [details] [review]: Looks good to me.
Review of attachment 214779 [details] [review]: A couple of the points from comment #42 haven’t been fixed yet. Please fix these and commit. I don’t want to see another iteration of this patch! ::: backends/telepathy/lib/tpf-persona.vala @@ +54,3 @@ private bool _is_constructed = false; + internal bool _can_notify_interactions = false; This can be private. @@ +409,3 @@ + } + + internal DateTime _last_im_interaction_datetime = null; This can also be private, and also needs to be nullable. @@ +438,3 @@ + } + + internal DateTime _last_call_interaction_datetime = null; This can also be private, and also needs to be nullable. @@ +1203,3 @@ + +#if HAVE_ZEITGEIST + internal void _increase_counter (string? id, string? interaction_type, Event event) id and interaction_type don’t need to be nullable. @@ +1224,3 @@ + this.notify_property ("last-im-interaction-datetime"); + } + debug ("Persona %s IM interaction details changed:\n - count: %u \n - timestamp: %lld\n", Is the format placeholder ‘%lld’ really correct for a string-formatted date-time? @@ +1241,3 @@ + this.notify_property ("last-call-interaction-datetime"); + } + debug ("Persona %s Call interaction details changed:\n - count: %u \n - timestamp: %lld\n", Same format placeholder problem as above.
Review of attachment 214779 [details] [review]: ::: backends/telepathy/lib/tpf-persona.vala @@ +54,3 @@ private bool _is_constructed = false; + internal bool _can_notify_interactions = false; I kinda modify it from tpf-presona-store. Should I create a set/get for it then if I make it private. I need it the first time i started increasing the counters, to no emit a signal on each during the "populate_counters" phase.
(In reply to comment #59) > Review of attachment 214779 [details] [review]: > > ::: backends/telepathy/lib/tpf-persona.vala > @@ +54,3 @@ > private bool _is_constructed = false; > > + internal bool _can_notify_interactions = false; > > I kinda modify it from tpf-presona-store. > Should I create a set/get for it then if I make it private. > I need it the first time i started increasing the counters, to no emit a signal > on each during the "populate_counters" phase. Oh, sorry, I missed that change. Please make sure to reply to all points in a review (either with a change to the patch, or a reason why the reviewer’s wrong) so that things don’t get missed. Anyway, this seems like a bit of an ugly way of implementing that. In general, I think using the ‘internal’ access specifier is a bit of a hack. Would it work to call persona.freeze_notify() before this._increase_persona_counter() for each persona in the loop in _populate_counters(). Then you can call persona.thaw_notify() in a second loop (over the ‘results’ variable) afterwards. This should give the same behaviour (and won’t result in duplicate signals being emitted if _increase_persona_counter() is called multiple times on a single persona), but without needing to add the ‘internal’ variable. You just need to make sure that freeze_notify() and thaw_notify() are called the same number of times for each persona.
so I tried what you suggested as follows. http://pastebin.com/8uHCJLEm The problem with that is that it seems that thaw notify is called but executed AFTER the 4 following notifications are sent out but frozen by the freeze that was set. Does that mean that thaw_notify waits for an idle signal to operate. It just isn't working for me.
(In reply to comment #61) > so I tried what you suggested as follows. > http://pastebin.com/8uHCJLEm > The problem with that is that it seems that thaw notify is called but executed > AFTER the 4 following notifications are sent out but frozen by the freeze that > was set. Does that mean that thaw_notify waits for an idle signal to operate. > It just isn't working for me. You need to be careful to ensure that freeze_notify() and thaw_notify() are called the same number of times for each persona. What I had in mind was: foreach (var e in results) { var interaction_type = e.get_subject (0).get_interpretation (); for (var i = 1; i < e.num_subjects (); i++) { var id = this._get_iid_from_event_metadata (e.get_subject (i).get_uri ()); this._personas.get (id).freeze_notify (); this._increase_persona_counter (id, interaction_type, e); } } foreach (var e in results) { var interaction_type = e.get_subject (0).get_interpretation (); for (var i = 1; i < e.num_subjects (); i++) { this._personas.get (id).thaw_notify (); } } thaw_notify() can’t be called in a loop over the ‘ids’ variable, as then it will only be called at most once for each Persona. There’s also no need to explicitly call notify_property() after calling thaw_notify(), since all the property notifications made by _increase_persona_counter() while the Personas were frozen are queued up and emitted when thaw_notify() is called enough times.
So I kept trying your suggestion with no success... I event tried very obvious stuff like: --- foreach (var persona in this.personas.values) { persona.freeze_notify (); } foreach (var e in results) { var interaction_type = e.get_subject (0).get_interpretation (); for (var i = 1; i < e.num_subjects (); i++) { var id = this._get_iid_from_event_metadata (e.get_subject (i).get_uri ()); this._increase_persona_counter (id, interaction_type, e); ids.add (id); } } foreach (var persona in this.personas.values) { persona.thaw_notify (); } --- That does not work. Somehow thaw_notify does not unleash the notifications.
sorry for the duplicate entry. I found the problem, it was in another class. So the code you suggested does not work really, but the code i just posted which is based on your idea does. Is that enough?
(In reply to comment #65) > sorry for the duplicate entry. I found the problem, it was in another class. So > the code you suggested does not work really, but the code i just posted which > is based on your idea does. Is that enough? Yeah, that’ll do. :-) Please commit to master after removing any remnants of the ‘ids’ variable (such as the “ids.add (id);” in comment #64)!
done
Please update NEWS: * include this bug under "Bugs fixed" * list all the new public symbols under "API changes" Thanks for all this work!
Created attachment 224027 [details] [review] Reduce timerang of interest to 30 days only This patch modifies the counter to take only the events of the last 30 days into account to reduce the amount of noise when trying to sort by most popular. This way empathy can sort its most popular within the last 3 days easily. Also reduced the needed code for querying Zeitgeist
(In reply to comment #69) > Created an attachment (id=224027) [details] [review] > This patch modifies the counter to take only the events Seif: This bug report is RESOLVED FIXED so not sure if anybody will see and review your patches here.
Review of attachment 224027 [details] [review]: OK. Please commit to master, and make sure to update the NEWS file. In future, please open a new bug for new patches like this.
Comment on attachment 224027 [details] [review] Reduce timerang of interest to 30 days only …or I could commit it. commit ee359e89f3332d2c8cb1bd29e8ea54e62798d65b Author: Seif Lotfy <seif@lotfy.com> Date: Tue Sep 11 12:10:40 2012 +0200 telepathy: Update behaviour of Zeitgeist properties Counter works for last 30 days instead of all time since it can mess up calculations easily. Reduce query size to Zeitgeist since events are filtered in the Persona class. Closes: https://bugzilla.gnome.org/show_bug.cgi?id=672709 (again) NEWS | 2 ++ backends/telepathy/lib/tpf-persona-store.vala | 17 +++++++---------- 2 files changed, 9 insertions(+), 10 deletions(-)
Comment on attachment 214778 [details] [review] Added new interaction properties to Individual and Persona This was committed ages ago.