From 10c7f08fa41ae2efb4217006ef68161ce3277962 Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Tue, 7 Jan 2020 11:28:19 +0100 Subject: [PATCH 1/8] Select style multiple select interactions removed This fixes issue 10486 by removing the event listeners when an interaction is removed from a map. --- src/ol/interaction/Select.js | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/ol/interaction/Select.js b/src/ol/interaction/Select.js index 11a3c74bd0..3bd762db51 100644 --- a/src/ol/interaction/Select.js +++ b/src/ol/interaction/Select.js @@ -167,6 +167,16 @@ class Select extends Interaction { const options = opt_options ? opt_options : {}; + /** + * @private + */ + this.boundAddFeature_ = this.addFeature_.bind(this); + + /** + * @private + */ + this.boundRemoveFeature_ = this.removeFeature_.bind(this); + /** * @private * @type {import("../events/condition.js").Condition} @@ -250,9 +260,8 @@ class Select extends Interaction { */ this.featureLayerAssociation_ = {}; - const features = this.getFeatures(); - features.addEventListener(CollectionEventType.ADD, this.addFeature_.bind(this)); - features.addEventListener(CollectionEventType.REMOVE, this.removeFeature_.bind(this)); + this.features_.addEventListener(CollectionEventType.ADD, this.boundAddFeature_); + this.features_.addEventListener(CollectionEventType.REMOVE, this.boundRemoveFeature_); } /** @@ -323,6 +332,10 @@ class Select extends Interaction { if (map && this.style_) { this.features_.forEach(this.applySelectedStyle_.bind(this)); } + if (!map) { + this.features_.removeEventListener(CollectionEventType.ADD, this.boundAddFeature_); + this.features_.removeEventListener(CollectionEventType.REMOVE, this.boundRemoveFeature_); + } } /** From a30a92a9635e40f99e4c19270c58ab0c2cc9abab Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Wed, 8 Jan 2020 10:53:00 +0100 Subject: [PATCH 2/8] CK-240: fix multiple select interactions on map event handlers have to be (de)activated when the interaction is added or removed to the map, not when constructed added unit test --- src/ol/interaction/Select.js | 12 +++--- test/spec/ol/interaction/select.test.js | 50 +++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/ol/interaction/Select.js b/src/ol/interaction/Select.js index 3bd762db51..a08c22bc54 100644 --- a/src/ol/interaction/Select.js +++ b/src/ol/interaction/Select.js @@ -259,9 +259,6 @@ class Select extends Interaction { * @type {Object} */ this.featureLayerAssociation_ = {}; - - this.features_.addEventListener(CollectionEventType.ADD, this.boundAddFeature_); - this.features_.addEventListener(CollectionEventType.REMOVE, this.boundRemoveFeature_); } /** @@ -329,8 +326,13 @@ class Select extends Interaction { this.features_.forEach(this.restorePreviousStyle_.bind(this)); } super.setMap(map); - if (map && this.style_) { - this.features_.forEach(this.applySelectedStyle_.bind(this)); + if (map) { + this.features_.addEventListener(CollectionEventType.ADD, this.boundAddFeature_); + this.features_.addEventListener(CollectionEventType.REMOVE, this.boundRemoveFeature_); + + if (this.style_) { + this.features_.forEach(this.applySelectedStyle_.bind(this)); + } } if (!map) { this.features_.removeEventListener(CollectionEventType.ADD, this.boundAddFeature_); diff --git a/test/spec/ol/interaction/select.test.js b/test/spec/ol/interaction/select.test.js index e64eb2dc28..65d47cea43 100644 --- a/test/spec/ol/interaction/select.test.js +++ b/test/spec/ol/interaction/select.test.js @@ -9,6 +9,7 @@ import Interaction from '../../../../src/ol/interaction/Interaction.js'; import Select from '../../../../src/ol/interaction/Select.js'; import VectorLayer from '../../../../src/ol/layer/Vector.js'; import VectorSource from '../../../../src/ol/source/Vector.js'; +import Style from '../../../../src/ol/style/Style.js'; describe('ol.interaction.Select', function() { @@ -406,4 +407,53 @@ describe('ol.interaction.Select', function() { }); }); + + describe('clear event listeners on interaction removal', function() { + let firstInteraction, secondInteraction, feature; + + beforeEach(function() { + feature = source.getFeatures()[3]; // top feature is selected + + const style = new Style({}); + const features = new Collection(); + + firstInteraction = new Select({ style, features }); + secondInteraction = new Select({ style, features }); + }); + + afterEach(function() { + map.removeInteraction(secondInteraction); + map.removeInteraction(firstInteraction); + }); + + // The base case + describe('with a single interaction added', function() { + it('changes the selected feature once', function() { + map.addInteraction(firstInteraction); + + const listenerSpy = sinon.spy(); + feature.on('change', listenerSpy); + + simulateEvent('singleclick', 10, -20, false); + + expect(listenerSpy.callCount).to.be(1); + }); + }); + + // The "difficult" case. To prevent regression + describe('with a replaced interaction', function() { + it('changes the selected feature once', function() { + map.addInteraction(firstInteraction); + map.removeInteraction(firstInteraction); + map.addInteraction(secondInteraction); + + const listenerSpy = sinon.spy(); + feature.on('change', listenerSpy); + + simulateEvent('singleclick', 10, -20, false); + + expect(listenerSpy.callCount).to.be(1); + }); + }); + }); }); From dc957ec10455f06ecd0b640dd0c52df7ef6a7682 Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Wed, 8 Jan 2020 11:04:33 +0100 Subject: [PATCH 3/8] CK-240: fix lint errors --- test/spec/ol/interaction/select.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/spec/ol/interaction/select.test.js b/test/spec/ol/interaction/select.test.js index 65d47cea43..f648ad94ca 100644 --- a/test/spec/ol/interaction/select.test.js +++ b/test/spec/ol/interaction/select.test.js @@ -417,8 +417,8 @@ describe('ol.interaction.Select', function() { const style = new Style({}); const features = new Collection(); - firstInteraction = new Select({ style, features }); - secondInteraction = new Select({ style, features }); + firstInteraction = new Select({style, features}); + secondInteraction = new Select({style, features}); }); afterEach(function() { From 2d7e55e26abb4eb90cb89676adc1dd06e747aba7 Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Wed, 8 Jan 2020 13:44:43 +0100 Subject: [PATCH 4/8] Small code cleanup drop superfluous if. --- src/ol/interaction/Select.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ol/interaction/Select.js b/src/ol/interaction/Select.js index a08c22bc54..9fde58ccbb 100644 --- a/src/ol/interaction/Select.js +++ b/src/ol/interaction/Select.js @@ -333,8 +333,7 @@ class Select extends Interaction { if (this.style_) { this.features_.forEach(this.applySelectedStyle_.bind(this)); } - } - if (!map) { + } else { this.features_.removeEventListener(CollectionEventType.ADD, this.boundAddFeature_); this.features_.removeEventListener(CollectionEventType.REMOVE, this.boundRemoveFeature_); } From e9e75cd8aff58ad96a0b7a93d16780ae0e54e0c2 Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Wed, 8 Jan 2020 15:31:33 +0100 Subject: [PATCH 5/8] temporarily disable test to observe impact --- test/spec/ol/interaction/select.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/spec/ol/interaction/select.test.js b/test/spec/ol/interaction/select.test.js index f648ad94ca..be2d03cc93 100644 --- a/test/spec/ol/interaction/select.test.js +++ b/test/spec/ol/interaction/select.test.js @@ -408,7 +408,7 @@ describe('ol.interaction.Select', function() { }); - describe('clear event listeners on interaction removal', function() { + xdescribe('clear event listeners on interaction removal', function() { let firstInteraction, secondInteraction, feature; beforeEach(function() { From 3909938a706acba76263f514a6cb29cd7b3f63e4 Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Wed, 8 Jan 2020 15:37:27 +0100 Subject: [PATCH 6/8] Experiment with test impact further --- test/spec/ol/interaction/select.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/spec/ol/interaction/select.test.js b/test/spec/ol/interaction/select.test.js index be2d03cc93..bc8aa66a97 100644 --- a/test/spec/ol/interaction/select.test.js +++ b/test/spec/ol/interaction/select.test.js @@ -408,7 +408,7 @@ describe('ol.interaction.Select', function() { }); - xdescribe('clear event listeners on interaction removal', function() { + describe('clear event listeners on interaction removal', function() { let firstInteraction, secondInteraction, feature; beforeEach(function() { @@ -441,7 +441,7 @@ describe('ol.interaction.Select', function() { }); // The "difficult" case. To prevent regression - describe('with a replaced interaction', function() { + xdescribe('with a replaced interaction', function() { it('changes the selected feature once', function() { map.addInteraction(firstInteraction); map.removeInteraction(firstInteraction); From cf1191505ead9cfd38aed30d2d93a150dac94f20 Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Wed, 8 Jan 2020 17:17:33 +0100 Subject: [PATCH 7/8] Experiment with test impact further (2) --- test/spec/ol/interaction/select.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/spec/ol/interaction/select.test.js b/test/spec/ol/interaction/select.test.js index bc8aa66a97..06aa5b4fb5 100644 --- a/test/spec/ol/interaction/select.test.js +++ b/test/spec/ol/interaction/select.test.js @@ -427,7 +427,7 @@ describe('ol.interaction.Select', function() { }); // The base case - describe('with a single interaction added', function() { + xdescribe('with a single interaction added', function() { it('changes the selected feature once', function() { map.addInteraction(firstInteraction); @@ -441,7 +441,7 @@ describe('ol.interaction.Select', function() { }); // The "difficult" case. To prevent regression - xdescribe('with a replaced interaction', function() { + describe('with a replaced interaction', function() { it('changes the selected feature once', function() { map.addInteraction(firstInteraction); map.removeInteraction(firstInteraction); From ad77143417041c6ec93d8a8a287b0bfb96d7dcb0 Mon Sep 17 00:00:00 2001 From: Geert Premereur Date: Wed, 8 Jan 2020 22:31:31 +0100 Subject: [PATCH 8/8] Experiment with test impact further (3) --- test/spec/ol/interaction/select.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/spec/ol/interaction/select.test.js b/test/spec/ol/interaction/select.test.js index 06aa5b4fb5..f648ad94ca 100644 --- a/test/spec/ol/interaction/select.test.js +++ b/test/spec/ol/interaction/select.test.js @@ -427,7 +427,7 @@ describe('ol.interaction.Select', function() { }); // The base case - xdescribe('with a single interaction added', function() { + describe('with a single interaction added', function() { it('changes the selected feature once', function() { map.addInteraction(firstInteraction);