GNOME Bugzilla – Bug 709153
Use catalogs to explain messages
Last modified: 2014-09-23 12:15:28 UTC
journalctl offers a -x switch which shows explanation texts for log messages from a catalog. gnome-logs should make this information available too.
There is some documentation on the systemd wiki about the catalogue format: http://www.freedesktop.org/wiki/Software/systemd/catalog/ There is a simple API to get the message from the catalogue for the current entry: http://www.freedesktop.org/software/systemd/man/sd_journal_get_catalog_for_message_id.html
Created attachment 256186 [details] screenshot of message catalogue in details view I pushed a simple commit to add information from the message catalogue to the detailed event view. I also added a wrapped version of the log message, and the view now seems a lot more useful. It might be nice to somehow format the message from the catalogue nicely, or maybe just use a fixed-width font instead. Screenshot attached (as it is somewhat difficult to find a log event with a message in the catalogue).
In order to finish off this feature, Logs should parse the message catalogue output and populate the event details view with the information, rather than just dumping the whole catalogue field as a label.
I'm working on this.
Created attachment 285713 [details] [review] patch for the bug: separate the catalog messages into several fields and show them in the UI. Use strtok function to parse result->catalog.
Review of attachment 285713 [details] [review]: The catalog parsing does not quite seem to cover what is specified in the documentation: http://www.freedesktop.org/wiki/Software/systemd/catalog/ Each header field can appear multiple times. You handle 2 documentation headers, but there could be 0, 1, 2, or more. The order of header fields is not specified, so you need to investigate if the fields can appear in different orders. Other than that, and the other comments, this looks good! ::: src/gl-eventviewdetail.c @@ +278,3 @@ { GlEventViewDetail *detail = GL_EVENT_VIEW_DETAIL (object); + The seems like an unnecessary whitespace change, and should be removed. ::: src/gl-journal.c @@ +245,2 @@ ret = sd_journal_get_catalog (journal, &result->catalog); + This looks like an unnecessary whitespace change, and should be removed. ::: src/gl-journal.h @@ +69,3 @@ + gchar *documentation1; + gchar *documentation2; + gchar *detailed_message; It does not make much sense to add fields to the GlJournalResult struct if the GlJournal API does not use them.
Created attachment 285902 [details] [review] Get it improved
Review of attachment 285902 [details] [review]: You should squash this patch into your previous patch, so that there is only a single patch to apply. There is also lots of indentation, and you should make sure that you are not using tabs for indentation, as well as matching the indentation of surrounding code. Using g_strlcat() seems weird, and it is probably better to just use g_strconcat(). ::: data/gl-eventviewdetail.ui @@ +188,3 @@ </child> + <child> + <object class="GtkLabel" id="subject_field_label"> Strange indentation here, and throughout the remainder of the file. ::: src/gl-eventviewdetail.c @@ +155,3 @@ + { + str = " "; + str_field = strtok (str_copy, str); There is no need to use a separate variable for the second argument to strtok(), just use a string literal instead. @@ +156,3 @@ + str = " "; + str_field = strtok (str_copy, str); + } Odd indentation again. @@ +166,2 @@ { + subject_count ++; No space needed before the ++. @@ +172,3 @@ + + if (str_message && *str_message) + { Strange indentation. @@ +180,3 @@ + else + { + str = "\n"; Odd indentation. @@ +183,3 @@ + str_field = strtok (NULL, str); + str_temp = gtk_label_get_text (GTK_LABEL (priv->subject_label)); + str = g_strdup (str_temp); There is no need for a separate variable here, just call g_strdup() on the label text directly. @@ +188,3 @@ + + if (str && *str) + { Indentation. @@ +206,3 @@ + + if (str_message && *str_message) + { Odd indentation. @@ +217,3 @@ + str_field = strtok (NULL, str); + str_temp = gtk_label_get_text (GTK_LABEL (priv->definedby_label)); + str = g_strdup (str_temp); Strange indentation. @@ +222,3 @@ + + if (str && *str) + { Weird indentation. @@ +244,3 @@ + gtk_widget_show (priv->support_field_label); + gtk_widget_show (priv->support_label); + } Weird indentation. @@ +251,3 @@ + str_field = strtok (NULL, str); + str_temp = gtk_label_get_text (GTK_LABEL (priv->support_label)); + str = g_strdup (str_temp); Indentation. @@ +274,3 @@ + + if (str_message && *str_message) + { Indentation. @@ +282,3 @@ + else + { + str = "\n"; Strange indentation. @@ +290,3 @@ + + if (str && *str) + { Odd indentation. ::: src/gl-journal.h @@ +20,3 @@ #define GL_JOURNAL_H_ +#include <gtk/gtk.h> This looks like an unnecessary change.
I got the weird indentation and other things fixed. But I don't know how to squash this patch into my previous patch. Can you help me?
Created attachment 285929 [details] [review] patch for the bug I merged the previous commit and here is the patch
Review of attachment 285929 [details] [review]: Looking quite good now. ::: src/gl-eventviewdetail.c @@ +265,3 @@ + str_field = strtok (NULL, "\n"); + str = g_strdup (gtk_label_get_text (GTK_LABEL (priv->subject_label))); + str = (str, "\n", str_field); I get a compiler warning about this line: src/gl-eventviewdetail.c: In function ‘gl_event_view_detail_create_detail’: src/gl-eventviewdetail.c:267:31: warning: left-hand operand of comma expression has no effect [-Wunused-value] str = (str, "\n", str_field); ^ src/gl-eventviewdetail.c:267:37: warning: left-hand operand of comma expression has no effect [-Wunused-value] str = (str, "\n", str_field); Can you fix it? @@ +277,3 @@ + } + } + } while (g_strcmp0 (str_field, "Subject:") == 0 || g_strcmp0 (str_field, "Defined-By:") == 0 || g_strcmp0 (str_field, "Support:") == 0 || g_strcmp0 (str_field, "Documentation:") == 0); This line is really long, can you break it up into something shorter?
Created attachment 285935 [details] [review] patch for the bug get it updated
Review of attachment 285935 [details] [review]: Looking good in general, just a few (mostly minor) problems now. ::: data/gl-style.css @@ +14,3 @@ } +.detail-audit_session, .detail-kernel_device, .detail-message, .detail-priority, .detail-time, .detail-subject, .detail-definedby, .detail-support, .detail-documentation1, .detail-documentation2, .detail-message { There is no "detail-documentation2" style classes any more, so it should be removed. "detail-documentation1 should be renamed "detail-documentation". ::: src/gl-eventviewdetail.c @@ +73,3 @@ + gint definedby_count = 0; + gint support_count = 0; + gint documentation_count = 0; You should reduce the scope of these variables, and move them within the "if (result->catalog != NULL)". @@ +161,3 @@ + if (subject_count == 1) + { + str = "\n"; Odd indentation here (and you should just use the string literal directly in the strtok() call). @@ +265,3 @@ + str_field = strtok (NULL, "\n"); + str = g_strdup (gtk_label_get_text (GTK_LABEL (priv->subject_label))); + str = g_strconcat(str, "\n", str_field); I get a compiler error from this line: src/gl-eventviewdetail.c: In function ‘gl_event_view_detail_create_detail’: src/gl-eventviewdetail.c:267:21: error: missing sentinel in function call [-Werror=format=] str = g_strconcat(str, "\n", str_field); @@ +280,3 @@ + g_strcmp0 (str_field, "Defined-By:") == 0 || + g_strcmp0 (str_field, "Support:") == 0 || + g_strcmp0 (str_field, "Documentation:") == 0); Looks much better now!
Created attachment 285997 [details] [review] git it updated again sorry for my carelessness, it would work this time
Created attachment 286004 [details] [review] minor UI and commit message changes I made some minor changes to the patch, such as making the catalog message span over 2 rows in the GtkGrid. I also modified the commit message slightly. The patch is ready to be merged after the UI freeze.
Review of attachment 286004 [details] [review]: I fixed a memory leak, did some minor reformatting and pushed this to master as e409919dffb83c758ab96a0f3d85eaa150808c44. Thanks!