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?

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.

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.

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.

(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.

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.

Here is my public branch for this bug-
http://gitorious.org/glassrose-gnome-repository/empathy/commits/debug-window-save-all-592994

(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

Created attachment 178846 [details] [review]
second commit of my branch

Danielle: can you please take a look on this as you did the first round of reviews?

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.

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.

(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.]

(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?

Can anyone elaborate on what needs to be done for this? How to produce a generic setup?

I wouldn't block merging of this branch on this tbh. Anything else blocking this branch?

Branch https://gitorious.org/glassrose-gnome/empathy/commits/debug-window-save-all-592994 updated and rebased. It can be merged now.

Danni: are you ok with  the final version of this branch?

Created attachment 187442 [details] [review]
from git repo

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.

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!)

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.

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?

should I be calling external command "pastebinit" using system()? This will require making pastebinit a dependency.

(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.

Couldn't we already merge this and adds the pastebin feature later in another bug?

Yeah, we should open another bug for the pastebin button.

We are past UI freeze so that probably won't make it for 3.2.

I don't think anything else needs to be done on this. Should I perform a fixup leaving only the major feature commit?

Rebased on top of current master.

Danni: is this good to go?

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.

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.

Any chance we can, finally, get this merged? :)

Chandni was refactoring it. I don't know if she's finished. I'm waiting for her to attach a new patch.

(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.

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

Created attachment 200310 [details] [review]
Free proxy_data before disposing service_store

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

Created attachment 200312 [details] [review]
Don't log critical messages on disconnecting proxies

Created attachment 200313 [details] [review]
Fixing "Clear" operation

Created attachment 200314 [details] [review]
Setting the correct store_filter on switching services

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.

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.

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.

Created attachment 200318 [details] [review]
Flush all pause buffers when Pause button is released

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.

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.

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.

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.

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.

(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.

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.

(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.

(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.

(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().

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.

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.

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.

Created attachment 201113 [details] [review]
Factor out create_proxy_to_get_messages

Created attachment 201114 [details] [review]
Store associations to proxies while creating them

Created attachment 201115 [details] [review]
Store useful proxies in service store by traversing each service row

Created attachment 201116 [details] [review]
Add "All" selection to service_chooser

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.

Created attachment 201118 [details] [review]
Connect service_store to row-changed signal

Created attachment 201119 [details] [review]
Copy messages to All's list store and set visibility

Created attachment 201120 [details] [review]
Adding a Debug message on saving proxy for a service

Created attachment 201121 [details] [review]
Un-pausing incoming messages should update All's store too

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)

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.

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.

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

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.

Created attachment 202676 [details] [review]
Deploy one pause-buffer per proxy

These will queue incoming debug messages while the window is paused

Created attachment 202677 [details] [review]
Fixed comments

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.

Created attachment 202679 [details] [review]
Factor out create_proxy_to_get_messages

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

Created attachment 202681 [details] [review]
Add "All" selection to service_chooser

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

Created attachment 202683 [details] [review]
Connect service_store to row-changed signal

Create service_chooser_row_changed_cb and invoke it as needed

Created attachment 202684 [details] [review]
Add incoming messages to All's list store

Created attachment 202685 [details] [review]
Set window sensitivity

Created attachment 202686 [details] [review]
Un-pausing incoming messages should update All's store too

reworded commits with 'list store' to 'active-buffer'

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

Is there anything else preventing this from going in? I hope we don't have to reset milestones again for this one.. :|

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.

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. :)

Done!

Link to branch:
https://gitorious.org/glassrose-gnome/empathy/commits/add-All-service-selection-in-debug-window-varchange-rebase

+  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.

(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.

(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.

Branch with changes:
https://gitorious.org/glassrose-gnome/empathy/commits/add-All-service-selection-in-debug-window-2

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.

(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.

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

Let's merge this, I want it in 3.4 :)

Thanks a lot for your work on this!

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

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.