GNOME Bugzilla – Bug 518414
Conversation logger should be a separated process
Last modified: 2011-08-29 10:12:28 UTC
As discussed yesterday during the great GNOME beer event, the conversation logger should be a separated process. We should log each conversation, even if the chat window isn't opened.
I agree but I need MC-nextgen channel dispatcher for that.
Also note that received messages have to be logged but unseen messages (as in "not displayed in a chat window") should probably trigger a "new message" notification when empathy starts up later.
*** Bug 549284 has been marked as a duplicate of this bug. ***
This is now possible in the brave MC5 world.
We should have an empathy-logger process implementing http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Client.Observer.html
Currently 17 Empathy tickets are set as GNOME 2.30 blockers, hence mass-removing. Guillaume: Please use normal Target Milestones instead. If you really think that this specific issue here is a 2.30 blocker then please restore the GNOME target and set corresponding importance values.
Current plan is to have a generic telepathy-logger. Empathy will be able to interact with it using a D-Bus API and possibly a lib for fetching log. http://telepathy.freedesktop.org/wiki/Logger
*** Bug 603906 has been marked as a duplicate of this bug. ***
Here http://git.collabora.co.uk/?p=user/kalfa/empathy-tpl.git/.git;a=summary can be found the Empathy branch supporting for Telepathy Logger support. It needs also the Telepathy Logger available from git, more info on http://telepathy.freedesktop.org/wiki/Logger (AKA TPL). Both are currently experimental. My suggestion to whom is willing to try it, is to install both empathy-tpl and TPL under /usr/local/empathy-tpl so that it won't touch any other lib/data and it will be easy to remove.
Created attachment 152010 [details] [review] Diff with master Attaching diff for easier review.
Review of attachment 152010 [details] [review]: I inlined some comments after a first quick review. Be sure that your branch pass "make check". What are the plans regarding tp-logger releases and API stability? If needed maybe we could ship it in the empathy source tree first using git submodule (as we do for Wocky in Gabble)? ::: libempathy-gtk/empathy-chat.c @@ +1737,3 @@ + +static void +_got_filtered_messages_cb (TplLogManager *manager, gpointer result, Don't prefix with '_'. @@ +1744,3 @@ + EmpathyChat *chat = EMPATHY_CHAT (user_data); + + if (error!=NULL) { Should be: "error != NULL" @@ +1793,3 @@ + chat, + _got_filtered_messages_cb, + (gpointer) chat, NULL); No need to cast. ::: libempathy-gtk/empathy-log-window.c @@ +343,2 @@ static void +_got_messages_for_date (TplLogManager *manager, gpointer result, No need to prefix static function with '_'. @@ +351,3 @@ + gboolean can_do_next; + + if (error!=NULL) { error != NULL @@ +355,3 @@ + empathy_message_new("Unable to retrieve messages for the selected date"); + DEBUG ("%s. Aborting", error->message); + empathy_chat_view_append_message (window->chatview_find, m); Shouldn't you use empathy_chat_view_append_event instead? @@ +439,3 @@ + date, + _got_messages_for_date, + (gpointer) window, NULL); No need to cast. @@ +454,2 @@ GtkTreeIter iter; + GtkListStore *store = (GtkListStore*) user_data; Casts from gpointer are implicit. @@ +457,1 @@ + if (error!=NULL) { error != NULL @@ +954,3 @@ +static void +_log_window_got_messages_for_date(TplLogManager *manager, + gpointer result, GError *error, gpointer user_data) Each arg should be on its own line. @@ +960,3 @@ + GList *l; + + Double \n @@ +965,3 @@ + empathy_message_new("Unable to retrieve messages for the selected date"); + DEBUG ("%s. Aborting", error->message); + empathy_chat_view_append_message (window->chatview_chats, m); Use append_event ::: libempathy/empathy-contact.c @@ +411,3 @@ + +EmpathyContact * Do we really need this function to be public? It's only used in this file. @@ +412,3 @@ + +EmpathyContact * +empathy_contact_from_details (const gchar *id, const gchar *name, gboolean is_user) This file is the TP coding style: http://telepathy.freedesktop.org/wiki/Style so no tab, one line per arg... @@ +427,3 @@ + TpContact *tp_contact; + + g_return_val_if_fail (TPL_IS_CONTACT(tpl_contact), NULL); TPL_IS_CONTACT (tpl_contact) @@ +430,3 @@ + + tp_contact = tpl_contact_get_contact (tpl_contact); + if (tp_contact) if (tp_contact != NULL) ::: libempathy/empathy-message.c @@ +265,3 @@ + g_return_val_if_fail (TPL_IS_LOG_ENTRY (logentry), NULL); + + //TODO TPL TplLogEntry API update What does that mean? Also, don't use C++ style comments. @@ +266,3 @@ + + //TODO TPL TplLogEntry API update + body = g_strdup (tpl_log_entry_text_get_message (logentry->entry.text)); Why are you dupping? You can pass it directly to message_new() ::: src/empathy.c @@ +1017,3 @@ /* Logging */ + log_manager = tpl_log_manager_dup_singleton (); + //tpl_log_manager_observe (log_manager, dispatcher); You don't need that anymore as the logging is done by the tp-logger now.
After discussions with Frédéric and Vincent from the release team, we agreed that the best way to integrate the tp-logger for GNOME 2.30 would be to use a git submodule. We could also have a configure option to use the system tp-logger instead of the copy shipped with Empathy.
Some more comments: empathy_message_from_tpl_log_entry: I'd use a simple if (check) return; instead of g_return_val_if_fail to avoid to raise critical warnings when user will upgrade their libtp-logger. empathy-chat - got_filtered_messages_cb: messages = (GList*) tpl_log_manager_async_operation_finish(result, &error); should be (GList *) tpl_log_manager_async_operation_finish (result, &error); also, this function and the GError declaration are not tab indented. Same problem in empathy-log-window.c:got_messages_for_date and log_manager_searched_new Btw, any reason with this function returns a gpointer instead of a GList* ? Having a generic _finish function used with all the _async calls seems weird to me. empathy-log-window this file still use the old style :( so new code should be tabs indented. log_manager_searched_new: I'd suffix this function with _cb to make it clearer that's a callback log_window_chats_get_messages: the comment about the unblock signal is not tab indented. Also, log_manager_got_dates doesn't unblock the signal in case of early return.
I fixed most of the coding style issues yesterday, they are still in my repo waiting for a general check later today, when I'll add the avatar_token code to the log-window. Committing later today along with the avatar code.
Could you push your latest branch please? Some more comments: + old_logstore = g_object_new (TPL_TYPE_LOG_STORE_EMPATHY, "name", "Empathy", + "writable", FALSE, "readable", TRUE, NULL); Surely, tp-logger should have a _new helper function for this. + //tpl_log_manager_register_logstore (log_manager, old_logstore); Why is this commented?
Created attachment 154128 [details] [review] avatar support Attaching diff of http://git.collabora.co.uk/?p=user/kalfa/empathy.git/.git;a=commitdiff;h=51c3f0aeb53ce02cf3ef8fff99e99a6ec514d4a4 for easier reviewing.
Review of attachment 154128 [details] [review]: ::: libempathy/empathy-contact.c @@ +412,2 @@ EmpathyContact * +empathy_contact_from_tpl_contact (TpAccount *account, TplContact *tpl_contact) 2nd arg should be on its own line. @@ +415,3 @@ + EmpathyContact *retval; + const gchar *id; + const gchar *alias; No need to use an id and alias variable. @@ +424,3 @@ + id = tpl_contact_get_identifier (tpl_contact); + avatar_token = tpl_contact_get_avatar_token (tpl_contact); + is_user = TPL_CONTACT_USER == tpl_contact_get_contact_type (tpl_contact); please add () to make this easier to read @@ +429,3 @@ + "id", id, + "name", alias, + "account", account, alignement is wrong. ::: libempathy/empathy-message.c @@ +261,3 @@ + EmpathyMessage *retval = NULL; + TpDBusDaemon *bus_daemon = NULL; + TpAccount *account = NULL; alig is wrong. @@ +272,3 @@ + if (bus_daemon == NULL) + { + /* TODO enable empathy debugger What's this TODO? @@ +279,3 @@ + + account = tp_account_new (bus_daemon, + tpl_log_entry_get_account_path (logentry), &error); Maybe you should use tp_account_manager_ensure_account instead.
I'm now using tp_account_manager_ensure_account() thus I do not need anymore any call to DEBUG(). The TODO was a memo about missing DEBUG FLAG for empathy-message module, so DEBUG() cannot be used. Is it any reason for which there is no flag? Just in case I need it in the future, I won't add it if it's meant not to be there. Pushed new fixes in http://git.collabora.co.uk/?p=user/kalfa/empathy.git/.git;a=shortlog;h=refs/heads/empathy-tpl-20100217
(In reply to comment #18) > The TODO was a memo about missing DEBUG FLAG for empathy-message module, so > DEBUG() cannot be used. > > Is it any reason for which there is no flag? > Just in case I need it in the future, I won't add it if it's meant not to be > there. There is not. Feel free to add it if needed.
(In reply to comment #12) > After discussions with Frédéric and Vincent from the release team, we agreed > that the best way to integrate the tp-logger for GNOME 2.30 would be to use a > git submodule. We could also have a configure option to use the system > tp-logger instead of the copy shipped with Empathy. Actually as we are already pretty late in this cycle, we're going to add an optionnal dependency on libtp-logger. If the dep it's not found, we use the existing code. That way, GNOME systems shouldn't be affected by the change.
Created attachment 154533 [details] [review] Diff from http://git.collabora.co.uk/?p=user/kalfa/empathy.git/.git;a=commitdiff;h=4648e067fac459896446e3641d049181b1cfc675 Attaching diff for easier review.
Review of attachment 154533 [details] [review]: I've done a quick check (mostly styles stuffs). I'll do one better review tomorrow when I'll have more brain power. ::: INSTALL @@ -1,2 @@ -Please visite the Empathy website for installation instructions: -http://live.gnome.org/Empathy/Install You shouldn't modify this file. ::: configure.ac @@ +176,3 @@ ]) + Why did you add those blank lines? @@ +331,2 @@ if test "x$enable_webkit" = "xyes" -a "x$have_webkit" != "xyes"; then + AC_MSG_ERROR([Could not find webkit dependencies.]) I'd prefer to have this change in a separated commit. @@ +558,3 @@ + Logging: + Telepathy Logger............: ${enable_tpl} The yes/no is miss-aligned. ::: libempathy-gtk/empathy-chat.c @@ +43,1 @@ +#ifndef ENABLE_TPL you could use #else here. @@ +48,3 @@ #include <libempathy/empathy-dispatcher.h> + No need for a blank line. @@ +3045,3 @@ empathy_tp_chat_acknowledge_all_messages (priv->tp_chat); } + What did you change here? ::: libempathy-gtk/empathy-contact-menu.c @@ +32,2 @@ #include <libempathy/empathy-call-factory.h> +#ifndef ENABLE_TPL use #else ::: libempathy-gtk/empathy-log-window.c @@ +364,3 @@ + gboolean can_do_previous; + gboolean can_do_next; + GError *error = NULL; Alig is wrong. @@ +369,3 @@ + + if (error != NULL) { + DEBUG ("Unable to retrieve messages for the selected date: %s. Aborting", alig. @@ +520,3 @@ + GtkTreeIter iter; + GtkListStore *store = user_data; + GError *error = NULL; alig @@ +834,3 @@ + GtkListStore *store; + GtkTreeIter iter; + GError *error = NULL; alig @@ +836,3 @@ + GError *error = NULL; + + chats = tpl_log_manager_async_operation_finish (result, &error); alig @@ +1147,3 @@ + } + + Double line here. @@ +1511,3 @@ + guint year_selected; + guint month_selected; + GError *error = NULL; alig @@ +1513,3 @@ + GError *error = NULL; + + dates = tpl_log_manager_async_operation_finish (result, &error); alig @@ +1516,3 @@ + + if (error != NULL) { + DEBUG ("Unable to retrieve messages' dates: %s. Aborting", alig @@ +1533,3 @@ + month_selected++; + + double line ::: libempathy/Makefile.am @@ +194,3 @@ empathy-irc-networks.dtd +if ENABLE_TPL why not use "if !ENABLE_TPL" directly? ::: libempathy/empathy-log-store.c @@ +21,3 @@ */ +#include <config.h> You don't seem to use this. ::: libempathy/empathy-message.c @@ +33,2 @@ #include <telepathy-glib/util.h> +#ifdef ENABLE_TPL You can group includes @@ +42,3 @@ #include "empathy-enum-types.h" + remove blank line @@ +277,3 @@ + + acc_man = tp_account_manager_dup (); + /* FIXME Currently Empathy shows in the log viewer only valid accounts, so it Alig of this block of comment is wrong. @@ +667,3 @@ priv2 = GET_PRIV (message2); + if (priv1->timestamp == priv2->timestamp && !tp_strdiff (priv1->body, priv2->body)) { Is this change harmless in the non logger case? ::: libempathy/empathy-message.h @@ +55,3 @@ }; +GType empathy_message_get_type (void) G_GNUC_CONST; Seems you changed all these lines. Please don't. ::: src/empathy.c @@ +59,3 @@ #include <libempathy/empathy-dispatcher.h> #include <libempathy/empathy-dispatch-operation.h> +#ifndef ENABLE_TPL use #else
Fixed. http://git.collabora.co.uk/?p=user/kalfa/empathy.git/.git;a=commit;h=f5fa1726357da8592830e6c2eed26a8713f6dc48
Created attachment 154580 [details] [review] diff
Review of attachment 154580 [details] [review]: ::: configure.ac @@ +152,3 @@ + PKG_CHECK_MODULES(TPL, + [ + telepathy-logger As discussed on IRC, you should depend on the 0.1 version ::: libempathy-gtk/empathy-log-window.c @@ +551,3 @@ + COL_FIND_ACCOUNT_NAME, account_name, + COL_FIND_ACCOUNT, hit->account, + COL_FIND_CHAT_NAME, hit->chat_id, /* FIXME */ What's this FIXME ? @@ +560,3 @@ + g_free (date_readable); + + /* FIXME: Update COL_FIND_CHAT_NAME */ And this one? @@ +856,3 @@ + gtk_list_store_append (store, &iter); + gtk_list_store_set (store, &iter, + COL_CHAT_ICON, "empathy-available", /* FIXME */ and this one? ::: libempathy/Makefile.am @@ +208,3 @@ $(ircnetworks_DATA) +if ENABLE_TPL +EXTRA_DIST += $(dtd_DATA) Aren't these files always used? ::: libempathy/empathy-message.c @@ +298,3 @@ + * cases. + */ + g_return_val_if_fail (TPL_IS_LOG_ENTRY_TEXT (logentry), NULL); I'd rather use a if () return here. g_return_val_if_fail will raise a critical warning which can be a crasher depending of your configuration. We shouldn't introduce crasher by updating the tp-logger library. ::: libempathy/empathy-message.h @@ +58,3 @@ EmpathyMessage * empathy_message_new (const gchar *body); +#ifdef ENABLE_TPL +EmpathyMessage * empathy_message_from_tpl_log_entry (TplLogEntry *logentry); No need for all these spaces. ::: po/de.po @@ +8,1 @@ # Michael Kanis <mkanis@gmx.de>, 2009. You shouldn't changet this file.
(In reply to comment #25) > @@ +856,3 @@ > + gtk_list_store_append (store, &iter); > + gtk_list_store_set (store, &iter, > + COL_CHAT_ICON, "empathy-available", /* FIXME */ Use gtk_list_store_insert_with_values() that way you only generate one signal. > +if ENABLE_TPL > +EXTRA_DIST += $(dtd_DATA) You don't want to do this. If the files aren't included in the distribution, no one will be able to compile a tarball version with TPL support.
the FIXME comments were already in the code, they are 'mirrored' in the related blocking function (when !ENABLE_TPL). I just kept them. About the g_return_val_if_fail, I guess it's the same for any other g_return_val_if_fail call added by the patch. The po files are touched because the patch has been rebased against 775fecfd93f0df135771692526f1af3ac864819d (the master's HEAD at the time) but your master is actually a commit ahead, which is in fact the german transl. update. I'll rebase it against the current master next patch.
the branch after the retrieve_backlog introduction (Bug 610994 filed) is at http://git.collabora.co.uk/?p=user/kalfa/empathy.git/.git;a=shortlog;h=refs/heads/empathy-tpl-20100224-ifdef if it passes the review you can push it.
Sjoerd pushed his channel dispatcher branch yesterday. Would be good if you could rebase your branch on top of current master and test it a bit to be sure everything still works as expected (it shouldn't affect the logger, but it's best to be sure). /* FIXME see Bug#610994 */ gboolean retrieving_backlogs; Alig of the FIXME is wrong Also, please add a comment explaining the semantic of this variable and how/why we use that. Describe the FIXME as well. + /* FIXME: See Bug#610994, we are forcing the ACK of the queue */ + priv->retrieving_backlogs = FALSE; alig is wrong priv->retrieving_backlogs = FALSE; should be done earlier in got_filtered_messages_cb. Atm, if retrieving logs fails for any reason, retrieving_backlogs will stay to FALSE forever and we'll never ack any messages. Also, if it fails, we should call show_pending_messages any way. You should probably use a goto instead of a return in the if (error) block + /* FIXME: Bug#610994 */ + if (!priv->retrieving_backlogs) { I'd use if (priv->retrieving_backlogs) return; that's easier to read.
2 years before we opened this bug, I can finally closed it \o/ This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.