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 762622 - Non-printing characters in log view
Non-printing characters in log view
Status: RESOLVED FIXED
Product: gnome-logs
Classification: Other
Component: general
3.19.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-logs maintainer(s)
gnome-logs maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-02-24 15:52 UTC by Allan Day
Modified: 2016-03-24 11:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (385.25 KB, image/png)
2016-02-24 15:52 UTC, Allan Day
  Details
gnome-logs: fix the non-printing characters in log view (1.55 KB, patch)
2016-03-10 19:52 UTC, Pranav Ganorkar
needs-work Details | Review
Replaced the '\n\n' characters with a space character. Changed the variable names. (1.63 KB, patch)
2016-03-14 11:11 UTC, Pranav Ganorkar
needs-work Details | Review
Added support for detecting unicode newline and replacing it with an empty string (1.87 KB, patch)
2016-03-14 17:48 UTC, Pranav Ganorkar
none Details | Review
Added support for detecting unicode newline and replacing it with an empty string (2.81 KB, patch)
2016-03-16 13:37 UTC, Pranav Ganorkar
needs-work Details | Review
Add support for displaying newline character (3.76 KB, patch)
2016-03-17 20:20 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (2.77 KB, patch)
2016-03-18 14:33 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (3.07 KB, patch)
2016-03-18 18:49 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (2.69 KB, patch)
2016-03-20 08:20 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (2.54 KB, patch)
2016-03-21 16:57 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (2.56 KB, patch)
2016-03-21 17:48 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (2.45 KB, patch)
2016-03-21 18:31 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (2.42 KB, patch)
2016-03-21 20:10 UTC, Pranav Ganorkar
none Details | Review
Add support for displaying newline character (2.42 KB, patch)
2016-03-22 16:59 UTC, Pranav Ganorkar
committed Details | Review

Description Allan Day 2016-02-24 15:52:13 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.
Comment 1 Pranav Ganorkar 2016-03-10 15:56:19 UTC
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.
Comment 2 Pranav Ganorkar 2016-03-10 19:52:09 UTC
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.
Comment 3 Jonathan Kang 2016-03-14 08:22:37 UTC
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.
Comment 4 Pranav Ganorkar 2016-03-14 11:11:55 UTC
Created attachment 323849 [details] [review]
Replaced the '\n\n' characters with a space character. Changed the variable names.
Comment 5 David King 2016-03-14 11:33:22 UTC
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.
Comment 6 Allan Day 2016-03-14 12:07:22 UTC
(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.
Comment 7 Pranav Ganorkar 2016-03-14 17:48:54 UTC
Created attachment 323898 [details] [review]
Added support for detecting unicode newline and replacing it with an empty string
Comment 8 David King 2016-03-15 07:41:18 UTC
(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.
Comment 9 David King 2016-03-15 07:45:43 UTC
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.
Comment 10 Allan Day 2016-03-15 14:31:33 UTC
(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.
Comment 11 David King 2016-03-15 15:21:22 UTC
(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.
Comment 12 Pranav Ganorkar 2016-03-16 13:37:42 UTC
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.
Comment 13 Allan Day 2016-03-16 14:06:55 UTC
(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.
Comment 14 David King 2016-03-16 15:01:25 UTC
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.
Comment 15 Pranav Ganorkar 2016-03-17 20:20:22 UTC
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
Comment 16 David King 2016-03-18 10:35:41 UTC
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()).
Comment 17 Pranav Ganorkar 2016-03-18 14:33:42 UTC
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
Comment 18 David King 2016-03-18 14:49:50 UTC
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.
Comment 19 Pranav Ganorkar 2016-03-18 18:49:47 UTC
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
Comment 20 David King 2016-03-18 19:38:34 UTC
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).
Comment 21 David King 2016-03-18 19:38:35 UTC
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).
Comment 22 Pranav Ganorkar 2016-03-20 08:20:29 UTC
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
Comment 23 David King 2016-03-21 15:33:53 UTC
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).
Comment 24 Pranav Ganorkar 2016-03-21 16:57:05 UTC
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
Comment 25 David King 2016-03-21 17:38:21 UTC
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.
Comment 26 Pranav Ganorkar 2016-03-21 17:48:49 UTC
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
Comment 27 David King 2016-03-21 17:59:07 UTC
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.
Comment 28 Pranav Ganorkar 2016-03-21 18:31:44 UTC
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.
Comment 29 Pranav Ganorkar 2016-03-21 20:10:59 UTC
Created attachment 324504 [details] [review]
Add support for displaying newline character

Replace newline characters with unicode newline sustitutor.Fixes bug
762622.
Comment 30 Pranav Ganorkar 2016-03-22 16:59:19 UTC
Created attachment 324545 [details] [review]
Add support for displaying newline character

Replace newline characters with unicode newline substitutor.Fixes bug 762622.
Comment 31 David King 2016-03-24 11:59:41 UTC
Review of attachment 324545 [details] [review]:

Pushed to master as commit 477fb02abbc83e47f3db58491c69333e3ec153c1, with a minor change to the commit message. Thanks!