GNOME Bugzilla – Bug 574362
[PATCH] Regex based filtering of log message
Last modified: 2009-03-18 11:36:38 UTC
This patch add support to filter log messages based on regular expressions and give them a different foreground/background color or hide them. This patch was done for Openismus Gmbh!
Created attachment 130188 [details] [review] Patch to add filtering features
Comment on attachment 130188 [details] [review] Patch to add filtering features Hey Johannes, thank you and thanks to Openismus for the cool patch! :) >Index: logview/logview-prefs.h >=================================================================== >+void logview_prefs_remove_filter (LogviewPrefs *prefs, >+ const gchar* name); >+void logview_prefs_add_filter (LogviewPrefs *prefs, >+ LogviewFilter* filter); >+LogviewFilter * logview_prefs_get_filter (LogviewPrefs *prefs, >+ const gchar *name); >+ > #endif /* __LOG_PREFS_H__ */ >\ No newline at end of file Please add the newline at the end of the file (though it can be that it's not there right now) and use FooType *bar instead of FooType* bar for function params. >Index: logview/logview-filter.c >=================================================================== >+static void >+logview_filter_finalize (GObject *object) >+{ >+ LogviewFilterPrivate *priv = LOGVIEW_FILTER (object)->priv; >+ if (priv->tag) >+ g_object_unref (priv->tag); >+ g_regex_unref (priv->regex); >+ >+ G_OBJECT_CLASS (logview_filter_parent_class)->finalize (object); >+} You seem to leak priv->name here. >+gboolean >+logview_filter_filter (LogviewFilter *filter, const gchar *line) >+{ >+ g_return_if_fail (LOGVIEW_IS_FILTER (filter)); Should be g_return_val_if_fail (LOGVIEW_IS_FILTER (filter), FALSE); Also, please add a g_return_val_if_fail (line != NULL, FALSE); >+ >+ GMatchInfo* match_info; >+ LogviewFilterPrivate* priv = filter->priv; >+ >+ g_regex_match (priv->regex, line, 0, &match_info); >+ >+ if (g_match_info_matches (match_info)) >+ { >+ return TRUE; You leak match_info in this case. >+GtkTextTag * >+logview_filter_get_tag (LogviewFilter *filter) >+{ >+ return filter->priv->tag; >+} >\ No newline at end of file Please add g_return_val_if_fail (LOGVIEW_IS_FILTER (filter), NULL) and a newline at the end of the file. >Index: logview/logview-filter-manager.c >=================================================================== >+static void >+logview_filter_manager_update_model (LogviewFilterManager *manager) >+{ >+ gtk_list_store_clear (GTK_LIST_STORE (manager->priv->model)); >+ GList *filters = logview_prefs_get_filters (logview_prefs_get()); >+ GList *filter; >+ >+ for (filter = filters; filter != NULL; filter = g_list_next (filter)) >+ { >+ gchar *name; >+ GtkTreeIter iter; >+ >+ g_object_get (G_OBJECT(filter->data), "name", &name, NULL); You don't need this cast to G_OBJECT here. >+static void >+on_dialog_add_edit_reponse (GtkWidget *dialog, int response_id, >+ GtkWidget *dialog = gtk_message_dialog_new (GTK_WINDOW (manager), >+ GTK_DIALOG_MODAL, >+ GTK_MESSAGE_ERROR, >+ GTK_BUTTONS_CLOSE, >+ "%s", >+ "Please specify either foreground or background color!"); This string is not marked for translation. >+ g_object_get (G_OBJECT(filter), >+ "name", &name, >+ "regex", ®ex, >+ "texttag", &tag, >+ NULL); >+ g_object_get (G_OBJECT(tag), >+ "foreground-gdk", &foreground, >+ "foreground-set", &foreground_set, >+ "background-gdk", &background, >+ "background-set", &background_set, >+ "invisible", &invisible, NULL); Those G_OBJECT casts are useless. >+ g_object_set_data (G_OBJECT(manager), "old_name", name); >+ g_object_set_data (G_OBJECT(manager), "__builder", builder); >+ g_signal_connect (G_OBJECT (dialog), "response", >+ G_CALLBACK (on_dialog_add_edit_reponse), manager); >+ gtk_dialog_run (GTK_DIALOG (dialog)); >+} I don't like using g_object_set_data() here to pass the GtkBuilder object around: can't you create it in _init() for the LogviewFilterManager and save a reference to it inside the private struct? >+ >+static void >+logview_filter_manager_finalize (GObject *object) >+{ >+ G_OBJECT_CLASS (logview_filter_manager_parent_class)->finalize (object); >+} If you don't use _finalize(), you can avoid chaining to it in _class_init() and declare it here. It might become useful if you save the GtkBuilder in the private struct though. >Index: logview/logview-window.c >=================================================================== >+static void >+update_filter_menu (LogviewWindow *window) You seem to have indentation mixed up here in this function. >+ g_object_get (G_OBJECT(l->data), "name", &name, NULL); Useless cast. >Index: logview/Makefile.am >- $(BUILT_SOURCES) >+ $(BUILT_SOURCES) \ >+ logview-filter.h \ >+ logview-filter.c \ >+ logview-filter-manager.h \ >+ logview-filter-manager.c Please add these before $(BUILT_SOURCES) >+static void >+save_filters (LogviewPrefs *prefs) >+{ >+ g_slist_foreach (filters, (GFunc) g_free, NULL); >+} Missing g_slist_free(filters). >+logview_prefs_add_filter (LogviewPrefs *prefs, >+ LogviewFilter *filter) >+{ >+ gchar* name; >+ g_object_get (filter, "name", &name, NULL); >+ >+ g_hash_table_insert (prefs->priv->filters, name, filter); >+ >+ save_filters (prefs); >+ >+ /* Take a reference as we will unref the filter when the hash table is >+ * destroyed */ >+ g_object_ref (filter); >+} You can use g_hash_table_insert (filters, name, g_object_ref (filter)) which is IMHO clearer. Other general comments: - please follow the style of the rest of logview, i.e. 1TBS with 2-spaces indentation. Also, I usually declare all the variables at the start of the function, and do not assign values to them at declaration, unless it's callback data or I'm accessing the priv struct pointer. - while testing the patch I noticed that when adding a filter, the "Add Filter" dialog appears in the top left corner of the screen (it should appear over the "Manage Filters" one), and if you move it over the "Manage Filters" dialog, it goes behind it instead of being over it. I did not investigate the issue further, but I think this might happen if you call gtk_dialog_run() while you're inside another gtk_dialog_run() call. - Tied to the previous comment, I don't like too much the use of gtk_dialog_run() and, whereas that might come handy for error dialogs, you should not use it to display the filter manager dialog, nor for the Add filter dialog. Let's try to get this feature in early for 2.27 :)
Thanks for the detailed rewiew. I am sorry about the coding style issues, I tried to check everything before I created the patch but obviously missed something. Some other issue I wanted your opinion about: When choosing hide as filter option, the text is hidden but the empty lines are still visible. I would rather like to have the lines disappear completely but I couldn't figure out how to do this yet. I will try to come up with a fixed patch soon.
(In reply to comment #3) > Some other issue I wanted your opinion about: When choosing hide as filter > option, the text is hidden but the empty lines are still visible. I would > rather like to have the lines disappear completely but I couldn't figure out > how to do this yet. The best way to do this that comes into my mind is to create a structure with two GtkTextMark objects for the start and the end points of consecutive matched lines, and an array of strings for the deleted lines. Then you mark the GtkTextBuffer with the marks at the right points and you delete the lines from the buffer If you keep track of these structures for each invisible filter you can add the lines back at a later time in the same place in the buffer. IMHO you could also separate the filter logic from the GtkTextTag one, creating something like logview_filter_apply_filter(filter, text_buffer, matches_only) that does what is now done by filter_buffer() but can treat the invisible filter special case in a better way. Or maybe you could even make LogviewFilter a base class with the regexp matching logic and an _apply_filter virtual method, a LogviewTagFilter implementation, which does what is done now by LogviewFilter, and a LogviewInvisibleFilter implementation, which would remove the matched lines and keep track of them.
Hi! > The best way to do this that comes into my mind is to create a structure with > two GtkTextMark objects for the start and the end points of consecutive matched > lines, and an array of strings for the deleted lines. > Then you mark the GtkTextBuffer with the marks at the right points and you > delete the lines from the buffer > If you keep track of these structures for each invisible filter you can add the > lines back at a later time in the same place in the buffer. > IMHO you could also separate the filter logic from the GtkTextTag one, creating > something like logview_filter_apply_filter(filter, text_buffer, matches_only) > that does what is now done by filter_buffer() but can treat the invisible > filter special case in a better way. That's sounds a bit dirty to me. I was thinking about using a GtkTreeView instead of a GtkTextView (one row per line, maybe split the date into an extra column) but I don't know if you like the idea and if it's ok performance-wise.
(In reply to comment #5) > That's sounds a bit dirty to me. I was thinking about using a GtkTreeView > instead of a GtkTextView (one row per line, maybe split the date into an extra > column) but I don't know if you like the idea and if it's ok performance-wise. Yeah, it is a bit dirty, that's why I also proposed the filter subclass, that's IMO a clean solution. Don't go for the GtkTreeView way: the first version of Logview, before I rewrote it, used GtkTreeView and it was way more difficult to handle, as the GtkTreeView is not designed for displaying text buffers.
Johannes, Cosimo: A much bugger problem with GtkTreeView is, that it doesn't support copy and paste or selection/highlighting of text starting within a row. Also I'd set "paragraph-background-gdk" instead of "background-gdk". This should highlight the entire line, not just the part covered by text. Anyway: Cool patch.
> >+static void > >+logview_filter_finalize (GObject *object) > >+{ > >+ LogviewFilterPrivate *priv = LOGVIEW_FILTER (object)->priv; > >+ if (priv->tag) > >+ g_object_unref (priv->tag); > >+ g_regex_unref (priv->regex); > >+ > >+ G_OBJECT_CLASS (logview_filter_parent_class)->finalize (object); > >+} > > You seem to leak priv->name here. I think g_type_class_add_private takes care of freeing the structure in GObject's finalize but maybe I am wrong. The docs don't say anything about it... > - while testing the patch I noticed that when adding a filter, the "Add Filter" > dialog appears in the top left corner of the screen (it should appear over the > "Manage Filters" one), and if you move it over the "Manage Filters" dialog, it > goes behind it instead of being over it. I did not investigate the issue > further, but I think this might happen if you call gtk_dialog_run() while > you're inside another gtk_dialog_run() call. I fixed that by using some appropriate gtk_window_set_transient_for() calls and it works as expected now.
Created attachment 130268 [details] [review] Patch to fix the issues mentioned in comment #2 This patch should fix the indentation/style things and the little bugs noted in comment #2. It also uses paragraph-background-gdk instead of background-gdk. It still missed one piece: When new lines are added to the log file they are not filtered automatically. I will fix this in the next patch but need to analyse the code a bit to call update_filter with the correct line number to avoid refiltering the whole buffer.
(In reply to comment #8) > I think g_type_class_add_private takes care of freeing the structure in > GObject's finalize but maybe I am wrong. The docs don't say anything about > it... Yeah, it does, but I wasn't referring to |priv| here, but |priv->name| which should be free'd separately. Thanks for the updated patch, I'll review it in a short while.
Comment on attachment 130268 [details] [review] Patch to fix the issues mentioned in comment #2 Hey Johannes, thanks again for the updated patch; here's some other comments. >Index: logview/logview-filter.c >+static void >+logview_filter_finalize (GObject *object) >+{ >+ LogviewFilterPrivate *priv = LOGVIEW_FILTER (object)->priv; >+ if (priv->tag) >+ g_object_unref (priv->tag); >+ g_regex_unref (priv->regex); Missing g_free (priv->name); (and you should surround that g_object_unref with braces). >+static void >+logview_filter_set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec) >+ if (err) >+ { >+ g_regex_unref (priv->regex); >+ priv->regex = NULL; >+ } Missing g_error_free (err); >+gboolean >+logview_filter_filter (LogviewFilter *filter, const gchar *line) >+{ >+ g_return_val_if_fail (LOGVIEW_IS_FILTER (filter), FALSE); >+ g_return_val_if_fail (line != NULL, FALSE); These lines should be moved after the variable declarations. >Index: logview/logview-filter-manager.c >+static void >+run_add_edit_dialog (LogviewFilterManager *manager, LogviewFilter *filter) >+ gtk_builder_add_from_file (manager->priv->builder, UI_FILE, NULL); >+ if (error) You should pass &error to gtk_builder_add_from_file. >+ if (foreground_set) >+ { >+ gtk_color_button_set_color (GTK_COLOR_BUTTON (color_foreground), >+ foreground); >+ gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (check_foreground), >+ TRUE); >+ gdk_color_free (foreground); >+ } Are you sure that the GdkColor struct is being filled also when the relevant *-set property is TRUE? Otherwise those structs are leaked if *-set is FALSE. Also, I saw a lot of calls to logview_prefs_get(). Maybe it makes sense to call logview_prefs_get() at init and store a reference in the private struct. >Index: logview/logview-marshal.c >Index: logview/logview-marshal.h These are autogenerated at compile time and should not be added in the patch. >Index: logview/logview-window.c > static void >+filter_buffer (LogviewWindow *logview, gint start_line) >+ GdkRectangle visible_rect; Why is this needed? >+static void >+on_filter_toggled (GtkToggleAction *action, LogviewWindow *logview) >+ logview_prefs_get_filter (logview_prefs_get (), >+ LogviewFilter* filter = logview_prefs_get_filter (logview_prefs_get (), You can use priv->prefs instead of logview_prefs_get(). Other general comments: - The coding stile is still not right: I see a lot of variables declared not at the start of the function or initialized in the same line of the declaration. Also, opening braces should be in the same line of the expression. If you don't want to lose too much time on this, it's not really an issue for merging; we can fix it later in a second iteration. - I don't like using gtk_dialog_run(), so I'd like to see those being replaced before committing the patch. It's fine to use them for error dialogs though.
> > >+ if (foreground_set) > >+ { > >+ gtk_color_button_set_color (GTK_COLOR_BUTTON (color_foreground), > >+ foreground); > >+ gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (check_foreground), > >+ TRUE); > >+ gdk_color_free (foreground); > >+ } > > Are you sure that the GdkColor struct is being filled also when the relevant > *-set property is TRUE? Otherwise those structs are leaked if *-set is FALSE. > It's allocated on stack, how should it be leaked? > Other general comments: > - The coding stile is still not right: I see a lot of variables declared not at > the start of the function or initialized in the same line of the declaration. I tried to get this right now.
Created attachment 130450 [details] [review] Next iteration of the patch This patch hopefully fixes most of the style issues, eliminates gtk_dialog_run() and fixes a stupid bug with background colors. "Live"-Filtering still needs to be done.
(In reply to comment #12) Thanks for the updated patch. I'll wait for the "live" filtering update to give it a final review. > It's allocated on stack, how should it be leaked? Hmm, are you sure it is? Otherwise why do you free it when foreground_set or background_set are TRUE?
Created attachment 130578 [details] [review] Updated patch with live filtering This patch finally implements live filtering of incoming messages which was far easier than I had expected (a one liner actually). It also fixes the leaked GdkColor problem.
I committed this latest version of the patch to trunk, now that we have branched, along with some HIG fixes to the dialogs and some coding style cleanup. If you find some bugs in this new code, please file new reports. Thanks again for the patch, closing as FIXED. 2009-03-18 Cosimo Cecchi <cosimoc@gnome.org> * Makefile.am: * data/Makefile.am: * data/logview-toolbar.xml: * logview-prefs.c: (do_finalize), (load_filters), (save_filter_foreach_func), (save_filters), (get_filters_foreach), (logview_prefs_init), (logview_prefs_get_filters), (logview_prefs_remove_filter), (logview_prefs_add_filter), (logview_prefs_get_filter): * logview-prefs.h: * logview-window.c: (if): implement regex based filtering of log files. Patch by Johannes Schmid (#574362).