Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select style multiple select interactions removed #10490

Merged

Conversation

bepremeg
Copy link
Contributor

@bepremeg bepremeg commented Jan 7, 2020

This fixes #10486 by removing the event listeners when an
interaction is removed from a map.

@jahow
Copy link
Contributor

jahow commented Jan 7, 2020

Thanks for your contribution! I think a new test would be in order, although your code seems like it should fix the problem. Please let me know if you need indications.

@bepremeg
Copy link
Contributor Author

bepremeg commented Jan 7, 2020

I had a look at the existing tests. I'll probably find a way how to test this. But I'm in doubt what the best place is. a top-level describe('replace Select'), something under describe('selecting a polygon'), or another place altogether?

@jahow
Copy link
Contributor

jahow commented Jan 7, 2020

I'd say a top level describe('clear event listeners on interaction removal') would make sense, so at the same level as this:

describe('selecting a polygon', function() {

Thanks

@bepremeg
Copy link
Contributor Author

bepremeg commented Jan 8, 2020

All right. I had some trouble finding the right way to test this. I couldn't get my test to fail when I reverted my fix :-(
Turns out that the issue also requires an external feature collection. Obvious in hindsight.
This also requires that the activation of the event listeners only happens when the interaction is added to the map. Otherwise the addFeature is invoked regardless of the interaction being associated with a map or not.
This change also makes the event handler association more symmetrical.

@bepremeg
Copy link
Contributor Author

bepremeg commented Jan 8, 2020

@jahow Could you have a look at the test result? There is one failing test for TileQueue on Circleci, but it works on my local test environment and I don't see how my change could impact that test.

@bepremeg
Copy link
Contributor Author

bepremeg commented Jan 9, 2020

I now have exactly the same test code as where I started but the build succeeds now.
As far as I'm concerned, this patch is ready.

@mike-000
Copy link
Contributor

mike-000 commented Feb 2, 2020

@jahow Could you have a look at the test result? There is one failing test for TileQueue on Circleci, but it works on my local test environment and I don't see how my change could impact that test.

Unrelated failed tests seem to be quite common, for example see #7437 (comment) Any change even an edit to a comment will rerun the tests as will squashing and force pushing which also tidies the commit history.

@bepremeg
Copy link
Contributor Author

bepremeg commented Feb 2, 2020

@jahow Indeed. The tests are passing now, even if the code is the same. I made a few commits that cancel each other.

Comment on lines +331 to +332
if (map) {
this.features_.addEventListener(CollectionEventType.ADD, this.boundAddFeature_);
this.features_.addEventListener(CollectionEventType.REMOVE, this.boundRemoveFeature_);

Copy link
Contributor

@michalzielanski michalzielanski Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (map) {
    if (!currentMap) {
        // ...
    }
    // ...

is needed.
For such a code:

import Map from 'ol/Map';
import Select from 'ol/interaction/Select';

var map1 = new Map(/* ... */);
var map2 = new Map(/* ... */);

var selectInteraction = new Select();
// Attach interaction to a map1.
selectInteraction.setMap(map1);
// Change map for interaction to map2.
selectInteraction.setMap(map2);

there will be two listeners per event.

This fixes issue 10486 by removing the event listeners when an
interaction is removed from a map.
event handlers have to be (de)activated when the interaction is added or removed to the map, not when constructed

added unit test
drop superfluous if.
@ahocevar ahocevar force-pushed the CK-240_RemoveSelectEventHandler branch from 7d11faf to ad77143 Compare April 2, 2020 18:05
@ahocevar
Copy link
Member

ahocevar commented Apr 2, 2020

Rebased; fixed merge conflicts.

@ahocevar ahocevar merged commit e683c0a into openlayers:master Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select style is not removed when using two Select interactions
5 participants