GNOME Bugzilla – Bug 668429
Add a new tool to configure extension preferences
Last modified: 2012-02-07 21:19:26 UTC
GNOME Shell Extensions all have different mechansisms to allow users to configure their behaviour. Some require the user to manually use the gsettings command line tool or dconf-editor. Some provide the user with configuration inside the widget itself, with a modal dialog or switch items in a menu dropdown. Yet others ship a Python or gjs script that simply pops open a GTK+ window with the various extension settings. How the script gets launched is another point of difference: some provide a menu item or button inside the extension that launches it. Others provide a .desktop file so that the preferences tool shows up in the "Applications" section, ready to launch. Let's standardize on a new model that allows a compromise between wanting cleanliness and uniformity, and extensions wanting to scratch their own preferences itch. A new tool, written in JavaScript, calls a new entry point in an extension that allows an extension to provide a random GTK+ widget, which gets supplanted into the tool itself. The tool itself is a very simple wrapper around these entry points. We won't try to provide an autogenerated UI for the schema or anything like this: doing so will probably provide a subpar (or downright unusable) experience for the user, at the cost of a lot of code. Additionally, since there's now once place that extension configuration is done, we can guarantee that we can launch it from the place where extensions belong: the GNOME Shell Extensions repository site.
Created attachment 205796 [details] [review] Makefile.am: Use global substitutions This allows us to make more than one of the same replacement in a .in file
Created attachment 205797 [details] [review] Split off the extension importing stuff into a new library, 'ShellJS' This allows us to create a separate utility to import things from shell extensions that does not have the entire Shell stack built up
Created attachment 205798 [details] [review] extensionSystem: Fix an error related to extension importing If an extension fails to import, we will pass the error object to logExtensionError, which fails to pass it onto DBus as an error object is not a string. To fix, convert the error object to a string before passing it to logExtensionError.
Created attachment 205799 [details] [review] Move a lot of miscellaneous code related to extensions into a new module ExtensionUtils is a new module that has a lot of miscellaneous things related to loading extensions and the extension system put into a place that does not depend on Shell or St. Note that this will break extensions that have with multiple files by replacing the old uuid-based importer with an object directly on the meta object.
Created attachment 205800 [details] [review] Add a new tool, 'gnome-shell-extension-prefs', which can configure extensions A new tool, 'gnome-shell-extension-prefs' can load a new entry point from extensions, 'prefs.js', which has an entry point to return a GTK+ widget. This allows extensions to have their own preferences dialog, without each extension needing to ship its own Python script and .desktop file.
Created attachment 205801 [details] [review] browser-plugin: Provide a new method, "launchExtensionPrefs" This way, SweetTooth can launch the extension prefereneces tool at the click of a button.
Created attachment 205803 [details] A sample "prefs.js" file, for the alternate-tab extension.
I'm currently implementing this in gnome-tweak-tool (on my github, or more specifically that of https://github.com/luv) I automatically discover schemas installed by extensions, and auto-configure them. Perhaps extension authors might want a way to annotate that some gsettings keys are useful to tweak, and others are not. I'm pretty surprised this is something you would consider putting in gnome-shell.
Created attachment 205897 [details] [review] Add a new tool, 'gnome-shell-extension-prefs', which can configure extensions A new tool, 'gnome-shell-extension-prefs' can load a new entry point from extensions, 'prefs.js', which has an entry point to return a GTK+ widget. This allows extensions to have their own preferences dialog, without each extension needing to ship its own Python script and .desktop file. Add a .desktop file for the tool.
Review of attachment 205796 [details] [review]: Looks good
Review of attachment 205797 [details] [review]: This looks fine. (Thinking through the alternative - what would the disadvantage be of simply importing the full gnome-shell JS module if we moved this from the global object to a module-scope function? Also, if we have this, do we want to move any of the util.c functions to this library preemeptively? Or do we just wait until people need them, if that happens?)
Review of attachment 205798 [details] [review]: Looks good
Review of attachment 205799 [details] [review]: Can you give an example of what changes have to be made to an extension for this patch? ::: js/misc/extensionUtils.js @@ +106,3 @@ +var _importing = null; + +function getImporter(path) { Should be called makeImporter() or something, since it's creating a new importer, and not getting an existing importer. @@ +108,3 @@ +function getImporter(path) { + let importer; + ShellJS.add_extension_importer('imports.misc.extensionUtils', '_importing', path); Maybe you should change the function rather than hacking around what it's meant to do like this? Is that possible? @@ +109,3 @@ + let importer; + ShellJS.add_extension_importer('imports.misc.extensionUtils', '_importing', path); + [importer, _importing] = [_importing, null]; obfuscates to save a line @@ +130,3 @@ + fileEnum = dir.enumerate_children('standard::*', Gio.FileQueryInfoFlags.NONE, null); + } catch(e) { + global.logError('' + e); return here rather than backtracing immediately afterwards? ::: js/ui/extensionSystem.js @@ +266,3 @@ + meta = ExtensionUtils.loadMetadata(uuid, dir, type); + } catch(e) { + logExtensionError(uuid, e.message); presumably you mean to return? @@ +275,3 @@ + if (!enabled) { + meta.state = ExtensionState.INITIALIZED; You reversed the order of the check for !enabled and out-of-date, so that an out-of-date disbled extension is disabled rather than out-of-date. What's the impact of that?
Do you really think a GSettings wrapper in gnome-tweak-tool wouldn't be enough?
Review of attachment 205801 [details] [review]: Generally looks mostly fine, but some questions about the approach of spawning from the browser plugin. ::: browser-plugin/browser-plugin.c @@ +641,3 @@ + if (!g_spawn_async (NULL, argv, NULL, + G_SPAWN_SEARCH_PATH | + G_SPAWN_STDOUT_TO_DEV_NULL, Would it be better to proxy the whole thing over to gnome-shell via D-Bus? That would allow the launched gnome-shell-extension-prefs be the one compatible with the running shell? It seems likely in the 3.5 devel cycle the plugin is likely to be more static than the set of interfaces that the prefs can use. Also, if you use the one in firefox's PATH, or you hardcoded the one from plugin build time, you might end up with a different set of extensions visible to the prefs tool than the ones that we've exposed in other parts of the plugin API, since the set of system of extensions would depend on the install path to the plugin. @@ +643,3 @@ + G_SPAWN_STDOUT_TO_DEV_NULL, + NULL, NULL, NULL, &error)) + g_error ("Error launching gnome-shell-extension-prefs: '%s'\n", error->message); Do not like the g_error() that makes the browser go boom
(In reply to comment #8) > I'm currently implementing this in gnome-tweak-tool (on my github, or more > specifically that of https://github.com/luv) > > I automatically discover schemas installed by extensions, and auto-configure > them. Perhaps extension authors might want a way to annotate that some > gsettings keys are useful to tweak, and others are not. > > I'm pretty surprised this is something you would consider putting in > gnome-shell. We don't require gnome-tweak-tool to use the extensions web site - it's generally meant to be enough to just go there in a browser to start using shell extensions. While I think extensions (like every other piece of software) should work out of the box, and just work in general, it's undeniable that certain extensions are going to require some amount of preferences - for instance, a weather extension will require configuring location, a micro-blogging extension configuring your account. If we don't want to require each author to provide a right-click somewhere to launch their own one-off extension tool, we need to ship something with the shell. In terms of technology, there are three choices: pygtk - I don't think asking extension authors to work in two languages is reasonable, autogenerated - for something like a weather applet, the potential exists to do *much* better if you can have code in your prefs dialog, or something like this.
Review of attachment 205897 [details] [review]: Looks mostly OK to me, though I haven't tried running it to see how it looks. I assume that gapplication magic makes it single instance? What happens if someone runs it, doesn't close it, installs a new extension, and tries to launch the preferences for that? Does this need to listen to shell signals and update the extension list? ::: data/gnome-shell-extension-prefs.desktop.in.in @@ +2,3 @@ +Type=Application +_Name=GNOME Shell Extension Preferences +_Comment=Configures GNOME Shell Extensions I believe style here is "Configure GNOME Shell Extensions" @@ +10,3 @@ +Categories=GNOME;GTK;Core; +OnlyShowIn=GNOME; +NoDisplay=true If you are already doing NoDisplay, then I'd say definitely just fix things so it isn't excluded. ::: js/extensionPrefs/main.js @@ +16,3 @@ + // frame understands border, but not background + let frame = new Gtk.Frame(); + frame.add(widget); Yuck! But if Company doesn't have a classier solution, OK. @@ +97,3 @@ + let meta = this._extensionMetas[uuid]; + let prefsModule = this._getExtensionPrefsModule(meta); + let widget = prefsModule.buildPrefsWidget(meta); maybe trap exceptions for this and show an error dialog? Definitely test what happens when you try to select an extension with a broken prefs.js, since that's going to happen with reasonable frequency. @@ +163,3 @@ + meta = ExtensionUtils.loadMetadata(uuid, dir, type); + } catch(e) { + global.logError('' + e); and return, right? @@ +198,3 @@ +}); + +function init() { There's no obvious rhyme or reason between main(), init() split here. If the idea is that init() simulates the shell environment, call it something initEnvironment()
> > In terms of technology, there are three choices: pygtk - I don't think asking > extension authors to work in two languages is reasonable, autogenerated - for > something like a weather applet, the potential exists to do *much* better if > you can have code in your prefs dialog, or something like this. s/pygtk/python+gi but anyway. Im not requiring extension authors to write python, I can get the list of schemas and keys and autogenerate their preferences if they install their schema with a predictable name (the uuid of the extension I presume). If they only want to expose some gsettings for configuration then I am suggesting they ship a tweak.json something like { version:1, tweaks:[ {key_name:foo, key_description:bar, key_type:(gvariant signature) key_options:etc}, {key_name:bar ... } (if provided then key_name is the whitelist, the other parameters are optional, I can work it out if not specified) So the technology choices for extension authors are 1) do nothing and I might expose too many of their keys 2) provide a tweaks.json and I will present only those listed 3) write 1/7th of a config ui in javascript and let the shell do the prefs. As I said, I dont think this fits in the shell, considering the previous animosity toward extensions. On the other hand I am happy to see them being embraced. Anyway, let me and luv know if you are going to do this in g-s and I will think of something else to work on.
(and IMO I think the autogenerated works fine for the majority of extensions which have very few preferences. If your use-case is enabling complex extensions with custom preferences UI like the weather applet then I am especially happy and surprised that g-s is encouraging such large extensions to be, well, extensions)
Review of attachment 205799 [details] [review]: Extensions that used multiple files will need to change their importer from "ExtensionSystem.extensions['foo@bar.baz'].submodule" to "meta.importer". ::: js/misc/extensionUtils.js @@ +108,3 @@ +function getImporter(path) { + let importer; + ShellJS.add_extension_importer('imports.misc.extensionUtils', '_importing', path); I don't understand the comment.
Review of attachment 205797 [details] [review]: I'm not sure any of the shell-util functions would help us here.
Review of attachment 205801 [details] [review]: ::: browser-plugin/browser-plugin.c @@ +641,3 @@ + if (!g_spawn_async (NULL, argv, NULL, + G_SPAWN_SEARCH_PATH | + G_SPAWN_STDOUT_TO_DEV_NULL, I don't understand at all. Since the prefs tool and shell are packaged together, we really shouldn't be in a situation where both are incompatible, no?
Review of attachment 205897 [details] [review]: > I assume that gapplication magic makes it single instance? Yes. > What happens if someone runs it, doesn't close it, installs a new extension, and tries to launch the preferences for that? Does this need to listen to shell signals and update the extension list? Yes. ::: js/extensionPrefs/main.js @@ +16,3 @@ + // frame understands border, but not background + let frame = new Gtk.Frame(); + frame.add(widget); Yeah, he didn't have one.
(In reply to comment #22) > Review of attachment 205801 [details] [review]: > > ::: browser-plugin/browser-plugin.c > @@ +641,3 @@ > + if (!g_spawn_async (NULL, argv, NULL, > + G_SPAWN_SEARCH_PATH | > + G_SPAWN_STDOUT_TO_DEV_NULL, > > I don't understand at all. Since the prefs tool and shell are packaged > together, we really shouldn't be in a situation where both are incompatible, > no? Development builds. Most people who are using jhbuild aren't going to jhbuild their browser.
(In reply to comment #20) > Review of attachment 205799 [details] [review]: > > Extensions that used multiple files will need to change their importer from > "ExtensionSystem.extensions['foo@bar.baz'].submodule" to "meta.importer". > > ::: js/misc/extensionUtils.js > @@ +108,3 @@ > +function getImporter(path) { > + let importer; > + ShellJS.add_extension_importer('imports.misc.extensionUtils', > '_importing', path); > > I don't understand the comment. The function is documented as: * This function sets a property named @target_property on the object * resulting from the evaluation of @target_object_script code, which * acts as a GJS importer for directory @directory. As I read your code, you call this as using the current module to assign it to the property _importer, then you copy that to a local variable and null out _importer. A hack, no? I suppose: - The limitations of the way add_extension_importer() works are coming from gjs_define_importer() - so doing better would require a GJS change. (Note that currently __moduleName__ and __parentModule__ are defined wrong on the importer) - Even if we did change GJS it would be hard to do too much better because we can't return JS objects out through gobject-introspection or pass them in. But I really don't like code that does something hacky than just proceeds as if it was the most natural thing in the world. If it's the best we can do, then document why. (What if we at least passed the parent object into extensionUtils so we just had to do: _temporaryParent = meta; ShellJS.add_extension_importer("extensionUtils._temporaryParent", "importer", dir); or something, instead of having to move things around?)
Created attachment 206184 [details] [review] Move a lot of miscellaneous code related to extensions into a new module ExtensionUtils is a new module that has a lot of miscellaneous things related to loading extensions and the extension system put into a place that does not depend on Shell or St. Note that this will break extensions that have with multiple files by replacing the old uuid-based importer with an object directly on the meta object.
Created attachment 206185 [details] [review] Add a new tool, 'gnome-shell-extension-prefs', which can configure extensions A new tool, 'gnome-shell-extension-prefs' can load a new entry point from extensions, 'prefs.js', which has an entry point to return a GTK+ widget. This allows extensions to have their own preferences dialog, without each extension needing to ship its own Python script and .desktop file.
Created attachment 206186 [details] [review] browser-plugin: Provide new APIs for launching extension preferences Add two new APIs, "launchExtensionPrefs" and "extensionHasPrefs", so SweetTooth can detect if an extension needs configuring, and let the user launch the extension preferences tool directly from the browser.
Review of attachment 206184 [details] [review]: Looks good
Review of attachment 206185 [details] [review]: Looks good
Review of attachment 206186 [details] [review]: I do like this approach better. Am I just not finding the patch that adds the gnome-shell side of this, or did you forget to attach? ::: browser-plugin/browser-plugin.c @@ +672,3 @@ + + g_dbus_proxy_call (obj->proxy, + "LaunchExtensionPrefs", Hmm, sucks that we don't have a timestamp to pass over, so we're not going to get proper focus-stealing prevention. I don't think it's worth linking to GTK+ to be able to call gtk_get_current_event_time() - and that wouldn't work for non-GTK+-linked-browsers in any case. We can just use global.display.get_current_time_roundtrip() in gnome-shell. @@ +701,3 @@ + + res = g_dbus_proxy_call_sync (obj->proxy, + "ExtensionHasPrefs", Would it be better to merge another key into the result of GetExtensionInfo and save roundtrips? (I don't have much of an idea of the flow of the browser-side JS code)
Review of attachment 206186 [details] [review]: ::: js/ui/shellDBus.js @@ +243,3 @@ + LaunchExtensionPrefs: function(uuid) { + Util.spawn(['gnome-shell-extension-prefs', uuid]); OK, missed this part of the patch, sorry. Hmm, so this isn't going to get focus-stealing-prevented, I guess, because you aren't going through the launch APIs - but it seems better to me to use global.display.get_current_time_roundtrip() and then launch - we have a desktop file for the prefs tool, so we should be able to use the launch apis.
Created attachment 206485 [details] [review] Add a new tool, 'gnome-shell-extension-prefs', which can configure extensions A new tool, 'gnome-shell-extension-prefs' can load a new entry point from extensions, 'prefs.js', which has an entry point to return a GTK+ widget. This allows extensions to have their own preferences dialog, without each extension needing to ship its own Python script and .desktop file. Add a quick trick so we can pass an 'extension:///' UUID.
Created attachment 206486 [details] [review] browser-plugin: Provide new APIs for launching extension preferences Add two new APIs, "launchExtensionPrefs" to let SweetTooth let the user launch the extension preferences tool directly from the browser. To allow SweetTooth to check if an extension can be configured, add a new key to the 'metadata', 'hasPrefs', which is returned by the GetExtensionInfo/ ListExtensions DBus methods. > Would it be better to merge another key into the result of GetExtensionInfo and > save roundtrips? (I don't have much of an idea of the flow of the browser-side > JS code) That's an excellent idea... don't know why I didn't think of it. > Hmm, so this isn't going to get focus-stealing-prevented, I guess, because you > aren't going through the launch APIs - but it seems better to me to use > global.display.get_current_time_roundtrip() and then launch - we have a desktop > file for the prefs tool, so we should be able to use the launch apis. It always seemed to pop up on top for me. Anyway, here's a version using the full launch APIs with round-trip timestamp and all.
Created attachment 206487 [details] [review] browser-plugin: Provide new APIs for launching extension preferences Add two new APIs, "launchExtensionPrefs" to let SweetTooth let the user launch the extension preferences tool directly from the browser. To allow SweetTooth to check if an extension can be configured, add a new key to the 'metadata', 'hasPrefs', which is returned by the GetExtensionInfo/ ListExtensions DBus methods. > Would it be better to merge another key into the result of GetExtensionInfo and > save roundtrips? (I don't have much of an idea of the flow of the browser-side > JS code) What an excellent idea! Don't know why I didn't think of that... > Hmm, so this isn't going to get focus-stealing-prevented, I guess, because you > aren't going through the launch APIs - but it seems better to me to use > global.display.get_current_time_roundtrip() and then launch - we have a desktop > file for the prefs tool, so we should be able to use the launch apis. It always appeared on top for me, but here's a version using the real launch APIs.
Created attachment 206489 [details] [review] extensionUtils: Inject "metadata" into the extension Extensions don't really have a way to get their metadata object at the module scope. Most extensions that rely on properties on the meta object set up variables that are only set by the 'init' object. With the new importer changes, extensions will have to rely on the meta object for even more, meaning more meaningless boilerplate code in the init method. Let's solve this with a few clever hacks. Allow extensions to get their current metadata object with: let meta = imports.misc.extensionUtils.currentMeta; As such, extensions can now get their own metadata object before the 'init' method is called, so they can import submodules or do other things at the module scope: const MySubModule = meta.importer.mySubModule; const dataPath = GLib.build_filenamev([meta.path, 'awesome-data.json']); -- Before "Move a lot of miscellaneous code ..." lands, I'd like to see if I can get this in as well. I hope the commit message explains the problem well enough, and the implementation is a little dirty, but I think the core idea is important. If you don't like this implementation, what about adding a new method to gjs to help grab the things from the stack, or extending the gjs importer system to inject things into the module.
Review of attachment 206487 [details] [review]: Looks good
I have enjoyed our discussion on this bug.
Review of attachment 206485 [details] [review]: Looks good
Review of attachment 206489 [details] [review]: I'm not really happy about how 'meta' has become a word here - it makes an extension a bit unreadable from the beginning when you come in. I think abbreviating metadata to meta is just a mistake, so then the question is does: let metadata = imports.misc.extensionUtils.currentMetadata; const MySubModule = metadata.importer.mySubModule; read right? And it doesn't really read that well. Plus having a property that returns different results based on context is weirdly magic. We actually, have some real injection going on already - __parentModule__ is set by the GJS importer. I don't see why you can't do: const MySubModule = __parentModule__.MySubModule; Though I haven't tested it. Which is generic GJS code and reads OK. More confusingly, in terms of reading, you should be able to do: let metadata = __parentModule__.__parentModule__; const dataPath = GLib.build_filenamev([metadata.path, 'awesome-data.json']); Though the metadata line is clearly black magic. If path is the main thing we want at module scope, we should consider patching GJS so: const dataPath = GLib.build_filenamev([__parentModule__.__path__, 'awesome-data.json']); or something works. We can also just set properties on the importer and make: let metadata = __parentModule__.metadata; work, right?
(In reply to comment #40) > Review of attachment 206489 [details] [review]: > > I'm not really happy about how 'meta' has become a word here - it makes an > extension a bit unreadable from the beginning when you come in. I think > abbreviating metadata to meta is just a mistake, so then the question is does: > > let metadata = imports.misc.extensionUtils.currentMetadata; > const MySubModule = metadata.importer.mySubModule; > > read right? And it doesn't really read that well. Plus having a property that > returns different results based on context is weirdly magic. It reads fine to me. Is the problem that we're putting "importer" on the metadata? If so, we could have a new "extension helper object" which contains a "metadata" property and an "importer" property: let helper = imports.misc.extensionUtils.currentHelper; const MySubModule = helper.importer.mySubModule; let myThing = GLib.build_filenamev([helper.metadata.path, 'awesome-data.path']); (we could even put "path", "dir", "state", "error" and other things on the helper object so that the metadata object remains a direct copy of the metadata.json file with no manipulation) > We actually, have some real injection going on already - __parentModule__ is > set by the GJS importer. I don't see why you can't do: > > const MySubModule = __parentModule__.MySubModule; I'm not a fan of the name __parentModule__ -- it's *really* unclear what's going on, to me, especially in the context of extensions. We have no parent module, so it's really the importer object that you're getting back.
(In reply to comment #41) > (In reply to comment #40) > > Review of attachment 206489 [details] [review] [details]: > > > > I'm not really happy about how 'meta' has become a word here - it makes an > > extension a bit unreadable from the beginning when you come in. I think > > abbreviating metadata to meta is just a mistake, so then the question is does: > > > > let metadata = imports.misc.extensionUtils.currentMetadata; > > const MySubModule = metadata.importer.mySubModule; > > > > read right? And it doesn't really read that well. Plus having a property that > > returns different results based on context is weirdly magic. > > It reads fine to me. Is the problem that we're putting "importer" on the > metadata? If so, we could have a new "extension helper object" which contains a > "metadata" property and an "importer" property: > > let helper = imports.misc.extensionUtils.currentHelper; > const MySubModule = helper.importer.mySubModule; > > let myThing = GLib.build_filenamev([helper.metadata.path, > 'awesome-data.path']); If you called it currentExtension and made it a function call to avoid the weirdness of a context-dependent property, and then called the 'importer' property on it imports const extension = imports.misc.extensionUtils.currentExtension(); const MySubModule = extension.imports.mySubModule; Then it would read OK. The question then becomes whether using the hack with the stack is OK, or we'd rather use the mechanisms that are already built into the gjs import system, even if that makes things a little less readable (I guess the disadvantage of using extension for the local variable is that if the submodule wants to refer back to extension.js it needs that as a local variable. Could be 'extensionObject' or 'thisExtension'.) > (we could even put "path", "dir", "state", "error" and other things on the > helper object so that the metadata object remains a direct copy of the > metadata.json file with no manipulation) > > > We actually, have some real injection going on already - __parentModule__ is > > set by the GJS importer. I don't see why you can't do: > > > > const MySubModule = __parentModule__.MySubModule; > > I'm not a fan of the name __parentModule__ -- it's *really* unclear what's > going on, to me, especially in the context of extensions. We have no parent > module, so it's really the importer object that you're getting back. I think it's pretty clear that the "parent module" is the module that contains all the extension JS files right? If in ui/main.js, __parentModule__.dash means ui/dash.js, then in fooExtension@example.com/extension.js __parentModule__.helper means fooExtension@example.com/helper.js. I agree that __parentModule__.__parentModule__ to get the metadata object or the extension object is unclear, but: const extension = __parentModule__.__extension__; is not too bad for boilerplate.
Created attachment 206707 [details] [review] Add a new tool, 'gnome-shell-extension-prefs', which can configure extensions A new tool, 'gnome-shell-extension-prefs' can load a new entry point from extensions, 'prefs.js', which has an entry point to return a GTK+ widget. This allows extensions to have their own preferences dialog, without each extension needing to ship its own Python script and .desktop file. I removed the styleMe hack in favor of using a standard GtkToolBar. Thanks to Cosimo for the idea - I just didn't think about doing that.
Created attachment 206710 [details] [review] extensionUtils: Create and allow access to a new "extension" object The "extension" object is what I previously called the "helper" object. It contains the extension importer object as well as the metadata object. Things that were previously added on to the metadata (state, path, dir, etc.) are now part of this new "extension" object. With the new importer changes brought on by the extension prefs tool, extensions are left without a way to import submodules at the global scope, which would make them rely on techniques like: var MySubModule; function init(meta) { MySubModule = meta.importer.mySubModule; } That is, there's now a lot more meaningless boilerplate that nobody wants to write and nobody wants to reivew. Let's solve this with a few clever hacks. Allow extensions to get their current extension object with: let extension = imports.misc.extensionUtils.getCurrentExtension(); As such, extensions can now get their own extension object before the 'init' method is called, so they can import submodules or do other things at the module scope: const MySubModule = extension.imports.mySubModule; const dataPath = GLib.build_filenamev([extension.metadata.path, 'awesome-data.json']); OK, you and I both talked about it a little bit, and I think this is what you want. We have a new "extension" object which encompasses all the metadata and all that.
Created attachment 206711 [details] [review] Add a new tool, 'gnome-shell-extension-prefs', which can configure extensions A new tool, 'gnome-shell-extension-prefs' can load a new entry point from extensions, 'prefs.js', which has an entry point to return a GTK+ widget. This allows extensions to have their own preferences dialog, without each extension needing to ship its own Python script and .desktop file. Fix a case where an error wasn't caught and shown when it happened while importing or initting the prefs module.
Review of attachment 206711 [details] [review]: Using toolbar sounds fine
Review of attachment 206710 [details] [review]: Generally looks good, few comments. The "extension" object is what I previously called the "helper" object. The helper object never appeared in the patch sequence, right? const dataPath = GLib.build_filenamev([extension.metadata.path, That's extension.path, right? ::: js/extensionPrefs/main.js @@ +209,2 @@ try { + extension = ExtensionUtils.loadMetadata(uuid, dir, type); Naming becomes a bit weird ::: js/misc/extensionUtils.js @@ +28,3 @@ + let extensionStackLine = stack.split('\n')[1]; + if (!extensionStackLine) + return null; Maybe clearer to throw an exception? @@ +36,3 @@ + // module scope, the first field is blank: + // @/home/user/data/gnome-shell/extensions/u@u.id/prefs.js:8 + let match = new RegExp('.*?@(.+):\\d+').exec(extensionStackLine); .*? is pointless here - can just start with the @ @@ +37,3 @@ + // @/home/user/data/gnome-shell/extensions/u@u.id/prefs.js:8 + let match = new RegExp('.*?@(.+):\\d+').exec(extensionStackLine); + if (!match || !match[1]) the match only succeeded if there were 1 characters in match, so checking if it match[1] is false is pointless ::: js/ui/lookingGlass.js @@ +730,3 @@ this.actor.add(this._extensionsList); + for (let uuid in ExtensionUtils.extensionMeta) extensionMeta is gone, right? @@ +754,2 @@ _onViewSource: function (actor) { + let meta = actor._extension; meta? @@ +840,2 @@ let viewerrors = new Link.Link({ label: _("Show Errors") }); + viewerrors.actor._extension = meta; Looks wrong ::: js/ui/shellDBus.js @@ +194,3 @@ + let extension = ExtensionUtils.extensions[uuid] || {}; + let obj = {}; + Lang.copyProperties(extension, obj); I think it's ugly to serialize whatever happens to be on the extension object if it hppens to be of an appropriate datatype. Maybe do: Extension.prototype.serializedProperties = [ 'hasPrefs' ... ] or something like that? What properties are serialized ends up being part of the API to the website.
Created attachment 206994 [details] [review] browser-plugin: Provide new APIs for launching extension preferences Add two new APIs, "launchExtensionPrefs" to let SweetTooth let the user launch the extension preferences tool directly from the browser. To allow SweetTooth to check if an extension can be configured, add a new key to the 'metadata', 'hasPrefs', which is returned by the GetExtensionInfo/ ListExtensions DBus methods. This should address most of the comments here. > The helper object never appeared in the patch sequence, right? No. I talked about adding a "helper" object in the mailing list posts and bugs when I started working on SweetTooth.
Created attachment 207025 [details] [review] extensionUtils: Create and allow access to a new "extension" object The "extension" object is what I previously called the "helper" object. It contains the extension importer object as well as the metadata object. Things that were previously added on to the metadata (state, path, dir, etc.) are now part of this new "extension" object. With the new importer changes brought on by the extension prefs tool, extensions are left without a way to import submodules at the global scope, which would make them rely on techniques like: var MySubModule; function init(meta) { MySubModule = meta.importer.mySubModule; } That is, there's now a lot more meaningless boilerplate that nobody wants to write and nobody wants to reivew. Let's solve this with a few clever hacks. Allow extensions to get their current extension object with: let extension = imports.misc.extensionUtils.getCurrentExtension(); As such, extensions can now get their own extension object before the 'init' method is called, so they can import submodules or do other things at the module scope: const MySubModule = extension.imports.mySubModule; const dataPath = GLib.build_filenamev([extension.path, 'awesome-data.json']); Whoops, attached the wrong thing.
Review of attachment 207025 [details] [review]: Looks good to me
Attachment 205796 [details] pushed as d1d4142 - Makefile.am: Use global substitutions Attachment 205797 [details] pushed as b2f33e2 - Split off the extension importing stuff into a new library, 'ShellJS' Attachment 205798 [details] pushed as 2f27b94 - extensionSystem: Fix an error related to extension importing Attachment 206184 [details] pushed as 80ff6ff - Move a lot of miscellaneous code related to extensions into a new module Attachment 206487 [details] pushed as 831099c - browser-plugin: Provide new APIs for launching extension preferences Attachment 206711 [details] pushed as b8a54fa - Add a new tool, 'gnome-shell-extension-prefs', which can configure extensions Attachment 207025 [details] pushed as a622aba - extensionUtils: Create and allow access to a new "extension" object OK, then! I'm writing a blog post about the new system which should be up shortly.