From b6db07001a0e8054ef8b7ba2addfeed6c87b44c7 Mon Sep 17 00:00:00 2001 From: David Manthey Date: Thu, 30 Aug 2018 13:52:47 -0400 Subject: [PATCH] Fix a possible polygon feature update after delete. On a geo.gl.polygonFeature with stroke, the update can be delayed one animation frame. If the polygonFeature is deleted during that delay, this could throw an error due to the styles of the feature being reset as part of the deletion process. This unschedules the update. This PR also introduces a `ready` property to features to ensure deleted features are not drawn. This makes `m_this` references in the feature module more consistent. --- CHANGELOG.md | 1 + src/canvas/object.js | 8 +++++--- src/d3/object.js | 6 ++++-- src/feature.js | 32 +++++++++++++++++++++++--------- src/gl/object.js | 8 +++++--- src/gl/polygonFeature.js | 7 +++++++ tests/cases/feature.js | 8 ++++++++ tests/cases/polygonFeature.js | 1 + 8 files changed, 54 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce0ee9e808..ed76fa4f4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ### Bug Fixes - Fixed an issue with overlapping, cropped tiles on old browsers (#901) +- Fixed an issue where a `geo.gl.polygonFeature` could be updated after it was deleted (#913) ### Changes - Changed build process: optional dependencies are now included in the bundle by default (#890) diff --git a/src/canvas/object.js b/src/canvas/object.js index efc7d84b1b..dd0dc828d3 100644 --- a/src/canvas/object.js +++ b/src/canvas/object.js @@ -70,9 +70,11 @@ var canvas_object = function (arg) { * @returns {this} The current object. */ this.draw = function () { - m_this._update(); - m_this.renderer()._render(); - s_draw(); + if (m_this.ready) { + m_this._update(); + m_this.renderer()._render(); + s_draw(); + } return m_this; }; diff --git a/src/d3/object.js b/src/d3/object.js index aee93825ad..01bb0dfc2a 100644 --- a/src/d3/object.js +++ b/src/d3/object.js @@ -48,8 +48,10 @@ var d3_object = function (arg) { * @returns {this} */ this.draw = function () { - m_this._update(); - s_draw(); + if (m_this.ready) { + m_this._update(); + s_draw(); + } return m_this; }; diff --git a/src/feature.js b/src/feature.js index 371e0ea8fd..80e5624fe1 100644 --- a/src/feature.js +++ b/src/feature.js @@ -105,6 +105,7 @@ var feature = function (arg) { var m_this = this, s_exit = this._exit, + m_ready, m_selectionAPI = arg.selectionAPI === undefined ? false : !!arg.selectionAPI, m_style = {}, m_layer = arg.layer === undefined ? null : arg.layer, @@ -122,16 +123,27 @@ var feature = function (arg) { // data items, such as individual vertices on lines or polygons. this._subfeatureStyles = {}; + /** + * @property {boolean} ready `true` if this feature has been initialized, + * `false` if it was destroyed, `undefined` if it was created but not + * initialized. + * @name geo.feature#ready + */ + Object.defineProperty(this, 'ready', { + get: function () { + return m_ready; + } + }); + /** * Private method to bind mouse handlers on the map element. This does * nothing if the selectionAPI is turned off. Otherwise, it first unbinds * any existing handlers and then binds handlers. */ this._bindMouseHandlers = function () { - // Don't bind handlers for improved performance on features that don't // require it. - if (!this.selectionAPI()) { + if (!m_this.selectionAPI()) { return; } @@ -217,7 +229,7 @@ var feature = function (arg) { // evt.previous. if (over.index.length > 1) { m_this.geoTrigger(geo_event.feature.mouseover_order, { - feature: this, + feature: m_this, mouse: mouse, previous: m_selectedFeatures, over: over @@ -334,7 +346,7 @@ var feature = function (arg) { // maintain evt.over.found. Handlers should not modify evt.over.extra. if (over.index.length > 1) { m_this.geoTrigger(geo_event.feature.mouseclick_order, { - feature: this, + feature: m_this, mouse: mouse, over: over }); @@ -463,7 +475,7 @@ var feature = function (arg) { if (util.isFunction(m_style[key])) { out = function () { return util.convertColor( - m_style[key].apply(this, arguments) + m_style[key].apply(m_this, arguments) ); }; } else { @@ -667,7 +679,7 @@ var feature = function (arg) { this.bin = function (val) { if (val === undefined) { if (m_bin === null) { - var parent = this.parent(), + var parent = m_this.parent(), idx = parent ? parent.children().indexOf(m_this) : -1; return idx >= 0 ? idx : 0; } @@ -772,10 +784,10 @@ var feature = function (arg) { arg = !!arg; if (arg !== m_selectionAPI || direct) { m_selectionAPI = arg; - this._unbindMouseHandlers(); - this._bindMouseHandlers(); + m_this._unbindMouseHandlers(); + m_this._bindMouseHandlers(); } - return this; + return m_this; }; /** @@ -810,6 +822,7 @@ var feature = function (arg) { {'opacity': 1.0}, arg.style === undefined ? {} : arg.style); m_this._bindMouseHandlers(); + m_ready = true; }; /** @@ -839,6 +852,7 @@ var feature = function (arg) { m_selectedFeatures = []; m_style = {}; s_exit(); + m_ready = false; }; this._init(arg); diff --git a/src/gl/object.js b/src/gl/object.js index 9ac1d25a75..2ee786581c 100644 --- a/src/gl/object.js +++ b/src/gl/object.js @@ -27,9 +27,11 @@ var gl_object = function (arg) { * @returns {this} */ this.draw = function () { - m_this._update({mayDelay: true}); - m_this.renderer()._render(); - s_draw(); + if (m_this.ready) { + m_this._update({mayDelay: true}); + m_this.renderer()._render(); + s_draw(); + } return m_this; }; diff --git a/src/gl/polygonFeature.js b/src/gl/polygonFeature.js index c3af384ae0..a06219d7c6 100644 --- a/src/gl/polygonFeature.js +++ b/src/gl/polygonFeature.js @@ -339,6 +339,9 @@ var gl_polygonFeature = function (arg) { * frame for the update. */ this._update = function (opts) { + if (!m_this.ready) { + return; + } if (opts && opts.mayDelay && m_builtOnce) { m_updateAnimFrameRef = m_this.layer().map().scheduleAnimationFrame(m_this._update); return; @@ -363,6 +366,10 @@ var gl_polygonFeature = function (arg) { * Destroy. */ this._exit = function () { + if (m_updateAnimFrameRef && m_this.layer()) { + m_this.layer().map().scheduleAnimationFrame(m_this._update, 'remove'); + m_updateAnimFrameRef = null; + } m_this.renderer().contextRenderer().removeActor(m_actor); s_exit(); }; diff --git a/tests/cases/feature.js b/tests/cases/feature.js index 0066865703..9dd81d5ad1 100644 --- a/tests/cases/feature.js +++ b/tests/cases/feature.js @@ -363,5 +363,13 @@ describe('geo.feature', function () { expect(feat.selectionAPI(true, true)).toBe(feat); expect(feat.selectionAPI()).toBe(true); }); + it('ready', function () { + feat = geo.feature({layer: layer}); + expect(feat.ready).toBe(true); + feat._exit(); + expect(feat.ready).toBe(false); + feat._init({}); + expect(feat.ready).toBe(true); + }); }); }); diff --git a/tests/cases/polygonFeature.js b/tests/cases/polygonFeature.js index fa60cac527..17e3c366da 100644 --- a/tests/cases/polygonFeature.js +++ b/tests/cases/polygonFeature.js @@ -406,6 +406,7 @@ describe('geo.polygonFeature', function () { var buildTime = polygons.buildTime().getMTime(); layer.deleteFeature(polygons); polygons.data(testPolygons); + polygons._update(); map.draw(); expect(buildTime).toEqual(polygons.buildTime().getMTime()); destroyMap();