From 7a419af4c2a9a4190fbe2ef550e20d38505cd648 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Fri, 10 Jun 2016 22:26:35 +0200 Subject: [PATCH 1/4] Rename plugin service and notification method Rename `Chart.pluginService` to `Chart.plugins` (so move the old Chart.plugins array as a private member of the service), and rename `notifyPlugins` to `notify` for consistency with other service methods. --- src/chart.js | 3 +++ src/core/core.controller.js | 26 +++++++++++++------------- src/core/core.legend.js | 2 +- src/core/core.plugin.js | 35 ++++++++++++++++++++++++++--------- src/core/core.title.js | 6 +++--- test/core.plugin.tests.js | 31 ++++++++++++++++--------------- 6 files changed, 62 insertions(+), 41 deletions(-) diff --git a/src/chart.js b/src/chart.js index f992a9e1c45..75bd4753841 100644 --- a/src/chart.js +++ b/src/chart.js @@ -1,3 +1,6 @@ +/** + * @namespace Chart + */ var Chart = require('./core/core.js')(); require('./core/core.helpers')(Chart); diff --git a/src/core/core.controller.js b/src/core/core.controller.js index db521c607eb..eb472d63cc1 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -45,7 +45,7 @@ module.exports = function(Chart) { initialize: function initialize() { var me = this; // Before init plugin notification - Chart.pluginService.notifyPlugins('beforeInit', [me]); + Chart.plugins.notify('beforeInit', [me]); me.bindEvents(); @@ -60,7 +60,7 @@ module.exports = function(Chart) { me.update(); // After init plugin notification - Chart.pluginService.notifyPlugins('afterInit', [me]); + Chart.plugins.notify('afterInit', [me]); return me; }, @@ -83,7 +83,7 @@ module.exports = function(Chart) { var newWidth = helpers.getMaximumWidth(canvas); var aspectRatio = chart.aspectRatio; var newHeight = (me.options.maintainAspectRatio && isNaN(aspectRatio) === false && isFinite(aspectRatio) && aspectRatio !== 0) ? newWidth / aspectRatio : helpers.getMaximumHeight(canvas); - + var sizeChanged = chart.width !== newWidth || chart.height !== newHeight; if (!sizeChanged) { @@ -97,7 +97,7 @@ module.exports = function(Chart) { // Notify any plugins about the resize var newSize = { width: newWidth, height: newHeight }; - Chart.pluginService.notifyPlugins('resize', [me, newSize]); + Chart.plugins.notify('resize', [me, newSize]); // Notify of resize if (me.options.onResize) { @@ -225,7 +225,7 @@ module.exports = function(Chart) { update: function update(animationDuration, lazy) { var me = this; - Chart.pluginService.notifyPlugins('beforeUpdate', [me]); + Chart.plugins.notify('beforeUpdate', [me]); // In case the entire data object changed me.tooltip._data = me.data; @@ -241,7 +241,7 @@ module.exports = function(Chart) { Chart.layoutService.update(me, me.chart.width, me.chart.height); // Apply changes to the dataets that require the scales to have been calculated i.e BorderColor chages - Chart.pluginService.notifyPlugins('afterScaleUpdate', [me]); + Chart.plugins.notify('afterScaleUpdate', [me]); // Can only reset the new controllers after the scales have been updated helpers.each(newControllers, function(controller) { @@ -254,14 +254,14 @@ module.exports = function(Chart) { }, me); // Do this before render so that any plugins that need final scale updates can use it - Chart.pluginService.notifyPlugins('afterUpdate', [me]); + Chart.plugins.notify('afterUpdate', [me]); me.render(animationDuration, lazy); }, render: function render(duration, lazy) { var me = this; - Chart.pluginService.notifyPlugins('beforeRender', [me]); + Chart.plugins.notify('beforeRender', [me]); var animationOptions = me.options.animation; if (animationOptions && ((typeof duration !== 'undefined' && duration !== 0) || (typeof duration === 'undefined' && animationOptions.duration !== 0))) { @@ -297,7 +297,7 @@ module.exports = function(Chart) { var easingDecimal = ease || 1; me.clear(); - Chart.pluginService.notifyPlugins('beforeDraw', [me, easingDecimal]); + Chart.plugins.notify('beforeDraw', [me, easingDecimal]); // Draw all the scales helpers.each(me.boxes, function(box) { @@ -307,7 +307,7 @@ module.exports = function(Chart) { me.scale.draw(); } - Chart.pluginService.notifyPlugins('beforeDatasetDraw', [me, easingDecimal]); + Chart.plugins.notify('beforeDatasetDraw', [me, easingDecimal]); // Draw each dataset via its respective controller (reversed to support proper line stacking) helpers.each(me.data.datasets, function(dataset, datasetIndex) { @@ -316,12 +316,12 @@ module.exports = function(Chart) { } }, me, true); - Chart.pluginService.notifyPlugins('afterDatasetDraw', [me, easingDecimal]); + Chart.plugins.notify('afterDatasetDraw', [me, easingDecimal]); // Finally draw the tooltip me.tooltip.transition(easingDecimal).draw(); - Chart.pluginService.notifyPlugins('afterDraw', [me, easingDecimal]); + Chart.plugins.notify('afterDraw', [me, easingDecimal]); }, // Get the single element that was clicked on @@ -470,7 +470,7 @@ module.exports = function(Chart) { canvas.style.width = me.chart.originalCanvasStyleWidth; canvas.style.height = me.chart.originalCanvasStyleHeight; - Chart.pluginService.notifyPlugins('destroy', [me]); + Chart.plugins.notify('destroy', [me]); delete Chart.instances[me.id]; }, diff --git a/src/core/core.legend.js b/src/core/core.legend.js index b3d392c05fc..78c89c55925 100644 --- a/src/core/core.legend.js +++ b/src/core/core.legend.js @@ -420,7 +420,7 @@ module.exports = function(Chart) { }); // Register the legend plugin - Chart.pluginService.register({ + Chart.plugins.register({ beforeInit: function(chartInstance) { var opts = chartInstance.options; var legendOpts = opts.legend; diff --git a/src/core/core.plugin.js b/src/core/core.plugin.js index 31be4d48b4a..7126dcd43d5 100644 --- a/src/core/core.plugin.js +++ b/src/core/core.plugin.js @@ -1,14 +1,20 @@ "use strict"; module.exports = function(Chart) { + var helpers = Chart.helpers; + var noop = helpers.noop; + + /** + * The plugin service singleton + * @namespace Chart.plugins + */ + Chart.plugins = { + _plugins: [], - // Plugins are stored here - Chart.plugins = []; - Chart.pluginService = { // Register a new plugin register: function(plugin) { - var p = Chart.plugins; + var p = this._plugins; if (p.indexOf(plugin) === -1) { p.push(plugin); } @@ -16,16 +22,20 @@ module.exports = function(Chart) { // Remove a registered plugin remove: function(plugin) { - var p = Chart.plugins; + var p = this._plugins; var idx = p.indexOf(plugin); if (idx !== -1) { p.splice(idx, 1); } }, - // Iterate over all plugins - notifyPlugins: function(method, args, scope) { - helpers.each(Chart.plugins, function(plugin) { + /** + * Calls registered plugins on the specified method, with the given args. This + * method immediately returns as soon as a plugin explicitly returns false. + * @returns {Boolean} false if any of the plugins return false, else returns true. + */ + notify: function(method, args, scope) { + helpers.each(this._plugins, function(plugin) { if (plugin[method] && typeof plugin[method] === 'function') { plugin[method].apply(scope, args); } @@ -33,7 +43,6 @@ module.exports = function(Chart) { } }; - var noop = helpers.noop; Chart.PluginBase = Chart.Element.extend({ // Plugin methods. All functions are passed the chart instance @@ -58,4 +67,12 @@ module.exports = function(Chart) { // Called during destroy destroy: noop }); + + /** + * Provided for backward compatibility, use Chart.plugins instead + * @namespace Chart.pluginService + * @deprecated since version 2.1.5 + * @todo remove me at version 3 + */ + Chart.pluginService = Chart.plugins; }; diff --git a/src/core/core.title.js b/src/core/core.title.js index b74f6f8291c..4e00b82b774 100644 --- a/src/core/core.title.js +++ b/src/core/core.title.js @@ -38,7 +38,7 @@ module.exports = function(Chart) { }, update: function(maxWidth, maxHeight, margins) { var me = this; - + // Update Lifecycle - Probably don't want to ever extend or overwrite this function ;) me.beforeUpdate(); @@ -155,7 +155,7 @@ module.exports = function(Chart) { fontFamily = valueOrDefault(opts.fontFamily, globalDefaults.defaultFontFamily), titleFont = helpers.fontString(fontSize, fontStyle, fontFamily), rotation = 0, - titleX, + titleX, titleY, top = me.top, left = me.left, @@ -187,7 +187,7 @@ module.exports = function(Chart) { }); // Register the title plugin - Chart.pluginService.register({ + Chart.plugins.register({ beforeInit: function(chartInstance) { var opts = chartInstance.options; var titleOpts = opts.title; diff --git a/test/core.plugin.tests.js b/test/core.plugin.tests.js index 520305e3980..62e5e218808 100644 --- a/test/core.plugin.tests.js +++ b/test/core.plugin.tests.js @@ -3,37 +3,38 @@ describe('Test the plugin system', function() { var oldPlugins; beforeAll(function() { - oldPlugins = Chart.plugins; + oldPlugins = Chart.plugins._plugins; }); + afterAll(function() { - Chart.plugins = oldPlugins; + Chart.plugins._plugins = oldPlugins; }); beforeEach(function() { - Chart.plugins = []; + Chart.plugins._plugins = []; }); it ('Should register plugins', function() { var myplugin = {}; - Chart.pluginService.register(myplugin); - expect(Chart.plugins.length).toBe(1); + Chart.plugins.register(myplugin); + expect(Chart.plugins._plugins.length).toBe(1); // Should only add plugin once - Chart.pluginService.register(myplugin); - expect(Chart.plugins.length).toBe(1); + Chart.plugins.register(myplugin); + expect(Chart.plugins._plugins.length).toBe(1); }); it ('Should allow unregistering plugins', function() { var myplugin = {}; - Chart.pluginService.register(myplugin); - expect(Chart.plugins.length).toBe(1); + Chart.plugins.register(myplugin); + expect(Chart.plugins._plugins.length).toBe(1); // Should only add plugin once - Chart.pluginService.remove(myplugin); - expect(Chart.plugins.length).toBe(0); + Chart.plugins.remove(myplugin); + expect(Chart.plugins._plugins.length).toBe(0); // Removing a plugin that doesn't exist should not error - Chart.pluginService.remove(myplugin); + Chart.plugins.remove(myplugin); }); it ('Should allow notifying plugins', function() { @@ -43,9 +44,9 @@ describe('Test the plugin system', function() { myplugin.count = chart.count; } }; - Chart.pluginService.register(myplugin); - - Chart.pluginService.notifyPlugins('trigger', [{ count: 10 }]); + Chart.plugins.register(myplugin); + + Chart.plugins.notify('trigger', [{ count: 10 }]); expect(myplugin.count).toBe(10); }); From a55c17d73fa7bfa9e2780fce3ea9a7770a11d8d3 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Fri, 10 Jun 2016 22:26:55 +0200 Subject: [PATCH 2/4] Enhance plugin notification system Change the plugin notification behavior: this method now returns false as soon as a plugin *explicitly* returns false, else returns true. Also, plugins are now called in their own scope (so remove the never used `scope` parameter). --- src/core/core.plugin.js | 31 +++++++++++++++++++++---------- test/core.plugin.tests.js | 39 +++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/core/core.plugin.js b/src/core/core.plugin.js index 7126dcd43d5..2b020ac687c 100644 --- a/src/core/core.plugin.js +++ b/src/core/core.plugin.js @@ -2,8 +2,7 @@ module.exports = function(Chart) { - var helpers = Chart.helpers; - var noop = helpers.noop; + var noop = Chart.helpers.noop; /** * The plugin service singleton @@ -30,21 +29,33 @@ module.exports = function(Chart) { }, /** - * Calls registered plugins on the specified method, with the given args. This - * method immediately returns as soon as a plugin explicitly returns false. + * Calls registered plugins on the specified extension, with the given args. This + * method immediately returns as soon as a plugin explicitly returns false. The + * returned value can be used, for instance, to interrupt the current action. + * @param {String} extension the name of the plugin method to call (e.g. 'beforeUpdate'). + * @param {Array} [args] extra arguments to apply to the extension call. * @returns {Boolean} false if any of the plugins return false, else returns true. */ - notify: function(method, args, scope) { - helpers.each(this._plugins, function(plugin) { - if (plugin[method] && typeof plugin[method] === 'function') { - plugin[method].apply(scope, args); + notify: function(extension, args) { + var plugins = this._plugins; + var ilen = plugins.length; + var i, plugin; + + for (i=0; i Date: Fri, 10 Jun 2016 22:27:06 +0200 Subject: [PATCH 3/4] Allow to register/unregister an array of plugins The plugins service now accepts an array of plugin instances to register or unregister (for consistency, renamed `Chart.plugins.remove` to `unregister`). Also added a few methods to manipulate registered plugins, such as `count`, `getAll` and `clear` (mainly used by our unit tests). --- src/core/core.plugin.js | 59 ++++++++++++++++++++++++++------ test/core.plugin.tests.js | 72 +++++++++++++++++++++++++++------------ 2 files changed, 98 insertions(+), 33 deletions(-) diff --git a/src/core/core.plugin.js b/src/core/core.plugin.js index 2b020ac687c..5fce4007715 100644 --- a/src/core/core.plugin.js +++ b/src/core/core.plugin.js @@ -7,25 +7,62 @@ module.exports = function(Chart) { /** * The plugin service singleton * @namespace Chart.plugins + * @since 2.1.0 */ Chart.plugins = { _plugins: [], - // Register a new plugin - register: function(plugin) { + /** + * Registers the given plugin(s) if not already registered. + * @param {Array|Object} plugins plugin instance(s). + */ + register: function(plugins) { var p = this._plugins; - if (p.indexOf(plugin) === -1) { - p.push(plugin); - } + ([]).concat(plugins).forEach(function(plugin) { + if (p.indexOf(plugin) === -1) { + p.push(plugin); + } + }); }, - // Remove a registered plugin - remove: function(plugin) { + /** + * Unregisters the given plugin(s) only if registered. + * @param {Array|Object} plugins plugin instance(s). + */ + unregister: function(plugins) { var p = this._plugins; - var idx = p.indexOf(plugin); - if (idx !== -1) { - p.splice(idx, 1); - } + ([]).concat(plugins).forEach(function(plugin) { + var idx = p.indexOf(plugin); + if (idx !== -1) { + p.splice(idx, 1); + } + }); + }, + + /** + * Remove all registered p^lugins. + * @since 2.1.5 + */ + clear: function() { + this._plugins = []; + }, + + /** + * Returns the number of registered plugins? + * @returns {Number} + * @since 2.1.5 + */ + count: function() { + return this._plugins.length; + }, + + /** + * Returns all registered plugin intances. + * @returns {Array} array of plugin objects. + * @since 2.1.5 + */ + getAll: function() { + return this._plugins; }, /** diff --git a/test/core.plugin.tests.js b/test/core.plugin.tests.js index c3c290128bb..0d4186af8d6 100644 --- a/test/core.plugin.tests.js +++ b/test/core.plugin.tests.js @@ -1,44 +1,72 @@ -// Plugin tests -describe('Test the plugin system', function() { +describe('Chart.plugins', function() { var oldPlugins; beforeAll(function() { - oldPlugins = Chart.plugins._plugins; + oldPlugins = Chart.plugins.getAll(); }); afterAll(function() { - Chart.plugins._plugins = oldPlugins; + Chart.plugins.register(oldPlugins); }); beforeEach(function() { - Chart.plugins._plugins = []; + Chart.plugins.clear(); }); - it ('Should register plugins', function() { - var myplugin = {}; - Chart.plugins.register(myplugin); - expect(Chart.plugins._plugins.length).toBe(1); + describe('Chart.plugins.register', function() { + it('should register a plugin', function() { + Chart.plugins.register({}); + expect(Chart.plugins.count()).toBe(1); + Chart.plugins.register({}); + expect(Chart.plugins.count()).toBe(2); + }); + + it('should register an array of plugins', function() { + Chart.plugins.register([{}, {}, {}]); + expect(Chart.plugins.count()).toBe(3); + }); - // Should only add plugin once - Chart.plugins.register(myplugin); - expect(Chart.plugins._plugins.length).toBe(1); + it('should succeed to register an already registered plugin', function() { + var plugin = {}; + Chart.plugins.register(plugin); + expect(Chart.plugins.count()).toBe(1); + Chart.plugins.register(plugin); + expect(Chart.plugins.count()).toBe(1); + Chart.plugins.register([{}, plugin, plugin]); + expect(Chart.plugins.count()).toBe(2); + }); }); - it ('Should allow unregistering plugins', function() { - var myplugin = {}; - Chart.plugins.register(myplugin); - expect(Chart.plugins._plugins.length).toBe(1); + describe('Chart.plugins.unregister', function() { + it('should unregister a plugin', function() { + var plugin = {}; + Chart.plugins.register(plugin); + expect(Chart.plugins.count()).toBe(1); + Chart.plugins.unregister(plugin); + expect(Chart.plugins.count()).toBe(0); + }); - // Should only add plugin once - Chart.plugins.remove(myplugin); - expect(Chart.plugins._plugins.length).toBe(0); + it('should unregister an array of plugins', function() { + var plugins = [{}, {}, {}]; + Chart.plugins.register(plugins); + expect(Chart.plugins.count()).toBe(3); + Chart.plugins.unregister(plugins.slice(0, 2)); + expect(Chart.plugins.count()).toBe(1); + }); - // Removing a plugin that doesn't exist should not error - Chart.plugins.remove(myplugin); + it('should succeed to unregister a plugin not registered', function() { + var plugin = {}; + Chart.plugins.register(plugin); + expect(Chart.plugins.count()).toBe(1); + Chart.plugins.unregister({}); + expect(Chart.plugins.count()).toBe(1); + Chart.plugins.unregister([{}, plugin]); + expect(Chart.plugins.count()).toBe(0); + }); }); describe('Chart.plugins.notify', function() { - it ('should call plugins with arguments', function() { + it('should call plugins with arguments', function() { var myplugin = { count: 0, trigger: function(chart) { From 53eb7667dd25f53fbf0d65e28370b32d90ab9448 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Sat, 11 Jun 2016 00:14:27 +0200 Subject: [PATCH 4/4] New datasets update plugin extensions Add `beforeDatasetsUpdate` and `afterDatasetsUpdate` plugin notifications during the chart update. Plugins are able to cancel the datasets update by explicitly returning false to `beforeDatasetsUpdate`. For consistency, rename `(before|after)DatasetDraw` to `(before|after)DatasetsDraw`. --- docs/09-Advanced.md | 10 +++++-- src/core/core.controller.js | 57 +++++++++++++++++++++++++++++++------ src/core/core.plugin.js | 7 +++-- 3 files changed, 61 insertions(+), 13 deletions(-) diff --git a/docs/09-Advanced.md b/docs/09-Advanced.md index 952ec88f51b..58dd50da996 100644 --- a/docs/09-Advanced.md +++ b/docs/09-Advanced.md @@ -378,6 +378,8 @@ Plugins will be called at the following times * End of initialization * Start of update * After the chart scales have calculated +* Start of datasets update +* End of datasets update * End of update (before render occurs) * Start of draw * End of draw @@ -396,9 +398,11 @@ Plugins should derive from Chart.PluginBase and implement the following interfac beforeUpdate: function(chartInstance) { }, afterScaleUpdate: function(chartInstance) { } + beforeDatasetsUpdate: function(chartInstance) { } + afterDatasetsUpdate: function(chartInstance) { } afterUpdate: function(chartInstance) { }, - // This is called at the start of a render. It is only called once, even if the animation will run for a number of frames. Use beforeDraw or afterDraw + // This is called at the start of a render. It is only called once, even if the animation will run for a number of frames. Use beforeDraw or afterDraw // to do something on each animation frame beforeRender: function(chartInstance) { }, @@ -406,8 +410,8 @@ Plugins should derive from Chart.PluginBase and implement the following interfac beforeDraw: function(chartInstance, easing) { }, afterDraw: function(chartInstance, easing) { }, // Before the datasets are drawn but after scales are drawn - beforeDatasetDraw: function(chartInstance, easing) { }, - afterDatasetDraw: function(chartInstance, easing) { }, + beforeDatasetsDraw: function(chartInstance, easing) { }, + afterDatasetsDraw: function(chartInstance, easing) { }, destroy: function(chartInstance) { } } diff --git a/src/core/core.controller.js b/src/core/core.controller.js index eb472d63cc1..2d0148b7749 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -13,7 +13,10 @@ module.exports = function(Chart) { // Controllers available for dataset visualization eg. bar, line, slice, etc. Chart.controllers = {}; - // The main controller of a chart + /** + * @class Chart.Controller + * The main controller of a chart. + */ Chart.Controller = function(instance) { this.chart = instance; @@ -40,7 +43,7 @@ module.exports = function(Chart) { return this; }; - helpers.extend(Chart.Controller.prototype, { + helpers.extend(Chart.Controller.prototype, /** @lends Chart.Controller */ { initialize: function initialize() { var me = this; @@ -248,10 +251,7 @@ module.exports = function(Chart) { controller.reset(); }); - // This will loop through any data and do the appropriate element update for the type - helpers.each(me.data.datasets, function(dataset, datasetIndex) { - me.getDatasetMeta(datasetIndex).controller.update(); - }, me); + me.updateDatasets(); // Do this before render so that any plugins that need final scale updates can use it Chart.plugins.notify('afterUpdate', [me]); @@ -259,6 +259,47 @@ module.exports = function(Chart) { me.render(animationDuration, lazy); }, + /** + * @method beforeDatasetsUpdate + * @description Called before all datasets are updated. If a plugin returns false, + * the datasets update will be cancelled until another chart update is triggered. + * @param {Object} instance the chart instance being updated. + * @returns {Boolean} false to cancel the datasets update. + * @memberof Chart.PluginBase + * @since version 2.1.5 + * @instance + */ + + /** + * @method afterDatasetsUpdate + * @description Called after all datasets have been updated. Note that this + * extension will not be called if the datasets update has been cancelled. + * @param {Object} instance the chart instance being updated. + * @memberof Chart.PluginBase + * @since version 2.1.5 + * @instance + */ + + /** + * Updates all datasets unless a plugin returns false to the beforeDatasetsUpdate + * extension, in which case no datasets will be updated and the afterDatasetsUpdate + * notification will be skipped. + * @protected + * @instance + */ + updateDatasets: function() { + var me = this; + var i, ilen; + + if (Chart.plugins.notify('beforeDatasetsUpdate', [ me ])) { + for (i = 0, ilen = me.data.datasets.length; i < ilen; ++i) { + me.getDatasetMeta(i).controller.update(); + } + + Chart.plugins.notify('afterDatasetsUpdate', [ me ]); + } + }, + render: function render(duration, lazy) { var me = this; Chart.plugins.notify('beforeRender', [me]); @@ -307,7 +348,7 @@ module.exports = function(Chart) { me.scale.draw(); } - Chart.plugins.notify('beforeDatasetDraw', [me, easingDecimal]); + Chart.plugins.notify('beforeDatasetsDraw', [me, easingDecimal]); // Draw each dataset via its respective controller (reversed to support proper line stacking) helpers.each(me.data.datasets, function(dataset, datasetIndex) { @@ -316,7 +357,7 @@ module.exports = function(Chart) { } }, me, true); - Chart.plugins.notify('afterDatasetDraw', [me, easingDecimal]); + Chart.plugins.notify('afterDatasetsDraw', [me, easingDecimal]); // Finally draw the tooltip me.tooltip.transition(easingDecimal).draw(); diff --git a/src/core/core.plugin.js b/src/core/core.plugin.js index 5fce4007715..d6f95c4d117 100644 --- a/src/core/core.plugin.js +++ b/src/core/core.plugin.js @@ -91,9 +91,12 @@ module.exports = function(Chart) { } }; + /** + * Plugin extension methods. + * @interface Chart.PluginBase + * @since 2.1.0 + */ Chart.PluginBase = Chart.Element.extend({ - // Plugin extensions. All functions are passed the chart instance - // Called at start of chart init beforeInit: noop,