GNOME Bugzilla – Bug 653208
shellDBus: Add GetErrors()
Last modified: 2011-08-17 13:36:12 UTC
This is currently used for the "Error" button you saw in the SweetTooth video. I'm unsure about this one: JS tracebacks for when the extension is loaded but fails would be extremely useful as well, but I'm not sure if it belong in GetErrors().
Created attachment 190490 [details] [review] extensionSystem: Save extension errors per-extension Extension developers may be confused about why their extensions aren't working: the LookingGlass isn't a very obvious place, or even which errors are theirs. To remedy this, save all errors per-UUID which allows them to be retrieved later.
Created attachment 190491 [details] [review] shellDBus: Add GetErrors() Allow DBus clients to get a list of errors that an extension has produced.
That name is way to generic for that imo.
Yeah, I thought about that this morning... GetExtensionErrors may be better.
Review of attachment 190491 [details] [review]: Marking needs-work based on the last two comments.
Created attachment 190634 [details] [review] shellDBus: Add GetExtensionErrors() Allow DBus clients to get a list of errors that an extension has produced. Renamed.
Created attachment 191270 [details] [review] extensionSystem: Start using OUT_OF_DATE We were defining OUT_OF_DATE as a possible state, but never using it properly. This isn't that related, but it's the best fit I have.
Review of attachment 190490 [details] [review]: Mostly fine, I think ::: js/ui/extensionSystem.js @@ +97,3 @@ function loadExtension(dir, enabled, type) { let info; let baseErrorString = 'While loading extension from "' + dir.get_parse_name() + '": '; Given that all extension related errors are logged with the uuid, can you remove this line and add the message inside logError? (Bonus point if you find a shorter string, as currently it is awfully long to read in looking glass)
Review of attachment 190634 [details] [review]: Straightforward, possibly enough to warrant a squash with previous. ::: js/ui/shellDBus.js @@ +87,3 @@ + GetExtensionErrors: function(uuid) { + return ExtensionSystem.errors[uuid] || []; No JSON here? Don't forget you can't pass structured stuff out of NPAPI, so you would need to JSON-ify it in the plugin.
(In reply to comment #9) > Review of attachment 190634 [details] [review]: > > Straightforward, possibly enough to warrant a squash with previous. > > ::: js/ui/shellDBus.js > @@ +87,3 @@ > > + GetExtensionErrors: function(uuid) { > + return ExtensionSystem.errors[uuid] || []; > > No JSON here? Don't forget you can't pass structured stuff out of NPAPI, so you > would need to JSON-ify it in the plugin. I do. See: https://github.com/magcius/sweettooth-plugin/blob/master/sweettooth-plugin.c#L390 (You should probably review my fork of the plugin, too.)
Review of attachment 191270 [details] [review]: ::: js/ui/extensionSystem.js @@ +160,1 @@ signals.emit('extension-state-changed', { uuid: uuid, Wait, where did this come from? There is no emit in the first patch (or did I review the wrong one?) @@ +227,3 @@ meta.state = ExtensionState.ERROR; + if (!versionCheck(meta['shell-version'], Config.PACKAGE_VERSION) || Why is this check moved?
(In reply to comment #11) > Review of attachment 191270 [details] [review]: > > ::: js/ui/extensionSystem.js > @@ +160,1 @@ > signals.emit('extension-state-changed', { uuid: uuid, > > Wait, where did this come from? There is no emit in the first patch (or did I > review the wrong one?) > > @@ +227,3 @@ > meta.state = ExtensionState.ERROR; > > + if (!versionCheck(meta['shell-version'], Config.PACKAGE_VERSION) || > > Why is this check moved? We need to set "meta.state = ExtensionState.OUT_OF_DATE", and meta is created above. Look closely and you'll see that there's a line inserted that sets that state.
(In reply to comment #11) > Review of attachment 191270 [details] [review]: > > ::: js/ui/extensionSystem.js > @@ +160,1 @@ > signals.emit('extension-state-changed', { uuid: uuid, > > Wait, where did this come from? There is no emit in the first patch (or did I > review the wrong one?) ... it's a complicated mess. 'signals' comes from the lookingGlass patch in bug #652614 drago01 thought it was a bit ugly, and I agree. The only other thing that I can think of is a big refactor to make a "ExtensionManager" and create an instance of that in main.js, but I'm not sure it's worth it.
Sorry for creating a tangled bug mess. I'll have a more linear patch set for all the DBus hacks I need soon.