GNOME Bugzilla – Bug 606363
Port to libpeas/libpeasui for plugin handling.
Last modified: 2010-06-25 20:27:45 UTC
SSIA
Created attachment 151009 [details] [review] Make libgedit.la an installable library. The reason is that if you don't do so, then the symbols which are not used in the binary get dropped by libtool when linking the static libgedit into the bonary, no matter what you do. This means that the gedit_plugin_* functions won't be available anymore once we switch to libpeas, as those are only meant to be used by plugins, and won't be used by the gedit core.
Created attachment 151010 [details] [review] Move bindings around and drop plugin loaders. This is some preliminary work for libpeas support. The plugin loaders won't be used anymore when we start to use libpeas.
Created attachment 151011 [details] [review] Port to libpeas/libpeasui for plugin handling.
Review of attachment 151009 [details] [review]: I took a first glance at this patch since it seems unrelated to the rest and I have a first round of comments: - moving to a real library sounds good. It is something that should also make our life easier on windows, but I think we need to be careful with what goes in the lib and what stays in the gedit "shell" - I do not like 2_20 hardcoded all over the place in the makefile, why do we need versioning in the first place? - I do not like making those hidden functions public, see first point about discussing what goes in the lib and what doesn't
Review of attachment 151010 [details] [review]: mmm... maybe it's the bugzilla review thingie that is not showing things correctly, but I do not see the bindings moved here ::: plugin-loaders/Makefile.am @@ -2,1 @@ why the rest of this makefile isn't gone too?
Review of attachment 151011 [details] [review]: ::: data/gedit.pc.in @@ +6,3 @@ Name: gedit Description: gedit +Requires: gtksourceview-2.0 libgpe-2.0 seems you forgot gpe here
SSIA does not really say it all :) Can you please post an overview of the changes and explain the implications on api and abi compat of plugins?
Created attachment 163528 [details] [review] Port gedit to libpeas 0.5.0 This patch uses the latest version of the libpeas API.
Created attachment 163529 [details] [review] Port the modelines plugin to libpeas
Review of attachment 163528 [details] [review]: Minor comments ::: gedit/dialogs/gedit-preferences-dialog.c @@ +1,3 @@ /* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ /* + gedit_plugins_engine_activate_plugins (gedit_plugins_engine_get_default (), and this? ::: gedit/gedit-window.c @@ +355,3 @@ window = GEDIT_WINDOW (object); + g_object_unref (window->priv->extensions); Paranoic comment: I'd move this to the dispose.
Review of attachment 163529 [details] [review]: Minor comment ::: plugins/modelines/gedit-modeline-plugin.h @@ +43,3 @@ + PeasExtensionBase parent; + + /*< private >*/ can't you add the priv struct like in other classes?
Review of attachment 163528 [details] [review]: ::: gedit/gedit-dirs.c @@ +288,2 @@ gchar * +gedit_dirs_get_gedit_plugins_data_dir (void) I know it was the case also in the old one, but why does this use "gedit" twice in the name? ::: gedit/gedit-metadata-manager.h @@ +33,2 @@ #include <glib.h> +#include <gio/gio.h> seems unrelated, if it is correct push it on its own otherwise drop it ::: gedit/gedit-plugins-engine.h @@ +35,2 @@ #include <glib.h> +#include <libpeas/peas-engine.h> I know it is not related to this patch, but any reason why you picked "libpeas", I would have expected <peas/peas-foo.h>
Created attachment 163533 [details] [review] Add a sample python plugin. This patch is not meant to be pushed into git, but is there to show python plugins do actually work!
(In reply to comment #10) > ::: gedit/gedit-window.c > @@ +355,3 @@ > window = GEDIT_WINDOW (object); > > + g_object_unref (window->priv->extensions); > > Paranoic comment: I'd move this to the dispose. I changed that. (In reply to comment #12) > Review of attachment 163528 [details] [review]: > > ::: gedit/gedit-dirs.c > @@ +288,2 @@ > gchar * > +gedit_dirs_get_gedit_plugins_data_dir (void) > > I know it was the case also in the old one, but why does this use "gedit" > twice in the name? I just created the name out of the new name for consistency, but I was tempted to rename both of these functions to get_system_plugins_dir(). I might do that as a separate patch if you wish. > ::: gedit/gedit-metadata-manager.h > @@ +33,2 @@ > #include <glib.h> > +#include <gio/gio.h> > > seems unrelated, if it is correct push it on its own otherwise drop it It causes an error because GFile doesn't exist. I pushed it separately. > ::: gedit/gedit-plugins-engine.h > @@ +35,2 @@ > #include <glib.h> > +#include <libpeas/peas-engine.h> > > I know it is not related to this patch, but any reason why you picked > "libpeas", I would have expected <peas/peas-foo.h> I just followed the implicit "convention" of other food-related libraries, like libsoup, and some others like libgnome and co. I felt like "peas" alone seemed out of place.
(In reply to comment #11) > Review of attachment 163529 [details] [review]: > > Minor comment > > ::: plugins/modelines/gedit-modeline-plugin.h > @@ +43,3 @@ > + PeasExtensionBase parent; > + > + /*< private >*/ > > can't you add the priv struct like in other classes? Because the library headers are always private API and as such I felt like it was not worth the pain to setup private struct data and so on. Do you guys want me to change that?
(In reply to comment #15) > (In reply to comment #11) > > Review of attachment 163529 [details] [review] [details]: > > > > Minor comment > > > > ::: plugins/modelines/gedit-modeline-plugin.h > > @@ +43,3 @@ > > + PeasExtensionBase parent; > > + > > + /*< private >*/ > > > > can't you add the priv struct like in other classes? > > Because the library headers are always private API and as such I felt like it > was not worth the pain to setup private struct data and so on. Do you guys want > me to change that? I would really prefer to keep the private struct for consistence.
Created attachment 163537 [details] [review] Port gedit to libpeas 0.5.0 No plugin have been updated yet.
Created attachment 163538 [details] [review] Port the modelines plugin to libpeas
Review of attachment 163537 [details] [review]: ::: gedit/gedit-window.c @@ -255,5 +268,0 @@ - /* First of all, force collection so that plugins - * really drop some of the references. - */ ... 2 more ... did you triple check this change? I recall we had to jump through many oops to get grabage collection working did you check with valgrind that objects are properly collected?
Review of attachment 163538 [details] [review]: ::: plugins/modelines/gedit-modeline-plugin.c @@ +39,3 @@ gulong tab_added_handler_id; gulong tab_removed_handler_id; +}; I am not sure I follow how does it work without "per-window" data @@ +207,3 @@ + GeditModelinePlugin *plugin = GEDIT_MODELINE_PLUGIN (activatable); + GeditWindow *window = GEDIT_WINDOW (object); + please do not mix declarations and code
Created attachment 163539 [details] [review] Port gedit to libpeas 0.5.0 No plugin have been updated yet. garbage collection change have been fixed.
(In reply to comment #20) > Review of attachment 163538 [details] [review]: > > ::: plugins/modelines/gedit-modeline-plugin.c > @@ +39,3 @@ > gulong tab_added_handler_id; > gulong tab_removed_handler_id; > +}; > > I am not sure I follow how does it work without "per-window" data In the current setup (using one PeasExtensionSet instance per window) there is actually one extension instance per window. So basically there is no need to have window data anymore.
Created attachment 163540 [details] [review] Port the modelines plugin to libpeas With last's remark from pbor taken into account.
Created attachment 164417 [details] [review] Port gedit to libpeas Updated to latest api changes.
Created attachment 164446 [details] [review] Port gedit to libpeas 0.5.0 No plugin have been updated yet.
Created attachment 164447 [details] [review] Port the modelines plugin to libpeas
Created attachment 164448 [details] [review] Introduce GeditWindowActivatable This interface is for extensions that should be activated against a window. It is placed within a new libgedit-private.so library because otherwise libtool removes unused symbols...
Created attachment 164449 [details] [review] Work around girepository ignoring app path when GI_TYPELIB_PATH is set. GIRepository has that annoying feature that it completely ignores prepend_search_path when the env var is set. So we update the env var or it won't load Gedit-3.0.typelib...
Review of attachment 164449 [details] [review]: Minor comment ::: gedit/gedit.c @@ +232,3 @@ + * around that, we just override this env var if it had been set before... + */ + gchar *new_env = g_strjoin (G_DIR_SEPARATOR_S, typelib_dir, env, NULL); I guess you here wants g_build_filename
Review of attachment 164448 [details] [review]: Minor comment: the indentation and the docs are wrong.
(In reply to comment #29) > Review of attachment 164449 [details] [review]: > I guess you here wants g_build_filename Actually I don't want to: I want path1:path2, not path1/path2 But I think what I really want to do is use G_SEARCHPATH_SEPARATOR_S...
Created attachment 164565 [details] [review] Introduce GeditWindowActivatable This interface is for extensions that should be activated against a window. It is placed within a new libgedit-private.so library because otherwise libtool removes unused symbols...
Created attachment 164566 [details] [review] Port the modelines plugin to libpeas This time it is converted directly to use the gedit-specific interface.
Created attachment 164595 [details] [review] Ensure the private Gedit typelib is always loaded. We load it ourselves, as g_irepository_prepend_search_path has no effect when GI_TYPELIB_PATH is set. This did prevent the gedit typelib to be loaded, hence gedit plugins did not work in this case (including in jhbuild).
Created attachment 164596 [details] [review] Ensure the private Gedit typelib is always loaded. We load it ourselves, as g_irepository_prepend_search_path has no effect when GI_TYPELIB_PATH is set. This did prevent the gedit typelib to be loaded, hence gedit plugins did not work in this case (including in jhbuild). Coding style fixed.
Created attachment 164597 [details] [review] Ensure the private Gedit typelib is always loaded. We load it ourselves, as g_irepository_prepend_search_path has no effect when GI_TYPELIB_PATH is set. This did prevent the gedit typelib to be loaded, hence gedit plugins did not work in this case (including in jhbuild). With coding style *really* fixed.
Created attachment 164604 [details] [review] Introduce GeditViewActivatable This interface is for extensions that should be activated against a gedit view (or the gedit document it contains)
Created attachment 164605 [details] [review] Port the modelines plugin to PeasViewActivatable PS. I think the previous "Port the modelines plugin to libpeas" should be committed as well as it shows the way to do things...
Review of attachment 164446 [details] [review]: ::: gedit/gedit-plugins-engine.c @@ +54,3 @@ { GSettings *plugin_settings; + gboolean setting_loaded_plugins; nitpick: I do not like the var name because the name does not make clear what it is for (which also prevents me from giving a better suggestion. I was also under the impression that the gsettings api was designed to avoid the necessity of flags to prevent loops, though maybe I dreamed it. If we keep it lets also use the usual :1 bitfield (not for the bits saved, but for consistency with the rest of the code) @@ +77,1 @@ { let's just use GeditPluginsEngine for the param and cast in the caller. (ditto for all the other methods where this makes sense... they are methods of GeditPluginsEngine afterall @@ +96,3 @@ + /* We won't save the plugin list if we are currently activating the + * plugins from the saved list */ + if (GEDIT_PLUGINS_ENGINE (engine)->priv->setting_loaded_plugins) this is a static function let's just pass GeditPluginsEngine as type and cast in the caller. Also we recently decided to always use {}, even for one-line if condtions @@ +125,1 @@ G_OBJECT_CLASS (gedit_plugins_engine_parent_class)->finalize (object); this finalize just chains up, so it can be omitted (gobject chains up by default) @@ +148,3 @@ return default_engine; + /* This should be moved to libpeas */ then why it is here? :-) ::: gedit/gedit-window.c @@ -266,3 @@ - gedit_plugins_engine_deactivate_plugins (gedit_plugins_engine_get_default (), - window); I do not understannd why this went away... the logic here was: garbage collect before deactivating deactivate garbage collect again which was something that came after tears and blood investigating refcounting leaks @@ +4135,3 @@ + GeditWindow *window) +{ + peas_extension_call (exten, "deactivate", window); random nitpick comment valid also for other part of the patch: there is no tax on blank lines... properly space the code before comments etc
Review of attachment 164565 [details] [review]: ::: gedit/Makefile.am @@ +6,3 @@ noinst_LTLIBRARIES = libgedit.la +lib_LTLIBRARIES = libgedit-private.la I must admit I am no too thrilled about this tinly shared object... we should just split gedit in a bigger shared object with all the meat (doc, view, etc) and a "shell" with main etc For now it's ok though
Review of attachment 164597 [details] [review]: It seems this patch should just be squashed with the one that introduces the typelib, I do not care to see the wrong way to load it in the history ::: gedit/gedit-plugins-engine.c @@ +151,3 @@ + filename = g_build_filename (lib_dir, + "girepository-1.0", + "Gedit-3.0.typelib", Does the typelib need to be versioned this way? it is only loaded by gedit itself and we do not version the .pc file either. I'd say to have Gedit.typelib, unless there is a strong convention in GI
Review of attachment 164566 [details] [review]: ::: plugins/modelines/gedit-modeline-plugin.c @@ +50,3 @@ +static void gedit_modeline_plugin_activate (GeditWindowActivatable *activatable, GeditWindow *window); +static void gedit_modeline_plugin_deactivate (GeditWindowActivatable *activatable, GeditWindow *window); +static void gedit_modeline_plugin_constructed (GObject *object); let's put constructed before class init so that we do not need prototypes @@ +86,3 @@ +static void +gedit_modeline_plugin_class_finalize (GeditModelinePluginClass *klass) +{ no need for the empty finalize. Beside even if we were to keep it it should check if parent->finalize is not null and in that case call it so that if tomorrow PeasPlugins gains a finalize, it is called @@ +248,3 @@ } +G_MODULE_EXPORT void I kind of liked that we had a macro to do this boilerplate code
Review of attachment 164605 [details] [review]: This one looks ok
Review of attachment 164604 [details] [review]: looks good
(In reply to comment #40) > +lib_LTLIBRARIES = libgedit-private.la > > I must admit I am no too thrilled about this tinly shared object... we should > just split gedit in a bigger shared object with all the meat (doc, view, etc) > and a "shell" with main etc > > For now it's ok though I agree, but we can still move stuff from the 'shell' to the 'private lib' when it's relevant and eventually end up with a plain libgedit-3.0.so some day...
(In reply to comment #41) > Does the typelib need to be versioned this way? it is only loaded by gedit > itself and we do not version the .pc file either. > > I'd say to have Gedit.typelib, unless there is a strong convention in GI Gnome-shell's have the version number in the filename as well, and I'm pretty sure girepository relies on it too, so I remained on the safe side and used the "conventional" filename.
(In reply to comment #42) > Review of attachment 164566 [details] [review]: > > ::: plugins/modelines/gedit-modeline-plugin.c > @@ +50,3 @@ > +static void gedit_modeline_plugin_activate (GeditWindowActivatable > *activatable, GeditWindow *window); > +static void gedit_modeline_plugin_deactivate (GeditWindowActivatable > *activatable, GeditWindow *window); > +static void gedit_modeline_plugin_constructed (GObject *object); > > let's put constructed before class init so that we do not need prototypes I did it this way to minimize the amount of moved code and to make the path more readable (the old constructor became constructed). Do you want me to move some code around? > @@ +86,3 @@ > +static void > +gedit_modeline_plugin_class_finalize (GeditModelinePluginClass *klass) > +{ > > no need for the empty finalize. Beside even if we were to keep it it should > check if parent->finalize is not null and in that case call it so that if > tomorrow PeasPlugins gains a finalize, it is called Actually, this is a class_finalize (to free class data for dynamic classes), and is required by G_DEFINE_DYNAMIC_TYPE, even if nobody ever uses it... > @@ +248,3 @@ > } > > +G_MODULE_EXPORT void > > I kind of liked that we had a macro to do this boilerplate code Sure. I was talking about that with nacho, and actually have a few macros like GEDIT_DEFINE_WINDOW_EXTENSION and GEDIT_DEFINE_VIEW_EXTENSION for the cases where we define a single plugin, but it gets more complicated if you want to provide more than a single extension. I removed the old gedit-like macros because they required to reimplement G_DEFINE_DYNAMIC_CLASS_EXTENDED because the recursive application of macros broke the {code} arguments passed in...
Created attachment 164640 [details] [review] Port gedit to libpeas 0.5.0 No plugin have been updated yet.
Created attachment 164643 [details] [review] Introduce GeditWindowActivatable squashed with the proper typelib loading
Created attachment 164644 [details] [review] Port the modelines plugin to libpeas with code moved around
Created attachment 164645 [details] [review] Introduce GeditViewActivatable This interface is for extensions that should be activated against a gedit view (or the gedit document it contains)
Created attachment 164646 [details] [review] Port the modelines plugin to PeasViewActivatable
Review of attachment 164643 [details] [review]: looks good just a comment (that we can also address later) ::: gedit/gedit-plugins-engine.c @@ +141,3 @@ + lib_dir = gedit_dirs_get_gedit_lib_dir (); + filename = g_build_filename (lib_dir, + "girepository-1.0", Sorry I did not catch this sooner, but I think you said gnome-shell does not use "girepository-1.0", it just puts the typelib in its own dir. I think we can try to follow the convention. We can also fix this in a separate commit if you prefer
Review of attachment 164640 [details] [review]: one tiny nitpick and we are good to go ::: gedit/gedit-plugins-engine.c @@ +89,2 @@ static void +gedit_plugins_engine_load_plugin (PeasEngine *engine, you missed this spot for using GeditPluginsEngine *engine in the prototytpe @@ +105,2 @@ static void +gedit_plugins_engine_unload_plugin (PeasEngine *engine, and this
(In reply to comment #54) > +gedit_plugins_engine_load_plugin (PeasEngine *engine, > +gedit_plugins_engine_unload_plugin (PeasEngine *engine, > > you missed this spot for using GeditPluginsEngine *engine in the prototytpe Well, both of these are virtuals, hence I can't really change the prototype
Attachment 164640 [details] pushed as dbc98da - Port gedit to libpeas 0.5.0 Attachment 164643 [details] pushed as 4fe7161 - Introduce GeditWindowActivatable Attachment 164644 [details] pushed as e1df93f - Port the modelines plugin to libpeas Attachment 164645 [details] pushed as 210ed41 - Introduce GeditViewActivatable Attachment 164646 [details] pushed as 4d16d6f - Port the modelines plugin to PeasViewActivatable Fixed in master.