717062 - Root preferences branch for simple-prefs needs to be configurable

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[blackwind]

I was pleased to see simple-prefs included in 1.4, but it has a fatal flaw -- the root preferences branch is hardcoded to "extensions.<jetpackID>.". I'd like to see an option to change this to either "extensions.<jetpackName>." or, if necessary, any arbitrary value the developer desires. As is, because my add-on uses "extensions.<jetpackName>." as its root branch, I'm forced to choose between not using simple-prefs at all, modifying simple-prefs myself every time a new version of the SDK is released, or forcing my users to reconfigure the add-on from scratch, none of which are desirable options.

Comment 1

[Erik Vold]

I think that I will try adding a "preferenceRoot" key to the package.json for this unless someone comes up with a better idea.

Comment 2

[blackwind]

Still waiting on this one. Any news?

Comment 3

[blackwind]

AMO has arbitrarily decided to stop approving SDK add-ons with modified SDK packages, so modifying simple-prefs is no longer an option. I need a resolution for this bug sooner rather than later, or I'll have to cease development permanently. Can we get an update, Erik?

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

This bug is important for devs porting old school add-ons, let's get it in the next version!

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

We need to decide on how this will be done.  Is adding a "preferenceRoot" key to the package.json good enough? It looks ugly to me..

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think the whole jetpackID is kind of annoying we should just:

1. Treat `name` as `id` and imply same naming restrictions as commonjs / node has.
2. We should rename current `name` property to a `title`.
3. Get read of stupid `id`.

I think strategy we should pursue should be following:

1. If `package.json` does not contains `title` property at `cfx <cmd>` we add
   `title` and copy `.name` value into it. Next we replace `.name` value with
   `.id` value.
2. SDK code then can use `.name` everywhere we use `.id` now.
3. If `.id` is present use that in `install.rdf` instead of `name`.

This specific case then could be addressed by changing `.name` back to what it used to be.

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #6)
> I think the whole jetpackID is kind of annoying we should just:
> 
> 1. Treat `name` as `id` and imply same naming restrictions as commonjs /
> node has.
> 2. We should rename current `name` property to a `title`.
> 3. Get read of stupid `id`.

Do you mean the add-on id?

> I think strategy we should pursue should be following:
> 
> 1. If `package.json` does not contains `title` property at `cfx <cmd>` we add
>    `title` and copy `.name` value into it. Next we replace `.name` value with
>    `.id` value.
> 2. SDK code then can use `.name` everywhere we use `.id` now.
> 3. If `.id` is present use that in `install.rdf` instead of `name`.
> 
> This specific case then could be addressed by changing `.name` back to what
> it used to be.

This sounds awfully complicated, and I don't think it'll address the concern.  For instance Scriptish uses extensions.scriptish.* for prefs and has an id of [e-mail].  I don't see how the above would allow this.

Comment 8

[blackwind]

If I'm understanding him correctly...

name = scriptish

This generates an add-on named "scriptish" with a preferences root of "extensions.scriptish." and an add-on ID of "scriptish".

title = Scriptish
name = scriptish

This generates an add-on named "Scriptish" with a preferences root of "extensions.scriptish." and an add-on ID of "scriptish".

title = Scriptish
name = scriptish
id = [e-mail]

This is used for existing add-ons with an existing ID and generates an add-on named "Scriptish" with a preferences root of "extensions.scriptish." and an add-on ID of "[e-mail]".

In short, "fullName" becomes "title", "id" is no longer required (but can be included), and "name" is used as "title" and "id" if they aren't specified.

I say we simply use "name" to generate our preferences branch (name = scriptish = "extensions.scriptish.") and leave everything else alone. Those developers with add-ons using the current simple-prefs root can simply copy their "id" value to "name", and all is once again well.

Comment 9

[blackwind]

"This bug is important for devs porting old school add-ons, let's get it in the next version!"

"Assignee: [e-mail] → [e-mail]"

Are we just ignoring this indefinitely, then?

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #6)
> > I think strategy we should pursue should be following:
> > 
> > 1. If `package.json` does not contains `title` property at `cfx <cmd>` we add
> >    `title` and copy `.name` value into it. Next we replace `.name` value with
> >    `.id` value.
> > 2. SDK code then can use `.name` everywhere we use `.id` now.
> > 3. If `.id` is present use that in `install.rdf` instead of `name`.
> > 
> > This specific case then could be addressed by changing `.name` back to what
> > it used to be.
> 
> This sounds awfully complicated, and I don't think it'll address the
> concern.  For instance Scriptish uses extensions.scriptish.* for prefs and
> has an id of [e-mail].  I don't see how the above would allow
> this.

Can I get a reply Irakli?

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Erik as blackwind replied, you'll be able to change you're add-on name to "scriptish" and happily use `extensions.scriptish.*` for prefs.

Also for new add-ons id will be obsolete it will be computed as name@jetpack.

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #11)
> Erik as blackwind replied, you'll be able to change you're add-on name to
> "scriptish" and happily use `extensions.scriptish.*` for prefs.
> 
> Also for new add-ons id will be obsolete it will be computed as name@jetpack.

I don't like this approach, the whole thing seems far to complex.  Too many conditions imo.

Comment 13

[blackwind]

If you have a better idea, Erik, please do present it for discussion. It seems like Irakli is content with his.

In terms of implementation, this may be more complex than we'd like, but ultimately, it simplifies things for add-on developers. That's what we should be striving for, right?

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Link to pull request 1149

Erik, can you look at this or pass it on to someone else?

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 785857 [details]
Link to pull request 1149

Let's get some feedback from Irakli first, I can't approve/disapprove design choices in the SDK.

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 785857 [details]
Link to pull request 1149

I would still prefer to go with a path outlined in the comments, although I don't particularity mind this approach as well. Only think that concerns me (reason I put -) is that we call `prefRoot` something that is not actually a root since it's being prefixed with `extensions.`. I think we should either allow add-ons to specify root or rename that attribute to something else maybe `preference-id` or something. I'm leaning towards later option. Also note that we use dash delimited naming in JSON rather than camelCase.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

how about 'preference-namespace' ?

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> how about 'preference-namespace' ?

Note: I'm just throwing another option out there, I don't favor one over another.

Comment 19

[Tomislav Jovanovic :zombie]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #16)
> I would still prefer to go with a path outlined in the comments, although I
> don't particularity mind this approach as well. Only think that concerns me
> (reason I put -) is that we call `prefRoot` something that is not actually a
> root since it's being prefixed with `extensions.`. I think we should either

i agree about the "root" name.  but as i understand things, SDK should guide devs to do "the right thing", so limiting the default, SDK-provided, high-level facilities (like [sdk/simple-prefs] and options.xul) to a branch under "extension." fits this purpose.  after all, every addon can still use any preference with the [sdk/preferences/service].


> allow add-ons to specify root or rename that attribute to something else
> maybe `preference-id` or something. I'm leaning towards later option. Also

how about prefsBranch?  

i think plural is more logical.  preferencesBranch seems too long, and there is precedent for shorthand with "simple-prefs". also


> note that we use dash delimited naming in JSON rather than camelCase.

first of all, i don't agree with this, as it prevents using obj.propName in both python and javascript.  and second, it's not true, according to:

https://addons.mozilla.org/en-US/developers/docs/sdk/Firefox-22/dev-guide/package-spec.html

(my first bug, so i did the homework ;)

Comment 20

[Tomislav Jovanovic :zombie]

should this setting also be exposed to addon code?  

it's accessible currently through require('@loader/options').prefsBranch, but maybe a higher level API is in order?  either as a property on [self] or [simple-prefs] module?  

both make sense, but i'm leaning towards the later..

Comment 21

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #18)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> > how about 'preference-namespace' ?
> 
> Note: I'm just throwing another option out there, I don't favor one over
> another.

I'm fine with `preference-namespace` although name is little overloaded IMO.

I'm also ok with `preferences-branch` or `prefs-branch`, but please stick to dash-delimited naming convention (no camelCase) in `package.json`. Although
`branch` only makes sense if you're aware of the preference service in firefox
and I don't really wanna be committed to it. Note that simple-prefs makes use of preference service but on other platforms it may not.

Erik: I'm not a native speaker so why don't you make decisions about what naming conventions make sense, I'm just against naming that gives wrong ideas about things like it was a case with `root`.

As of exposing this property, I don't have strong opinions but I would go against exposing things that we don't already have use cases for. So far my understanding is it may be useful, let's revisit exposure once it becomes necessary & there will be no better alternative.

Comment 22

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #21)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #18)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> > > how about 'preference-namespace' ?
> > 
> > Note: I'm just throwing another option out there, I don't favor one over
> > another.
> 
> I'm fine with `preference-namespace` although name is little overloaded IMO.
> 
> I'm also ok with `preferences-branch` or `prefs-branch`, but please stick to
> dash-delimited naming convention (no camelCase) in `package.json`. Although
> `branch` only makes sense if you're aware of the preference service in
> firefox
> and I don't really wanna be committed to it. Note that simple-prefs makes
> use of preference service but on other platforms it may not.
> 
> Erik: I'm not a native speaker so why don't you make decisions about what
> naming conventions make sense, I'm just against naming that gives wrong
> ideas about things like it was a case with `root`.


Alright let's go with `"preferences-branch"` then.

> As of exposing this property, I don't have strong opinions but I would go
> against exposing things that we don't already have use cases for. So far my
> understanding is it may be useful, let's revisit exposure once it becomes
> necessary & there will be no better alternative.

I agree, let's leave this out for now and evaluate the need first.

Comment 23

[Tomislav Jovanovic :zombie]

Attachment: Link to pull request 1156

messed up some things with my fork repo.  
new pull request is harder better faster..

Comment 24

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 788249 [details]
Link to pull request 1156

This looks great! well done, I have to r- because it needs a few more tests:

* Need a test for an addon that uses an invalid value for `"preferences-branch"`
* Need a test addon for an addon with an id like `{whatever}` (it should have been there already, which is partly my fault, but we still need it)
* Need a test for `{ preferencesBranch } = require('@loader/options');` when no `"preferences-branch"` is provided

and I mentioned a couple of nits that should be resolved.  That should be all!  once the pull is updated just send my another review request!

Comment 25

[Tomislav Jovanovic :zombie]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #24)
> * Need a test for an addon that uses an invalid value for
> `"preferences-branch"`
> * Need a test addon for an addon with an id like `{whatever}` (it should
> have been there already, which is partly my fault, but we still need it)

done. i used one test addon for both (still two separate tests), hope that's ok.


> * Need a test for `{ preferencesBranch } = require('@loader/options');` when
> no `"preferences-branch"` is provided

i already added this one to the simple-prefs test addon. did you miss it, or is that not enough (what else needs to be tested)?

https://github.com/zombie/addon-sdk/blob/6691eb/test/addons/simple-prefs/lib/main.js#L83-85


> and I mentioned a couple of nits that should be resolved. 

about `extensions.[extension-id].sdk.console.logLevel`, i reverted that change, but to document my reasoning: as an addon developer, if i set my preferences-branch, i would expect this setting to move as well.

Comment 26

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 788249 [details]
Link to pull request 1156

I mentioned a few more tests that we need, sorry I didn't mention them in my last review, I should have.

Basically I want to make sure that the preferencesBranch value, and the require('sdk/self').id values are as expected in all possible situations.

There is more information in the pull request about specific tests I would like to see.  Let me know if you have questions about this.

Comment 27

[Tomislav Jovanovic :zombie]

Comment on attachment 788249 [details]
Link to pull request 1156

all done

Comment 28

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/9cc6a9136c17e5e342bdc2aba9b6a93413884bbf
bug 717062 - preferences-branch now configurable

added preferences-branch (package.json), modified cfx (xpi builder),
packages (simple-prefs), adapted tests (cfx, packages, addons), added
new ones.  all tests pass..

https://github.com/mozilla/addon-sdk/commit/6691eb99c24bb67ad16f9423e8653866d6de4027
bug 717062 - preferences-branch, moar tests

added tests for curly braces ID and invalid preferences-branch value

https://github.com/mozilla/addon-sdk/commit/5c4b3777f944d45a4bf08eabb5a4ae470f655786
bug 717062 - preferences-branch, even more..

..tests

https://github.com/mozilla/addon-sdk/commit/79f1cec46b6585ca9fd5a6d182180abc7a9c5a69
Bug 717062 adding a test for simple prefs where a predefined id with an @ symbol is used

https://github.com/mozilla/addon-sdk/commit/0249e4b940cd5570ca20f062231f1b1ffc003be1
Bug 717062 - Root preferences branch for simple-prefs needs to be configurable r=@erikvold

Merge branch '717062'

Comment 29

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 788249 [details]
Link to pull request 1156

Thanks!

Comment 30

[Erik Vold [:erikvold] (please needinfo? me)]

I made bug 905000 to document this.

Comment 31

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/3784678ed07b074295bacd7d67ca259b043c0b8c
Revert "Bug 717062 - Root preferences branch for simple-prefs needs to be configurable r=@erikvold"

This reverts commit 0249e4b940cd5570ca20f062231f1b1ffc003be1, reversing
changes made to e050ccdb90aaf07d3d3a05331a5bdcbb0ab6f193.

Comment 32

[Erik Vold [:erikvold] (please needinfo? me)]

I had to revert this change due to bug 906682.  My bad.

We'll need a test that we avoid this regression before merging the changes again.  Doing so will be tricky with our current test framework however.. I'm not sure how best to approach this atm, I need to think about it.

Comment 33

[Erik Vold [:erikvold] (please needinfo? me)]

Not such a good first bug after all, it's damn hard ;) Anyhow it shouldn't be displayed on the list any longer.

Comment 34

[Erik Vold [:erikvold] (please needinfo? me)]

So in order to fix the problem we should use `require('@loader/options').preferencesBranch || require('sdk/self').id` for the preferences branch, and we should probably expose this as a property in require('sdk/self').

Comment 35

[Tomislav Jovanovic :zombie]

any update on this Erik?  

i'm happy to do any work needed here (tests, whatever), if you think i'm competent enough. 
please advise me on what exactly needs to get done/what i can do to help?

Comment 36

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1211

Pointer to Github pull-request

Comment 37

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tomislav Jovanovic [:zombie] from comment #35)
> any update on this Erik?  
> 
> i'm happy to do any work needed here (tests, whatever), if you think i'm
> competent enough. 
> please advise me on what exactly needs to get done/what i can do to help?

Hey, we still need a way to test these types of regressions, and we haven't been able to come up with anything yet.  The issue is bug 907343 which I've now made this bug depend on.

Comment 38

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 833147 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1211

Regression test made! \o/

Comment 39

[Erik Vold [:erikvold] (please needinfo? me)]

Mossop, I just need a review for my commits following the revert commit, which is pull request 1156 that I had alreadly r+'d

Comment 40

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/61cb9deb9a12e3b344b7d095a038710a59262ca6
Revert "Revert "Bug 717062 - Root preferences branch for simple-prefs needs to be configurable r=@erikvold""

This reverts commit 3784678ed07b074295bacd7d67ca259b043c0b8c.

https://github.com/mozilla/addon-sdk/commit/e1b232c4d1bb8511aa5582ebf4897a2b438c8a75
Bug 717062 exposing require("sdk/self").preferencesBranch

https://github.com/mozilla/addon-sdk/commit/6914e9a0e6d3a58458fe1fa88436ead3e526c127
Bug 717062 Adding regression test

https://github.com/mozilla/addon-sdk/commit/ddabd6cd90392e7db352d95717ec15eba6651aa6
Merge pull request #1211 from erikvold/717062v2

Bug 717062 Redux - Root preferences branch for simple-prefs is now configurable r=@Mossop