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.

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

Still waiting on this one. Any news?

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?

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

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..

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.

(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 scriptish@erikvold.com. I don't see how the above would allow this.

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 = scriptish@erikvold.com 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 "scriptish@erikvold.com". 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.

"This bug is important for devs porting old school add-ons, let's get it in the next version!" "Assignee: evold@mozilla.com → nobody@mozilla.org" Are we just ignoring this indefinitely, then?

(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 scriptish@erikvold.com. I don't see how the above would allow > this. Can I get a reply Irakli?

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.

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

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?

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

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 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.

how about 'preference-namespace' ?

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

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

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..

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

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

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

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!

(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 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 on attachment 788249 [details] Link to pull request 1156 all done

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 on attachment 788249 [details] Link to pull request 1156 Thanks!

I made bug 905000 to document this.

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.

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.

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

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').

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?

Pointer to Github pull-request

(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 on attachment 833147 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1211 Regression test made! \o/

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

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