GNOME Bugzilla – Bug 653209
Live extension enabling and disabling.
Last modified: 2011-08-17 13:36:12 UTC
This is the meat and potatoes of the system here. Most of the details were talked about in the gnome-shell-list thread, "Extension Infrastructure Work": http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html Quick overview: * main() is removed and replaced with init() * init() can return an object which is the extension's state object. - If init() doesn't return anything, then the module is the state object. - init() is guaranteed to be called at most once per Shell session. * enable() and disable() are required methods on the state object. - enable() and disable() can be called at any time. * enable() is automatically called at the start of each Shell session if the extension is enabled. If disabled, then disabled() will *NOT* be called. * Extensions can't guarantee init() being called at the start of the Shell session, nor than they guarantee it being called if it is disabled. * Extensions shouldn't change shell state in init(). Porting should be fairly straightforward. * For extensions that connect to signals in the main() and don't really use classes, connect the signals in enable() and disconnect them in disable() instead. * For extensions that usually have a one-line main() that creates a new object, return it and add enable() and disable() on the prototype.
Created attachment 190494 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html
Created attachment 190495 [details] [review] shellDBus: Add DisableExtension() and EnableExtension()
Created attachment 190825 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html Extremely simple code clean-up: s/extensionMeta[meta.uuid]./meta./ at the end of loadExtension.
Review of attachment 190495 [details] [review]: This alone is not of much use, so I think it should be squashed.
(In reply to comment #4) > Review of attachment 190495 [details] [review]: > > This alone is not of much use, so I think it should be squashed. OK. Any comments on the big patch?
Review of attachment 190825 [details] [review]: ::: js/ui/extensionSystem.js @@ +82,3 @@ + + let extensionState = extensionStateObjs[uuid]; + extensionState.disable(); This is a call into extension code. Should it be try-catch protected? @@ +92,3 @@ + return; + + if (meta.state != ExtensionState.DISABLED) Maybe you should return something, indicating that the extension wasn't enabled e.g. due to version incompatibility. At the protocol level, this would become a dbus error. @@ +230,3 @@ return; } + if (!extensionModule.init) { Should you check for enable/disable as well? @@ +243,3 @@ } + + if (!extensionState) Can you return null or false, e.g. to indicate error conditions in the extension? If so, you should check === undefined. @@ +287,3 @@ let name = info.get_name(); + // Enable all but disabled extensions if enabledExtensions is not set. + // If it is set, enable one those, except they are disabled as well. Whitespace change
Created attachment 191932 [details] [review] extensionSystem: Implement new live enable/disable system The rough sketches of the system are here: http://mail.gnome.org/archives/gnome-shell-list/2011-June/msg00283.html So, this is somewhat tricky, as I'm juggling a few things at once. This is rebased on top of a branch I have locally, so it won't apply at all. Hopefully everything will be a bit better soon. I squashed the patches. (In reply to comment #6) > Review of attachment 190825 [details] [review]: > > ::: js/ui/extensionSystem.js > @@ +82,3 @@ > + > + let extensionState = extensionStateObjs[uuid]; > + extensionState.disable(); > > This is a call into extension code. Should it be try-catch protected? Good catch. > @@ +92,3 @@ > + return; > + > + if (meta.state != ExtensionState.DISABLED) > > Maybe you should return something, indicating that the extension wasn't enabled > e.g. due to version incompatibility. At the protocol level, this would become a > dbus error. I don't understand you. > @@ +230,3 @@ > return; > } > + if (!extensionModule.init) { > > Should you check for enable/disable as well? Good catch. > @@ +243,3 @@ > } > + > + if (!extensionState) > > Can you return null or false, e.g. to indicate error conditions in the > extension? > If so, you should check === undefined. If extensions want to indicate an error condition, they can throw an error. > @@ +287,3 @@ > let name = info.get_name(); > + // Enable all but disabled extensions if enabledExtensions is not set. > + // If it is set, enable one those, except they are disabled as well. > > Whitespace change Somewhat intentional.
> @@ +92,3 @@ > + return; > + > + if (meta.state != ExtensionState.DISABLED) > > Maybe you should return something, indicating that the extension wasn't enabled > e.g. due to version incompatibility. At the protocol level, this would become a > dbus error. ACK. Don't know about the dbus error part, but please add a return value to the dbus methods to indicate if an extension was enabled successfully.
(In reply to comment #8) > > @@ +92,3 @@ > > + return; > > + > > + if (meta.state != ExtensionState.DISABLED) > > > > Maybe you should return something, indicating that the extension wasn't enabled > > e.g. due to version incompatibility. At the protocol level, this would become a > > dbus error. > > ACK. Don't know about the dbus error part, but please add a return value to the > dbus methods to indicate if an extension was enabled successfully. Do you want state == ERROR or state != ENABLED? The latter would catches things like OUT_OF_DATE, so do you want me to return false, and then you would do a GetExtensionInfo yourself to find out what the state is? Do you want me to return the new state instead?
(In reply to comment #9) > (In reply to comment #8) > > > @@ +92,3 @@ > > > + return; > > > + > > > + if (meta.state != ExtensionState.DISABLED) > > > > > > Maybe you should return something, indicating that the extension wasn't enabled > > > e.g. due to version incompatibility. At the protocol level, this would become a > > > dbus error. > > > > ACK. Don't know about the dbus error part, but please add a return value to the > > dbus methods to indicate if an extension was enabled successfully. > > Do you want state == ERROR or state != ENABLED? The latter, return state != ENABLED. > The latter would catches things like OUT_OF_DATE, so do you want me to return > false, and then you would do a GetExtensionInfo yourself to find out what the > state is? Do you want me to return the new state instead? Its a gray area, the values of the ENABLED/OUT_OF_DATE/etc constants are not in the public API anywhere, so it is probabbly safest to just return a boolean. Instead of calling GetExtensionInfo in the fail case, how about making GetExtensionErrors always return something meaningful. I cant find where the GetExtensionErrors patch lives, but how about making it return a {reason:OUT_OF_DATE, message:"Extension Out of Date", detail:""} or {reason:ERROR, message:"Extension failed to load", detail:"traceback goes here"}
(In reply to comment #10) > (In reply to comment #9) > > The latter would catches things like OUT_OF_DATE, so do you want me to return > > false, and then you would do a GetExtensionInfo yourself to find out what the > > state is? Do you want me to return the new state instead? > > Its a gray area, the values of the ENABLED/OUT_OF_DATE/etc constants are not in > the public API anywhere, so it is probabbly safest to just return a boolean. Consider them part of the public API. Even if I remove or stop using a state, I'll never remove it completely. I need these to be stable because I use them in the web UI. > Instead of calling GetExtensionInfo in the fail case, how about making > GetExtensionErrors always return something meaningful. I cant find where the > GetExtensionErrors patch lives bug #653208 > but how about making it return a > > {reason:OUT_OF_DATE, message:"Extension Out of Date", detail:""} > > or > > {reason:ERROR, message:"Extension failed to load", detail:"traceback goes > here"} Right now GetExtensionErrors returns a DBus list of error messages. I doubt I'm going to change that. There *is* also a DBus signal that you can connect to: 'extension-state-changed'. It's in my local tree, though, and not on BGO yet. I basically want to get all this figured out and reviewed before I create another tangled set of bugs.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > The latter would catches things like OUT_OF_DATE, so do you want me to return > > > false, and then you would do a GetExtensionInfo yourself to find out what the > > > state is? Do you want me to return the new state instead? > > > > Its a gray area, the values of the ENABLED/OUT_OF_DATE/etc constants are not in > > the public API anywhere, so it is probabbly safest to just return a boolean. > > Consider them part of the public API. Even if I remove or stop using a state, > I'll never remove it completely. I need these to be stable because I use them > in the web UI. Ok, lets return the new state instead; > > > Instead of calling GetExtensionInfo in the fail case, how about making > > GetExtensionErrors always return something meaningful. I cant find where the > > GetExtensionErrors patch lives > > bug #653208 > > > but how about making it return a > > > > {reason:OUT_OF_DATE, message:"Extension Out of Date", detail:""} > > > > or > > > > {reason:ERROR, message:"Extension failed to load", detail:"traceback goes > > here"} > > Right now GetExtensionErrors returns a DBus list of error messages. I doubt I'm > going to change that. There *is* also a DBus signal that you can connect to: > 'extension-state-changed'. It's in my local tree, though, and not on BGO yet. I disagree, I think the reasons for failing to load do constitute extension errors, but it is your call. Anyway, its fine if you just return the new state, I can work with that.
Sorry for creating a tangled bug mess. I'll have a more linear patch set for all the DBus hacks I need soon.