Fix #7442: make API consistent between PrefMan and Prefixed PrefMan.

The slightly higher level API provided by PreferencesManager (over the
PreferencesBase.PreferencesSystem) worked nicely but was inconsistent
with the API an extension author gets when they call
`PreferencesManager.getExtensionPrefs`. This change pushes that extra
behavior down into PreferencesBase.PreferencesSystem so that the
APIs are consistent.

This is a non-breaking change that basically adds to the API:

* doNotSave flag for set: calls to `set()` save automatically unless this flag is set. If extension code is already calling `save()`, that means that there will be an extra call to `save()`, but that should have minimal performance impact because `save()` does no saving when the scopes aren't dirty
* support for string contexts: extensions would have to use complete object contexts previously. This adds support for the simpler string contexts and doesn't change the behavior for object contexts.

Author: dangoor

src/preferences/PreferencesBase.js

@@ -927,10 +927,12 @@ define(function (require, exports, module) {
          * @param {string} id Identifier of the preference to set
          * @param {Object} value New value for the preference
          * @param {{location: ?Object, context: ?Object}=} options Specific location in which to set the value or the context to use when setting the value
-         * @return {boolean} true if a value was set
+         * @param {boolean=} doNotSave True if the preference change should not be saved automatically.
+         * @return {valid:  {boolean}, true if no validator specified or if value is valid
+         *          stored: {boolean}} true if a value was stored
          */
-        set: function (id, value, options) {
-            return this.base.set(this.prefix + id, value, options);
+        set: function (id, value, options, doNotSave) {
+            return this.base.set(this.prefix + id, value, options, doNotSave);
         },
 
         /**
@@ -1035,8 +1037,15 @@ define(function (require, exports, module) {
      * 
      * It also provides the ability to register preferences, which gives a fine-grained
      * means for listening for changes and will ultimately allow for automatic UI generation.
+     * 
+     * The contextNormalizer is used to customize get/set contexts based on the needs of individual
+     * context systems. It can be passed in at construction time or set later.
+     * 
+     * @param {function=} contextNormalizer function that is passed the context used for get or set to adjust for specific PreferencesSystem behavior
      */
-    function PreferencesSystem() {
+    function PreferencesSystem(contextNormalizer) {
+        this.contextNormalizer = contextNormalizer;
+        
         this._knownPrefs = {};
         this._scopes = {
             "default": new Scope(new MemoryStorage())
@@ -1339,8 +1348,13 @@ define(function (require, exports, module) {
          * @return {{scopeOrder: string, filename: ?string}} context object
          */
         _getContext: function (context) {
-            context = context || this._defaultContext;
-            return context;
+            if (context) {
+                if (this.contextNormalizer) {
+                    context = this.contextNormalizer(context);
+                }
+                return context;
+            }
+            return this._defaultContext;
         },
 
         /**
@@ -1485,10 +1499,11 @@ define(function (require, exports, module) {
          * @param {string} id Identifier of the preference to set
          * @param {Object} value New value for the preference
          * @param {{location: ?Object, context: ?Object}=} options Specific location in which to set the value or the context to use when setting the value
+         * @param {boolean=} doNotSave True if the preference change should not be saved automatically.
          * @return {valid:  {boolean}, true if no validator specified or if value is valid
          *          stored: {boolean}} true if a value was stored
          */
-        set: function (id, value, options) {
+        set: function (id, value, options, doNotSave) {
             options = options || {};
             var context = this._getContext(options.context),
 
@@ -1525,6 +1540,9 @@ define(function (require, exports, module) {
 
             var wasSet = scope.set(id, value, context, location);
             if (wasSet) {
+                if (!doNotSave) {
+                    this.save();
+                }
                 this._triggerChange({
                     ids: [id]
                 });

src/preferences/PreferencesManager.js

@@ -446,6 +446,8 @@ define(function (require, exports, module) {
         return context;
     }
 
+    PreferencesImpl.manager.contextNormalizer = _normalizeContext;
+    
     /**
      * @private
      * 
@@ -463,50 +465,6 @@ define(function (require, exports, module) {
 
     PreferencesImpl.manager.on("scopeOrderChange", _updateCurrentProjectContext);
 
-    /**
-     * Look up a preference in the given context. The default is 
-     * CURRENT_FILE (preferences as they would be applied to the
-     * currently edited file).
-     * 
-     * @param {string} id Preference ID to retrieve the value of
-     * @param {Object|string=} context CURRENT_FILE, CURRENT_PROJECT or a filename
-     */
-    function get(id, context) {
-        context = _normalizeContext(context);
-        return PreferencesImpl.manager.get(id, context);
-    }
-    
-    /**
-     * Sets a preference and notifies listeners that there may
-     * have been a change. By default, the preference is set in the same location in which
-     * it was defined except for the "default" scope. If the current value of the preference
-     * comes from the "default" scope, the new value will be set at the level just above
-     * default.
-     * 
-     * The preferences are saved automatically unless doNotSave is true.
-     * 
-     * As with the `get()` function, the context can be a filename,
-     * CURRENT_FILE, CURRENT_PROJECT or a full context object as supported by
-     * PreferencesSystem.
-     * 
-     * @param {string} id Identifier of the preference to set
-     * @param {Object} value New value for the preference
-     * @param {{location: ?Object, context: ?Object|string}=} options Specific location in which to set the value or the context to use when setting the value
-     * @param {boolean=} doNotSave True if the preference change should not be saved automatically.
-     * @return {valid:  {boolean}, true if no validator specified or if value is valid
-     *          stored: {boolean}} true if a value was stored
-     */
-    function set(id, value, options, doNotSave) {
-        if (options && options.context) {
-            options.context = _normalizeContext(options.context);
-        }
-        var wasSet = PreferencesImpl.manager.set(id, value, options);
-        if (!doNotSave) {
-            PreferencesImpl.manager.save();
-        }
-        return wasSet;
-    }
-
     /**
      * @private
      */
@@ -541,7 +499,7 @@ define(function (require, exports, module) {
      */
     function setValueAndSave(id, value, options) {
         DeprecationWarning.deprecationWarning("setValueAndSave called for " + id + ". Use set instead.");
-        var changed = set(id, value, options).stored;
+        var changed = exports.set(id, value, options).stored;
         PreferencesImpl.manager.save();
         return changed;
     }
@@ -594,8 +552,8 @@ define(function (require, exports, module) {
 
     exports.ready               = PreferencesImpl.managerReady;
     exports.getUserPrefFile     = getUserPrefFile;
-    exports.get                 = get;
-    exports.set                 = set;
+    exports.get                 = PreferencesImpl.manager.get.bind(PreferencesImpl.manager);
+    exports.set                 = PreferencesImpl.manager.set.bind(PreferencesImpl.manager);
     exports.save                = PreferencesImpl.manager.save.bind(PreferencesImpl.manager);
     exports.on                  = PreferencesImpl.manager.on.bind(PreferencesImpl.manager);
     exports.off                 = PreferencesImpl.manager.off.bind(PreferencesImpl.manager);

test/spec/PreferencesBase-test.js

@@ -281,16 +281,18 @@ define(function (require, exports, module) {
                 expect(pm.set("foo", foo).stored).toBe(true);
 
                 expect(pm.get("foo").value).toBe(42);
-                expect(scope._dirty).toBe(true);
+                expect(scope._dirty).toBe(false);
 
                 // Explicitly save it in order to clear dirty flag.
                 pm.save();
                 expect(scope._dirty).toBe(false);
 
                 foo.value = "!!!";
                 expect(foo.value).toBe("!!!");
-                expect(pm.set("foo", foo).stored).toBe(true);
+                expect(pm.set("foo", foo, undefined, true).stored).toBe(true);
                 expect(scope._dirty).toBe(true);
+                pm.save();
+                expect(scope._dirty).toBe(false);
 
                 var fooCopyFromPref = pm.get("foo");
                 expect(fooCopyFromPref.value).toBe("!!!");
@@ -1013,7 +1015,8 @@ define(function (require, exports, module) {
 
             it("can provide an automatically prefixed version of itself", function () {
                 var pm = new PreferencesBase.PreferencesSystem();
-                pm.addScope("user", new PreferencesBase.MemoryStorage());
+                var scope = new PreferencesBase.Scope(new PreferencesBase.MemoryStorage());
+                pm.addScope("user", scope);
                 pm.set("spaceUnits", 10);
                 pm.set("linting.enabled", true);
 
@@ -1032,13 +1035,17 @@ define(function (require, exports, module) {
                     scope: "user"
                 });
 
-                prefixedPM.set("collapsed", false);
+                // set the value with doNotSave set. Will check to verify that save did not occur.
+                prefixedPM.set("collapsed", false, undefined, true);
 
                 expect(events).toEqual([{
                     ids: ["collapsed"]
                 }]);
 
                 expect(prefixedPM.get("collapsed")).toBe(false);
+                
+                // verify that the scope was not saved automatically
+                expect(scope._dirty).toBe(true);
                 expect(pm.get("collapsed")).toBeUndefined();
                 expect(pm.get("linting.collapsed")).toBe(false);
 
@@ -1080,6 +1087,46 @@ define(function (require, exports, module) {
                 expect(pm.set("spaceUnits", -1).valid).toBe(false); // fail: out-of-range lower
                 expect(pm.get("spaceUnits")).toBe(4);               // expect default
             });
+            
+            it("should handle context normalization", function () {
+                var normalizer = function (context) {
+                    if (typeof context === "string") {
+                        return {
+                            scopeOrder: [context]
+                        };
+                    }
+                    return context;
+                };
+                
+                var pm = new PreferencesBase.PreferencesSystem(normalizer);
+                pm.addScope("user", new PreferencesBase.MemoryStorage({
+                    value: 1
+                }));
+                pm.addScope("session", new PreferencesBase.MemoryStorage({
+                    value: 2
+                }));
+                expect(pm.get("value")).toBe(2);
+                
+                // Test passing in a string for the get context
+                expect(pm.get("value", "user")).toBe(1);
+                
+                // This will set in the scope in which the value was set. Without a context,
+                // that means "session".
+                pm.set("value", 3);
+                expect(pm.get("value")).toBe(3);
+                expect(pm.get("value", "user")).toBe(1);
+                
+                // Now, set with a context. This should cause the value to be set in "user"
+                // scope.
+                pm.set("value", 4, {
+                    context: "user"
+                });
+                expect(pm.get("value")).toBe(3);
+                expect(pm.get("value", "user")).toBe(4);
+                
+                expect(pm.getPreferenceLocation("value").scope).toBe("session");
+                expect(pm.getPreferenceLocation("value", "user").scope).toBe("user");
+            });
         });
 
         describe("File Storage", function () {