GNOME Bugzilla – Bug 641774
[PATCH] Zeitgeist plugin
Last modified: 2019-03-23 20:32:42 UTC
Created attachment 180335 [details] [review] Zeitgeist plugin Hi, I'd like to propose our Zeitgeist plugin for inclusion into Gedit. The plugin sends events to Zeitgeist when a document is opened and closed, which provides Zeitgeist with much finer precision than just watching what is pushed into GtkRecentManager.
I like the idea, but I'd rather see this in the gedit-plugins package.
actually I do not understand what extra info the plugin would store... it seems bad to need a plugin for every application. What info is missing from GtkRecent? can we add it there?
> What info is missing from GtkRecent? can we add it there? Zeitgeist works best when it has information about when you started working with a document, when you finished, and it also cares moments when you saved it in between. GtkRecentManager tries to provide *some* of this, but it's based on file timestamps, and it doesn't really distinguish in many cases a simple viewing of a document vs modifying it.
can't you provide some api in GtkRecentManager for this?
No, the backend (GBookmarkFile) doesn't support such fields.
As a more general comment: 1) Could we rewrite the plugin in C (it would be nice to enable it by default I think)? 2) Can it simply soft-depend on zeitgeist? Meaning it only needs dbus and will handle transparently whether or not zeitgeist is actually installed? 3) Can we just name it zeitgeist instead of zeitgeist-dp?
1) Sure, we could port it to C. 2) Zeitgeist API is quite complex, it'd be better to link to libzeitgeist (and perhaps do what's proposed for totem, ie. a --enable-zeitgeist configure switch which enables/disables the plugin) 3) There might be more zeitgeist-* plugins in the future ;) But it's your call.
1) That would I think be good, especially since it's so simple 2) It seems you're only calling one simple thing, no? I don't know anything about Zeitgeist, but as I understood it has a nice dbus API? I'm also fine with --enable-zeitgeist (we can default to autodetection and stuff) 3) There will not be more than one officially supported zeitgeist plugin I think, so we should be quite fine
One more thing, I have the impression that you could do with subclassing ViewActivatable instead of WindowActivatable, that would save you from doing some things by yourself in the plugin.
Created attachment 180472 [details] [review] C plugin Hi, attached is the plugin ported to C.
Review of attachment 180472 [details] [review]: Let's use the GeditViewActivatable instead as jaliste proposed. ::: plugins/zeitgeist/Makefile.am @@ +19,3 @@ + +uidir = $(GEDIT_PLUGINS_DATA_DIR)/zeitgeist +ui_DATA = why this? let's remove it ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +163,3 @@ + GFile *f = gedit_document_get_location (doc); + + if (f == NULL) return; return in the next line and with {} ::: plugins/zeitgeist/zeitgeist.plugin.desktop.in @@ +1,2 @@ +[Plugin] +Module=gedit-zeitgeist let's call it just zeitgeist @@ +3,3 @@ +IAge=3 +_Name=Zeitgeist dataprovider +_Description=Logs access and leave event for documents used with GEdit the name is gedit not GEdit or gEdit @@ +4,3 @@ +_Name=Zeitgeist dataprovider +_Description=Logs access and leave event for documents used with GEdit +Icon=gnome-mime-text-x-python remove this line, it is not a python module
Created attachment 180512 [details] [review] C plugin Changed to use ViewActivatable. Fixed review comments. > let's call it just zeitgeist I would really like to avoid that, there's already system installed libzeitgeist.so, so it could cause issues.
You can call the module libzeitgeistplugin.so. Do you plan on adding other zeitgeist-related functionnality to gedit?
Review of attachment 180512 [details] [review]: Some more comments. ::: plugins/zeitgeist/Makefile.am @@ +24,3 @@ +plugin_DATA = $(plugin_in_files:.plugin.desktop.in=.plugin) + +EXTRA_DIST = $(ui_DATA) $(plugin_in_files) ui_data here @@ +28,3 @@ +CLEANFILES = $(plugin_DATA) +DISTCLEANFILES = $(plugin_DATA) + let's remove this empy line ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +149,3 @@ + ZeitgeistEvent *event; + ZeitgeistSubject *subject; + GFile *f = gedit_document_get_location (doc); let's call it location, also first the declaration and after the assignment. Also put the zeitgeist stuff before the char *. And you are leaking f @@ +155,3 @@ + return; + } + doc_uri = g_file_get_uri (f); empty line before this @@ +164,3 @@ + zeitgeist_manifestation_for_uri (doc_uri), + gedit_document_get_mime_type (doc), + g_path_get_dirname (doc_uri), you are leaking this @@ +173,3 @@ + zeitgeist_log_insert_events_no_reply (zg_log, event, NULL); + + if (display_name) g_free (display_name); just g_free (display_name) g_free already tests this @@ +195,3 @@ + priv = GEDIT_ZEITGEIST_PLUGIN (plugin)->priv; + + gchar *doc_uri = g_file_get_uri (gedit_document_get_location (doc)); leaking a lot here @@ +247,3 @@ + doc = GEDIT_DOCUMENT (gtk_text_view_get_buffer (GTK_TEXT_VIEW (priv->view))); + + priv->signals[SIGNAL_DOC_SAVED] = g_signal_connect_swapped ( I prefer the newline in the = @@ +252,3 @@ + G_CALLBACK (gedit_zeitgeist_plugin_document_saved), + activatable); + priv->signals[SIGNAL_DOC_LOADED] = g_signal_connect_swapped ( ditto @@ +271,3 @@ + doc = GEDIT_DOCUMENT (gtk_text_view_get_buffer (GTK_TEXT_VIEW (priv->view))); + + if (priv->signals[SIGNAL_DOC_SAVED]) is this check really needed?
Created attachment 180517 [details] [review] C plugin Fixed review comments, renamed to zeitgeistplugin. > Do you plan on adding other zeitgeist-related functionnality to gedit? Yes, we do have something up our sleeves :)
Review of attachment 180517 [details] [review]: See the comments. Also I'd rather see the copyright with the gpl snippet. For this enable the snippets plugin. Type gpl and press tab. ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +152,3 @@ + location = gedit_document_get_location (doc); + + if (location == NULL) let's do as location != NULL and get the uri inside @@ +176,3 @@ + display_name, + NULL); + event = zeitgeist_event_new_full (interpretation, this event looks suspicious, I say this as I don't know the zeitgeist api @@ +212,3 @@ + gchar *doc_uri = g_file_get_uri (location); + + if (doc_uri != NULL) let's check here for location != NULL and get the uri inside it. if location != NULL uri will have some value. The reason for this is to avoid checking for location != NULL in the unreffing @@ +234,3 @@ + { + zg_log = zeitgeist_log_new (); + g_object_add_weak_pointer (G_OBJECT (zg_log), (void**) &zg_log); gpointer * for the casting @@ +246,3 @@ + ZeitgeistEvent *event; + + event = zeitgeist_event_new_full (NULL, NULL, this event looks like it is leaking.... @@ +253,3 @@ + zg_dsr = zeitgeist_data_source_registry_new (); + zeitgeist_data_source_registry_register_data_source (zg_dsr, + zeitgeist_data_source_new_full ( split this. new_full in a var, also do you need to unref it? @@ +257,3 @@ + "Gedit dataprovider", + "Logs events about accessed documents", + ptr_arr), this ptr looks suspicious of mem leak @@ +268,3 @@ + g_signal_connect_swapped (doc, + "saved", + G_CALLBACK (gedit_zeitgeist_plugin_document_saved), let's rename as document_saved, document_loaded, they are private anyway @@ +289,3 @@ + doc = GEDIT_DOCUMENT (gtk_text_view_get_buffer (GTK_TEXT_VIEW (priv->view))); + + g_signal_handler_disconnect (doc, priv->signals[SIGNAL_DOC_SAVED]); can't we use a for loop here? the point of using the enum like this is for that, isn't it?
Created attachment 180519 [details] [review] C plugin More fixes based on the review. As for Zeitgeist[Subject|Event|DataSource], these are GInitiallyUnowned, so no, they don't need to be unreffed. Also the ptr_arr is consumed by the function.
Review of attachment 180519 [details] [review]: Almost ready ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +157,3 @@ + doc_uri = g_file_get_uri (location); + + if (doc_uri != NULL) no need for this check afaik also move the doc_uri declaration inside this block @@ +214,3 @@ + gchar *doc_uri = g_file_get_uri (location); + + if (doc_uri != NULL) ditto, also why do you do this? by checking for location it should be enough to send the event, even more. in send_event you are already checking the location, so just call this method @@ +271,3 @@ + + priv->signals[SIGNAL_DOC_SAVED] = + g_signal_connect_swapped (doc, let's do a normal connect, I don't see the point to do it swapped
Review of attachment 180519 [details] [review]: ::: configure.ac @@ +288,3 @@ +fi + +if test "x$enable_zeitgeist" = "xyes" ; then you could use a elif here. PKG_CHECK_EXISTS already told you the package is present.
Created attachment 180531 [details] [review] C plugin More review comments fixed. > you could use a elif here. PKG_CHECK_EXISTS already told you the package is > present. Not really, if auto is used, it sets it to yes/no.
wrong patch?
Created attachment 180532 [details] [review] C plugin Sorry, wrong file...
Review of attachment 180532 [details] [review]: Comments and same copyright comment as before for the .h file. ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +160,3 @@ + + doc_uri = g_file_get_uri (location); + let's remove this empty line @@ +207,3 @@ + location = gedit_document_get_location (doc); + + if (location != NULL) why? as said in send_event you already do this check. just do like in document_saved
Created attachment 180533 [details] [review] C plugin Right, missed those...
Review of attachment 180533 [details] [review]: More comments and the .h copyright ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +188,3 @@ +static void +document_saved (GeditDocument *doc, + gpointer object, this should be a const GError * @@ +196,3 @@ +static void +document_loaded (GeditDocument *doc, + gpointer object, const GError * @@ +199,3 @@ + GeditZeitgeistPlugin *plugin) +{ + GeditZeitgeistPluginPrivate *priv; no need for priv var.
Created attachment 180539 [details] [review] C plugin More fixes, made the plugin also implement AppActivatable which makes it easier to get and release the zeitgeist log and data_source_registry instances.
Created attachment 180541 [details] [review] C plugin ...and the copyright
Review of attachment 180541 [details] [review]: Just last minor comment ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +321,3 @@ +gedit_view_activatable_iface_init (GeditViewActivatableInterface *iface) +{ + iface->activate = gedit_zeitgeist_plugin_activate; let's call this view_activate/deactivate
Review of attachment 180541 [details] [review]: Sorry for nitpicking, but we like perfect code :) Please follow gedit coding style. This means using tabs instead of spaces, always using curly braces, putting curly braces on new lines, etc. (you can have a look at the gedit sources). Some small comments inline, other than that, great work! ::: plugins/zeitgeist/gedit-zeitgeist-plugin.c @@ +191,3 @@ + dir_name, + display_name, + NULL); Add newline for readability @@ +195,3 @@ + ZEITGEIST_ZG_USER_ACTIVITY, + "application://gedit.desktop", + subject, NULL); Add newline for readability @@ +210,3 @@ +document_saved (GeditDocument *doc, + GError *error, + GeditZeitgeistPlugin *plugin) Align arguments @@ +218,3 @@ +document_loaded (GeditDocument *doc, + GError *error, + GeditZeitgeistPlugin *plugin) Align arguments @@ +239,3 @@ + "saved", + G_CALLBACK (document_saved), + activatable); Add newline for readability
Also it is const GError *, no GError * :)
Created attachment 180558 [details] [review] C plugin Fixed remaining comments.
Created attachment 180559 [details] [review] C plugin .. and made the Errors const
Created attachment 180561 [details] [review] C plugin And a few more indents...
Created attachment 180564 [details] [review] C plugin Adapted to gedit's new indent style.
Created attachment 180565 [details] [review] C plugin Added the plugin file to POTFILES
Comment on attachment 180565 [details] [review] C plugin Go for it.