After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 592994 - Debug: Store messages for each component in a separate GtkListStore and have them copied(and later updated with each new message) in a separate store for a new "All"(selected by default) selection.
Debug: Store messages for each component in a separate GtkListStore and have ...
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
unspecified
Other Linux
: Normal enhancement
: 3.4
Assigned To: Danielle Madeley
empathy-maint
Depends on:
Blocks: 658724
 
 
Reported: 2009-08-25 09:28 UTC by Guillaume Desmottes
Modified: 2012-02-17 09:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
empathy debug window saving logs directly from cache (9.06 KB, patch)
2010-11-02 19:25 UTC, Chandni Verma
needs-work Details | Review
corrected patch (11.03 KB, patch)
2010-11-03 22:39 UTC, Chandni Verma
needs-work Details | Review
second commit of my branch (20.42 KB, patch)
2011-01-20 16:05 UTC, Chandni Verma
needs-work Details | Review
Amended patch (8.69 KB, patch)
2011-01-24 01:54 UTC, Chandni Verma
none Details | Review
from git repo (23.07 KB, patch)
2011-05-08 04:27 UTC, Danielle Madeley
reviewed Details | Review
Debug window - Use one list store per service (21.06 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
needs-work Details | Review
Free proxy_data before disposing service_store (3.53 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
reviewed Details | Review
Removing unused data and functions (3.13 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
reviewed Details | Review
Don't log critical messages on disconnecting proxies (1.50 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
none Details | Review
Fixing "Clear" operation (1.03 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
none Details | Review
Setting the correct store_filter on switching services (2.39 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
none Details | Review
Adding COL_PAUSED and COL_PAUSE_BUFFER to service_store (6.19 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
none Details | Review
Storing service-iter as an association in proxy (1.89 KB, patch)
2011-10-31 02:45 UTC, Chandni Verma
none Details | Review
Setting up pause-buffers to queue incoming messages (8.47 KB, patch)
2011-10-31 02:46 UTC, Chandni Verma
none Details | Review
Flush all pause buffers when Pause button is released (1.85 KB, patch)
2011-10-31 02:46 UTC, Chandni Verma
none Details | Review
No need to keep a pointer to new-debug-message-signal (6.13 KB, patch)
2011-11-10 04:05 UTC, Chandni Verma
none Details | Review
Remove unrequired data associated with proxies (10.98 KB, patch)
2011-11-10 04:05 UTC, Chandni Verma
none Details | Review
Factor out create_proxy_to_get_messages (5.52 KB, patch)
2011-11-10 04:05 UTC, Chandni Verma
none Details | Review
Store associations to proxies while creating them (2.67 KB, patch)
2011-11-10 04:05 UTC, Chandni Verma
none Details | Review
Store useful proxies in service store by traversing each service row (2.32 KB, patch)
2011-11-10 04:06 UTC, Chandni Verma
none Details | Review
Add "All" selection to service_chooser (3.54 KB, patch)
2011-11-10 04:06 UTC, Chandni Verma
none Details | Review
Destroy "All" selection's list_store when needed (3.00 KB, patch)
2011-11-10 04:06 UTC, Chandni Verma
none Details | Review
Connect service_store to row-changed signal (7.00 KB, patch)
2011-11-10 04:06 UTC, Chandni Verma
none Details | Review
Copy messages to All's list store and set visibility (4.00 KB, patch)
2011-11-10 04:06 UTC, Chandni Verma
none Details | Review
Adding a Debug message on saving proxy for a service (1.14 KB, patch)
2011-11-10 04:06 UTC, Chandni Verma
none Details | Review
Un-pausing incoming messages should update All's store too (2.19 KB, patch)
2011-11-10 04:07 UTC, Chandni Verma
none Details | Review
Debug window - Use one list store per service (25.55 KB, patch)
2011-12-03 00:50 UTC, Chandni Verma
committed Details | Review
Adding COL_PAUSE_BUFFER to service_store (6.50 KB, patch)
2011-12-03 00:50 UTC, Chandni Verma
committed Details | Review
Deploy one pause-buffer per proxy (8.00 KB, patch)
2011-12-03 00:50 UTC, Chandni Verma
committed Details | Review
Fixed comments (6.69 KB, patch)
2011-12-03 00:50 UTC, Chandni Verma
committed Details | Review
Remove unrequired data associated with proxies (13.00 KB, patch)
2011-12-03 00:50 UTC, Chandni Verma
committed Details | Review
Factor out create_proxy_to_get_messages (5.50 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
committed Details | Review
Preparing for adding "All" option to service_chooser (4.81 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
committed Details | Review
Add "All" selection to service_chooser (3.55 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
committed Details | Review
Destroy "All" selection's list_store when needed (3.35 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
none Details | Review
Connect service_store to row-changed signal (7.71 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
committed Details | Review
Add incoming messages to All's list store (1.95 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
committed Details | Review
Set window sensitivity (2.90 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
committed Details | Review
Un-pausing incoming messages should update All's store too (2.22 KB, patch)
2011-12-03 00:51 UTC, Chandni Verma
committed Details | Review
Destroy "All" selection's active-buffer when needed (3.31 KB, patch)
2011-12-03 01:08 UTC, Chandni Verma
committed Details | Review

Description Guillaume Desmottes 2009-08-25 09:28:50 UTC
When an user experiences an issue, the cause of his problem could be:
- the CM
- MC
- Empathy itself

We shouldn't expect users to know which component is to blame. Furthemore, logs from other components can be useful to help us to understand the issue.
So, I think we should encourage bug reporter to give us as most log as possible.
One way to do that could be to add a "Save all" button in the Debug dialog which will save all the available logs (in a tarball). Then, user will just have to attach them to his bug report.
Comment 1 Chandni Verma 2010-10-17 00:07:59 UTC
From your experience, what approach do you suggest? Should we merge the cached lists of messages of a level caused by different services 'on-demand' when the user clicks the save-all button and prepare a file, or should a separate list be created for 'All' services with messages appended to it, as they appear(which will be definitely faster but will need more memory space)?

Also instead of a 'save all' button, can we put an 'All' option in the service chooser which might look more integrated and seamless?
Comment 2 Chandni Verma 2010-11-02 19:25:41 UTC
Created attachment 173708 [details] [review]
empathy debug window saving logs directly from cache

This patch appends unmixed logs of the different services one after the other to an output log file on clicking the 'Save all' button.

Also, modification has been made to the previous 'Save' button functionality so that now it saves logs by reading the cache directly instead of reading them from 'store', thereby optimizing the function's performance.
Comment 3 Danielle Madeley 2010-11-02 21:10:01 UTC
Review of attachment 173708 [details] [review]:

::: src/empathy-debug-window.c
@@ +1113,3 @@
 }
 
+static GFileOutputStream *get_file_output_stream (const char *filename)

Declaration is not in Telepathy format:

static GFileOutputStream *
get_file_output_stream (arg1,
    arg2,
    arg3)

@@ +1121,1 @@
+  gfile = g_file_new_for_path (filename);

You never unref the GFile at the end of this method.

@@ +1127,3 @@
+      DEBUG ("Failed to open file for writing: %s", error->message);
+      g_error_free (error);
+      return NULL;

You'll leak the GFile here too, use a "goto finally;"

@@ +1134,2 @@
+static void save_one_service_from_cache (char *name, GList *messages,
+    GFileOutputStream *output_stream)

Function declaration not in Telepathy style.

@@ +1164,1 @@
+      time_str = debug_window_format_timestamp (dm->timestamp);

Is there any way to share this code in common with the function you copied it from?

@@ +1195,2 @@
   gchar *filename = NULL;
+  char *name = NULL;

Don't need to assign this to NULL. It's not an allocated string, so it probably should be const gchar *.

@@ +1200,3 @@
     goto OUT;
 
+  /*getting cached message list for active service*/

Missing spaces.

@@ +1206,1 @@
+  /*getting output stream*/

Missing spaces.

@@ +1206,2 @@
+  /*getting output stream*/
+  filename = gtk_file_chooser_get_filename (GTK_FILE_CHOOSER (dialog));

You don't free @filename.

@@ +1214,3 @@
       goto OUT;
     }
+  /*writing list to output stream*/

Missing spaces.

@@ +1283,3 @@
+  char *name = NULL;
+  GFileOutputStream *output_stream = NULL;
+  //void (*write_log) (char *, GList *, GFileOutputStream *);

What is this?

@@ +1288,3 @@
+    goto OUT;
+
+  /*getting output stream*/

Missing spaces.

@@ +1299,3 @@
+    }
+
+  /*writing cached messages for each service to log*/

Missing spaces.

@@ +1308,3 @@
+
+  if (filename != NULL)
+    g_free (filename);

You don't need to test before freeing. g_free() already does that.

@@ +1316,3 @@
+debug_window_save_all_clicked_cb (GtkToolButton *tool_button,
+    EmpathyDebugWindow *debug_window)
+{

Can you make most of this method in common?

@@ +1550,3 @@
+    priv->save_all_button = gtk_tool_button_new (image, _("Save all"));
+    g_signal_connect (priv->save_all_button, "clicked",
+            G_CALLBACK (debug_window_save_all_clicked_cb), object);

Incorrect indentation.
Comment 4 Chandni Verma 2010-11-03 22:39:40 UTC
Created attachment 173805 [details] [review]
corrected patch

Hey Danielle,
   This patch should have all issues resolved except:

>> For this:

  char *name = NULL;
Don't need to assign this to NULL. It's not an allocated string, so it probably should be const gchar *.

  I did not change it to const as 7 lines down I am assigning it a value using get_active_service_name (debug_window);

   and
>> I did not understand this:

 filename = gtk_file_chooser_get_filename (GTK_FILE_CHOOSER (dialog));
You don't free @filename.

Thanks.
Comment 5 Danielle Madeley 2010-11-03 22:57:48 UTC
(In reply to comment #4)

>   char *name = NULL;
> Don't need to assign this to NULL. It's not an allocated string, so it probably
> should be const gchar *.
> 
>   I did not change it to const as 7 lines down I am assigning it a value using
> get_active_service_name (debug_window);

const char * means that the contents of the string don't change, not the pointer. You can still assign a pointer to it, but you don't want to change the string contents. You use it for strings that belong to some other part of the program and you didn't allocate and mustn't free.

> >> I did not understand this:
> 
>  filename = gtk_file_chooser_get_filename (GTK_FILE_CHOOSER (dialog));
> You don't free @filename.

You need to free the string 'filename' once you're finished with it.
Comment 6 Danielle Madeley 2010-11-03 23:10:05 UTC
Review of attachment 173805 [details] [review]:

Style things to fix.

::: src/empathy-debug-window.c
@@ +231,3 @@
 static void
+split_domain_category (const gchar *domain_category, gchar **domain_ptr,
+    gchar **category_ptr)

Declaration not in Telepathy style.

@@ +257,3 @@
   EmpathyDebugWindowPriv *priv = GET_PRIV (debug_window);
   gchar *domain, *category;
+  gchar **domain_ptr, **category_ptr;

Unrequired.

@@ +266,3 @@
 
+  domain_ptr = &domain;
+  category_ptr = &category;

The use of these ptrs is unrequired and confusing.

@@ +267,3 @@
+  domain_ptr = &domain;
+  category_ptr = &category;
+  split_domain_category (domain_category, domain_ptr, category_ptr);

split_domain_category (domain_category, &domain, &category

@@ +1138,3 @@
+      DEBUG ("Failed to open file for writing: %s", error->message);
+      g_error_free (error);
+      output_stream = NULL;

g_file_replace() is already defined to return NULL on error.

@@ +1164,3 @@
+      domain_ptr = &domain;
+      category_ptr = &category;
+      split_domain_category (dm->domain, domain_ptr, category_ptr);

Again here, you don't need these extra pointers. It's confusing.

@@ +1219,3 @@
+    {
+      goto OUT;
+    }

Empty line here please.

@@ +1274,1 @@
     EmpathyDebugWindow *debug_window)

Perhaps just pass in a GCallback instead of the specific prototype.

@@ +1321,3 @@
+  name = get_active_service_name (debug_window);
+
+  create_save_file_chooser("Save", name,

Missing space.

@@ +1331,3 @@
+  gchar *name = "empathy_debug_full";
+
+  create_save_file_chooser("Save all", name,

Missing space.

@@ +1332,3 @@
+
+  create_save_file_chooser("Save all", name,
+		  debug_window_save_all_file_chooser_response_cb, debug_window);

Wrong indentation.
Comment 7 Chandni Verma 2010-11-10 13:24:09 UTC
Here is my public branch for this bug-
http://gitorious.org/glassrose-gnome-repository/empathy/commits/debug-window-save-all-592994
Comment 8 Chandni Verma 2010-11-19 04:16:15 UTC
(In reply to comment #7)
> Here is my public branch for this bug-
> http://gitorious.org/glassrose-gnome-repository/empathy/commits/debug-window-save-all-592994

My public branch is now this: http://gitorious.org/glassrose-gnome/empathy/commits/debug-window-save-all-592994
Comment 9 Chandni Verma 2011-01-20 16:05:24 UTC
Created attachment 178846 [details] [review]
second commit of my branch
Comment 10 Guillaume Desmottes 2011-01-21 12:06:38 UTC
Danielle: can you please take a look on this as you did the first round of reviews?
Comment 11 Danielle Madeley 2011-01-23 11:44:31 UTC
Review of attachment 178846 [details] [review]:

::: src/empathy-debug-window.c
@@ +254,3 @@
+  UPDATE_CACHE = 1<<0,
+  UPDATE_UI = 1<<1,
+} MessageUpdationFlags;

Updation isn't a word.

@@ +303,3 @@
+  EmpathyDebugWindow *window;
+  MessageUpdationFlags updation_flags;
+} DebugWindowNewDebugMessageData;

Do you need to use a struct? Can you pass @window as the weak_obj and @flags as the user_data?

@@ +410,3 @@
+  for (l = messages; l != NULL; l = l->next)
+    {
+      dm = l->data;

Declare @dm here in the block where it's used.

@@ +505,3 @@
+  if (window_n_outstream->output_stream != NULL)
+    {
+      if (save_all_pending_calls == 1)

Rather than do this, I wonder if it would be possible to add a multi-call structure to tp-glib or Empathy...

@@ +507,3 @@
+      if (save_all_pending_calls == 1)
+        {
+    	  GError *error2 = NULL;

You shouldn't need to declare a second GError, and instead be able to use the one that's already declared.

@@ +519,3 @@
+	    	  DEBUG ("Could not close the file output stream: %s", error2->message);
+	    	  g_error_free (error2);
+            }

Indentation here is wrong.

@@ +530,3 @@
+    }
+  else
+    g_slice_free (DebugWindowGetMessagesData, window_n_outstream);

Missing braces per Telepathy style.

@@ +546,3 @@
+  MessageUpdationFlags updation_flags = UPDATE_CACHE | UPDATE_UI;
+
+  debug_window_get_messages(proxy, messages, error,

Missing space.

@@ +560,3 @@
+  MessageUpdationFlags updation_flags = UPDATE_CACHE;
+
+  debug_window_get_messages(proxy, messages, error,

Missing space.

@@ +605,3 @@
 static void
+create_proxy_to_get_messages (DebugWindowGetMessagesData *data,
+	GtkTreeIter *iter, emp_cli_debug_callback_for_get_messages get_messages_cb)

One parameter per line per Telepathy style.
Comment 12 Chandni Verma 2011-01-24 01:54:00 UTC
Created attachment 179126 [details] [review]
Amended patch

Hey, in this incremental patch (not yet committed to public branch), I have made the corrections you noticed except that:

I didn't understand this bit-
> @@ +505,3 @@
> +  if (window_n_outstream->output_stream != NULL)
> +    {
> +      if (save_all_pending_calls == 1)
> 
> Rather than do this, I wonder if it would be possible to add a multi-call
> structure to tp-glib or Empathy...

and for this-
> @@ +507,3 @@
> +      if (save_all_pending_calls == 1)
> +        {
> +          GError *error2 = NULL;
> 
> You shouldn't need to declare a second GError, and instead be able to use the
> one that's already declared.

the existing @error is a const GError * and for this purpose we need a GError * so I created a new one to treat the warnings.
Comment 13 Danielle Madeley 2011-01-25 12:27:44 UTC
(In reply to comment #12)

> I didn't understand this bit-
> > @@ +505,3 @@
> > +  if (window_n_outstream->output_stream != NULL)
> > +    {
> > +      if (save_all_pending_calls == 1)
> > 
> > Rather than do this, I wonder if it would be possible to add a multi-call
> > structure to tp-glib or Empathy...

It would be nice to have a structure/functions that handled when you needed to wait for multiple calls to return either in empathy or telepathy-glib. Frequently we seem to implement it via methods like this.

Cassidy, what do you think?

> and for this-
> > @@ +507,3 @@
> > +      if (save_all_pending_calls == 1)
> > +        {
> > +          GError *error2 = NULL;
> > 
> > You shouldn't need to declare a second GError, and instead be able to use the
> > one that's already declared.
> 
> the existing @error is a const GError * and for this purpose we need a GError *
> so I created a new one to treat the warnings.

My mistake. [I'm not aware of a convention here, I personally started using in_error and error at some point.]
Comment 14 Guillaume Desmottes 2011-01-25 14:27:26 UTC
(In reply to comment #13)
> (In reply to comment #12)
> 
> > I didn't understand this bit-
> > > @@ +505,3 @@
> > > +  if (window_n_outstream->output_stream != NULL)
> > > +    {
> > > +      if (save_all_pending_calls == 1)
> > > 
> > > Rather than do this, I wonder if it would be possible to add a multi-call
> > > structure to tp-glib or Empathy...
> 
> It would be nice to have a structure/functions that handled when you needed to
> wait for multiple calls to return either in empathy or telepathy-glib.
> Frequently we seem to implement it via methods like this.
> 
> Cassidy, what do you think?

Yep that sounds like an useful things to have. I guess we could prototype in Empathy or tp-glib and submit to glib later?
Comment 15 Chandni Verma 2011-01-26 22:12:55 UTC
Can anyone elaborate on what needs to be done for this? How to produce a generic setup?
Comment 16 Guillaume Desmottes 2011-03-30 13:43:07 UTC
I wouldn't block merging of this branch on this tbh. Anything else blocking this branch?
Comment 17 Chandni Verma 2011-04-07 12:45:09 UTC
Branch https://gitorious.org/glassrose-gnome/empathy/commits/debug-window-save-all-592994 updated and rebased. It can be merged now.
Comment 18 Guillaume Desmottes 2011-04-08 09:50:44 UTC
Danni: are you ok with  the final version of this branch?
Comment 19 Danielle Madeley 2011-05-08 04:27:13 UTC
Created attachment 187442 [details] [review]
from git repo
Comment 20 Danielle Madeley 2011-05-08 04:38:48 UTC
Review of attachment 187442 [details] [review]:

I think these two patches should be squashed together, because there is a significant change in direction between them. Reviewed as one patch for this reason.

Almost all of this is style, but I've personally not tested it.

::: src/empathy-debug-window.c
@@ +254,3 @@
+{
+  UPDATE_CACHE = 1<<0,
+    }

Add space between operators. 1 << 0, just like 1 + 0.

@@ +281,3 @@
+        string = g_strchomp (g_strdup (message));
+      else
+        string = g_strdup (message);

Don't need g_str_has_suffix() or the branch. g_strchomp() is safe to call anyway.

@@ +394,3 @@
+
+  if (messages == NULL)
+  GList *l;

Don't need this. Loop already won't iterate anything.

@@ +495,3 @@
+        {
+    	  /* Time to print logs for all services to output stream */
+

I personally dislike "error2", it becomes confusing which error you're meant to be using. Instead I like to name the const GError * parameter to something like in_error or const_error, so you know not to poke at it.

@@ +502,3 @@
+
+          g_output_stream_close (G_OUTPUT_STREAM
+        {

Empty line here.

@@ +668,3 @@
+        {
+          gtk_combo_box_set_active (chooser, 0);
+  DebugWindowGetMessagesData *data;

Braces unrequired. Empty line after block.

@@ +688,3 @@
+  data = g_slice_new0 (DebugWindowGetMessagesData);
+
+      return;

Should you hold a ref? If so, I'd prefer all allocation and free'ing of DebugWindowGetMessagesData to be separated out into new() and free() funcs.

@@ +689,3 @@
+
+  data->debug_window = debug_window;
+      return;

Unrequired. Already 0'ed.

@@ +1314,1 @@
     }

Braces unrequired.

@@ +1319,3 @@
+finally:
+  if (output_stream != NULL)
+    g_object_unref (output_stream);

tp_clear_object()

@@ +1351,1 @@
     }

Braces unrequired.

@@ +1362,3 @@
+      &iter))
+  while (gtk_tree_model_iter_next (GTK_TREE_MODEL (priv->service_store),
+      &iter))

Weird indenting.

Replace this with a for-loop:
for (valid = get_iter_first(..); valid; valid = iter_next(..))

@@ +1377,3 @@
 static void
+create_save_file_chooser (gchar *title,
+    gchar *name,

Should be const gchar *

@@ +1426,3 @@
+{
+  gchar *name;
+  name = get_active_service_name (debug_window);

Leaks!

Line between declaration and code, or combine into one line.

@@ +1436,3 @@
+    EmpathyDebugWindow *debug_window)
+{
+  create_save_file_chooser("Save", name,

Should be const gchar *, or just pass it inline.
Comment 21 Will Thompson 2011-07-20 13:47:49 UTC
We could have an “All” entry in the component selector, which shows all the logs interleaved, and have the [Save] button there have this save-all behaviour. Then a naïve user will have the right behaviour from just mashing [Save], without cluttering the toolbar with the choice of [Save] [Save all].

(Relatedly, we could have a [Send to pastie.org] button!)
Comment 22 Chandni Verma 2011-07-20 21:01:46 UTC
yeah, just like what I said in second part of https://bugzilla.gnome.org/show_bug.cgi?id=592994#c1.

I have a branch which takes care of this bug using "save all" button but it's crashing after a few reattempts. I am looking into it. Once fixed, I can modify it to have an "all" option in service chooser which will cause "save" button to imitate the save all behavior, if cassidy also supports it.
Comment 23 Chandni Verma 2011-08-20 00:06:32 UTC
https://gitorious.org/glassrose-gnome/empathy/commits/debug-window-save-all-592994

Updated branch with style fixes and "All" option in service chooser. Haven't done a fixup yet for maintaining readability. If looks good, I'll do it. 

Any clues on how to implement a "Send to pastie.com" button?
Comment 24 Chandni Verma 2011-08-21 09:36:06 UTC
should I be calling external command "pastebinit" using system()? This will require making pastebinit a dependency.
Comment 25 Danielle Madeley 2011-08-21 23:10:45 UTC
(In reply to comment #24)
> should I be calling external command "pastebinit" using system()? This will
> require making pastebinit a dependency.

Don't do that. I assume there's a trivial way to make a POST request to paste something, which you could do, e.g. via libsoup. Have a look for something documenting their web API or at the source code of pastebinit.
Comment 26 Guillaume Desmottes 2011-08-29 11:21:17 UTC
Couldn't we already merge this and adds the pastebin feature later in another bug?
Comment 27 Danielle Madeley 2011-08-30 00:47:32 UTC
Yeah, we should open another bug for the pastebin button.
Comment 28 Guillaume Desmottes 2011-09-06 07:40:34 UTC
We are past UI freeze so that probably won't make it for 3.2.
Comment 29 Chandni Verma 2011-09-11 02:25:29 UTC
I don't think anything else needs to be done on this. Should I perform a fixup leaving only the major feature commit?
Comment 30 Chandni Verma 2011-09-19 17:41:47 UTC
Rebased on top of current master.
Comment 31 Guillaume Desmottes 2011-09-27 13:18:43 UTC
Danni: is this good to go?
Comment 32 Danielle Madeley 2011-10-10 03:39:59 UTC
There's lots of indenting mistakes and trailing spaces in this patch. Some coding style glitches too, for instance:

+      for(valid = gtk_tree_model_get_iter_first (GTK_TREE_MODEL (

Whole thing needs 'make check' run on it, and to check for tabs which shouldn't be.

+	  if (g_strcmp0 (name, "All") != 0)

tp_strdiff()

~

Also if either 'if' or 'else' requires braces, then the other should also have braces.

i.e.

if (blah)
  {
    stuff
  }
else
  something

is wrong.

~

+guint save_all_pending_calls;
+guint number_of_clients;

I don't like the global variables. To start with, they should be static, so they don't leak into other files, but instead, they should be somewhere else.

To be honest, I find the whole patch to be rather confusingly structured. Perhaps because it's trying to fit in within the current structure of the code, whereas a more radical refactoring could take place to make it clearer.
Comment 33 Chandni Verma 2011-10-12 08:50:14 UTC
comments fixed

> I don't like the global variables. To start with, they should be static, so
> they don't leak into other files, but instead, they should be somewhere else.

Made it static as what it correctly should be but where else to put it? They have to be global.
 
> To be honest, I find the whole patch to be rather confusingly structured.
> Perhaps because it's trying to fit in within the current structure of the code,
> whereas a more radical refactoring could take place to make it clearer.

done much changes with all styles in mind. Hopefully looks better now.
Comment 34 Guillaume Desmottes 2011-10-28 11:19:01 UTC
Any chance we can, finally, get this merged? :)
Comment 35 Danielle Madeley 2011-10-29 01:03:09 UTC
Chandni was refactoring it. I don't know if she's finished. I'm waiting for her to attach a new patch.
Comment 36 Chandni Verma 2011-10-31 02:26:29 UTC
(need to practice with git bz a little)
anyways, for now, danni, this is the base refactoring we decided upon:

https://gitorious.org/glassrose-gnome/empathy/commits/debug-window-overhaul

My next branch will be following it.
Comment 37 Chandni Verma 2011-10-31 02:45:03 UTC
Created attachment 200309 [details] [review]
Debug window - Use one list store per service

-Remove cache and store.
-Add one proxy and and related data per service in service_chooser
-Add one list store per service in service_chooser to save logs
-Make the service chooser switch the correct list store to display
Comment 38 Chandni Verma 2011-10-31 02:45:08 UTC
Created attachment 200310 [details] [review]
Free proxy_data before disposing service_store
Comment 39 Chandni Verma 2011-10-31 02:45:13 UTC
Created attachment 200311 [details] [review]
Removing unused data and functions

-struct DebugMessage
 -debug_message_list_free
 -debug_window_cache_new_message
 -debug_window_add_log_messages_from_cache
 -debug_message_new
 -debug_message_free
Comment 40 Chandni Verma 2011-10-31 02:45:23 UTC
Created attachment 200312 [details] [review]
Don't log critical messages on disconnecting proxies
Comment 41 Chandni Verma 2011-10-31 02:45:28 UTC
Created attachment 200313 [details] [review]
Fixing "Clear" operation
Comment 42 Chandni Verma 2011-10-31 02:45:32 UTC
Created attachment 200314 [details] [review]
Setting the correct store_filter on switching services
Comment 43 Chandni Verma 2011-10-31 02:45:37 UTC
Created attachment 200315 [details] [review]
Adding COL_PAUSED and COL_PAUSE_BUFFER to service_store

We will need to pause all services when "Pause" is pressed so that
we can see what happened in all services at the same time and the
messages that arrive while Pause is on will be buffered in various
services' pause-buffers which will be stored in  COL_PAUSE_BUFFER.

COL_PAUSED has been added for maintainability. It can be used for
making Pause a per-service function, if desired in future.
Comment 44 Chandni Verma 2011-10-31 02:45:50 UTC
Created attachment 200316 [details] [review]
Storing service-iter as an association in proxy

This cuts-off an O(no. of services) lookup for appropriate iter,
to obtain the value of COL_PAUSED, whenever a new message arrives.
Comment 45 Chandni Verma 2011-10-31 02:46:00 UTC
Created attachment 200317 [details] [review]
Setting up pause-buffers to queue incoming messages

When "Pause" is pressed, all proxies stop updating their models
and queue messages in their pause buffers respectively. The first
message on unpausing ensures that queue is flushed before saving it.
Comment 46 Chandni Verma 2011-10-31 02:46:04 UTC
Created attachment 200318 [details] [review]
Flush all pause buffers when Pause button is released
Comment 47 Danielle Madeley 2011-10-31 03:05:13 UTC
Review of attachment 200309 [details] [review]:

::: src/empathy-debug-window.c
@@ +238,3 @@
   GtkTreeIter iter;
   gchar *string;
+  GtkListStore *proxy_list_store;

'store' would be a fine name.

@@ +294,3 @@
+  TpProxy *proxy;
+  guint invalid_signal_id;
+} ProxyData;

Can you not just store both of these separately in the model?

@@ +307,3 @@
+  gtk_combo_box_get_active_iter (GTK_COMBO_BOX (priv->chooser), &iter);
+  gtk_tree_model_get (GTK_TREE_MODEL (priv->service_store), &iter,
+      COL_PROXY_DATA, &data, -1);

Put the -1 on the next line.

@@ +314,1 @@
       "Enabled", val, NULL, NULL, NULL, NULL);

This seems to enable the currently selected proxy. Surely you want to enable all of them?

@@ +369,3 @@
+  val = tp_g_value_slice_new_boolean (FALSE);
+  tp_cli_dbus_properties_call_set (proxy, -1, TP_IFACE_DEBUG,
+  GValue *val;

Leaks 'val'.

It would be nice to abstract this chunk of code into a set_enabled() function that takes a proxy and a boolean.

@@ +419,1 @@
       return;

Instead of this, do a 'goto finally;'

@@ +426,1 @@
+  gtk_combo_box_get_active_iter (GTK_COMBO_BOX (priv->chooser), &iter);

I'm confused. What are you doing here with the selected proxy. I thought you wanted to do this for all proxies.

@@ +426,3 @@
+  gtk_combo_box_get_active_iter (GTK_COMBO_BOX (priv->chooser), &iter);
+  gtk_tree_model_get (GTK_TREE_MODEL (priv->service_store), &iter,
+      COL_LIST_STORE, &proxy_list_store, COL_PROXY_DATA, &proxy_data, -1);

One line per column-pointer pair please.

@@ +452,3 @@
+      new_debug_message_signal);
+
+  g_object_unref (proxy_list_store);

Move this down after finally:

@@ +459,2 @@
   /* Set Enabled as appropriate */
   debug_window_set_enabled (debug_window, !priv->paused);

finally: goes here

@@ +513,3 @@
   gtk_tree_model_get (GTK_TREE_MODEL (priv->service_store), &iter,
+      COL_NAME, &name, COL_GONE, &gone, COL_LIST_STORE, &stored_list_store,
+      COL_PROXY_DATA, &proxy_data, -1);

One column-pointer pair per line.

@@ +774,3 @@
+      list_store = gtk_list_store_new (NUM_DEBUG_COLS, G_TYPE_DOUBLE,
+          G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING,
+          G_TYPE_UINT);

One type per row, put a comment next to each row with the name of the column, e.g. /* COL_PROXY */

@@ +782,3 @@
+          COL_GONE, FALSE,
+          COL_LIST_STORE, list_store,
+          COL_PROXY_DATA, NULL,

Split this structure out into two separate items.

@@ +892,3 @@
+          list_store = gtk_list_store_new (NUM_DEBUG_COLS, G_TYPE_DOUBLE,
+              G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING,
+              G_TYPE_UINT);

Again, one item per line. With the names of the columns in comments next to them.

@@ +917,3 @@
+          list_store = gtk_list_store_new (NUM_DEBUG_COLS, G_TYPE_DOUBLE,
+              G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING,
+              G_TYPE_UINT);

You seem to have a lot of code paths that create list stores. This is going to be hard to maintain if someone wants to change the store.

You should have one code path to create the store for the chooser, and one code path to create each store for a view. If you need to create this from multiple points, make it a function.

@@ +923,3 @@
+          if (stored_proxy_data != NULL)
+            {
+              TpProxy *stored_proxy = stored_proxy_data->proxy;

Empty line between declarations and code please.

@@ +930,3 @@
+                      found_at_iter, NULL);
+                  g_object_unref (stored_proxy);
+	      if (stored_proxy != NULL)

Empty line after blocks too.

@@ +1033,3 @@
+  list_store = gtk_list_store_new (NUM_DEBUG_COLS, G_TYPE_DOUBLE,
+      G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING,
+      G_TYPE_UINT);

More list store creation! See comment above.

@@ +1587,3 @@
   priv->service_store = gtk_list_store_new (NUM_COLS, G_TYPE_STRING,
+      G_TYPE_STRING, G_TYPE_BOOLEAN, G_TYPE_OBJECT, G_TYPE_BOOLEAN,
+      G_TYPE_OBJECT, G_TYPE_POINTER);

Again, see comment above.

@@ +1834,3 @@
   g_free (priv->select_name);
 
+  (G_OBJECT_CLASS (empathy_debug_window_parent_class)->finalize) (object);

Outer brackets are not required.
Comment 48 Danielle Madeley 2011-10-31 03:06:50 UTC
Review of attachment 200310 [details] [review]:

::: src/empathy-debug-window.c
@@ +381,2 @@
   g_signal_handler_disconnect (proxy, data->invalid_signal_id);
+  g_object_unref (proxy);

Seems suspect, the store should own the only reference to the proxy.
Comment 49 Danielle Madeley 2011-10-31 03:08:18 UTC
Review of attachment 200311 [details] [review]:

If these are unused, they would have stopped your first patch from compiling, and as such should be included in the same patch. Individual patches should be able to compile, otherwise it makes it hard to git-bisect our code.
Comment 50 Danielle Madeley 2011-10-31 03:14:38 UTC
Review of attachment 200316 [details] [review]:

Ultimately, we don't care about the linear search to find the correct service. The total number of services is ~10, we spend orders of magnitude more CPU drawing lines on the screen.

::: src/empathy-debug-window.c
@@ +386,3 @@
+   * of a service directly from the corresponding row in service_store. */
+  g_object_set_data (G_OBJECT (proxy), "service-iter",
+      &proxy_data->service_iter);

Suspect &.

@@ +503,3 @@
+
+      /* The iter will not change for a service name for the lifetime of
+       * debug-window */

This may not be true. You shouldn't really store iters.
Comment 51 Danielle Madeley 2011-10-31 03:23:05 UTC
So having read this code, it's only be activating proxies and calling GetMessages() when the proxy is first selected. I can see how this makes sense, but I don't wonder if it wouldn't be simpler just to call GetMessages() on all proxies at start-up (although I do wonder about the D-Bus bandwidth for doing this). This would also have the benefit being able to not include clients unsupportive of Debug in the dropdown.
Comment 52 Chandni Verma 2011-10-31 10:33:15 UTC
(In reply to comment #51)
> So having read this code, it's only be activating proxies and calling
> GetMessages() when the proxy is first selected. I can see how this makes sense,
> but I don't wonder if it wouldn't be simpler just to call GetMessages() on all
> proxies at start-up (although I do wonder about the D-Bus bandwidth for doing
> this). 

I have not included the "All" option in this branch yet. I am going to add it as the first option in the service_chooser and set it as the one selected by default in my next branch. It will call GetMessages on all the proxies in a row in the very beginning excluding the need to make those calls every time a service is selected which has already successfully fetched messages earlier. This branch just laid the basis. :)


This would also have the benefit being able to not include clients
> unsupportive of Debug in the dropdown.

This can be done if really required. It just requires to launch the NameOwnerChanged call and GetMessages call together before adding a service to the chooser. 
But I doubt it that will be useful for what we are trying to do in any case.
Also we might want the users to see which all clients are telepathy clients for instance and explore a chance to make these clients do support the Debug Interface (which is the actual bug to fix, ignoring it doesn't sound reasonable).

I'll fix your other comments and insert the "All" option in the next branch.
Comment 53 Chandni Verma 2011-11-02 19:42:00 UTC
Updated branch with comments relevant to this branch fixed, except a few:

> @@ +294,3 @@
> +  TpProxy *proxy;
> +  guint invalid_signal_id;
> +} ProxyData;
> 
> Can you not just store both of these separately in the model?
> 
No, I would need a unique identifier (or GtkIter) for a service row then, which we don't want to use.


> @@ +419,1 @@
>        return;
> 
> Instead of this, do a 'goto finally;'
> 
Code following Finally would be having anything in it since I don't won't to unref the proxy if it is successful in obtaining the debug messages from the dbus call.


> @@ +452,3 @@
> +      new_debug_message_signal);
> +
> +  g_object_unref (proxy_list_store);
> 
> Move this down after finally:
> 
> @@ +459,2 @@
>    /* Set Enabled as appropriate */
>    debug_window_set_enabled (debug_window, !priv->paused);
> 
> finally: goes here
> 

same as ^

 
> @@ +1834,3 @@
>    g_free (priv->select_name);
> 
> +  (G_OBJECT_CLASS (empathy_debug_window_parent_class)->finalize) (object);
> 
> Outer brackets are not required.

Not my code, should I edit it?


Also, removed the unwanted pointers to signal connection in the existing code and the COL_PAUSED column from the service_store.
Comment 54 Danielle Madeley 2011-11-02 22:27:51 UTC
(In reply to comment #53)

> No, I would need a unique identifier (or GtkIter) for a service row then, which
> we don't want to use.

Well, since there is no longer anything else in the struct? You've removed the signal right, what else is in the struct?

Here are some things you could look up: store pointer, service name from the proxy, value of the string we display.

> > @@ +419,1 @@
> >        return;
> > 
> > Instead of this, do a 'goto finally;'
> > 
> Code following Finally would be having anything in it since I don't won't to
> unref the proxy if it is successful in obtaining the debug messages from the
> dbus call.

I misread this code, between proxy and proxy_store. Never mind.
Comment 55 Chandni Verma 2011-11-02 22:39:53 UTC
(In reply to comment #54)
> (In reply to comment #53)
> 
> > No, I would need a unique identifier (or GtkIter) for a service row then, which
> > we don't want to use.
> 
> Well, since there is no longer anything else in the struct? You've removed the
> signal right, what else is in the struct?
> 
> Here are some things you could look up: store pointer, service name from the
> proxy, value of the string we display.
> 
No, I have removed the new-debug-message-signal but I still need the invalid-signal-id which is needed to destroy the proxy in case it becomes disconnected for any reason. It can't be stored as an association onto proxy as its a gulong and can't be converted to a gpointer.
Comment 56 Danielle Madeley 2011-11-03 01:25:40 UTC
(In reply to comment #55)

> No, I have removed the new-debug-message-signal but I still need the
> invalid-signal-id which is needed to destroy the proxy in case it becomes
> disconnected for any reason. It can't be stored as an association onto proxy as
> its a gulong and can't be converted to a gpointer.

A ulong and a pointer are by definition the same size. So you can store one in the other quite happily. You could also store it in the model as G_TYPE_ULONG. Furthermore, I don't think you actually need this signal id, you can use g_signal_handlers_disconnect_by_func().
Comment 57 Chandni Verma 2011-11-10 04:04:17 UTC
You were right, someone added those pointers unnecessarily and even I didn't notice.
That's done and the "All" option is added in here-
https://gitorious.org/glassrose-gnome/empathy/commits/add-All-service-selection-in-debug-window

Individual patches follow for review.
Comment 58 Chandni Verma 2011-11-10 04:05:30 UTC
Created attachment 201111 [details] [review]
No need to keep a pointer to new-debug-message-signal

We should just destroy the proxy if it gets disconnected for
whatsoever reason.
Comment 59 Chandni Verma 2011-11-10 04:05:42 UTC
Created attachment 201112 [details] [review]
Remove unrequired data associated with proxies

No need to hold pointer to invalidated signal handler for proxies
or new-debug-message dbus signal connections as we don't need to
disconnect the dbus signal connection when a proxy invalidates.
Comment 60 Chandni Verma 2011-11-10 04:05:47 UTC
Created attachment 201113 [details] [review]
Factor out create_proxy_to_get_messages
Comment 61 Chandni Verma 2011-11-10 04:05:58 UTC
Created attachment 201114 [details] [review]
Store associations to proxies while creating them
Comment 62 Chandni Verma 2011-11-10 04:06:09 UTC
Created attachment 201115 [details] [review]
Store useful proxies in service store by traversing each service row
Comment 63 Chandni Verma 2011-11-10 04:06:20 UTC
Created attachment 201116 [details] [review]
Add "All" selection to service_chooser
Comment 64 Chandni Verma 2011-11-10 04:06:30 UTC
Created attachment 201117 [details] [review]
Destroy "All" selection's list_store when needed

This is required when either any service's proxy invalidates
or when a service's name owner changes.
Comment 65 Chandni Verma 2011-11-10 04:06:40 UTC
Created attachment 201118 [details] [review]
Connect service_store to row-changed signal
Comment 66 Chandni Verma 2011-11-10 04:06:46 UTC
Created attachment 201119 [details] [review]
Copy messages to All's list store and set visibility
Comment 67 Chandni Verma 2011-11-10 04:06:51 UTC
Created attachment 201120 [details] [review]
Adding a Debug message on saving proxy for a service
Comment 68 Chandni Verma 2011-11-10 04:07:01 UTC
Created attachment 201121 [details] [review]
Un-pausing incoming messages should update All's store too
Comment 69 Chandni Verma 2011-11-25 12:33:50 UTC
branches-
https://gitorious.org/glassrose-gnome/empathy/commits/debug-window-overhaul-592994 and
https://gitorious.org/glassrose-gnome/empathy/commits/add-All-service-selection-in-debug-window
are workable and tested. (I am not attaching individual patches, they are just too many)
Comment 70 Chandni Verma 2011-11-25 13:13:51 UTC
I have tried to keep the commits as small and atomic as possible while keeping one feature spanning a branch. https://gitorious.org/glassrose-gnome/empathy/commits/debug-window-overhaul-592994 Is just redesigning which adds no new feature.
https://gitorious.org/glassrose-gnome/empathy/commits/add-All-service-selection-in-debug-window built on top of the previous branch adds an "All" services drop down option.
Comment 71 Chandni Verma 2011-12-03 00:49:25 UTC
list store variables changed to active_buffer and pause_buffer and squashed many a few- https://gitorious.org/glassrose-gnome/empathy/commits/add-All-service-selection-in-debug-window-varchange . This obsoletes old patches.
Comment 72 Chandni Verma 2011-12-03 00:50:26 UTC
Created attachment 202674 [details] [review]
Debug window - Use one list store per service

-Remove cache and store.
-Add one proxy and and related data per service in service_chooser
-Add one list store per service in service_chooser to save logs
-Make the service chooser switch the correct list store to display
-Free proxy_data before disposing service_store
-Fixing "Clear" operation
-Setting the correct store_filter on switching services
Comment 73 Chandni Verma 2011-12-03 00:50:34 UTC
Created attachment 202675 [details] [review]
Adding COL_PAUSE_BUFFER to service_store

We will need to pause all services when "Pause" is pressed so that
we can see what happened in all services at the same time and the
messages that arrive while Pause is on will be buffered in various
services' pause-buffers which will be stored in  COL_PAUSE_BUFFER.
Comment 74 Chandni Verma 2011-12-03 00:50:40 UTC
Created attachment 202676 [details] [review]
Deploy one pause-buffer per proxy

These will queue incoming debug messages while the window is paused
Comment 75 Chandni Verma 2011-12-03 00:50:47 UTC
Created attachment 202677 [details] [review]
Fixed comments
Comment 76 Chandni Verma 2011-12-03 00:50:54 UTC
Created attachment 202678 [details] [review]
Remove unrequired data associated with proxies

No need to hold pointer to invalidated signal handler for proxies
or to new-debug-message dbus signal connections as it's not required
to disconnect a connection when the corresponding proxy invalidates.
Comment 77 Chandni Verma 2011-12-03 00:51:01 UTC
Created attachment 202679 [details] [review]
Factor out create_proxy_to_get_messages
Comment 78 Chandni Verma 2011-12-03 00:51:08 UTC
Created attachment 202680 [details] [review]
Preparing for adding "All" option to service_chooser

-Store associations to proxies while creating them
-Find service_store iter to store useful proxies
Comment 79 Chandni Verma 2011-12-03 00:51:14 UTC
Created attachment 202681 [details] [review]
Add "All" selection to service_chooser
Comment 80 Chandni Verma 2011-12-03 00:51:20 UTC
Created attachment 202682 [details] [review]
Destroy "All" selection's list_store when needed

This is required when either any service's proxy invalidates
or when a service's name owner changes.

https://bugzilla.gnome.org/show_bug.cgi?id=592994

Conflicts:

	src/empathy-debug-window.c
Comment 81 Chandni Verma 2011-12-03 00:51:26 UTC
Created attachment 202683 [details] [review]
Connect service_store to row-changed signal

Create service_chooser_row_changed_cb and invoke it as needed
Comment 82 Chandni Verma 2011-12-03 00:51:32 UTC
Created attachment 202684 [details] [review]
Add incoming messages to All's list store
Comment 83 Chandni Verma 2011-12-03 00:51:39 UTC
Created attachment 202685 [details] [review]
Set window sensitivity
Comment 84 Chandni Verma 2011-12-03 00:51:45 UTC
Created attachment 202686 [details] [review]
Un-pausing incoming messages should update All's store too
Comment 85 Chandni Verma 2011-12-03 01:02:59 UTC
reworded commits with 'list store' to 'active-buffer'
Comment 86 Chandni Verma 2011-12-03 01:08:35 UTC
Created attachment 202689 [details] [review]
Destroy "All" selection's active-buffer when needed

This is required when either any service's proxy invalidates
or when a service's name owner changes
Comment 87 Chandni Verma 2011-12-17 00:23:31 UTC
Is there anything else preventing this from going in? I hope we don't have to reset milestones again for this one.. :|
Comment 88 Chandni Verma 2012-01-13 12:17:10 UTC
ok, since the requirements changed 3 times while I was working on it and since the comment summary no longer represents the actual bug, I am changing it to be more indicative.
Comment 89 Danielle Madeley 2012-01-23 04:57:32 UTC
Review of glassrose/add-All-service-selection-in-debug-window-varchange. Looks pretty good. Some minor things:

+  if (priv->paused)
+    {
+      gtk_list_store_append (pause_buffer, &iter);
+      gtk_list_store_set (pause_buffer, &iter,
+          COL_DEBUG_TIMESTAMP, timestamp,
+          COL_DEBUG_DOMAIN, domain,
+          COL_DEBUG_CATEGORY, category,
+          COL_DEBUG_LEVEL_STRING, log_level_to_string (level),
+          COL_DEBUG_MESSAGE, string,
+          COL_DEBUG_LEVEL_VALUE, level,
+          -1);
+    }
+  else
+    {
+      /* Append 'this' message to the store */
+      gtk_list_store_append (active_buffer, &iter);
+      gtk_list_store_set (active_buffer, &iter,
+          COL_DEBUG_TIMESTAMP, timestamp,
+          COL_DEBUG_DOMAIN, domain,
+          COL_DEBUG_CATEGORY, category,
+          COL_DEBUG_LEVEL_STRING, log_level_to_string (level),
+          COL_DEBUG_MESSAGE, string,
+          COL_DEBUG_LEVEL_VALUE, level,
+          -1);
+    }

Should made this one codepath with gtk_list_store_insert_with_values (priv->paused ? paused : active, ...

Having big blocks of repeated code leads to hard to spot differences between them.

+  gtk_list_store_append (active_buffer, &active_buffer_iter);
+  gtk_list_store_set (active_buffer, &active_buffer_iter,

Use gtk_list_store_insert_with_values() where possible.

-      G_TYPE_POINTER);/* COL_PROXY_DATA */
+      G_TYPE_OBJECT); /* COL_PROXY */
   gtk_combo_box_set_model (GTK_COMBO_BOX (priv->choos

TP_TYPE_PROXY.

+          if (gone)
+            gtk_tree_model_foreach (GTK_TREE_MODEL (service_active_buffer),
+                copy_buffered_messages, new_all_active_buffer);
+
+	  else
+            if (proxy != NULL)
+              {
+                if (service_active_buffer == NULL)
+                  break;

Fraught indenting. Mixing tabs and spaces? Also use braces here, otherwise it's very hard to track where blocks end.

+                if (error != NULL)
+                    DEBUG ("Failed at duping the dbus daemon: %s", error->message);

Must free error. Also get out of this code block.

+          gtk_tree_view_set_model (GTK_TREE_VIEW (priv->view),
+              priv->store_filter);
+
+	  debug_window_set_toolbar_sensitivity (debug_window, TRUE);
+        }
+

More fraught indenting. Lots of other examples in this commit too. Check your editor settings.

-      for (valid_iter = gtk_tree_model_get_iter_first (model, &iter);
+      /* Skipping the first iter which is reserved for "All" */
+      gtk_tree_model_get_iter_first (model, &iter);
+      for (valid_iter = gtk_tree_model_iter_next (model, &iter);

You should check validity here.

+      gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->service_store),
+          &all_iter);
+      gtk_list_store_set (priv->service_store, &all_iter,
+          COL_ACTIVE_BUFFER, NULL,
+          -1);

And here. And once more in this function.

You do this particular bit of code a lot (at least 3 times?), why not just store All's active_buffer in priv?

~

No need to keep attaching all the patches with git-bz, this bug report is getting long. Just post a link to the branch that needs review. :)
Comment 91 Danielle Madeley 2012-01-24 07:37:43 UTC
+  gtk_list_store_append (active_buffer, &active_buffer_iter);
+  gtk_list_store_set (active_buffer, &active_buffer_iter,

Replace these with insert_with_values() -- all occurrences.

+  EmpathyDebugWindowPriv *priv = GET_PRIV ((EmpathyDebugWindow *) user_data);

Would prefer EmpathyDebugWindow *self = user_data; and then GET_PRIV() after that.

Although we're getting rid of the GET_PRIV macro, and replacing it with self->priv everywhere, if you felt like adding one more commit to do that.

I still think cd83166a46e950aadf2b could use gtk_list_store_clear(). This will let you store the model for All in priv. Rather than having to constantly get it from the service model.

+          if (gone)
+            gtk_tree_model_foreach (GTK_TREE_MODEL (service_active_buffer),
+                copy_buffered_messages, new_all_active_buffer);
+
+          else
+            if (proxy != NULL)

Need braces around this block, it's confusing to read.

afcbd8d47f7b9 recreates the mess of multiple codepaths again. If you need, create a function that does the insert_with_values() for a given proxy.

Also ideally squash c4071ad3eb29 into your first commit because the proxy_data stuff is confusing to read around.
Comment 92 Chandni Verma 2012-01-26 16:40:36 UTC
(In reply to comment #91)
> +  gtk_list_store_append (active_buffer, &active_buffer_iter);
> +  gtk_list_store_set (active_buffer, &active_buffer_iter,
> 
> Replace these with insert_with_values() -- all occurrences.

Done.
I didn't knew -1 indicates position of the last row. Perhaps I should file a bug for this function's incomplete documentation.

> 
> +  EmpathyDebugWindowPriv *priv = GET_PRIV ((EmpathyDebugWindow *) user_data);
> 
> Would prefer EmpathyDebugWindow *self = user_data; and then GET_PRIV() after
> that.
> 
> Although we're getting rid of the GET_PRIV macro, and replacing it with
> self->priv everywhere, if you felt like adding one more commit to do that.
> 
OK. We can do it later. I am going away :)


> I still think cd83166a46e950aadf2b could use gtk_list_store_clear(). This will
> let you store the model for All in priv. Rather than having to constantly get
> it from the service model.

Using gtk_list_store_clear would mean we make All's active buffer update each time any property of any of the services changes. This could lead in huge mess if we have to avoid exponential creation of proxies and slow runtime of our program. I had considered this solution before coming up with the current approach which is fastest and simplest by all means.

For having all's active buffer stored in priv, I can still do that if you want but that would mean no more than creating redundant variables unless you want to reinvent the wheel and introduce more complexity in the process.

> 
> +          if (gone)
> +            gtk_tree_model_foreach (GTK_TREE_MODEL (service_active_buffer),
> +                copy_buffered_messages, new_all_active_buffer);
> +
> +          else
> +            if (proxy != NULL)
> 
> Need braces around this block, it's confusing to read.
> 
Edited in the commit that introduced it.

> afcbd8d47f7b9 recreates the mess of multiple codepaths again. If you need,
> create a function that does the insert_with_values() for a given proxy.
> 
Done and also did it for all occurrences in the file in a separately added patch.

> Also ideally squash c4071ad3eb29 into your first commit because the proxy_data
> stuff is confusing to read around.

Done.
Comment 93 Chandni Verma 2012-01-26 16:46:55 UTC
(In reply to comment #91)
> +  gtk_list_store_append (active_buffer, &active_buffer_iter);
> +  gtk_list_store_set (active_buffer, &active_buffer_iter,
> 
> Replace these with insert_with_values() -- all occurrences.

Done.
I didn't knew -1 indicates position of the last row. Perhaps I should file a bug for this function's incomplete documentation.

> 
> +  EmpathyDebugWindowPriv *priv = GET_PRIV ((EmpathyDebugWindow *) user_data);
> 
> Would prefer EmpathyDebugWindow *self = user_data; and then GET_PRIV() after
> that.
> 
> Although we're getting rid of the GET_PRIV macro, and replacing it with
> self->priv everywhere, if you felt like adding one more commit to do that.
> 
OK. We can do it later. I am not going away :)


> I still think cd83166a46e950aadf2b could use gtk_list_store_clear(). This will
> let you store the model for All in priv. Rather than having to constantly get
> it from the service model.

Using gtk_list_store_clear would mean we make All's active buffer update each time any property of any of the services changes. This could lead in huge mess if we have to avoid exponential creation of proxies and slow runtime of our program. I had considered this solution before coming up with the current approach which is fastest and simplest of all those I have tried.

For having all's active buffer stored in priv, I can still do that if you want but that would mean no more than creating redundant variables unless you want to reinvent the wheel and introduce more complexity in the process.

> 
> +          if (gone)
> +            gtk_tree_model_foreach (GTK_TREE_MODEL (service_active_buffer),
> +                copy_buffered_messages, new_all_active_buffer);
> +
> +          else
> +            if (proxy != NULL)
> 
> Need braces around this block, it's confusing to read.
> 
Edited in the commit that introduced it.

> afcbd8d47f7b9 recreates the mess of multiple codepaths again. If you need,
> create a function that does the insert_with_values() for a given proxy.
> 
Done and also did it for all occurrences in the file in a separately added patch.

> Also ideally squash c4071ad3eb29 into your first commit because the proxy_data
> stuff is confusing to read around.

Done.
Comment 95 Danielle Madeley 2012-01-26 23:06:26 UTC
Ok, this is looking a lot cleaner!

(In reply to comment #93)
> Using gtk_list_store_clear would mean we make All's active buffer update each
> time any property of any of the services changes. This could lead in huge mess
> if we have to avoid exponential creation of proxies and slow runtime of our
> program. I had considered this solution before coming up with the current
> approach which is fastest and simplest of all those I have tried.

I don't understand, I'm just saying you clear it every time that you currently destroy and recreate it.

e.g.

+      /* If a new service arrives when "All" is selected, the view will
+       * not show its messages which we do not want. So we destroy All's
+       * list store and it will be created afresh utilizing the new proxy,
+       * in the row-changed-cb for "All" selection.
+       * Similarly for when a service with an already seen service name
+       * appears. */

Just clear it instead. And then when you rebuild it, you get the same result, but you've held onto the pointer for all_active_buffer, which means you can keep that in priv, rather than always having to look it up from the service_store.

~

8f257ab397308e95c358da870b7aad21cb411341 duplicates the same code block 3 times. Make that code block a function.

~

+proxy_invalidated_cb (TpProxy *proxy,
+    guint domain,
+    gint code,
+    gchar *msg,
+    gpointer user_data)
+{
+  /* Proxy has been invalidated so we destroy it */
+  tp_clear_object (&proxy);
+}

This looks suspicious. Whose reference are you destroying? If it's the one held by the service_store then you need to instead be setting the entry in the service_store to NULL.

Although it looks to me that you're still holding your initial reference from when you created, rather than just passing that reference to the service store. Which should be fine here, but means there's a reference hanging around that you're not actually holding a pointer for.
Comment 96 Chandni Verma 2012-01-27 12:21:22 UTC
(In reply to comment #95)
> Ok, this is looking a lot cleaner!

Thanks!

> 
> (In reply to comment #93)
> > Using gtk_list_store_clear would mean we make All's active buffer update each
> > time any property of any of the services changes. This could lead in huge mess
> > if we have to avoid exponential creation of proxies and slow runtime of our
> > program. I had considered this solution before coming up with the current
> > approach which is fastest and simplest of all those I have tried.
> 
> I don't understand, I'm just saying you clear it every time that you currently
> destroy and recreate it.

Ohk! I got utterly confused. I misunderstood what you meant. I thought you wanted all's active store updation to be called whenever any property of any service changes which could be hazardous and could have implied using gtk_list_store_clear.
What you say can easily be done! Apologies for the mistake. I'll update the branch shortly.

> 
> 8f257ab397308e95c358da870b7aad21cb411341 duplicates the same code block 3
> times. Make that code block a function.
ok

> 
> ~
> 
> +proxy_invalidated_cb (TpProxy *proxy,
> +    guint domain,
> +    gint code,
> +    gchar *msg,
> +    gpointer user_data)
> +{
> +  /* Proxy has been invalidated so we destroy it */
> +  tp_clear_object (&proxy);
> +}
> 
> This looks suspicious. Whose reference are you destroying? If it's the one held
> by the service_store then you need to instead be setting the entry in the
> service_store to NULL.

I was just saving a service lookup here but I'll do that if it's unclear.
Comment 97 Chandni Verma 2012-01-28 07:27:10 UTC
Alright and done! I think we've just pointed our gun to this big fat rat :)

link: https://gitorious.org/glassrose-gnome/empathy/commits/add-All-service-selection-in-debug-window
Comment 98 Guillaume Desmottes 2012-02-17 09:21:10 UTC
Let's merge this, I want it in 3.4 :)

Thanks a lot for your work on this!
Comment 99 Guillaume Desmottes 2012-02-17 09:22:07 UTC
Attachment 202679 [details] pushed as 52715c3 - Factor out create_proxy_to_get_messages
Attachment 202680 [details] pushed as 7a6a03a - Preparing for adding "All" option to service_chooser
Attachment 202685 [details] pushed as 4a6fb67 - Set window sensitivity
Comment 100 Guillaume Desmottes 2012-02-17 09:27:55 UTC
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.