Refactor GradientStop: Improve handling of optionally defined color and rampPoint.

Relates to https://github.com/paperjs/paper.js/issues/1001#issuecomment-197557990

src/style/Color.js

@@ -607,7 +607,7 @@ var Color = Base.extend(new function() {
             // Default fallbacks: rgb, black
             this._type = type || 'rgb';
             // Define this Color's unique id in its own private id pool.
-            // NOTE: This is required by SVG Export code!
+            // NOTE: This is only required by SVG Export code!
             this._id = UID.get(Color);
             if (!components) {
                 // Produce a components array now, and parse values. Even if no
@@ -856,7 +856,11 @@ var Color = Base.extend(new function() {
             }
             for (var i = 0, l = stops.length; i < l; i++) {
                 var stop = stops[i];
-                canvasGradient.addColorStop(stop._rampPoint,
+                // Use the defined offset, and fall back to automatic linear
+                // calculation.
+                // NOTE: that if _rampPoint is 0 for the first entry, the fall
+                // back will be so too.
+                canvasGradient.addColorStop(stop._rampPoint || i / (l - 1),
                         stop._color.toCanvasStyle());
             }
             return this._canvasStyle = canvasGradient;

src/style/Gradient.js

@@ -72,10 +72,11 @@ var Gradient = Base.extend(/** @lends Gradient# */{
             stops = radial = null;
         if (!this._stops)
             this.setStops(stops || ['white', 'black']);
-        if (this._radial == null)
+        if (this._radial == null) {
             // Support old string type argument and new radial boolean.
             this.setRadial(typeof radial === 'string' && radial === 'radial'
                     || radial || false);
+        }
     },
 
     _serialize: function(options, dictionary) {
@@ -91,8 +92,9 @@ var Gradient = Base.extend(/** @lends Gradient# */{
     _changed: function() {
         // Loop through the gradient-colors that use this gradient and notify
         // them, so they can notify the items they belong to.
-        for (var i = 0, l = this._owners && this._owners.length; i < l; i++)
+        for (var i = 0, l = this._owners && this._owners.length; i < l; i++) {
             this._owners[i]._changed();
+        }
     },
 
     /**
@@ -122,8 +124,9 @@ var Gradient = Base.extend(/** @lends Gradient# */{
      */
     clone: function() {
         var stops = [];
-        for (var i = 0, l = this._stops.length; i < l; i++)
+        for (var i = 0, l = this._stops.length; i < l; i++) {
             stops[i] = this._stops[i].clone();
+        }
         return new Gradient(stops, this._radial);
     },
 
@@ -138,23 +141,20 @@ var Gradient = Base.extend(/** @lends Gradient# */{
     },
 
     setStops: function(stops) {
-        // If this gradient already contains stops, first remove
+        if (stops.length < 2) {
-        // this gradient as their owner.
-        if (this.stops) {
-            for (var i = 0, l = this._stops.length; i < l; i++)
-                this._stops[i]._owner = undefined;
-        }
-        if (stops.length < 2)
             throw new Error(
                     'Gradient stop list needs to contain at least two stops.');
-        this._stops = GradientStop.readAll(stops, 0, { clone: true });
-        // Now reassign ramp points if they were not specified.
-        for (var i = 0, l = this._stops.length; i < l; i++) {
-            var stop = this._stops[i];
-            stop._owner = this;
-            if (stop._defaultRamp)
-                stop.setRampPoint(i / (l - 1));
         }
+        // If this gradient already contains stops, first remove their owner.
+        var _stops = this._stops;
+        if (_stops) {
+            for (var i = 0, l = _stops.length; i < l; i++)
+                _stops[i]._owner = undefined;
+        }
+        _stops = this._stops = GradientStop.readAll(stops, 0, { clone: true });
+        // Now assign this gradient as the new gradients' owner.
+        for (var i = 0, l = _stops.length; i < l; i++)
+            _stops[i]._owner = this;
         this._changed();
     },
 
@@ -182,13 +182,17 @@ var Gradient = Base.extend(/** @lends Gradient# */{
     equals: function(gradient) {
         if (gradient === this)
             return true;
-        if (gradient && this._class === gradient._class
+        if (gradient && this._class === gradient._class) {
-                && this._stops.length === gradient._stops.length) {
+            var stops1 = this._stops,
-            for (var i = 0, l = this._stops.length; i < l; i++) {
+                stops2 = gradient._stops,
-                if (!this._stops[i].equals(gradient._stops[i]))
+                length = stops1.length;
-                    return false;
+            if (length === stops2.length) {
+                for (var i = 0; i < length; i++) {
+                    if (!stops1[i].equals(stops2[i]))
+                        return false;
+                }
+                return true;
             }
-            return true;
         }
         return false;
     }

src/style/GradientStop.js

@@ -23,28 +23,30 @@ var GradientStop = Base.extend(/** @lends GradientStop# */{
      * Creates a GradientStop object.
      *
      * @param {Color} [color=new Color(0, 0, 0)] the color of the stop
-     * @param {Number} [rampPoint=0] the position of the stop on the gradient
+     * @param {Number} [rampPoint=null] the position of the stop on the gradient
-     * ramp as a value between 0 and 1
+     * ramp as a value between `0` and `1`; `null` or `undefined` for automatic
+     * assignment.
      */
     initialize: function GradientStop(arg0, arg1) {
-        if (arg0) {
+        // (color, rampPoint)
-            var color, rampPoint;
+        var color = arg0,
-            if (arg1 === undefined && Array.isArray(arg0)) {
+            rampPoint = arg1;
-                // [color, rampPoint]
+        if (typeof arg0 === 'object' && arg1 === undefined) {
+            // Make sure the first entry in the array is not a number, in which
+            // case the whole array would be a color, and the assignments would
+            // already have occurred correctly above.
+            if (Array.isArray(arg0) && typeof arg0[0] !== 'number') {
+                // ([color, rampPoint])
                 color = arg0[0];
                 rampPoint = arg0[1];
-            } else if (arg0.color) {
+            } else if ('color' in arg0 || 'rampPoint' in arg0) {
-                // stop
+                // (stop)
                 color = arg0.color;
                 rampPoint = arg0.rampPoint;
-            } else {
-                // color, rampPoint
-                color = arg0;
-                rampPoint = arg1;
             }
-            this.setColor(color);
-            this.setRampPoint(rampPoint);
         }
+        this.setColor(color);
+        this.setRampPoint(rampPoint);
     },
 
     // TODO: Do we really need to also clone the color here?
@@ -56,8 +58,10 @@ var GradientStop = Base.extend(/** @lends GradientStop# */{
     },
 
     _serialize: function(options, dictionary) {
-        return Base.serialize([this._color, this._rampPoint], options, true,
+        var color = this._color,
-                dictionary);
+            rampPoint = this._rampPoint;
+        return Base.serialize(rampPoint == null ? [color] : [color, rampPoint],
+                options, true, dictionary);
     },
 
     /**
@@ -115,8 +119,7 @@ var GradientStop = Base.extend(/** @lends GradientStop# */{
     },
 
     setRampPoint: function(rampPoint) {
-        this._defaultRamp = rampPoint == null;
+        this._rampPoint = rampPoint;
-        this._rampPoint = rampPoint || 0;
         this._changed();
     },
 
@@ -162,13 +165,13 @@ var GradientStop = Base.extend(/** @lends GradientStop# */{
         return this._color;
     },
 
-    setColor: function(color) {
+    setColor: function(/* color */) {
         // Make sure newly set colors are cloned, since they can only have
         // one owner.
-        this._color = Color.read(arguments);
+        var color = Color.read(arguments, 0, { clone: true });
-        if (this._color === color)
+        if (color)
-            this._color = color.clone();
+            color._owner = this;
-        this._color._owner = this;
+        this._color = color;
         this._changed();
     },
 

src/svg/SvgExport.js

@@ -231,12 +231,14 @@ new function() {
             var stops = gradient._stops;
             for (var i = 0, l = stops.length; i < l; i++) {
                 var stop = stops[i],
+                    offset = stop._rampPoint,
                     stopColor = stop._color,
                     alpha = stopColor.getAlpha();
-                attrs = {
+                attrs = {};
-                    offset: stop._rampPoint,
+                if (offset != null)
-                    'stop-color': stopColor.toCSS(true)
+                    attrs.offset = offset;
-                };
+                if (stopColor)
+                    attrs['stop-color'] = stopColor.toCSS(true);
                 // See applyStyle for an explanation of why there are separated
                 // opacity / color attributes.
                 if (alpha < 1)

test/tests/Color.js

@@ -222,3 +222,16 @@ test('Color#divide', function() {
     var color = new Color(1, 1, 1);
     equals(color.divide(4), new Color([0.25, 0.25, 0.25]));
 });
+
+test('Gradient', function() {
+    var stop1 = new GradientStop({ rampPoint: 0 });
+    var stop2 = new GradientStop('red', 0.75);
+    var stop3 = new GradientStop(['white', 1]);
+    var gradient = new Gradient([stop1, stop2, stop3], true);
+    equals(function() { return stop1.color; }, new Color(0, 0, 0));
+    equals(function() { return stop2.color; }, new Color(1, 0, 0));
+    equals(function() { return stop3.color; }, new Color(1, 1, 1));
+    equals(function() { return stop1.rampPoint; }, 0);
+    equals(function() { return stop2.rampPoint; }, 0.75);
+    equals(function() { return stop3.rampPoint; }, 1);
+});

test/tests/JSON.js

@@ -219,7 +219,11 @@ test('Color#importJSON()', function() {
         // that runs between the two points we defined earlier:
         fillColor: {
             gradient: {
-                stops: ['yellow', 'red', 'blue']
+                stops: [
+                    ['yellow', 0],
+                    ['red', 0.5],
+                    ['blue', 1]
+                ]
             },
             origin: topLeft,
             destination: bottomRight