From bce27b861d88d8bf3447b1167f5d70eaa9fbc865 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Sun, 22 Jan 2017 20:13:40 +0100 Subject: [PATCH 1/3] Plugin hooks and jsdoc enhancements Make all `before` hooks cancellable (except `beforeInit`), meaning that if any plugin return explicitly `false`, the current action is not performed. Ensure that `init` hooks are called before `update` hooks and add associated calling order unit tests. Deprecate `Chart.PluginBase` in favor of `IPlugin` (no more an inheritable class) and document plugin hooks (also rename `extension` by `hook`). --- docs/09-Advanced.md | 2 +- src/core/core.controller.js | 140 +++++++++++++++------------ src/core/core.plugin.js | 174 +++++++++++++++++++++++++++------- test/core.controller.tests.js | 71 ++++++++++++++ 4 files changed, 296 insertions(+), 91 deletions(-) diff --git a/docs/09-Advanced.md b/docs/09-Advanced.md index 4c677c904d4..d3d69d9afc9 100644 --- a/docs/09-Advanced.md +++ b/docs/09-Advanced.md @@ -412,7 +412,7 @@ Plugins will be called at the following times * Before an animation is started * When an event occurs on the canvas (mousemove, click, etc). This requires the `options.events` property handled -Plugins should derive from Chart.PluginBase and implement the following interface +Plugins should implement the `IPlugin` interface: ```javascript { beforeInit: function(chartInstance) { }, diff --git a/src/core/core.controller.js b/src/core/core.controller.js index 1d2366152ec..e6c19659387 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -3,6 +3,7 @@ module.exports = function(Chart) { var helpers = Chart.helpers; + var plugins = Chart.plugins; var platform = Chart.platform; // Create a dictionary of chart types, to allow for extension of existing types @@ -101,16 +102,17 @@ module.exports = function(Chart) { } me.initialize(); + me.update(); return me; }; - helpers.extend(Chart.Controller.prototype, /** @lends Chart.Controller */ { + helpers.extend(Chart.Controller.prototype, /** @lends Chart.Controller.prototype */ { initialize: function() { var me = this; // Before init plugin notification - Chart.plugins.notify(me, 'beforeInit'); + plugins.notify(me, 'beforeInit'); helpers.retinaScale(me.chart); @@ -125,10 +127,9 @@ module.exports = function(Chart) { me.ensureScalesHaveIDs(); me.buildScales(); me.initToolTip(); - me.update(); // After init plugin notification - Chart.plugins.notify(me, 'afterInit'); + plugins.notify(me, 'afterInit'); return me; }, @@ -170,7 +171,7 @@ module.exports = function(Chart) { if (!silent) { // Notify any plugins about the resize var newSize = {width: newWidth, height: newHeight}; - Chart.plugins.notify(me, 'resize', [newSize]); + plugins.notify(me, 'resize', [newSize]); // Notify of resize if (me.options.onResize) { @@ -287,7 +288,6 @@ module.exports = function(Chart) { /** * Reset the elements of all datasets - * @method resetElements * @private */ resetElements: function() { @@ -299,19 +299,20 @@ module.exports = function(Chart) { /** * Resets the chart back to it's state before the initial animation - * @method reset */ reset: function() { this.resetElements(); this.tooltip.initialize(); }, - update: function(animationDuration, lazy) { var me = this; updateConfig(me); - Chart.plugins.notify(me, 'beforeUpdate'); + + if (plugins.notify(me, 'beforeUpdate') === false) { + return; + } // In case the entire data object changed me.tooltip._data = me.data; @@ -324,10 +325,7 @@ module.exports = function(Chart) { me.getDatasetMeta(datasetIndex).controller.buildOrUpdateElements(); }, me); - Chart.layoutService.update(me, me.chart.width, me.chart.height); - - // Apply changes to the datasets that require the scales to have been calculated i.e BorderColor changes - Chart.plugins.notify(me, 'afterScaleUpdate'); + me.updateLayout(); // Can only reset the new controllers after the scales have been updated helpers.each(newControllers, function(controller) { @@ -337,7 +335,7 @@ module.exports = function(Chart) { me.updateDatasets(); // Do this before render so that any plugins that need final scale updates can use it - Chart.plugins.notify(me, 'afterUpdate'); + plugins.notify(me, 'afterUpdate'); if (me._bufferedRender) { me._bufferedRequest = { @@ -350,51 +348,64 @@ module.exports = function(Chart) { }, /** - * @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 + * Updates the chart layout unless a plugin returns `false` to the `beforeLayout` + * hook, in which case, plugins will not be called on `afterLayout`. + * @private */ + updateLayout: function() { + var me = this; - /** - * @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 - */ + if (plugins.notify(me, 'beforeLayout') === false) { + return; + } + + Chart.layoutService.update(this, this.chart.width, this.chart.height); + + /** + * Provided for backward compatibility, use `afterLayout` instead. + * @method IPlugin#afterScaleUpdate + * @deprecated since version 2.5.0 + * @todo remove at version 3 + */ + plugins.notify(me, 'afterScaleUpdate'); + plugins.notify(me, 'afterLayout'); + }, /** - * 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 + * Updates all datasets unless a plugin returns `false` to the `beforeDatasetsUpdate` + * hook, in which case, plugins will not be called on `afterDatasetsUpdate`. + * @private */ updateDatasets: function() { var me = this; - var i, ilen; - if (Chart.plugins.notify(me, 'beforeDatasetsUpdate')) { - for (i = 0, ilen = me.data.datasets.length; i < ilen; ++i) { - me.getDatasetMeta(i).controller.update(); - } + if (plugins.notify(me, 'beforeDatasetsUpdate') === false) { + return; + } - Chart.plugins.notify(me, 'afterDatasetsUpdate'); + for (var i = 0, ilen = me.data.datasets.length; i < ilen; ++i) { + me.getDatasetMeta(i).controller.update(); } + + plugins.notify(me, 'afterDatasetsUpdate'); }, render: function(duration, lazy) { var me = this; - Chart.plugins.notify(me, 'beforeRender'); + + if (plugins.notify(me, 'beforeRender') === false) { + return; + } var animationOptions = me.options.animation; + var onComplete = function() { + plugins.notify(me, 'afterRender'); + var callback = animationOptions && animationOptions.onComplete; + if (callback && callback.call) { + callback.call(me); + } + }; + if (animationOptions && ((typeof duration !== 'undefined' && duration !== 0) || (typeof duration === 'undefined' && animationOptions.duration !== 0))) { var animation = new Chart.Animation(); animation.numSteps = (duration || animationOptions.duration) / 16.66; // 60 fps @@ -411,15 +422,14 @@ module.exports = function(Chart) { // user events animation.onAnimationProgress = animationOptions.onProgress; - animation.onAnimationComplete = animationOptions.onComplete; + animation.onAnimationComplete = onComplete; Chart.animationService.addAnimation(me, animation, duration, lazy); } else { me.draw(); - if (animationOptions && animationOptions.onComplete && animationOptions.onComplete.call) { - animationOptions.onComplete.call(me); - } + onComplete(); } + return me; }, @@ -428,31 +438,45 @@ module.exports = function(Chart) { var easingDecimal = ease || 1; me.clear(); - Chart.plugins.notify(me, 'beforeDraw', [easingDecimal]); + plugins.notify(me, 'beforeDraw', [easingDecimal]); // Draw all the scales helpers.each(me.boxes, function(box) { box.draw(me.chartArea); }, me); + if (me.scale) { me.scale.draw(); } - Chart.plugins.notify(me, 'beforeDatasetsDraw', [easingDecimal]); + me.drawDatasets(easingDecimal); + + // Finally draw the tooltip + me.tooltip.transition(easingDecimal).draw(); + + plugins.notify(me, 'afterDraw', [easingDecimal]); + }, + + /** + * Draws all datasets unless a plugin returns `false` to the `beforeDatasetsDraw` + * hook, in which case, plugins will not be called on `afterDatasetsDraw`. + * @private + */ + drawDatasets: function(easingValue) { + var me = this; + + if (plugins.notify(me, 'beforeDatasetsDraw', [easingValue]) === false) { + return; + } // Draw each dataset via its respective controller (reversed to support proper line stacking) helpers.each(me.data.datasets, function(dataset, datasetIndex) { if (me.isDatasetVisible(datasetIndex)) { - me.getDatasetMeta(datasetIndex).controller.draw(ease); + me.getDatasetMeta(datasetIndex).controller.draw(easingValue); } }, me, true); - Chart.plugins.notify(me, 'afterDatasetsDraw', [easingDecimal]); - - // Finally draw the tooltip - me.tooltip.transition(easingDecimal).draw(); - - Chart.plugins.notify(me, 'afterDraw', [easingDecimal]); + plugins.notify(me, 'afterDatasetsDraw', [easingValue]); }, // Get the single element that was clicked on @@ -551,7 +575,7 @@ module.exports = function(Chart) { me.chart.ctx = null; } - Chart.plugins.notify(me, 'destroy'); + plugins.notify(me, 'destroy'); delete Chart.instances[me.id]; }, @@ -642,7 +666,7 @@ module.exports = function(Chart) { var changed = me.handleEvent(e); changed |= tooltip && tooltip.handleEvent(e); - changed |= Chart.plugins.notify(me, 'onEvent', [e]); + changed |= plugins.notify(me, 'onEvent', [e]); var bufferedRequest = me._bufferedRequest; if (bufferedRequest) { diff --git a/src/core/core.plugin.js b/src/core/core.plugin.js index 657c85a3545..f89b412d2e1 100644 --- a/src/core/core.plugin.js +++ b/src/core/core.plugin.js @@ -3,7 +3,6 @@ module.exports = function(Chart) { var helpers = Chart.helpers; - var noop = helpers.noop; Chart.defaults.global.plugins = {}; @@ -86,15 +85,15 @@ module.exports = function(Chart) { }, /** - * Calls enabled plugins for chart, on the specified extension and with the given args. + * Calls enabled plugins for `chart` on the specified hook and 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 {Object} chart chart instance for which plugins should be called. - * @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. + * @param {Object} chart - The chart instance for which plugins should be called. + * @param {String} hook - The name of the plugin method to call (e.g. 'beforeUpdate'). + * @param {Array} [args] - Extra arguments to apply to the hook call. * @returns {Boolean} false if any of the plugins return false, else returns true. */ - notify: function(chart, extension, args) { + notify: function(chart, hook, args) { var descriptors = this.descriptors(chart); var ilen = descriptors.length; var i, descriptor, plugin, params, method; @@ -102,7 +101,7 @@ module.exports = function(Chart) { for (i=0; i Date: Sun, 22 Jan 2017 21:01:46 +0100 Subject: [PATCH 2/3] Make `beforeDraw` cancellable and fix easing value --- src/core/core.controller.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index e6c19659387..f67b29ff934 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -433,12 +433,18 @@ module.exports = function(Chart) { return me; }, - draw: function(ease) { + draw: function(easingValue) { var me = this; - var easingDecimal = ease || 1; + me.clear(); - plugins.notify(me, 'beforeDraw', [easingDecimal]); + if (easingValue === undefined || easingValue === null) { + easingValue = 1; + } + + if (plugins.notify(me, 'beforeDraw', [easingValue]) === false) { + return; + } // Draw all the scales helpers.each(me.boxes, function(box) { @@ -449,12 +455,12 @@ module.exports = function(Chart) { me.scale.draw(); } - me.drawDatasets(easingDecimal); + me.drawDatasets(easingValue); // Finally draw the tooltip - me.tooltip.transition(easingDecimal).draw(); + me.tooltip.transition(easingValue).draw(); - plugins.notify(me, 'afterDraw', [easingDecimal]); + plugins.notify(me, 'afterDraw', [easingValue]); }, /** From 60aa596873121de30ebb6789b4b01f4899626ffa Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Sun, 22 Jan 2017 21:10:17 +0100 Subject: [PATCH 3/3] Replace `onEvent` by `before/afterEvent` --- docs/09-Advanced.md | 9 +++------ src/core/core.controller.js | 10 +++++++--- src/core/core.legend.js | 2 +- src/core/core.plugin.js | 16 ++++++++++++++++ test/platform.dom.tests.js | 2 +- 5 files changed, 28 insertions(+), 11 deletions(-) diff --git a/docs/09-Advanced.md b/docs/09-Advanced.md index d3d69d9afc9..05642fcab43 100644 --- a/docs/09-Advanced.md +++ b/docs/09-Advanced.md @@ -439,12 +439,9 @@ Plugins should implement the `IPlugin` interface: destroy: function(chartInstance) { } - /** - * Called when an event occurs on the chart - * @param e {Core.Event} the Chart.js wrapper around the native event. e.native is the original event - * @return {Boolean} true if the chart is changed and needs to re-render - */ - onEvent: function(chartInstance, e) {} + // Called when an event occurs on the chart + beforeEvent: function(chartInstance, event) {} + afterEvent: function(chartInstance, event) {} } ``` diff --git a/src/core/core.controller.js b/src/core/core.controller.js index f67b29ff934..b38df58b291 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -664,7 +664,10 @@ module.exports = function(Chart) { eventHandler: function(e) { var me = this; var tooltip = me.tooltip; - var hoverOptions = me.options.hover; + + if (plugins.notify(me, 'beforeEvent', [e]) === false) { + return; + } // Buffer any update calls so that renders do not occur me._bufferedRender = true; @@ -672,7 +675,8 @@ module.exports = function(Chart) { var changed = me.handleEvent(e); changed |= tooltip && tooltip.handleEvent(e); - changed |= plugins.notify(me, 'onEvent', [e]); + + plugins.notify(me, 'afterEvent', [e]); var bufferedRequest = me._bufferedRequest; if (bufferedRequest) { @@ -684,7 +688,7 @@ module.exports = function(Chart) { // We only need to render at this point. Updating will cause scales to be // recomputed generating flicker & using more memory than necessary. - me.render(hoverOptions.animationDuration, true); + me.render(me.options.hover.animationDuration, true); } me._bufferedRender = false; diff --git a/src/core/core.legend.js b/src/core/core.legend.js index b80bdbe25ad..4816b285f2e 100644 --- a/src/core/core.legend.js +++ b/src/core/core.legend.js @@ -526,7 +526,7 @@ module.exports = function(Chart) { delete chartInstance.legend; } }, - onEvent: function(chartInstance, e) { + afterEvent: function(chartInstance, e) { var legend = chartInstance.legend; if (legend) { legend.handleEvent(e); diff --git a/src/core/core.plugin.js b/src/core/core.plugin.js index f89b412d2e1..bda2ff45739 100644 --- a/src/core/core.plugin.js +++ b/src/core/core.plugin.js @@ -274,6 +274,22 @@ module.exports = function(Chart) { * @param {Number} easingValue - The current animation value, between 0.0 and 1.0. * @param {Object} options - The plugin options. */ + /** + * @method IPlugin#beforeEvent + * @desc Called before processing the specified `event`. If any plugin returns `false`, + * the event will be discarded. + * @param {Chart.Controller} chart - The chart instance. + * @param {IEvent} event - The event object. + * @param {Object} options - The plugin options. + */ + /** + * @method IPlugin#afterEvent + * @desc Called after the `event` has been consumed. Note that this hook + * will not be called if the `event` has been previously discarded. + * @param {Chart.Controller} chart - The chart instance. + * @param {IEvent} event - The event object. + * @param {Object} options - The plugin options. + */ /** * @method IPlugin#resize * @desc Called after the chart as been resized. diff --git a/test/platform.dom.tests.js b/test/platform.dom.tests.js index 19c5bbe1aab..a022cc75d5a 100644 --- a/test/platform.dom.tests.js +++ b/test/platform.dom.tests.js @@ -320,7 +320,7 @@ describe('Platform.dom', function() { it('should notify plugins about events', function() { var notifiedEvent; var plugin = { - onEvent: function(chart, e) { + afterEvent: function(chart, e) { notifiedEvent = e; } };