GNOME Bugzilla – Bug 579725
Implement debug dialog using o.fd.Tp.Debug
Last modified: 2009-06-04 22:23:16 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
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(-)
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?
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?
- 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)
(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.
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(-)
(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/
(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.
- 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?
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.
(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
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
Merged to master.