GNOME Bugzilla – Bug 762622
Non-printing characters in log view
Last modified: 2016-03-24 11:59:55 UTC
Created attachment 322253 [details] screenshot See attached screenshot: there are quite a lot of non-printing characters in the log view. Would be nice to have those.
Presumably, it's the newline character which is being displayed as the non-printing characters.I think we should remove all such characters from the label before displaying it.
Created attachment 323660 [details] [review] gnome-logs: fix the non-printing characters in log view The problem is that when viewing certain log messages expecially coredump messages , non-printable characters appear in the label. To fix the problem, it was detected that the characters were due to an empty line in the message or two consecutive newline characters. In this patch, I replaced the two consecutive newline characters with empty character to solve the issue. Now, the non-printable characters don't appear anymore.
Review of attachment 323660 [details] [review]: ::: src/gl-eventviewrow.c @@ +260,3 @@ } + gchar *temp_message_label = g_strdup(gl_journal_entry_get_message (entry)); Memory leak here. You should free temp_message_label after using it. Also, "temp_message" should be a better name, since it's a string rather than a GtkLabel. @@ +261,3 @@ + gchar *temp_message_label = g_strdup(gl_journal_entry_get_message (entry)); + const gchar *message_label = g_strdelimit(temp_message_label,"\n\n",'\0'); Does it make sense to replace "\n\n" with "\0"? In that way, the message is gonna be incomplete. "message" should be a better option as described above.
Created attachment 323849 [details] [review] Replaced the '\n\n' characters with a space character. Changed the variable names.
Review of attachment 323849 [details] [review]: Replacing a double-newline with a space, and leaving a single newline unchanged does not make much sense, as you will still see substituted characters in the label. Unicode contains some useful code points for indicating newlines in a single-line string, such as "" (the symbol for newline). You could then treat a double-newline as a paragraph separator, although it is probably better to simply substitute all newlines for the newline symbol. You will probably be unable to use g_strdelimit() to substitute for the newline character, so you should look at the g_utf8_*() family of functions. ::: src/gl-eventviewrow.c @@ +263,3 @@ + temp_message = g_strdup (gl_journal_entry_get_message (entry)); + + priv->message_label = gtk_label_new ( g_strdelimit (temp_message, "\n\n", ' ')); g_strdelimit() replaces all characters in the second argument with the replacement from the third argument, so listing a character multiple times in the second argument does not have the expected effect.
(In reply to David King from comment #5) ... > Replacing a double-newline with a space, and leaving a single newline > unchanged does not make much sense, as you will still see substituted > characters in the label. Unicode contains some useful code points for > indicating newlines in a single-line string, such as "" (the symbol for > newline). That  character is impossible to read here, and I doubt that many people would know what it means. It might be best to substitute line breaks with a small block of spacing (equivalent to 3/4 monospace characters) - this is conceptually similar to a new line.
Created attachment 323898 [details] [review] Added support for detecting unicode newline and replacing it with an empty string
(In reply to Allan Day from comment #6) > That  character is impossible to read here, and I doubt that many people > would know what it means. It might be best to substitute line breaks with a > small block of spacing (equivalent to 3/4 monospace characters) - this is > conceptually similar to a new line. That is something that should be fixed in the font.
Review of attachment 323898 [details] [review]: ::: src/gl-eventviewrow.c @@ +265,3 @@ + + for (unichar = message; unichar && *unichar; + unichar = g_utf8_next_char (unichar)) The documentation for g_utf8_next_char() mentions that the string must have been validated before using it. You need to call g_utf8_validate(). @@ +273,3 @@ + if( c=='\n' ) + { + *unichar=' '; Use the newline substitution character rather than a space.
(In reply to David King from comment #8) > (In reply to Allan Day from comment #6) > > That  character is impossible to read here, and I doubt that many people > > would know what it means. It might be best to substitute line breaks with a > > small block of spacing (equivalent to 3/4 monospace characters) - this is > > conceptually similar to a new line. > > That is something that should be fixed in the font. The rendering is an issue with the font and (whatever's being used for font rendering), certainly. However, the choice of character is not. If users aren't familiar with a character, we shouldn't use it. Likewise, we shouldn't use visible characters when they're not needed, as they have an overhead in terms of polluting the view and distracting from actual log content.
(In reply to Allan Day from comment #10) > The rendering is an issue with the font and (whatever's being used for font > rendering), certainly. However, the choice of character is not. If users > aren't familiar with a character, we shouldn't use it. Likewise, we > shouldn't use visible characters when they're not needed, as they have an > overhead in terms of polluting the view and distracting from actual log > content. A visible character is needed as a substitute in this case, because it is difficult (impossible, if the same character sequence is used) to distinguish a non-visible character from other log entry content. In the case of newlines occuring in a single-line label, splitting the string at the point of the first newline and showing a continuation character (such as an ellipsis) instead would be an alternative. The details view shows the log message as a multi-line label, so the newlines are interpreted literally in that case.
Created attachment 324099 [details] [review] Added support for detecting unicode newline and replacing it with an empty string https://bugzilla.gnome.org/show_bug.cgi?id=762622 -Add support for validating the string before displaying. -Replace newline characters with unicode newline sustitutor. -Add error message if string is not a valid utf8 string.
(In reply to David King from comment #11) ... > A visible character is needed as a substitute in this case, because it is > difficult (impossible, if the same character sequence is used) to > distinguish a non-visible character from other log entry content. Can you give an example of when this would be necessary in order to understand a log message? I'm not sure I understand. > In the case of newlines occuring in a single-line label, splitting the > string at the point of the first newline and showing a continuation > character (such as an ellipsis) instead would be an alternative. The details > view shows the log message as a multi-line label, so the newlines are > interpreted literally in that case. Ellipsis would be better than the newline character, although I still fear that it would be rather noisy.
Review of attachment 324099 [details] [review]: The first line of the commit message is way too long, and should be kept to a maximum of 50 characters. ::: src/gl-eventviewrow.c @@ +224,3 @@ + + message = g_strdup (oldmessage); + newmessage = g_string_sized_new (200); Rather than using a fixed allocation, you should calculate the difference in size between the original string (counting the number of newlines), and then add on the extra space needed for the newline substitute. @@ +233,3 @@ + c = g_utf8_get_char (current_char); + + if (c == '\n') Rather than appending character by character, it is better to search for a newline, then append the whole string up to that character (possibly using g_string_append_len()). @@ +295,3 @@ + message = gl_journal_entry_get_message (entry); + + if (g_utf8_validate (message, -1, NULL) == TRUE) Instead of comparing against TRUE, simply do: if (g_utf8_validate (message, -1, NULL)) @@ +301,3 @@ + else + { + message_label = "Unable to display the log message."; This needs to be a translatable string: _("Unable to display the log message") Also, remember that if you g_free() the string later, you need to g_malloc() (or g_strdup() or similar) it here.
Created attachment 324214 [details] [review] Add support for displaying newline character https://bugzilla.gnome.org/show_bug.cgi?id=762622 -Add support for validating the string before displaying. -Replace newline characters with unicode newline sustitutor. -display error message if unable to read log message
Review of attachment 324214 [details] [review]: ::: src/gl-eventviewrow.c @@ +238,3 @@ + c = g_utf8_get_char (current_char); + + if (c == '\n') You should just use strchr() to search for newline characters. There is no need to use the g_utf8_*() functions here, as "\n" is in the ASCII range. @@ +250,3 @@ + else + { + newmessage = g_string_sized_new (strlen (message) + (3 * newline_count)); Why 3? This needs a comment. @@ +257,3 @@ + while (newline_index != NULL) + { + Empty line. @@ +264,3 @@ + substr[newline_index-prevpos] = '\0'; + + g_string_append (newmessage, substr); Why copy the string, and then append to the GString, when you can just append the string to the GString directly (as the length of the substring is known)? @@ +373,2 @@ g_free (time); + g_free (message_label); You must not call g_free() on something not allocated by g_malloc() (or a function which calls g_malloc()).
Created attachment 324278 [details] [review] Add support for displaying newline character https://bugzilla.gnome.org/show_bug.cgi?id=762622 -Add support for validating the string before displaying. -Replace newline characters with unicode newline sustitutor. -display error message if unable to read log message
Review of attachment 324278 [details] [review]: ::: src/gl-eventviewrow.c @@ +245,3 @@ + } + + null_index = strchr (prevpos, '\0'); Use strlen() instead of strchr(str, '\0'). @@ +253,3 @@ + } + + return newmessage->str; You never free message in this case. @@ +313,3 @@ + } + + priv->message_label = gtk_label_new (message_label); Now you have a memory leak, as you duplicate the string and do not free it. gtk_label_new() makes a copy, in other words.
Created attachment 324298 [details] [review] Add support for displaying newline character https://bugzilla.gnome.org/show_bug.cgi?id=762622 -Add support for validating the string before displaying. -Replace newline characters with unicode newline sustitutor. -display error message if unable to read log message
Review of attachment 324298 [details] [review]: It would be better overall if you could avoid duplicating the string at all, as in the common case there will be no newlines present. It would be simpler if you (in gl_event_view_row_constructed()) check for a newline, and if found, call the function to substitute the newlines (and always return a newly-allocated string). This means that you should keep track of whether a newline was found (and therefore whether the string needs to be freed) in _constructed(). ::: src/gl-eventviewrow.c @@ +344,2 @@ g_free (time); + g_free (message_label); This still calls g_free() on a string that is obtained from gettext() (the "Unable to display the log message" string).
Created attachment 324357 [details] [review] Add support for displaying newline character https://bugzilla.gnome.org/show_bug.cgi?id=762622 -Add support for validating the string before displaying. -Replace newline characters with unicode newline sustitutor. -display error message if unable to read log message
Review of attachment 324357 [details] [review]: ::: src/gl-eventviewrow.c @@ +221,3 @@ + GString *newmessage; + gchar *newline_index; + gchar *prevpos; This should probably be "const gchar *". @@ +230,3 @@ + while (newline_index != NULL) + { + g_string_append_len (newmessage, prevpos, newline_index-prevpos); Add spaces around the '-' @@ +240,3 @@ + + message = g_malloc (newmessage->len + 1); + g_stpcpy (message, newmessage->str); Why are you copying the string before returning it? Instead, call g_string_free() with the second parameter as FALSE, and return the result. @@ +301,3 @@ + { + message_label = g_strdup (message); + message_label = gl_event_view_row_replace_newline (message_label); You have a memory leak here, as you duplicate the string before passing it to gl_event_view_row_replace_newline(), then create a GString in that function, which you then return (before you have freed the original message_label).
Created attachment 324486 [details] [review] Add support for displaying newline character https://bugzilla.gnome.org/show_bug.cgi?id=762622 -Add support for validating the string before displaying. -Replace newline characters with unicode newline sustitutor. -display error message if unable to read log message
Review of attachment 324486 [details] [review]: ::: src/gl-eventviewrow.c @@ +239,3 @@ + g_string_append (newmessage, prevpos); + + return newmessage->str; You leak a GString here, and need to call g_string_free(). @@ +295,3 @@ + if (newline_index) + { + message_label = gl_event_view_row_replace_newline (message); As message_label is only used in this scope, you should declare the variable here too.
Created attachment 324491 [details] [review] Add support for displaying newline character https://bugzilla.gnome.org/show_bug.cgi?id=762622 -Add support for validating the string before displaying. -Replace newline characters with unicode newline sustitutor. -display error message if unable to read log message
Review of attachment 324491 [details] [review]: Looks good. You can improve the commit message a bit (such as by writing full sentences in the long part of the message), but the patch is fine to be pushed after hard code freeze ends.
Created attachment 324493 [details] [review] Add support for displaying newline character https://bugzilla.gnome.org/show_bug.cgi?id=762622 Replace newline characters with unicode newline sustitutor.
Created attachment 324504 [details] [review] Add support for displaying newline character Replace newline characters with unicode newline sustitutor.Fixes bug 762622.
Created attachment 324545 [details] [review] Add support for displaying newline character Replace newline characters with unicode newline substitutor.Fixes bug 762622.
Review of attachment 324545 [details] [review]: Pushed to master as commit 477fb02abbc83e47f3db58491c69333e3ec153c1, with a minor change to the commit message. Thanks!