From da33b1bb273ac008811521f837ded10891b30a9e Mon Sep 17 00:00:00 2001 From: Jukka Kurkela Date: Thu, 27 Aug 2020 16:14:08 +0300 Subject: [PATCH] Fix shared option handling (#7731) Fix shared option handling --- package-lock.json | 6 +++ package.json | 1 + rollup.config.js | 6 ++- src/controllers/controller.bar.js | 17 ++----- src/controllers/controller.bubble.js | 8 +++- src/controllers/controller.doughnut.js | 7 +-- src/controllers/controller.line.js | 11 +++-- src/core/core.animation.js | 18 ++++++++ src/core/core.animations.js | 63 ++++++++++++++++++-------- src/core/core.animator.js | 1 + src/core/core.controller.js | 8 ++-- src/core/core.datasetController.js | 42 +++++++++-------- test/specs/core.animations.tests.js | 36 +++++++++++++++ types/core/index.d.ts | 5 +- 14 files changed, 162 insertions(+), 67 deletions(-) diff --git a/package-lock.json b/package-lock.json index 374f637ee..0bedf73d1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6965,6 +6965,12 @@ "integrity": "sha512-7PiHtLll5LdnKIMw100I+8xJXR5gW2QwWYkT6iJva0bXitZKa/XMrSbdmg3r2Xnaidz9Qumd0VPaMrZlF9V9sA==", "dev": true }, + "promise-polyfill": { + "version": "8.1.3", + "resolved": "https://registry.npmjs.org/promise-polyfill/-/promise-polyfill-8.1.3.tgz", + "integrity": "sha512-MG5r82wBzh7pSKDRa9y+vllNHz3e3d4CNj1PQE4BQYxLme0gKYYBm9YENq+UkEikyZ0XbiGWxYlVw3Rl9O/U8g==", + "dev": true + }, "psl": { "version": "1.8.0", "resolved": "https://registry.npmjs.org/psl/-/psl-1.8.0.tgz", diff --git a/package.json b/package.json index c37fa2a95..5e7cb3061 100644 --- a/package.json +++ b/package.json @@ -74,6 +74,7 @@ "karma-safari-private-launcher": "^1.0.0", "moment": "^2.27.0", "pixelmatch": "^5.2.1", + "promise-polyfill": "^8.1.3", "resize-observer-polyfill": "^1.5.1", "rollup": "^2.25.0", "rollup-plugin-babel": "^4.4.0", diff --git a/rollup.config.js b/rollup.config.js index b65a2864a..f30161bc1 100644 --- a/rollup.config.js +++ b/rollup.config.js @@ -40,7 +40,8 @@ module.exports = [ input, plugins: [ inject({ - ResizeObserver: 'resize-observer-polyfill' + ResizeObserver: 'resize-observer-polyfill', + Promise: 'promise-polyfill' }), json(), resolve(), @@ -61,7 +62,8 @@ module.exports = [ input, plugins: [ inject({ - ResizeObserver: 'resize-observer-polyfill' + ResizeObserver: 'resize-observer-polyfill', + Promise: 'promise-polyfill' }), json(), resolve(), diff --git a/src/controllers/controller.bar.js b/src/controllers/controller.bar.js index 32769d59b..0e663c983 100644 --- a/src/controllers/controller.bar.js +++ b/src/controllers/controller.bar.js @@ -219,6 +219,7 @@ export default class BarController extends DatasetController { initialize() { const me = this; + me.enableOptionSharing = true; super.initialize(); @@ -241,14 +242,14 @@ export default class BarController extends DatasetController { const horizontal = vscale.isHorizontal(); const ruler = me._getRuler(); const firstOpts = me.resolveDataElementOptions(start, mode); - const sharedOptions = me.getSharedOptions(mode, rectangles[start], firstOpts); + const sharedOptions = me.getSharedOptions(firstOpts); const includeOptions = me.includeOptions(mode, sharedOptions); - let i; + me.updateSharedOptions(sharedOptions, mode, firstOpts); - for (i = 0; i < rectangles.length; i++) { + for (let i = 0; i < rectangles.length; i++) { const index = start + i; - const options = me.resolveDataElementOptions(index, mode); + const options = sharedOptions || me.resolveDataElementOptions(index, mode); const vpixels = me._calculateBarValuePixels(index, options); const ipixels = me._calculateBarIndexPixels(index, ruler, options); @@ -261,19 +262,11 @@ export default class BarController extends DatasetController { width: horizontal ? undefined : ipixels.size }; - // all borders are drawn for floating bar - /* TODO: float bars border skipping magic - if (me.getParsed(i)._custom) { - model.borderSkipped = null; - } - */ if (includeOptions) { properties.options = options; } me.updateElement(rectangles[i], index, properties, mode); } - - me.updateSharedOptions(sharedOptions, mode); } /** diff --git a/src/controllers/controller.bubble.js b/src/controllers/controller.bubble.js index b7be677bc..386317b10 100644 --- a/src/controllers/controller.bubble.js +++ b/src/controllers/controller.bubble.js @@ -3,6 +3,10 @@ import {resolve} from '../helpers/helpers.options'; import {resolveObjectKey} from '../helpers/helpers.core'; export default class BubbleController extends DatasetController { + initialize() { + this.enableOptionSharing = true; + super.initialize(); + } /** * Parse array of objects @@ -69,7 +73,7 @@ export default class BubbleController extends DatasetController { const reset = mode === 'reset'; const {xScale, yScale} = me._cachedMeta; const firstOpts = me.resolveDataElementOptions(start, mode); - const sharedOptions = me.getSharedOptions(mode, points[start], firstOpts); + const sharedOptions = me.getSharedOptions(firstOpts); const includeOptions = me.includeOptions(mode, sharedOptions); for (let i = 0; i < points.length; i++) { @@ -95,7 +99,7 @@ export default class BubbleController extends DatasetController { me.updateElement(point, index, properties, mode); } - me.updateSharedOptions(sharedOptions, mode); + me.updateSharedOptions(sharedOptions, mode, firstOpts); } /** diff --git a/src/controllers/controller.doughnut.js b/src/controllers/controller.doughnut.js index bf6cdaef5..78e79a4fe 100644 --- a/src/controllers/controller.doughnut.js +++ b/src/controllers/controller.doughnut.js @@ -44,6 +44,7 @@ export default class DoughnutController extends DatasetController { constructor(chart, datasetIndex) { super(chart, datasetIndex); + this.enableOptionSharing = true; this.innerRadius = undefined; this.outerRadius = undefined; this.offsetX = undefined; @@ -129,7 +130,7 @@ export default class DoughnutController extends DatasetController { const innerRadius = animateScale ? 0 : me.innerRadius; const outerRadius = animateScale ? 0 : me.outerRadius; const firstOpts = me.resolveDataElementOptions(start, mode); - const sharedOptions = me.getSharedOptions(mode, arcs[start], firstOpts); + const sharedOptions = me.getSharedOptions(firstOpts); const includeOptions = me.includeOptions(mode, sharedOptions); let startAngle = opts.rotation; let i; @@ -152,13 +153,13 @@ export default class DoughnutController extends DatasetController { innerRadius }; if (includeOptions) { - properties.options = me.resolveDataElementOptions(index, mode); + properties.options = sharedOptions || me.resolveDataElementOptions(index, mode); } startAngle += circumference; me.updateElement(arc, index, properties, mode); } - me.updateSharedOptions(sharedOptions, mode); + me.updateSharedOptions(sharedOptions, mode, firstOpts); } calculateTotal() { diff --git a/src/controllers/controller.line.js b/src/controllers/controller.line.js index b7202dca7..fae5d17a1 100644 --- a/src/controllers/controller.line.js +++ b/src/controllers/controller.line.js @@ -5,6 +5,11 @@ import {resolve} from '../helpers/helpers.options'; export default class LineController extends DatasetController { + initialize() { + this.enableOptionSharing = true; + super.initialize(); + } + update(mode) { const me = this; const meta = me._cachedMeta; @@ -31,7 +36,7 @@ export default class LineController extends DatasetController { const reset = mode === 'reset'; const {xScale, yScale, _stacked} = me._cachedMeta; const firstOpts = me.resolveDataElementOptions(start, mode); - const sharedOptions = me.getSharedOptions(mode, points[start], firstOpts); + const sharedOptions = me.getSharedOptions(firstOpts); const includeOptions = me.includeOptions(mode, sharedOptions); const spanGaps = valueOrDefault(me._config.spanGaps, me.chart.options.spanGaps); const maxGapLength = isNumber(spanGaps) ? spanGaps : Number.POSITIVE_INFINITY; @@ -51,7 +56,7 @@ export default class LineController extends DatasetController { }; if (includeOptions) { - properties.options = me.resolveDataElementOptions(index, mode); + properties.options = sharedOptions || me.resolveDataElementOptions(index, mode); } me.updateElement(point, index, properties, mode); @@ -59,7 +64,7 @@ export default class LineController extends DatasetController { prevParsed = parsed; } - me.updateSharedOptions(sharedOptions, mode); + me.updateSharedOptions(sharedOptions, mode, firstOpts); } /** diff --git a/src/core/core.animation.js b/src/core/core.animation.js index 09e7e03a6..f2dcff009 100644 --- a/src/core/core.animation.js +++ b/src/core/core.animation.js @@ -36,6 +36,7 @@ export default class Animation { this._prop = prop; this._from = from; this._to = to; + this._promises = undefined; } active() { @@ -62,6 +63,7 @@ export default class Animation { // update current evaluated value, for smoother animations me.tick(Date.now()); me._active = false; + me._notify(false); } } @@ -79,6 +81,7 @@ export default class Animation { if (!me._active) { me._target[prop] = to; + me._notify(true); return; } @@ -93,4 +96,19 @@ export default class Animation { me._target[prop] = me._fn(from, to, factor); } + + wait() { + const promises = this._promises || (this._promises = []); + return new Promise((res, rej) => { + promises.push({res, rej}); + }); + } + + _notify(resolved) { + const method = resolved ? 'res' : 'rej'; + const promises = this._promises || []; + for (let i = 0; i < promises.length; i++) { + promises[i][method](); + } + } } diff --git a/src/core/core.animations.js b/src/core/core.animations.js index c1b17687f..36549aa9a 100644 --- a/src/core/core.animations.js +++ b/src/core/core.animations.js @@ -57,10 +57,10 @@ defaults.set('animation', { function copyOptions(target, values) { const oldOpts = target.options; const newOpts = values.options; - if (!oldOpts || !newOpts || newOpts.$shared) { + if (!oldOpts || !newOpts) { return; } - if (oldOpts.$shared) { + if (oldOpts.$shared && !newOpts.$shared) { target.options = Object.assign({}, oldOpts, newOpts, {$shared: false}); } else { Object.assign(oldOpts, newOpts); @@ -115,29 +115,25 @@ export default class Animations { /** * Utility to handle animation of `options`. - * This should not be called, when animating $shared options to $shared new options. * @private - * @todo if new options are $shared, target.options should be replaced with those new shared - * options after all animations have completed */ _animateOptions(target, values) { const newOptions = values.options; - let animations = []; + const options = resolveTargetOptions(target, newOptions); + if (!options) { + return []; + } - if (!newOptions) { - return animations; - } - let options = target.options; - if (options) { - if (options.$shared) { - // If the current / old options are $shared, meaning other elements are - // using the same options, we need to clone to become unique. - target.options = options = Object.assign({}, options, {$shared: false, $animations: {}}); - } - animations = this._createAnimations(options, newOptions); - } else { - target.options = newOptions; + const animations = this._createAnimations(options, newOptions); + if (newOptions.$shared && !options.$shared) { + // Going from distinct options to shared options: + // After all animations are done, assing the shared options object to the element + // So any new updates to the shared options are observed + awaitAll(target.$animations, newOptions).then(() => { + target.options = newOptions; + }); } + return animations; } @@ -214,3 +210,32 @@ export default class Animations { } } +function awaitAll(animations, properties) { + const running = []; + const keys = Object.keys(properties); + for (let i = 0; i < keys.length; i++) { + const anim = animations[keys[i]]; + if (anim && anim.active()) { + running.push(anim.wait()); + } + } + // @ts-ignore + return Promise.all(running); +} + +function resolveTargetOptions(target, newOptions) { + if (!newOptions) { + return; + } + let options = target.options; + if (!options) { + target.options = newOptions; + return; + } + if (options.$shared && !newOptions.$shared) { + // Going from shared options to distinct one: + // Create new options object containing the old shared values and start updating that. + target.options = options = Object.assign({}, options, {$shared: false, $animations: {}}); + } + return options; +} diff --git a/src/core/core.animator.js b/src/core/core.animator.js index bed5e2854..64771ff5f 100644 --- a/src/core/core.animator.js +++ b/src/core/core.animator.js @@ -1,6 +1,7 @@ import {requestAnimFrame} from '../helpers/helpers.extras'; /** + * @typedef { import("./core.animation").default } Animation * @typedef { import("./core.controller").default } Chart */ diff --git a/src/core/core.controller.js b/src/core/core.controller.js index 490c4029f..1bd8d39de 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -608,11 +608,9 @@ class Chart { me._updateLayout(); // Can only reset the new controllers after the scales have been updated - if (me.options.animation) { - each(newControllers, (controller) => { - controller.reset(); - }); - } + each(newControllers, (controller) => { + controller.reset(); + }); me._updateDatasets(mode); diff --git a/src/core/core.datasetController.js b/src/core/core.datasetController.js index f79efba87..7f5a958cc 100644 --- a/src/core/core.datasetController.js +++ b/src/core/core.datasetController.js @@ -155,6 +155,10 @@ function optionKey(key, active) { return active ? 'hover' + _capitalize(key) : key; } +function isDirectUpdateMode(mode) { + return mode === 'reset' || mode === 'none'; +} + export default class DatasetController { /** @@ -174,6 +178,8 @@ export default class DatasetController { this._parsing = false; this._data = undefined; this._objectData = undefined; + this._sharedOptions = undefined; + this.enableOptionSharing = false; this.initialize(); } @@ -610,7 +616,7 @@ export default class DatasetController { me.configure(); me._cachedAnimations = {}; me._cachedDataOpts = {}; - me.update(mode); + me.update(mode || 'default'); meta._clip = toClip(valueOrDefault(me._config.clip, defaultClip(meta.xScale, meta.yScale, me.getMaxOverflow()))); } @@ -684,6 +690,7 @@ export default class DatasetController { if (active) { me._addAutomaticHoverColors(index, options); } + return options; } @@ -718,9 +725,11 @@ export default class DatasetController { * @protected */ resolveDataElementOptions(index, mode) { + mode = mode || 'default'; const me = this; const active = mode === 'active'; const cached = me._cachedDataOpts; + const sharing = me.enableOptionSharing; if (cached[mode]) { return cached[mode]; } @@ -736,12 +745,12 @@ export default class DatasetController { if (info.cacheable) { // `$shared` indicades this set of options can be shared between multiple elements. // Sharing is used to reduce number of properties to change during animation. - values.$shared = true; + values.$shared = sharing; // We cache options by `mode`, which can be 'active' for example. This enables us // to have the 'active' element options and 'default' options to switch between // when interacting. - cached[mode] = values; + cached[mode] = sharing ? Object.freeze(values) : values; } return values; @@ -809,17 +818,14 @@ export default class DatasetController { } /** - * Utility for checking if the options are shared and should be animated separately. + * Utility for getting the options object shared between elements * @protected */ - getSharedOptions(mode, el, options) { - if (!mode) { - // store element option sharing status for usage in interactions - this._sharedOptions = options && options.$shared; - } - if (mode !== 'reset' && options && options.$shared && el && el.options && el.options.$shared) { - return {target: el.options, options}; + getSharedOptions(options) { + if (!options.$shared) { + return; } + return this._sharedOptions || (this._sharedOptions = Object.assign({}, options)); } /** @@ -827,10 +833,7 @@ export default class DatasetController { * @protected */ includeOptions(mode, sharedOptions) { - if (mode === 'hide' || mode === 'show') { - return true; - } - return mode !== 'resize' && !sharedOptions; + return !sharedOptions || isDirectUpdateMode(mode); } /** @@ -838,7 +841,7 @@ export default class DatasetController { * @protected */ updateElement(element, index, properties, mode) { - if (mode === 'reset' || mode === 'none') { + if (isDirectUpdateMode(mode)) { Object.assign(element, properties); } else { this._resolveAnimations(index, mode).update(element, properties); @@ -849,9 +852,9 @@ export default class DatasetController { * Utility to animate the shared options, that are potentially affecting multiple elements. * @protected */ - updateSharedOptions(sharedOptions, mode) { + updateSharedOptions(sharedOptions, mode, newOptions) { if (sharedOptions) { - this._resolveAnimations(undefined, mode).update(sharedOptions.target, sharedOptions.options); + this._resolveAnimations(undefined, mode).update({options: sharedOptions}, {options: newOptions}); } } @@ -860,7 +863,8 @@ export default class DatasetController { */ _setStyle(element, index, mode, active) { element.active = active; - this._resolveAnimations(index, mode, active).update(element, {options: this.getStyle(index, active)}); + const options = this.getStyle(index, active); + this._resolveAnimations(index, mode, active).update(element, {options: this.getSharedOptions(options) || options}); } removeHoverStyle(element, datasetIndex, index) { diff --git a/test/specs/core.animations.tests.js b/test/specs/core.animations.tests.js index cc668291c..92cd0e276 100644 --- a/test/specs/core.animations.tests.js +++ b/test/specs/core.animations.tests.js @@ -43,4 +43,40 @@ describe('Chart.animations', function() { options: undefined })).toBeUndefined(); }); + + it('should assign shared options to target after animations complete', function(done) { + const chart = { + draw: function() {}, + options: { + animation: { + debug: false + } + } + }; + const anims = new Chart.Animations(chart, {value: {duration: 100}, option: {duration: 200}}); + + const target = { + value: 1, + options: { + option: 2 + } + }; + const sharedOpts = {option: 10, $shared: true}; + + expect(anims.update(target, { + options: sharedOpts + })).toBeTrue(); + + expect(target.options !== sharedOpts).toBeTrue(); + + Chart.animator.start(chart); + + setTimeout(function() { + expect(Chart.animator.running(chart)).toBeFalse(); + expect(target.options === sharedOpts).toBeTrue(); + + Chart.animator.remove(chart); + done(); + }, 300); + }); }); diff --git a/types/core/index.d.ts b/types/core/index.d.ts index 1a487965d..820d34e46 100644 --- a/types/core/index.d.ts +++ b/types/core/index.d.ts @@ -320,6 +320,7 @@ export class DatasetController; + enableOptionSharing: boolean; linkScales(): void; getAllParsedValues(scale: Scale): number[]; @@ -345,7 +346,7 @@ export class DatasetController