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 579725 - Implement debug dialog using o.fd.Tp.Debug
Implement debug dialog using o.fd.Tp.Debug
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: empathy-maint
empathy-maint
Depends on:
Blocks:
 
 
Reported: 2009-04-21 14:38 UTC by Jonny Lamb
Modified: 2009-06-04 22:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
debug dialog (35.79 KB, patch)
2009-04-21 17:00 UTC, Jonny Lamb
reviewed Details | Review
diff (39.71 KB, patch)
2009-04-24 01:05 UTC, Jonny Lamb
committed Details | Review

Description Jonny Lamb 2009-04-21 14:38:39 UTC
Gabble has a branch that implements the org.freedesktop.Telepathy.Debug interface.

I've started making a dialog to support reading this output:

Branch: http://git.collabora.co.uk/?p=user/jonny/empathy.git;a=shortlog;h=refs/heads/debug

Screenshot of current progress: http://people.collabora.co.uk/~jonny/debug-dialog-4.png
Comment 1 Jonny Lamb 2009-04-21 17:00:20 UTC
Created attachment 133057 [details] [review]
debug dialog

 extensions/Debug.xml       |  112 ++++++
 extensions/Makefile.am     |    3 +-
 extensions/misc.xml        |    1 +
 src/Makefile.am            |    1 +
 src/empathy-debug-dialog.c |  911 ++++++++++++++++++++++++++++++++++++++++++++
 src/empathy-debug-dialog.h |   63 +++
 src/empathy-main-window.c  |    9 +
 src/empathy-main-window.ui |    7 +
 8 files changed, 1106 insertions(+), 1 deletions(-)
Comment 2 Guillaume Desmottes 2009-04-21 18:12:51 UTC
I'd be curious to know how hard it would be to extend this dialog to display Empathy's debug messages as well. Should the spec be changed/extended to support clients too?
Comment 3 Jonny Lamb 2009-04-21 18:45:05 UTC
That's a good question. I suppose now would be time to investigate it.

One easy way to do it would be also let Empathy implement and expose the o.fd.Tp.Debug interface. From there, you'd only need to change the list of CMs to add an "empathy" -> "org.gnome.Empathy" (or whatever Empathy's bus name is, which I suppose depends on bug #560159) and it'd immediately work (I think).

However, that seems a little weird to send Empathy debug messages over D-Bus and pick them up in the same process, but it would be a pretty easy implementation. After all, hopefully the debugger in gabble could be thrown into tp-glib so that all CMs, and perhaps clients, could immediately use it.

But, yeah, is that too much on crack?
Comment 4 Guillaume Desmottes 2009-04-23 10:35:31 UTC
- What's the status of the Debug spec? We have to wait it's merged (as a DRAFT) to tp-spec before merging this branch.

- New files should follow latest tp coding style. Seems you used the old function definition style. See http://telepathy.freedesktop.org/wiki/Style

- I'm not convinced by "Debug Window" in the Help menu. Why not just "Debug"?

- CM not supporting Debug should be insensitive in the combobox (and so the 'Pause' button).

- The CM combobox is not updated when new CM are added or removed

- I still think a button to copy the full buffer to the clipboard would be nice (as bug-buddy)

- The save button looks weird. The icon is too close from the label.

- debug_dialog_cm_chooser_changed_cb:
  if (priv->signal_connection)
  if (priv->proxy)
you should check != NULL

-   debug_dialog_cm_chooser_changed_cb (GTK_COMBO_BOX (priv->cm_chooser), debug_dialog);
this line is too big

- line 314: any reason to not use a guint?

- debug_dialog_button_press_event_cb: some comments would be good. Why '3' for example.

- debug_dialog_store_filter_foreach: is FILE still the best API to save files in the 21th century? :)

- line 627 and 677 are too big

- debug_dialog_dispose: use (if ptr != NULL)
Comment 5 Jonny Lamb 2009-04-24 00:57:19 UTC
(In reply to comment #4)
> - What's the status of the Debug spec? We have to wait it's merged (as a DRAFT)
> to tp-spec before merging this branch.

I've started a spec branch, and made the interface nicer than the XML in this branch. Sure, this won't be merged until the debug is merged as a DRAFT, but there's no harm in reviewing this at the same time, right? The spec branch can be found here:

http://git.collabora.co.uk/?p=user/jonny/telepathy-spec.git;a=shortlog;h=refs/heads/debug

> - New files should follow latest tp coding style. Seems you used the old
> function definition style. See http://telepathy.freedesktop.org/wiki/Style

Fixed. :-(

> - I'm not convinced by "Debug Window" in the Help menu. Why not just "Debug"?

Changed.

> - CM not supporting Debug should be insensitive in the combobox (and so the
> 'Pause' button).

I'm really unsure about how to go about achieving this. I can't call tp_proxy_has_interface because I'm already calling tp_proxy_add_interface_by_id, and so forcing the former function to always say yes. (Shock horror I don't understand TpProxy enough)

Perhaps I'll ask Simon tomorrow and see if there is a way.

> - The CM combobox is not updated when new CM are added or removed

So, I've implemented this, but I really don't like it. It works, but, yeah... If I'm just being silly and not seeing the proper way of doing it then great. Otherwise, are you sure you want this essential feature for the cost of ugly code? Or, am I just being overdramatic?

(As an aside, it's also a little weird because once you disconnect an account so the CM no longer has any connections, the CM times out after a few seconds (unless GABBLE/SALUT_PERSIST is set) so it visually looks a little weird.)

If a CM dies on us, then the CM is removed from the chooser and if there is at least one CM still left in the combo box, then the first one is chosen. If there are no more CMs connected, then the combo box is empty. This is great, but the tree view is emptied in either event. I think one problem is if you were trying to get logs from the CM to find out why it crashed, but if it disappears off the bus, you lose access to logs.

But, is this actually a problem? As in, is this dialog a target for that kind of debugging? Any logs from a crashed CM might actually be useless anyway without a backtrace so perhaps not. Thoughts?

> - I still think a button to copy the full buffer to the clipboard would be nice
> (as bug-buddy)

I thought about this, and I definitely do not think this is useful. The common use case for a debug output like this for either getting the entire log and attaching it to a bug report (or pastebin) or looking at a single line and I think I've catered for both cases: The save button saves the contents to a file, and if you right-click on an item in the treeview then you can copy that line to the clipboard. The latter is useful for IRC, for example.

I just don't see how keeping all that output in your clipboard over a file is useful?

> - The save button looks weird. The icon is too close from the label.

Hmm, I agree, but it's just a GTK_STOCK_SAVE. Are you suggesting that I rewrite my own GtkToolButton? I would advise against that.

> - debug_dialog_cm_chooser_changed_cb:
>   if (priv->signal_connection)
>   if (priv->proxy)
> you should check != NULL

Fixed.

> -   debug_dialog_cm_chooser_changed_cb (GTK_COMBO_BOX (priv->cm_chooser),
> debug_dialog);
> this line is too big

Fixed.

> - line 314: any reason to not use a guint?

Nope, changed.

> - debug_dialog_button_press_event_cb: some comments would be good. Why '3' for
> example.

Added.

> - debug_dialog_store_filter_foreach: is FILE still the best API to save files
> in the 21th century? :)

Changed to using GIO and all its magic.

> - line 627 and 677 are too big

Fixed.

> - debug_dialog_dispose: use (if ptr != NULL)

Fixed.

I've also fixed a few other things, like storing CM bus names in the CM chooser instead of connection well-known names so CMs don't appear more than once if there are more than connections active. I also removed the "parent" property and now use GtkWindow's "transient-for" property in empathy_debug_dialog_new, which is a little nicer.
Comment 6 Jonny Lamb 2009-04-24 01:05:53 UTC
Created attachment 133232 [details] [review]
diff

 extensions/Debug.xml       |  112 +++++
 extensions/Makefile.am     |    3 +-
 extensions/misc.xml        |    1 +
 src/Makefile.am            |    1 +
 src/empathy-debug-dialog.c | 1107 ++++++++++++++++++++++++++++++++++++++++++++
 src/empathy-debug-dialog.h |   63 +++
 src/empathy-main-window.c  |    9 +
 src/empathy-main-window.ui |    7 +
 8 files changed, 1302 insertions(+), 1 deletions(-)
Comment 7 Guillaume Desmottes 2009-04-24 09:38:13 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > - What's the status of the Debug spec? We have to wait it's merged (as a DRAFT)
> > to tp-spec before merging this branch.
> 
> I've started a spec branch, and made the interface nicer than the XML in this
> branch. Sure, this won't be merged until the debug is merged as a DRAFT, but
> there's no harm in reviewing this at the same time, right? The spec branch can
> be found here:

Sure, having the branch ready and merge it as soon as the DRAFT is merged would be great.

> > - New files should follow latest tp coding style. Seems you used the old
> > function definition style. See http://telepathy.freedesktop.org/wiki/Style
> 
> Fixed. :-(

I added a Style section on http://live.gnome.org/Empathy/ to avoid confusion in the future.

> > - CM not supporting Debug should be insensitive in the combobox (and so the
> > 'Pause' button).
> 
> I'm really unsure about how to go about achieving this. I can't call
> tp_proxy_has_interface because I'm already calling
> tp_proxy_add_interface_by_id, and so forcing the former function to always say
> yes. (Shock horror I don't understand TpProxy enough)
>
> Perhaps I'll ask Simon tomorrow and see if there is a way.

I'm not a TpProxy guru either. Time to ask to Simon :)

> > - The CM combobox is not updated when new CM are added or removed
> 
> So, I've implemented this, but I really don't like it. It works, but, yeah...
> If I'm just being silly and not seeing the proper way of doing it then great.
> Otherwise, are you sure you want this essential feature for the cost of ugly
> code? Or, am I just being overdramatic?
> 
> (As an aside, it's also a little weird because once you disconnect an account
> so the CM no longer has any connections, the CM times out after a few seconds
> (unless GABBLE/SALUT_PERSIST is set) so it visually looks a little weird.)
> 
> If a CM dies on us, then the CM is removed from the chooser and if there is at
> least one CM still left in the combo box, then the first one is chosen. If
> there are no more CMs connected, then the combo box is empty. This is great,
> but the tree view is emptied in either event. I think one problem is if you
> were trying to get logs from the CM to find out why it crashed, but if it
> disappears off the bus, you lose access to logs.
> 
> But, is this actually a problem? As in, is this dialog a target for that kind
> of debugging? Any logs from a crashed CM might actually be useless anyway
> without a backtrace so perhaps not. Thoughts?

Humm I'm not sure about this. If the others think that's not needed then I'll join their advice.
Xavier, Davyd: thoughts?

> > - I still think a button to copy the full buffer to the clipboard would be nice
> > (as bug-buddy)
> 
> I thought about this, and I definitely do not think this is useful. The common
> use case for a debug output like this for either getting the entire log and
> attaching it to a bug report (or pastebin) or looking at a single line and I
> think I've catered for both cases: The save button saves the contents to a
> file, and if you right-click on an item in the treeview then you can copy that
> line to the clipboard. The latter is useful for IRC, for example.
> 
> I just don't see how keeping all that output in your clipboard over a file is
> useful?

For pastebin, it's a lot easier to click on a button and then paste it to your web browser than:
- save logs to a file
- open the file
- select everything
- copy it
- paste to the browser

> > - The save button looks weird. The icon is too close from the label.
> 
> Hmm, I agree, but it's just a GTK_STOCK_SAVE. Are you suggesting that I rewrite
> my own GtkToolButton? I would advise against that.

Oh it's just a plain stock? I'm surprised it looks like this.
I won't change it then.


> > - debug_dialog_store_filter_foreach: is FILE still the best API to save files
> > in the 21th century? :)
> 
> Changed to using GIO and all its magic.

\o/

Comment 8 Jonny Lamb 2009-04-24 12:52:32 UTC
(In reply to comment #7)
> I'm not a TpProxy guru either. Time to ask to Simon :)

So, after talking to tp-glib people today, I've fixed another problem in my branch (creating a TpConnection instead of TpProxy), but I think we should just leave this and direct our efforts into implementing debug in all the CMs.

After all, selecting a CM that doesn't support it doesn't do anything bad. It just doesn't get any output.

> For pastebin, it's a lot easier to click on a button and then paste it to your
> web browser than:
> - save logs to a file
> - open the file
> - select everything
> - copy it
> - paste to the browser

Hm, okay. Done.
Comment 9 Guillaume Desmottes 2009-04-28 09:46:54 UTC
- According to Sjoerd, the debug window could/should be in its own process. That's really easy to do with current implementation, right? (just have to instantiate the object).
Note that in that case, it would make sense to expose Empathy's debug messages on the bus.

- I tried to build your branch and got:
empathy-debug-dialog.c:859: erreur: format ‘%u’ expects type ‘unsigned int’, but argument 4 has type ‘size_t’
Use G_GSIZE_FORMAT

- Not detecting CM which doesn't implement Debug still make me cry. Maybe insensitive buttons (and their entry in the combobox) if GetMessages failed?
Comment 10 Guillaume Desmottes 2009-04-28 17:12:20 UTC
We agreed during spec meeting that clients could implement org.freedesktop.Telepathy.Debug as well. Once Empathy does (bug #580631), this dialog should be implement to display Empathy's debug output.
Comment 11 Jonny Lamb 2009-06-03 14:34:13 UTC
(In reply to comment #9)
> - I tried to build your branch and got:
> empathy-debug-dialog.c:859: erreur: format ‘%u’ expects type ‘unsigned
> int’, but argument 4 has type ‘size_t’
> Use G_GSIZE_FORMAT

Done.

> - Not detecting CM which doesn't implement Debug still make me cry. Maybe
> insensitive buttons (and their entry in the combobox) if GetMessages failed?

Yeaahhhhh, let's just implement it in CMs really quick! I definitely think having this kind of check will be super hacky...

I also updated the spec to the XML from tp-spec:

http://git.collabora.co.uk/?p=user/jonny/empathy.git;a=shortlog;h=refs/heads/debug
Comment 12 Jonny Lamb 2009-06-04 15:55:10 UTC
I implemented a "not supported" display[0] and make check passes.

Ready to go.

0. http://people.collabora.co.uk/~jonny/debug-dialog-6.png
Comment 13 Guillaume Desmottes 2009-06-04 22:23:16 UTC
Merged to master.