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 767996 - Add a search popover to gnome logs
Add a search popover to gnome logs
Status: RESOLVED FIXED
Product: gnome-logs
Classification: Other
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-logs maintainer(s)
gnome-logs maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-06-24 06:38 UTC by Pranav Ganorkar
Modified: 2016-10-20 08:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Changed the "uid" variable in GlJournalEntry from gint to gchar* (4.08 KB, patch)
2016-06-24 06:42 UTC, Pranav Ganorkar
none Details | Review
Implemented Search Popover with Parameter filtering (40.65 KB, patch)
2016-06-24 06:43 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (31.31 KB, patch)
2016-07-22 13:44 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (31.31 KB, patch)
2016-07-22 13:49 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (31.23 KB, patch)
2016-07-22 13:54 UTC, Pranav Ganorkar
none Details | Review
Add search type option to search popover (21.38 KB, patch)
2016-07-23 14:21 UTC, Pranav Ganorkar
none Details | Review
Add range filtering to search popover (38.97 KB, patch)
2016-07-24 16:50 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (31.35 KB, patch)
2016-07-25 18:02 UTC, Pranav Ganorkar
none Details | Review
Add search type option to search popover (23.71 KB, patch)
2016-07-26 18:08 UTC, Pranav Ganorkar
none Details | Review
Add range filtering to search popover (38.95 KB, patch)
2016-07-26 18:41 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (34.86 KB, patch)
2016-08-10 14:07 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (38.03 KB, patch)
2016-08-10 17:05 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (38.01 KB, patch)
2016-08-11 15:02 UTC, Pranav Ganorkar
none Details | Review
Add search type option to search popover (29.41 KB, patch)
2016-08-11 15:27 UTC, Pranav Ganorkar
none Details | Review
Add range filtering to search popover (43.50 KB, patch)
2016-08-11 16:56 UTC, Pranav Ganorkar
none Details | Review
Add range filtering to search popover (42.14 KB, patch)
2016-08-14 18:36 UTC, Pranav Ganorkar
none Details | Review
Add custom range submenu to search popover (138.80 KB, patch)
2016-08-14 18:37 UTC, Pranav Ganorkar
none Details | Review
Add custom range submenu to search popover (138.49 KB, patch)
2016-08-14 18:48 UTC, Pranav Ganorkar
none Details | Review
Add custom range submenu to search popover (139.98 KB, patch)
2016-08-14 19:15 UTC, Pranav Ganorkar
none Details | Review
Add search popover with parameter filtering (38.26 KB, patch)
2016-08-15 13:22 UTC, Pranav Ganorkar
none Details | Review
Add search type option to search popover (29.38 KB, patch)
2016-08-15 13:23 UTC, Pranav Ganorkar
none Details | Review
Add range filtering to search popover (42.30 KB, patch)
2016-08-15 13:25 UTC, Pranav Ganorkar
none Details | Review
Add search popover with journal field filtering (36.43 KB, patch)
2016-08-17 10:24 UTC, Pranav Ganorkar
none Details | Review
Add search type option to search popover (29.33 KB, patch)
2016-08-17 10:26 UTC, Pranav Ganorkar
none Details | Review
Add journal timestamp range filters (41.28 KB, patch)
2016-08-17 10:28 UTC, Pranav Ganorkar
none Details | Review
Add search popover with journal field filtering (36.44 KB, patch)
2016-08-17 13:13 UTC, Pranav Ganorkar
none Details | Review
Add search type option to search popover (29.33 KB, patch)
2016-08-17 13:15 UTC, Pranav Ganorkar
none Details | Review
Add search popover with journal field filtering (36.50 KB, patch)
2016-08-17 13:23 UTC, Pranav Ganorkar
none Details | Review
Add search type option to search popover (29.38 KB, patch)
2016-08-17 13:24 UTC, Pranav Ganorkar
none Details | Review
Add search popover with journal field filtering (35.95 KB, patch)
2016-08-19 13:03 UTC, Pranav Ganorkar
committed Details | Review
Add search type option to search popover (29.08 KB, patch)
2016-08-19 13:05 UTC, Pranav Ganorkar
committed Details | Review
Add journal timestamp range filters (40.45 KB, patch)
2016-08-19 13:21 UTC, Pranav Ganorkar
none Details | Review
Add journal timestamp range filters (40.46 KB, patch)
2016-08-19 14:34 UTC, Pranav Ganorkar
committed Details | Review
Add custom journal timestamp range submenu (140.71 KB, patch)
2016-08-20 16:05 UTC, Pranav Ganorkar
committed Details | Review

Description Pranav Ganorkar 2016-06-24 06:38:47 UTC
The search popover can be selected from the drop down button besides the search entry. It allows you to select the parameter like PID, UID, Message, Systemd Unit against which the search text will be matched.
Comment 1 Pranav Ganorkar 2016-06-24 06:42:18 UTC
Created attachment 330293 [details] [review]
Changed the "uid" variable in GlJournalEntry from gint to gchar*
Comment 2 Pranav Ganorkar 2016-06-24 06:43:05 UTC
Created attachment 330294 [details] [review]
Implemented Search Popover with Parameter filtering
Comment 3 Pranav Ganorkar 2016-07-22 13:44:25 UTC
Created attachment 331995 [details] [review]
Add search popover with parameter filtering

The search popover can be selected from the drop down menu besides the search bar. Individual parameters can be selected from the search popover.
Comment 4 Pranav Ganorkar 2016-07-22 13:49:15 UTC
Created attachment 331998 [details] [review]
Add search popover with parameter filtering

The search popover can be selected from the drop down menu besides the search bar.Individual parameters can be selected from the search popover.
Comment 5 Pranav Ganorkar 2016-07-22 13:54:03 UTC
Created attachment 332001 [details] [review]
Add search popover with parameter filtering

The search popover can be selected from the drop down menu besides the search bar.Individual parameters can be selected from the search popover.
Comment 6 Pranav Ganorkar 2016-07-23 14:21:03 UTC
Created attachment 332026 [details] [review]
Add search type option to search popover

A radio button group was addded to select between "Exact" and "Substring" search types. We hide the search type radio button group if "All Available Fields" is selected as exact search doesn't make sense in that case. The GlJournalModel class was modified to handle the case when all search fields are exact and there are no substring matches. Also, it was modified to handle the case when the exact search match string has a null or empty value in which case we do not pass it to gl_journal_set_matches().
Comment 7 Pranav Ganorkar 2016-07-24 16:50:53 UTC
Created attachment 332038 [details] [review]
Add range filtering to search popover

The range filtering option in search popover can used to set the timestamp range of journal.The default journal range is current boot. The query object was modified to include two new parameters for storing the start and end timestamps of the journal. The gl-journal-set-matches() was modified to remove setting the boot match if no boot match is specified.
Comment 8 David King 2016-07-25 14:11:18 UTC
Review of attachment 332001 [details] [review]:

::: Makefile.am
@@ +31,3 @@
 	src/gl-eventviewdetail.c \
 	src/gl-eventviewlist.c \
+    src/gl-search-popover.c \

Use tabs, not spaces.

::: data/gl-eventviewlist.ui
@@ +42,3 @@
+                                <child>
+                                    <object class="GtkMenuButton" id="search_dropdown_button">
+                                        <property name="direction">GTK_ARROW_DOWN</property>

This is the default, so there is no point specifying it explicitly.

::: data/gl-searchpopover.ui
@@ +17,3 @@
+                        <property name="can_focus">False</property>
+                        <property name="vhomogeneous">False</property>
+                        <property name="transition_duration">250</property>

Do not specify properties if the default is sensible.

@@ +119,3 @@
+                                <property name="can_focus">False</property>
+                                <property name="margin_top">10</property>
+                                <property name="label" translatable="yes">What</property>

This needs a translator comment.

@@ +127,3 @@
+                            <packing>
+                                <property name="name">what-label</property>
+                                <property name="title" translatable="yes">page0</property>

Why is this property translatable?

@@ +135,3 @@
+                                <property name="can_focus">False</property>
+                                <property name="margin_top">10</property>
+                                <property name="label" translatable="yes">Select Parameter...</property>

Use the Unicode ellipsis “…” and not three full stops.

@@ +143,3 @@
+                            <packing>
+                                <property name="name">select-parameter-label</property>
+                                <property name="title" translatable="yes">page1</property>

Again, this should not be translatable.

@@ +170,3 @@
+            </row>
+            <row>
+                <col id="0" translatable="yes">Seperator</col>

Is this column ever shown? If not, the contents should not be translated.

::: src/gl-eventviewlist.c
@@ +742,3 @@
 static void
+search_popover_parameter_group_changed (GlSearchPopover *popover,
+                                        gint parameter_group,

parameter_group is an int here, a gsize in the private struct and a gulong in the list store. Choose one type and use it consistently.

::: src/gl-search-popover.c
@@ +83,3 @@
+
+    gtk_tree_model_get (GTK_TREE_MODEL (priv->parameter_liststore), iter,
+                        2, &show_seperator,

Rather than specifying the column number like this, use an (anonymous) enum containing an identifier for each column. There is an example in the GtkListStore documentation.

::: src/gl-search-popover.h
@@ +9,3 @@
+{
+    GL_PARAMETER_GROUP_ALL_AVAILABLE_FIELDS,
+    GL_PARAMETER_GROUP_PID = 2,

Add a comment explaining why this value is given explicitly.
Comment 9 David King 2016-07-25 14:29:53 UTC
Review of attachment 332026 [details] [review]:

::: data/gl-searchpopover.ui
@@ +190,3 @@
+                                                <property name="can_focus">True</property>
+                                                <property name="receives_default">False</property>
+                                                <property name="tooltip_text" translatable="yes">Do a Substring match</property>

This needs a better description - something like “Match in any part of the string” (although this in fact searches for the term inside any of the allowed fields).

@@ +208,3 @@
+                                                <property name="can_focus">True</property>
+                                                <property name="receives_default">False</property>
+                                                <property name="tooltip_text" translatable="yes">Do an Exact match</property>

This aldo needs a better description - I suggest “Exact matches only”.

::: src/gl-journal-model.c
@@ +268,3 @@
 
+void
+gl_query_set_exact_search (GlQuery *query, GlQuerySearchType search_type)

You should be consistent, and use GlQuerySearchType for the search type.

::: src/gl-journal-model.h
@@ +63,3 @@
                                                                          const gchar *boot_match);
 
+void                    gl_query_set_exact_search                       (GlQuery *query,

This sets the search type, so should be named gl_query_set_search_type ().

::: src/gl-search-popover.c
@@ +179,3 @@
                                              G_TYPE_INT);
 
+    signals[SEARCH_TYPE] = g_signal_new ("search-type",

This should be a property, not a signal, and then you should connect to the "notify::search-type" signal.
Comment 10 David King 2016-07-25 15:03:23 UTC
Review of attachment 332038 [details] [review]:

::: data/gl-searchpopover.ui
@@ +162,3 @@
+                                <property name="visible">True</property>
+                                <property name="can_focus">False</property>
+                                <property name="label" translatable="yes">When</property>

This needs a translator comment.

@@ +170,3 @@
+                            <packing>
+                                <property name="name">when-label</property>
+                                <property name="title" translatable="yes">page0</property>

This should not be translatable.

@@ +178,3 @@
+                                <property name="can_focus">False</property>
+                                <property name="margin_top">10</property>
+                                <property name="label" translatable="yes">Show Logs from....</property>

Use a Unicode ellipsis “…” rather than multiple full stops.

@@ +186,3 @@
+                            <packing>
+                                <property name="name">show-log-from-label</property>
+                                <property name="title" translatable="yes">page1</property>

This is not a translatable string.

@@ +202,3 @@
+                        <property name="can_focus">False</property>
+                        <property name="vhomogeneous">False</property>
+                        <property name="transition_duration">250</property>

Use the default transition duration.

@@ +213,3 @@
+                                        <property name="can_focus">True</property>
+                                        <property name="receives_default">True</property>
+                                        <property name="tooltip_text" translatable="yes">Select Journal Range</property>

Improve the tooltip; I suggest “Show results for the given range”.

@@ +217,3 @@
+                                        <signal name="clicked" handler="select_range_button_clicked"/>
+                                        <child>
+                                            <object class="GtkBox">

Why is there a box inside a button containing an image and a label? Replicate what is done in the Nautilus search popover, instead of coming up with something different.

@@ +319,3 @@
+                            <packing>
+                                <property name="name">range-list</property>
+                                <property name="title" translatable="yes">page0</property>

Not a translatable string (and it would be a duplicate of the other “page0” and therefore merged by gettext).

@@ +501,3 @@
+            </row>
+            <row>
+                <col id="0" translatable="yes">Seperator</col>

If this is never shown to the user, it should not be translated.

::: src/gl-eventviewlist.c
@@ +570,3 @@
+    GlJournalBootID *boot_id;
+
+    boot_ids = gl_event_view_list_get_boot_ids (view);

Move this to the scope where it is required.

@@ +615,3 @@
+        case GL_RANGE_GROUP_ENTIRE_JOURNAL:
+        {
+

Remove this empty statement, but add a comment and fall through to a default statement.

@@ +635,3 @@
     query = gl_query_new ();
 
+    /* Set Journal Range */

Range of what? Specify that in the comment, otherwise it is difficult to know given the lack of context.

::: src/gl-journal-model.c
@@ +445,3 @@
+
+void
+gl_query_set_journal_range (GlQuery *query,

Think of a better name, that explains the subject of the range.

::: src/gl-journal.c
@@ +627,3 @@
+/* Sets the starting position of journal from which to start reading the entries */
+void
+gl_journal_set_journal_start_position (GlJournal *journal,

Should be gl_journal_set_start_position(), as there is no need to add the extra “_journal”.

@@ +653,3 @@
+/* Check if current entry timestamp is less than the end timestamp */
+gboolean
+gl_journal_entry_check_journal_end (GlJournalEntry *entry,

As you only use this in GlJournalModel, it is best to move it there and make it static (or a macro).
Comment 11 Pranav Ganorkar 2016-07-25 18:02:54 UTC
Created attachment 332116 [details] [review]
Add search popover with parameter filtering

The search popover can be selected from the drop down menu besides the search bar. Individual parameters can be selected from the search popover.
Comment 12 Pranav Ganorkar 2016-07-26 18:08:30 UTC
Created attachment 332174 [details] [review]
Add search type option to search popover

A radio button group was addded to select between "Exact" and "Substring" search types. We hide the search type radio button group if "All Available Fields" is selected as exact search doesn't make sense in that case. The GlJournalModel class was modified to handle the case when all search fields are exact and there are no substring matches. Also, it was modified to handle the case when the exact search match string has a null or empty value in which case we do not pass it to gl_journal_set_matches().
Comment 13 Pranav Ganorkar 2016-07-26 18:41:06 UTC
Created attachment 332175 [details] [review]
Add range filtering to search popover

The range filtering option in search popover can used to set the timestamp range of journal.The default journal range is current boot. The query object was modified to include two new parameters for storing the start and end timestamps of the journal. The gl-journal-set-matches() was modified to remove setting the boot match if no boot match is specified.
Comment 14 David King 2016-08-08 15:49:14 UTC
Review of attachment 332116 [details] [review]:

::: Makefile.am
@@ +31,3 @@
 	src/gl-eventviewdetail.c \
 	src/gl-eventviewlist.c \
+	src/gl-search-popover.c \

This is not the correct filename.

::: data/gl-eventviewlist.ui
@@ +30,2 @@
                                 <property name="visible">True</property>
+                                <property name="spacing">0</property>

This is the same as the default value for the property. Check that you are not specifying default values.

::: data/gl-searchpopover.ui
@@ +1,3 @@
+<interface domain="gnome-logs">
+    <template class="GlSearchPopover" parent="GtkPopover">
+        <property name="can_focus">False</property>

Default value. Check all other values to ensure that you are not setting default values for properties.

@@ +11,3 @@
+                <property name="border_width">20</property>
+                <property name="row_spacing">8</property>
+                <property name="column_spacing">18</property>

Where do the borders and spacings come from?

@@ +23,3 @@
+                                <property name="can_focus">True</property>
+                                <property name="receives_default">True</property>
+                                <property name="tooltip_text" translatable="yes">Select a Journal Parameter</property>

Tooltips should use sentence capitalization. Also, a better explanation is required. Why is the user selecting a journal parameter (and what is a journal parameter)?

::: src/gl-eventviewlist.c
@@ +53,3 @@
     GtkWidget *search_entry;
+    GtkWidget *search_dropdown_button;
+    gulong parameter_group;

parameter_group is not a very descriptive name. Choose something more fitting, such as journal_field or search_field.

@@ +461,3 @@
+    {
+        case GL_PARAMETER_GROUP_ALL_AVAILABLE_FIELDS:
+        {

There is no need for the curly braces to place the following statements into a new scope, as there are no extra variables to declare.

@@ +757,3 @@
+/* Get the view elements from ui file and link it with the drop down button */
+static void
+setup_search_popover (GlEventViewList *view)

The verb is "set up", not "setup" (which is a noun).

@@ +765,3 @@
+    priv = gl_event_view_list_get_instance_private (view);
+
+    /* Create Search Popover Object */

Useless comment.

@@ +772,3 @@
+    g_signal_connect_swapped (search_popover, "closed", (GCallback) gtk_widget_grab_focus, view);
+
+    g_signal_connect (search_popover, "parameter-group",

This signal should be a property, and you should then connect to the notify signal.

::: src/gl-search-popover.c
@@ +23,3 @@
+enum
+{
+    PARAMETER_GROUP,

Change this signal to a property, as previously mentioned.

::: src/gl-search-popover.h
@@ +19,3 @@
+    GL_PARAMETER_GROUP_AUDIT_SESSION,
+    GL_PARAMETER_GROUP_EXECUTABLE_PATH
+} GlParameterGroups;

The name of the enum should match the constant names.
Comment 15 David King 2016-08-08 15:54:02 UTC
Review of attachment 332174 [details] [review]:

::: data/gl-searchpopover.ui
@@ +160,3 @@
+                            <object class="GtkBox">
+                                <property name="visible">True</property>
+                                <property name="can_focus">False</property>

Same comment as the previous patch about not specifying default properties.

::: src/gl-search-popover.h
@@ +29,3 @@
+
+GType gl_query_search_type_get_type (void);
+#define GL_TYPE_QUERY_SEARCH_TYPE (gl_query_search_type_get_type())

Why is this defined here and not alongside GlQuery?
Comment 16 David King 2016-08-08 15:59:56 UTC
Review of attachment 332175 [details] [review]:

::: data/gl-searchpopover.ui
@@ +161,3 @@
+                            <object class="GtkLabel" id="when_dim_label">
+                                <property name="visible">True</property>
+                                <property name="can_focus">False</property>

Usual comment about not setting default values for properties.

::: src/gl-search-popover.c
@@ +35,3 @@
 {
     PARAMETER_GROUP,
+    RANGE_GROUP,

This should be a property and not a signal.

@@ +382,3 @@
+
+    gtk_label_set_label (GTK_LABEL (priv->range_button_label),
+                           _("Current Boot"));

Indentiation is off.

::: src/gl-search-popover.h
@@ +30,3 @@
+    GL_RANGE_GROUP_LAST_3_DAYS = 5,
+    GL_RANGE_GROUP_ENTIRE_JOURNAL = 7
+} GlRangeGroups;

The enum name should match the identifier names.
Comment 17 Pranav Ganorkar 2016-08-10 14:07:53 UTC
Created attachment 333067 [details] [review]
Add search popover with parameter filtering

The search popover can be selected from the drop down menu besides the search bar. Individual parameters can be selected from the search popover.The search popover design was copied from Nautilus.If Nautilus implementation of search popover changes,gnome-logs implementation of search popover should be kept in sync with it.
Comment 18 David King 2016-08-10 14:34:42 UTC
Review of attachment 333067 [details] [review]:

You should mention the bug in the commit message, such as by including a link in a separate paragraph at the end.

::: data/gl-searchpopover.ui
@@ +123,3 @@
+                                <property name="visible">True</property>
+                                <property name="margin_top">10</property>
+                                <property name="label" translatable="yes">Select Parameter…</property>

It is a journal field, not a parameter (just "Select field…" is probably fine).

::: src/gl-searchpopover.c
@@ +1,1 @@
+#include "gl-searchpopover.h"

Missing a license header (see the comments at the top of other gnome-logs files).

@@ +159,3 @@
+    if (!type)
+    {
+        static const GEnumValue _gl_journal_search_field_filter_values[] = {

Do not register the type manually, but instead use glib-mkenums.

::: src/gl-searchpopover.h
@@ +1,1 @@
+#ifndef GL_SEARCH_POPOVER_H_

Also missing a license header.

@@ +9,3 @@
+typedef enum
+{
+    GL_JOURNAL_SEARCH_FIELD_FILTER_ALL_AVAILABLE_FIELDS,

If these enum values use the GlJournal prefix, they should be in gl-journal.h, so you should rename them to something that starts with GlSearchPopover, like GlSearchPopoverField. Additionally, it is bad practice to expose internal implementation details (such as whether a row is used as a separator) as API, so you should find a different way to match up the items in the tree model with the enums, such as by storing the enum nick in the tree model.
Comment 19 Pranav Ganorkar 2016-08-10 17:05:08 UTC
Created attachment 333071 [details] [review]
Add search popover with parameter filtering

The search popover can be selected from the drop down menu besides the search bar. Individual parameters can be selected from the search popover.The search popover design was copied from Nautilus. If Nautilus implementation of search popover changes, gnome-logs implementation of search popover should be kept in sync with it.
Comment 20 David King 2016-08-11 14:05:32 UTC
Review of attachment 333071 [details] [review]:

Just a cosmetic change required, everything else looks fine.

::: src/gl-eventviewlist.c
@@ +475,3 @@
+
+        case GL_SEARCH_POPOVER_JOURNAL_FIELD_FILTER_PID:
+

You should drop some of these blank lines after the case statements.
Comment 21 Pranav Ganorkar 2016-08-11 15:02:36 UTC
Created attachment 333119 [details] [review]
Add search popover with parameter filtering

The search popover can be selected from the drop down menu besides the search bar. Individual parameters can be selected from the search popover.The search popover design was copied from Nautilus.If Nautilus implementation of search popover changes,gnome-logs implementation of search popover should be kept in sync with it.
Comment 22 Pranav Ganorkar 2016-08-11 15:27:54 UTC
Created attachment 333124 [details] [review]
Add search type option to search popover

A radio button group was addded to select between "Exact" and "Substring" search types. We hide the search type radio button group if "All Available Fields" is selected as exact search doesn't make sense in that case. The GlJournalModel class was modified to handle the case when all search fields are exact and there are no substring matches. Also, it was modified to handle the case when the exact search match string has a null or empty value in which case we do not pass it to gl_journal_set_matches(). Also, a GL_QUERY_* prefix was added to the enum nicks for making it consistent with the enum name GlQuerySearchType.
Comment 23 Pranav Ganorkar 2016-08-11 16:56:31 UTC
Created attachment 333125 [details] [review]
Add range filtering to search popover

The range filtering option in search popover can used to set the timestamp range of journal.The default journal range is current boot. The query object was modified to include two new parameters for storing the start and end timestamps of the journal. The gl-journal-set-matches() was modified to remove setting the boot match if no boot match is specified.
Comment 24 Pranav Ganorkar 2016-08-14 18:36:23 UTC
Created attachment 333290 [details] [review]
Add range filtering to search popover

The range filtering option in search popover can used to set the timestamp range of journal.The default journal range is current boot. The query object was modified to include two new parameters for storing the start and end timestamps of the journal. The gl-journal-set-matches() was modified to remove setting the boot match if no boot match is specified. The search popover range button label is made consistent when user selects boot through the toolbar boot selection menu.
Comment 25 Pranav Ganorkar 2016-08-14 18:37:53 UTC
Created attachment 333291 [details] [review]
Add custom range submenu to search popover

The custom range submenu is opened through the select range treeview. If the user doesn't specify any date, then today's date is taken as default. If the user doesn't specify any time, then 11:59:59 PM is taken as default start time and 12:00:00 AM is taken default end time.The journal is filtered according to the entered start date-time and end date-time range and the logs are shown accordingly.Invalid values in spinboxes are set to nearest correct values. Future dates fail silently.If the user selects any other filter, then the options in the custom range submenu are reset. Depending upon whether the user has selected start date-time or end date-time, we update the range button label accordingly.
Comment 26 Pranav Ganorkar 2016-08-14 18:48:07 UTC
Created attachment 333292 [details] [review]
Add custom range submenu to search popover

The custom range submenu is opened through the select range treeview.If the user doesn't specify any date, then today's date is taken as default. If the user doesn't specify any time, then 11:59:59 PM is taken as default start time and 12:00:00 AM is taken default end time.The journal is filtered according to the entered start date-time and end date-time range and the logs are shown accordingly.Invalid values in spinboxes are set to nearest correct values. Future dates fail silently.If the user selects any other filter, then the options in the custom range submenu are reset. Depending upon whether the user has selected start date-time or end date-time, we update the range button label accordingly.
Comment 27 Pranav Ganorkar 2016-08-14 19:15:47 UTC
Created attachment 333293 [details] [review]
Add custom range submenu to search popover

The custom range submenu is opened through the select range treeview. If the user doesn't specify any date, then today's date is taken as default. If the user doesn't specify any time, then 11:59:59 PM is taken as default start time and 12:00:00 AM is taken default end time.The journal is filtered according to the entered start date-time and end date-time range and the logs are shown accordingly.Invalid values in spinboxes are set to nearest correct values. Future dates fail silently.If the user selects any other filter, then the options in the custom range submenu are reset. Depending upon whether the user has selected start date-time or end date-time, we update the range button label accordingly.If time zone format is 24hr, we show only the hour-minute-second spinboxes.
Comment 28 David King 2016-08-15 10:25:30 UTC
Review of attachment 333119 [details] [review]:

::: src/gl-eventviewlist.c
@@ +461,3 @@
+    {
+        case GL_SEARCH_POPOVER_JOURNAL_FIELD_FILTER_ALL_AVAILABLE_FIELDS:
+

Still many unnecessary blank lines, but this is easy to fix when pushing.
Comment 29 David King 2016-08-15 10:30:54 UTC
Review of attachment 333124 [details] [review]:

Seems fine.
Comment 30 David King 2016-08-15 10:33:14 UTC
Review of attachment 333290 [details] [review]:

::: data/gl-searchpopover.ui
@@ +447,3 @@
+            <row>
+                <col id="0" translatable="yes">Current Boot</col>
+                <col id="1">0</col>

There should be no reason to have an ID stored inside the model. The timestamp range is already stored, so the ID is useless duplication.
Comment 31 David King 2016-08-15 10:47:58 UTC
Review of attachment 333293 [details] [review]:

::: data/gl-searchpopover.ui
@@ +459,3 @@
+                                                <property name="visible">True</property>
+                                                <property name="can_focus">True</property>
+                                                <property name="placeholder-text">Eg. 22 January 2016 or 1/22/2016</property>

Do not use Latin in English UIs.

@@ +763,3 @@
                                                 <property name="visible">True</property>
                                                 <property name="can_focus">True</property>
+                                                <property name="placeholder-text">Eg. 22 January 2016 or 1/22/2016</property>

Same comment about avoiding Latin.

::: src/gl-searchpopover.c
@@ +117,3 @@
+typedef enum
+{
+    AM,

Use prefixes for the enum identifiers.

@@ +749,3 @@
+                                                     now, priv->clock_format, FALSE);
+
+        range_button_label = g_strdup_printf (_("From %s"), display_time);

Needs a translator comment.

@@ +758,3 @@
+                                                     now, priv->clock_format, FALSE);
+
+        range_button_label = g_strdup_printf (_("Until %s"), display_time);

Needs a translator comment.

@@ +791,3 @@
+    date = g_date_time_new_local (year, month + 1, day, 0, 0, 0);
+
+    date_label = g_date_time_format (date, "%e %B %Y");

Translator comment required.

@@ +929,3 @@
+    if (time_period == AM)
+    {
+        time_period_string = g_strdup_printf ("AM");

If the string is shown in the UI then it should be translatable.

@@ +1267,3 @@
+    if (priv->clock_format == GL_UTIL_CLOCK_FORMAT_12HR)
+    {
+        button_label = g_date_time_format (end_date_time, "%I:%M:%S %p");

Should be translatable if it is shown in the UI.

@@ +1271,3 @@
+    else
+    {
+        button_label = g_date_time_format (end_date_time, "%T");

Translatable string.
Comment 32 Pranav Ganorkar 2016-08-15 13:22:31 UTC
Created attachment 333337 [details] [review]
Add search popover with parameter filtering

Removed the id column from the tree model and instead iterate through the tree model to make the selection in treeview.
Comment 33 Pranav Ganorkar 2016-08-15 13:23:52 UTC
Created attachment 333338 [details] [review]
Add search type option to search popover

rebased the patch on changes in the earlier patch
Comment 34 Pranav Ganorkar 2016-08-15 13:25:11 UTC
Created attachment 333339 [details] [review]
Add range filtering to search popover

Removed the id column from the tree model and instead iterate through the tree model to make the selection in treeview.
Comment 35 David King 2016-08-17 09:24:19 UTC
Review of attachment 333337 [details] [review]:

::: src/gl-eventviewlist.c
@@ +461,3 @@
+    {
+        case GL_SEARCH_POPOVER_JOURNAL_FIELD_FILTER_ALL_AVAILABLE_FIELDS:
+

Still some odd blank lines here, which should be removed.

::: src/gl-searchpopover.c
@@ +1,3 @@
+/*
+ *  GNOME Logs - View and search logs
+ *  Copyright (C) 2013, 2014, 2015  Red Hat, Inc.

I am pretty sure that you wrote this yourself, so you should have the copyright. :-)

@@ +173,3 @@
+    eclass = g_type_class_ref (GL_TYPE_SEARCH_POPOVER_JOURNAL_FIELD_FILTER);
+
+    if (strstr ("GL_SEARCH_POPOVER_JOURNAL_FIELD_FILTER_ALL_AVAILABLE_FIELDS", journal_field_enum_name))

Rather than having this literal string, you should query it from the enum type information. Check out the GEnumValue functions in GObject to find alternative ways to do this.
Comment 36 David King 2016-08-17 09:28:54 UTC
Review of attachment 333338 [details] [review]:

Looks good.
Comment 37 David King 2016-08-17 09:30:35 UTC
Review of attachment 333339 [details] [review]:

::: src/gl-searchpopover.c
@@ +480,3 @@
+    eclass = g_type_class_ref (GL_TYPE_SEARCH_POPOVER_JOURNAL_TIMESTAMP_RANGE);
+
+    if (strstr ("GL_SEARCH_POPOVER_JOURNAL_TIMESTAMP_RANGE_CURRENT_BOOT", journal_range_enum_name))

Same comment as a previous patch about getting the enum values from the type information.
Comment 38 Pranav Ganorkar 2016-08-17 10:24:37 UTC
Created attachment 333478 [details] [review]
Add search popover with journal field filtering

* Changed the commit message

* Removed the empty lines from the case blocks

* Changed the copyright author

* Used g_enum_get_value_by_name() to get enum value instead of using string       literals in if-else statements.
Comment 39 Pranav Ganorkar 2016-08-17 10:26:33 UTC
Created attachment 333479 [details] [review]
Add search type option to search popover

* Rebased the patch with changes in earlier patch

* Changed the commit message
Comment 40 Pranav Ganorkar 2016-08-17 10:28:52 UTC
Created attachment 333480 [details] [review]
Add journal timestamp range filters

* Changed the commit message

* Used g_enum_get_value_by_name() to get enum value instead of using string       literals in if-else statements.
Comment 41 Pranav Ganorkar 2016-08-17 13:13:41 UTC
Created attachment 333493 [details] [review]
Add search popover with journal field filtering

* Changed the commit message

* Removed the empty lines from the case blocks

* Changed the copyright author

* Used g_enum_get_value_by_name() to get enum value instead of using string       literals in if-else statements.
Comment 42 Pranav Ganorkar 2016-08-17 13:15:13 UTC
Created attachment 333494 [details] [review]
Add search type option to search popover

* Rebased the patch with changes in earlier patch

* Changed the commit message

* Changed the enum order in GlQuerySearchType enum
Comment 43 Pranav Ganorkar 2016-08-17 13:23:56 UTC
Created attachment 333498 [details] [review]
Add search popover with journal field filtering

* Changed the commit message

* Removed the empty lines from the case blocks

* Changed the copyright author

* Used g_enum_get_value_by_name() to get enum type instead of using string       literals in if-else statements.
Comment 44 Pranav Ganorkar 2016-08-17 13:24:41 UTC
Created attachment 333499 [details] [review]
Add search type option to search popover

* Rebased the patch with changes in earlier patch

* Changed the commit message

* Changed the enum order in GlQuerySearchType enum
Comment 45 David King 2016-08-19 11:00:42 UTC
Review of attachment 333498 [details] [review]:

::: data/gl-searchpopover.ui
@@ +149,3 @@
+            <column type="gchararray"/>
+            <column type="gboolean"/>
+            <column type="gchararray"/>

Shouldn't this be stored as a GlSearchPopoverJournalField? You should then be able to avoid doing string comparisons to check which enum value matches up.
Comment 46 Pranav Ganorkar 2016-08-19 13:03:06 UTC
Created attachment 333623 [details] [review]
Add search popover with journal field filtering

* Changed the column type from "gchararray" to "GlSearchPopoverJournalFieldFilter" in the tree model so as to directly get the enum value without comparing strings
Comment 47 Pranav Ganorkar 2016-08-19 13:05:28 UTC
Created attachment 333624 [details] [review]
Add search type option to search popover

* Set the value of GlQuerySearchType enum directly without using GEnumClass and GEnumValue.
Comment 48 Pranav Ganorkar 2016-08-19 13:21:06 UTC
Created attachment 333625 [details] [review]
Add journal timestamp range filters

* Changed the column type from "gchararray" to "GlSearchPopoverJournalTimestampRange" in the tree model so as to directly get the enum value without comparing strings

* Set the value of GlQuerySearchType enum directly without using GEnumClass and GEnumValue.
Comment 49 David King 2016-08-19 13:46:54 UTC
Review of attachment 333623 [details] [review]:

Looks good.
Comment 50 David King 2016-08-19 13:59:30 UTC
Review of attachment 333624 [details] [review]:

Looks fine.
Comment 51 David King 2016-08-19 14:06:20 UTC
Review of attachment 333625 [details] [review]:

Commit message is wrong (function names do not have dashes).
Comment 52 Pranav Ganorkar 2016-08-19 14:34:15 UTC
Created attachment 333630 [details] [review]
Add journal timestamp range filters

* Replaced dashes with underscores in the function name in commit message
Comment 53 David King 2016-08-19 14:36:30 UTC
Review of attachment 333630 [details] [review]:

Looks fine.
Comment 54 Pranav Ganorkar 2016-08-20 16:05:31 UTC
Created attachment 333767 [details] [review]
Add custom journal timestamp range submenu

* Added translators comment to button label strings shown in search popover.

* Made the button label strings translatable.

* Changed the commit message.
Comment 55 David King 2016-08-22 06:44:08 UTC
Review of attachment 333767 [details] [review]:

Looks fine.
Comment 56 Jonathan Kang 2016-09-27 07:05:45 UTC
@David

What's the update here? I suppose these patches can be pushed to master.
Comment 57 Jonathan Kang 2016-10-20 08:56:16 UTC
Comment on attachment 333623 [details] [review]
Add search popover with journal field filtering

Attachment 333623 [details] has been pushed to master as commit b58ad1fcc1973da8869b39e28b65c751619d3036.
Comment 58 Jonathan Kang 2016-10-20 08:57:09 UTC
Comment on attachment 333624 [details] [review]
Add search type option to search popover

Pushed to master as commit 3f028d5430e06253fd66c4294a9652d02eb31a21.
Comment 59 Jonathan Kang 2016-10-20 08:57:53 UTC
Comment on attachment 333630 [details] [review]
Add journal timestamp range filters

Pushed to master as commit ec6b654fea3466707efa421a0894ffc9834e3b19.
Comment 60 Jonathan Kang 2016-10-20 08:58:26 UTC
Comment on attachment 333767 [details] [review]
Add custom journal timestamp range submenu

Pushed to master as commit 9eb24061bc6d2ed745ba2996e46e30e7b4ac3d18.
Comment 61 Jonathan Kang 2016-10-20 08:59:39 UTC
Closing this as RESOLVED FIXED.

@Pranav, David
Thanks very much for the hard work on this new feature.