From d93aca6b5c953aa13956cc9fbc945cd63e5acd06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrg=20Lehni?= Date: Thu, 17 Mar 2016 09:38:42 +0100 Subject: [PATCH] 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 | 8 +++++-- src/style/Gradient.js | 50 +++++++++++++++++++++------------------ src/style/GradientStop.js | 49 ++++++++++++++++++++------------------ src/svg/SvgExport.js | 10 ++++---- test/tests/Color.js | 13 ++++++++++ test/tests/JSON.js | 6 ++++- 6 files changed, 83 insertions(+), 53 deletions(-) diff --git a/src/style/Color.js b/src/style/Color.js index 971dfa93..ef9b0bf2 100644 --- a/src/style/Color.js +++ b/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; diff --git a/src/style/Gradient.js b/src/style/Gradient.js index 938342b4..0e436a27 100644 --- a/src/style/Gradient.js +++ b/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 - // 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) + 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 - && this._stops.length === gradient._stops.length) { - for (var i = 0, l = this._stops.length; i < l; i++) { - if (!this._stops[i].equals(gradient._stops[i])) - return false; + if (gradient && this._class === gradient._class) { + var stops1 = this._stops, + stops2 = gradient._stops, + length = stops1.length; + 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; } diff --git a/src/style/GradientStop.js b/src/style/GradientStop.js index aa14904d..f7b50083 100644 --- a/src/style/GradientStop.js +++ b/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 - * ramp as a value between 0 and 1 + * @param {Number} [rampPoint=null] the position of the stop on the gradient + * ramp as a value between `0` and `1`; `null` or `undefined` for automatic + * assignment. */ initialize: function GradientStop(arg0, arg1) { - if (arg0) { - var color, rampPoint; - if (arg1 === undefined && Array.isArray(arg0)) { - // [color, rampPoint] + // (color, rampPoint) + var color = arg0, + rampPoint = arg1; + 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) { - // stop + } else if ('color' in arg0 || 'rampPoint' in arg0) { + // (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, - dictionary); + var color = this._color, + 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 || 0; + this._rampPoint = rampPoint; 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); - if (this._color === color) - this._color = color.clone(); - this._color._owner = this; + var color = Color.read(arguments, 0, { clone: true }); + if (color) + color._owner = this; + this._color = color; this._changed(); }, diff --git a/src/svg/SvgExport.js b/src/svg/SvgExport.js index 14168265..543ab730 100644 --- a/src/svg/SvgExport.js +++ b/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 = { - offset: stop._rampPoint, - 'stop-color': stopColor.toCSS(true) - }; + attrs = {}; + if (offset != null) + 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) diff --git a/test/tests/Color.js b/test/tests/Color.js index 5467399d..aace4335 100644 --- a/test/tests/Color.js +++ b/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); +}); diff --git a/test/tests/JSON.js b/test/tests/JSON.js index 1a424d9b..bd50f0d5 100644 --- a/test/tests/JSON.js +++ b/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