Merge pull request #7564 from adobe/dangoor/7442-pref-consistency

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

Author: redmunds

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