511764 - Implement Settings API

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect, P1)

Description

[Nick Nguyen [:osunick]]

The settings JEP hasn't been written yet, pinging Aza for more info.

Comment 1

[Nick Nguyen [:osunick]]

https://wiki.mozilla.org/Labs/Jetpack/JEP/24

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So I guess Myk is taking this over? I haven't had a chance to start yet, so this is probably for the best.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress: a Settings module and jQuery UI dialog widget

Comment 6

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1

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;

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses various issues

Here's an updated patch that resolves some of the issues noted earlier in the bug along with some others that came up along the way.

* it stores passwords in Firefox's password manager, using flexible membranes
  to make them available via the regular settings store;

* it automagically returns default values for settings that have default values but do not have user-set values;

* it replaces the jetpack.settings.init call with a manifest object that jetpacks can define at runtime, since a manifest is a more appropriate interface for this static information, and it reduces confusion about whether or not settings can be modified at runtime;

* it renames the 'id' property to 'name', which is more consistent with terminology elsewhere (in JavaScript code and HTML) and does not inaccurately imply uniqueness;


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?);

* 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;

* deleting its settings when a user uninstalls a jetpack;

* 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);


Note: although the patch is large, much of it is jQuery UI code.  The portion that needs to be reviewed is much smaller than the total size of the patch.

I'll push the changes to the JEP shortly.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Note: here's a jetpack that provides a manifest with setting information for testing out the patch:

http://mykzilla.org/jetpack/settings-test.html

Comment 10

[Atul Varma [:atul]]

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.

Comment 11

[Atul Varma [:atul]]

Also, addressing the remaining issues in followup bugs sounds good to me.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(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 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 407004 [details] [diff] [review]
patch v2: addresses various issues

Canceling review request given that the patch has already been committed.