[Bug 26590] GetFrequentContacts
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Mon Apr 26 18:57:32 CEST 2010
https://bugs.freedesktop.org/show_bug.cgi?id=26590
Cosimo Alfarano <cosimo.alfarano at collabora.co.uk> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard| |review-
--- Comment #4 from Cosimo Alfarano <cosimo.alfarano at collabora.co.uk> 2010-04-26 09:57:31 PDT ---
my rebased tree, on which I did my review, is at
http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/danni-get-frequent-contacts
970196c97d84814e0f8d269dc79c141a67d17148:
extentions/Logger.xml
+ <arg direction="out" name="Frequent_Contacts" type="a(sd)"
+ tp:type="Frequent_Contact[]">
Simple observation in case it wasn't meant: I usually see Frequent_Contact_List
to indicate an array of Frequent_Contact.
in tpl_dbus_service_get_recent_contacts()
+ g_list_foreach (chats, (GFunc) tpl_log_manager_search_hit_free, NULL);
@chats is obtained by tpl_log_manager_get_chats() which might return a quite
long list of entries.
Probably it would be better to use
g_list_delete_link() against the list's head, instead.
Maybe it's just me, but I usually prefer that way, even if it doesn't change
from being O(n).
e78fa71ad34a34e0db2884ec750a09451a097df3:
+ <arg direction="out" name="Frequent_Contacts" type="a(sd)"
+ tp:type="Frequent_Contact[]">
Same above idea.
in tpl_dbus_service_get_frequent_contacts()
+ g_list_foreach (chats, (GFunc) tpl_log_manager_search_hit_free, NULL);
same comment about using g_list_delete_link(), if you liked the idea.
+ g_value_set_double (value, g_value_get_double (value) / maxfreq);
I'd also write
g_value_set_double (value, (g_value_get_double (value) / maxfreq));
which IMO is more readable.
3ea381791c984a3997bd4c3a0169b2979c6c9943:
in tpl_dbus_service_get_recent_contacts() and
tpl_dbus_service_get_frequent_contacts():
- if (hit->is_chatroom && !include_chatrooms)
+ if (!((filter_flags & TPL_IDENTIFIER_FILTER_ROOM && hit->is_chatroom) ||
+ (filter_flags & TPL_IDENTIFIER_FILTER_CONTACT &&
!hit->is_chatroom)
+ ))
continue;
Even though it's pretty intuitive what you're doing, I'd use more parenthesis
to help the eye to identify single expressions and probably split it into a "if
... else if ..." which IMO is more readable.
f600080726fd7a7d3e4c0948a70c852ada41822b:
telepathy-logger/Makefile.am:
+ $(BUILT_SOURCES)
$(NULL)
would probably be
+ $(BUILT_SOURCES) \
$(NULL)
27ef375add699f94cec8debc860cb4cd9bbb17c7:
@@ -734,25 +700,17 @@ tpl_dbus_service_get_frequent_contacts (TplSvcLogger
*self,
/* copy from the GList to the GPtrArray */
for (ptr = contacts; ptr != NULL; ptr = ptr->next)
{
- GValueArray *varray = ptr->data;
- GValue *value = g_value_array_get_nth (varray, 1);
-
- /* normalise the value on [0, 1] */
- g_value_set_double (value, g_value_get_double (value) / maxfreq);
- g_ptr_array_add (array, varray);
+ g_ptr_array_add (array, ptr->data);
}
g_list_free (contacts);
- tpl_svc_logger_return_from_get_recent_contacts (context, array);
+ tpl_svc_logger_return_from_get_frequent_contacts (context, array);
This should be in 27ef375add699f94cec8debc860cb4cd9bbb17c7 and not in a
tpl_get_account() related commit.
cb7a6556b854e49645fd915c879fd330b2de5411:
+ <arg name="Frequent_Contacts" type="a(sd)" tp:type="Frequent_Contact[]">
+ <tp:docstring>
+ A list of the contacts whos frequency has changed and their new
+ frequency.
+ </tp:docstring>
+ </arg>
typo, s/whos/whose/
@@ -552,6 +570,35 @@ tpl_log_store_sqlite_add_message_counter (TplLogStore
*self,
goto out;
}
+ {
+ const char *path;
+ TpAccount *tpaccount;
+ double frequency;
+ GError *lerror = NULL;
+
+ path = tpl_log_entry_get_account_path (message);
+ tpaccount = tpl_get_tp_account (path, &lerror);
+
+ if (lerror != NULL)
+ {
+ DEBUG ("Error getting TpAccount: %s", lerror->message);
+ g_error_free (lerror);
+ lerror = NULL;
+ }
+ else
+ {
+ // FIXME: keep a cache of frequencies, the new frequency is just
+ // the old frequency +1 (unless first message after midnight)
+ frequency = tpl_log_store_sqlite_get_frequency (
+ TPL_LOG_STORE_SQLITE (self), tpaccount, identifier);
+
+ g_signal_emit (self, _signals[FREQUENCY_CHANGED], 0,
+ path, identifier, frequency);
+
+ g_object_unref (tpaccount);
+ }
+ }
+
retval = TRUE;
Is this isolated block intended or just a temporary thing?
You might put it in an "else" branch or create a separate function for it since
it's big enough and does exactly one thing.
4d1c28d70f287005574cc0c28a4dccd0ac2a5174:
@@ -693,47 +693,117 @@ tpl_dbus_service_get_frequent_contacts (TplSvcLogger
*self,
GError *error = NULL;
double maxfreq = 0;
- account = tpl_get_tp_account (account_path, &error);
- if (account == NULL)
+ if (!tp_strdiff (account_path, "/"))
...
+ else
+ {
+ DEBUG ("Looking up frequent contacts for account %s", account_path);
Isn't it possible to call from the first "if" branch
tpl_dbus_service_get_frequent_contacts(account_path) for each valid account?
It would remove a lot of duplicated code.
+ /* create a GValueArray for this contact, and store it in a GList */
+ contacts = g_list_insert_sorted (contacts,
+ tp_value_array_build (3,
+ DBUS_TYPE_G_OBJECT_PATH, tp_proxy_get_object_path (account),
+ G_TYPE_STRING, hit->chat_id,
+ G_TYPE_DOUBLE, freq,
+ G_TYPE_INVALID),
+ cmp_frequent_contacts);
Why not passing directly account_path that you already have (when in "else"
branch above)
The rest seems OK, you just need to squash commits after everything is OK.
If you use my rebased branch, be aware that in
e78fa71ad34a34e0db2884ec750a09451a097df3
+ TplLogStore *store = tpl_log_store_counter_dup ();
should be changed in tpl_log_store_sqlite_dup().
There are also other occurrences in the module using _counter_ instead of
_sqlite_.
Squashing c1232feacceb10a0f9197aed17130076887f6736 into
e78fa71ad34a34e0db2884ec750a09451a097df3 would fix it.
970196c97d84814e0f8d269dc79c141a67d17148 also had some references to _counter_
symbols, but rebasing there was a conflict so I fixed it to make it compile.
--
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.
More information about the telepathy-bugs
mailing list