Closed Bug 511764 Opened 13 years ago Closed 13 years ago
Implement Settings API
The settings JEP hasn't been written yet, pinging Aza for more info.
So I guess Myk is taking this over? I haven't had a chance to start yet, so this is probably for the best.
Paul: Nick asked me to take a look at this, and I have cycles to work on it over the next few weeks. If you're still planning to do it, that's fine with me, just assign it back to yourself; otherwise I'll start working on it.
Status: NEW → ASSIGNED
I talked to Paul in person, and he's fine with me working on this bug. I've updated the JEP based on issues I raised in the discussion forum and discussions I've had with folks: https://wiki.mozilla.org/Labs/Jetpack/JEP/24 Here's a rough implementation plan with estimated time required to complete tasks (in hours): 2 get up to speed on jetpack API development 4 add settings object to jetpack.storage 4 implement base settings interface 1 implement jetpack.settings.open settings interface access point 1 implement jetpack.settings.init settings specification method 1 implement group setting type 1 implement boolean setting type 1 implement text setting type 4 implement number setting type 8 implement password setting type 2 implement set setting type 4 implement range setting type 2 implement context menu access point 2 implement install list access point 2 implement first run page access point -------------- 39 total hours Assuming I spend half my work time working on this over the next two weeks, I estimate it'll take me two weeks to do the work. Here are some unknowns that may impact the schedule: * How ready is jetpack.storage to have settings storage built on top of it? * How ready are feature context menus to accommodate an additional item? * What's the work required to use the Password Manager to store confidential settings? * For the potential first run page access point, how ready are first run pages to accommodate them? * What are we missing?
I've been working out architecture for the feature and have settled on an initial implementation with the following features: * uses jQuery UI widgets as controls where standard HTML controls are insufficient (f.e. the slider widget for the range setting type); * initial access point is via a "settings" button next to each feature in the install list; * initial implementation as a jQuery UI dialog (although ultimately this should get embedded into the install list in a way that doesn't require that the user perform window management); * UI construction via simple JS HTML templates and/or jQuery DOM manipulation. I plan to have a reviewable patch with this implementation and supporting most setting types (excepting, in particular, the password type) this week.
This patch is readyish for review. The only missing functionality is the settings.open() call, which I'm not sure how to implement while the settings dialog is part of about:jetpack. A few changes from the existing JEP: * the set setting type has been renamed to member, which seems more accurate (since the value of the preference is the member of a set, not the other way around); * the min and max properties of the range setting type have been made optional, as we can guess sensible defaults for these (0 and 100, respectively); Some remaining issues: * how to securely store passwords (use flexible membranes to catch the setting of such values and transparently store them in the password manager instead of the regular simple store?); * how to notify jetpacks that their settings have changed (flexible membranes to the rescue again?); * how to provide labels for set elements (allow elements to be objects with label properties?); * whether (and how) or not to support sets whose values are not strings;
Note: here's a jetpack that provides a manifest with setting information for testing out the patch: http://mykzilla.org/jetpack/settings-test.html
Thanks, Myk. Committed this in http://hg.mozilla.org/labs/jetpack/rev/a8f1a64fc65e and added a jetpack test for this in http://hg.mozilla.org/labs/jetpack/rev/caf2b26a822c.
Also, addressing the remaining issues in followup bugs sounds good to me.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Atul, for future reference, you should make sure you either set the author correctly when you commit or if the author has commit privileges, let him/her do it. It's good to have blame set correctly, especially as the project develops. /me gets off soapbox
(In reply to comment #8) > Remaining issues, which should perhaps be addressed in followup bugs: > > * how to notify jetpacks that their settings have changed (callback functions > that the flexible membrane calls?); I filed this as bug 526685. > * how to provide labels for set elements (allow elements to be objects with > label properties?); I'm going to let this sleeping dog lie for now. > * whether (and how) or not to support sets whose values are not strings; There's some conflation of data type and representation in the current API, since "number" is a setting type that explicitly defines its data type, while "member" is a setting type that implies a representation and only implicitly defines its data type (and that without reason). I think we should fix this, at least for numbers, although I'm not 100% sure of it. In any case, I filed this as bug 526690, which we can wontfix if we decide not to fix this. > * deleting its settings when a user uninstalls a jetpack; I filed this as bug 526688. > * jetpack.settings.open for opening the settings interface programmatically > (which needs some thought, as about:jetpack might not be open, and opening it > causes jetpacks to get reloaded, including the one that made the call and whose > state may need to be preserved across reload); I filed this as bug 526692. > I'll push the changes to the JEP shortly. Aza made one of these changes (id -> name), and I made the rest, so the JEP is now up-to-date with the current state of the code (except for the parts of the JEP which are to be implemented in followup bugs).
Comment on attachment 407004 [details] [diff] [review] patch v2: addresses various issues Canceling review request given that the patch has already been committed.
You need to log in before you can comment on or make changes to this bug.