GNOME Bugzilla – Bug 646597
port extension system to libpeas
Last modified: 2012-08-10 10:02:26 UTC
I'm working on this, I'll be pushing to peas branch: http://git.gnome.org/browse/epiphany/log/?h=peas just in case someone wants to take a look. Of course *right now* it's not yet done.
Created attachment 185486 [details] [review] Port Epiphany to libpeas This removes the old extension loaders, the extensions manager and does a minor refactor of how ephy-shell and ephy-session were hoooking into the session. THIS BREAKS EPHY EXTENSIONS. A wip 'peas' branch is available in git.gnome.org. It's really simple to migrate. Bug #646597
Review of attachment 185486 [details] [review]: ::: src/ephy-extensions-manager.c @@ +70,2 @@ +#if 0 + /* This should be moved to libpeas */ I'd just remove this lines. ::: src/ephy-window.c @@ +3550,3 @@ + peas_extension_call (exten, "activate"); + peas_extension_call (exten, "attach_window", window); + peas_extension_call (exten, "attach_tab", window->priv->active_embed); This is compatibility sugar. I think we should keep it. @@ +3560,3 @@ +{ + peas_extension_call (exten, "detach_window", window); + peas_extension_call (exten, "deactivate"); Same as previous one.
Some comments: You need this if you want to make plugins to be configurables and to use peas activatable. +#if 0 + /* This should be moved to libpeas */ + if (!g_irepository_require (g_irepository_get_default (), + "Peas", "1.0", 0, &error)) { - ephy_extension_attach_tab (EPHY_EXTENSION (l->data), - window, embed); + g_warning ("Could not load Peas repository: %s", error->message); + g_error_free (error); + error = NULL; } -} -static void -impl_detach_tab (EphyExtension *extension, - EphyWindow *window, - EphyEmbed *embed) -{ - EphyExtensionsManager *manager = EPHY_EXTENSIONS_MANAGER (extension); - GList *l; - - LOG ("Detach window %p embed %p", window, embed); - - g_object_ref (window); - g_object_ref (embed); - - for (l = manager->priv->extensions; l; l = l->next) + if (!g_irepository_require (g_irepository_get_default (), + "PeasGtk", "1.0", 0, &error)) { - ephy_extension_detach_tab (EPHY_EXTENSION (l->data), - window, embed); + g_warning ("Could not load PeasGtk repository: %s", error->message); + g_error_free (error); + error = NULL; } +#endif
+PEAS_REQUIRED=0.7.0 You need at least 0.7.3, although I would just use 1.0.0
(In reply to comment #3) > Some comments: > > You need this if you want to make plugins to be configurables and to use peas > activatable. > Care to explain how the activatable stuff works? I see gedit has a lot of boilerplate for <widget>Activatable classes all around.
Well in our case we have 3 activatables. GeditAppActivatable: you must implement this extension point if you want to share data against the whole instance. GeditWindowActivtable: one extension point is created per window GeditViewActivatable: one extension point is created per view, this was added to avoid the plugin writer having to connect to page-added, page-removed to access the view etc. Then we have the configurable stuff from Peas. You implement that extension point if you want a configure widget for the plugin. I.e iterate over the gedit plugins and you will see that some plugins have a configure widget.
Created attachment 190561 [details] [review] Almost same as before. Now attaching also an ephy-extension port patch as reference. This works ok, I've been testing it today and hasn't exploded yet. We need to decide where to put the extensions enable/disable treeview (preferences like gedit/eog?). -- Port Epiphany to libpeas This removes the old extension loaders, the extensions manager and does a minor refactor of how ephy-shell and ephy-session were hoooking into the session. Bug #646597
Created attachment 190562 [details] [review] all: port to libpeas Update a lot of things to work with peas enabled Epiphany: - remove the now uneeded extensions-manager-ui - update m4 and other Makefile macros - rename .ephy-extension files to .plugin - rebase on Peas something - fix some trivial old API usage while updating Notice that non default extensions were ignored. Bug #646597
Hey, see http://git.gnome.org/browse/gedit/commit/?id=51db205e09f18dbf743d40596e33217538de43ec in case you didn't update it for the last deprecated code
"What could possibly go wrong" #2 of the month. Thanks, I'll check that.
btw, you don't bump the IAge version?
If we're going to break extension compatibility slightly with the libpeas port, does anybody think that, whilst we're at it, we should move the user-extension dir? It could be a separate but small patch. Currently user extensions are essentially stored under <~/.gnome2/epiphany/extensions>---though the <~/.gnome2> bit can vary if you force a different profile directory. This directory name isn't very, um, GNOME 3 like. Nor is it consistent with other libpeas apps. Gedit stores its user plugins directly in <~/.local/share/totem/plugins/>, where <~/.local/share/> comes from g_get_user_data_dir (). AFAIK Totem stores its user plugins in plugin-specific subdirectories of <~/.local/share/totem/plugins> Should Epiphany do something similar? If yes, there's then the question of whether you're consistent with the libpeas plugin vs extension terminology, or whether you keep the extensions terminology because Epiphany Extensions sounds better than Epiphany Plugins ;).
Created attachment 206148 [details] [review] peas: port to libpeas considerations: - new need e-extensions - drops seed support (can be reenabled with one API call) - drops seed support because seed sucks - removes dependency on seed - removes the extension manager, loaders - converts e-session and e-lockdown into GObjects, not extensions - adds Extensions to the supermenu - kills code, a lot - adds the obvious libpeas dependency - reindents ephy-extension == A first serious iteration of this patch. The e-extensions patch is mostly unchanged. My biggest problem with this is that peas has a really ugly extension manager dialog. Not that ours was better, but I just hate this one. Other than that, I think there are no big problems. Lots of deleted code: 28 files changed, 262 insertions(+), 2285 deletions(-)
about the manager, we were having a look at it with lapo in the gedit hackfest and we changed it a bit. If you don't like it please feel free to provide patches or ideas
Review of attachment 206148 [details] [review]: Patch looks good to me, a couple of inline comments.Also I'd rename extensions-manager to extensions-engine. As a future improvement might be a good idea to have several extension points. ::: src/ephy-extension.c @@ +28,2 @@ GType ephy_extension_get_type (void) Use G_DEFINE_INTERFACE? ::: src/ephy-window.c @@ +2722,3 @@ G_CALLBACK (embed_modal_alert_cb), window, G_CONNECT_AFTER); + data = g_new0 (ExtensionCallbackInfo, 1); g_new should be enough here
*** Bug 668310 has been marked as a duplicate of this bug. ***
So after talking with people and thinking about this for some time, my current thinking is: - Our current extension model is broken. It's difficult to maintain, exposes way too many implementation details to extensions, is insecure, and makes epiphany more unstable when extensions are loaded. On top of that, it's not very popular, very few extensions have been written. - We should move to something like Mozilla's JetPack or Chrome extensions, were: * Extensions are written in JS/HTML5/CSS * They run in a different process * They can access an extremely limited amount of platform APIs Whether or not we want to be compatible with those remains to be seen, but I think the model in general is the right one. Hopefully once it's done we'll see more and better extensions available for Epiphany. Given this, I think there's no point in moving to libpeas since the fate of this code is to be removed anyway. We should just keep it as it is until we have a replacement and minimize friction and needless changes. I think Diego agrees with this, and so do Claudio and Carlos, so I think there's consensus enough among the people writing most of the code these days.
Comment on attachment 206148 [details] [review] peas: port to libpeas Rejected per comment #18.
Comment on attachment 190562 [details] [review] all: port to libpeas Rejected per comment #18.
Closing.