+1, How is gnome logs populated? Any pointer to specific function or file?

I agree that this would look much cleaner.

Allan, what do you think should happen when scrolling? In the mockup you linked, the first visible row shows the shared timestamp for the next four rows. When that row scrolls out of view, should we move the timestamp to the next row down? If so, should we move it when it scrolls out of view completely or also when it is only half visible?

Created attachment 323750 [details] [review]
gnome-logs: show time label only once for similar group of log messages

This patch shows the date only once for a similar group
of log messages having same date.The date is shown only for
the most recent log message having a particular date.

(In reply to Kunaal Jain from comment #1)
> +1, How is gnome logs populated? Any pointer to specific function or file?

Logs reads journal from systemd-journald by calling sd_journal_open[1].


*[1] https://www.freedesktop.org/software/systemd/man/sd_journal_open.html

Created attachment 369053 [details] [review]
gl-eventviewrow.c: show time label only once for similar group of log messages

In the above comments, an issue has been pointed out where we have to think about cases when dates go out of view. In this patch, dates have been kept intact with compressed-row-entries headers so that we won't have to scroll much when we have like 30-40 compressed entries. And we often have compressed entries. So, visible date entries will be almost near to each other (not much scrolls required). And with current patch we even decreased repetitions.
So, repetitions exist for headers but less in number.

Created attachment 369055 [details]
screenshot of above patch

Review of attachment 369053 [details] [review]:

Error due to additional g_print.

Created attachment 369087 [details] [review]
gl-eventviewrow.c: show time label only once for similar group of log messages

Review of attachment 369087 [details] [review]:

::: src/gl-eventviewrow.c
@@ +374,3 @@
                                          now, priv->clock_format, FALSE);
+    previous_time = gl_util_timestamp_to_display (gl_journal_entry_get_timestamp (previous_entry),
+                                         now, priv->clock_format, FALSE);

Strange indentation here. Where does previous_time get freed?

@@ +379,3 @@
+
+    if (g_strcmp0 (time, previous_time) == 0)
+        priv->time_label = gtk_label_new ("     ");

Why use an empty string in the label?

::: src/gl-journal-model.c
@@ +34,2 @@
     GlJournalEntry *journal_entry;
+    GlJournalEntry *previous_journal_entry;

It seems odd to keep a pointer to the previous entry, when the model itself can be queried for any entry (using g_list_model_get_item() or simiar). Can you adapt the patch to not modify the journal model?

@@ +1179,2 @@
   g_clear_object (&row_entry->journal_entry);
+  g_clear_object (&row_entry->previous_journal_entry);

gl_row_entry_get_journal_entry() does not increment the reference count, so why is g_clear_object() used here?

Created attachment 369587 [details] [review]
gnome-logs: show time label once for similar group of log messages

Review of attachment 369587 [details] [review]:

::: src/gl-eventviewrow.c
@@ +374,3 @@
+    if(gl_journal_entry_get_display_time_label (entry))
+    {
+        priv->time_label = gtk_label_new (time);

This is duplicated with line 374.

@@ +379,3 @@
+    {
+        priv->time_label = gtk_label_new ("      ");/*space necessary to avoid message_label
+						      in time_label space*/

Please follow the code style for comments in the project.

::: src/gl-journal-model.c
@@ +100,3 @@
+    gchar *previous_entry_time_label;
+    gchar *current_entry_time_label;
+    GDateTime *now;

Declaring these values in the block where they are used can be better option.

@@ +167,3 @@
+
+                previous_entry_time_label = gl_util_timestamp_to_display (gl_journal_entry_get_timestamp (previous_entry),
+                                         				  now, GL_UTIL_CLOCK_FORMAT_12HR, FALSE);

The clock format should not be fixed. You should get the value from GSettings schema.
There are related codes in gl-util.c which you can refer to.

Emm. this shouldn't be much of a problem, as we're just comparing these two strings.

Created attachment 369649 [details] [review]
gnome-logs: show time label only once for similar group of log messages

Review of attachment 369649 [details] [review]:

No need to use "gnome-logs" in the commit message. Also, you should give a link to the bug in the description.

::: src/gl-eventviewrow.c
@@ +372,3 @@
     g_date_time_unref (now);
+
+    if(gl_journal_entry_get_display_time_label (entry))

Missing a space before the "if".

@@ +378,3 @@
+    else
+    {
+	/*blank space necessary to avoid message_label to extend in place of time_label*/

Missing spaces around the comment, and a full stop.

::: src/gl-journal-model.c
@@ +174,3 @@
+
+                /* TODO: Timestamp should be compared directly in future. */
+                if(g_strcmp0 (previous_entry_time_label, current_entry_time_label) == 0)

Missing a space after the "if", and curly braces around the statements (or you could instead use a ternary expression).

::: src/gl-journal.c
@@ +884,3 @@
+gl_journal_entry_set_display_time_label (GlJournalEntry *entry, gboolean value)
+{
+  entry->display_time_label = value;

Missing a g_return_if_fail() check.

Created attachment 369676 [details] [review]
Show time label only once for repeated time labels of entries.

This patch shows time labels only for those entries whose time label is different from their previous one.

Review of attachment 369676 [details] [review]:

Try to keep the first line of the commit message under 50 characters, and do not end it with a full stop.

::: src/gl-eventviewrow.c
@@ +380,3 @@
+	/* Blank space is necessary to avoid message_label to extend in place of time_label. */
+        priv->time_label = gtk_label_new ("      ");
+    }

Leave a blank line after the closing curly brace.

::: src/gl-journal.c
@@ +884,3 @@
+gl_journal_entry_set_display_time_label (GlJournalEntry *entry, gboolean value)
+{
+  g_return_val_if_fail (GL_IS_JOURNAL_ENTRY (entry), NULL);

You cannot return a value from a function that returns void. Did this not generate a compiler warning?

Created attachment 369716 [details] [review]
Show time-stamps once for entries with similar time-stamps

This patch shows time labels only for those entries whose time label is different from their previous one.

Review of attachment 369716 [details] [review]:

I pushed a slightly-modified patch to master as commit 1f529a54536d78d50abb9671bb182429a4179a4a. I fixed a few lines with tabs used as indentation, and reflowed the commit message. Otherwise, it was perfect!