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

Add an option to remove feature silently in ol.source.Vector. #15643

Conversation

ger-benjamin
Copy link
Contributor

For #15642

Be able to remove a feature from vector source without triggering events. Useful to batch remove features.

Copy link

📦 Preview the website for this branch here: https://deploy-preview-15643--ol-site.netlify.app/.

@ahocevar
Copy link
Member

I'm not sure this really fixes #15642. Please see my #15642 (comment).

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! From an API consistency perspective, it would make sense to add a removeFeatures() method, instead of adding a silent option to removeFeature(). Would you be able/willing to change this pull request accordingly? Then we had a sensible counterpart to addFeatures().

@ger-benjamin ger-benjamin force-pushed the feature-15642-vector-source-remove-feature-silently branch from 3115fb9 to 51ae7b4 Compare March 15, 2024 15:04
@ger-benjamin
Copy link
Contributor Author

ger-benjamin commented Mar 15, 2024

I've added a first simple version of removeFeatures/removeFeature.
removeFeature(feature) calls now this.removeFeatures([feature])
I'll check next steps on Monday. Have a nice weekend

@ahocevar
Copy link
Member

ahocevar commented Mar 15, 2024

I think going the other way around would be easier, something like

--- a/src/ol/source/Vector.js
+++ b/src/ol/source/Vector.js
@@ -1093,6 +1093,20 @@ class VectorSource extends Source {
     }
   }
 
+  removeFeatures(features) {
+    const removedFeatures = [];
+    for (let i = 0, ii = features.length; i < ii; ++i) {
+      const removedFeature = this.removeFeatureInternal(features[i]);
+      if (!removedFeature) {
+        continue;
+      }
+      removedFeatures.push(removedFeature);
+    }
+    if (removedFeatures.length > 0) {
+      this.changed();
+    }
+  }
+
   /**
    * Remove feature without firing a `change` event.
    * @param {FeatureType} feature Feature.
@@ -1113,9 +1127,11 @@ class VectorSource extends Source {
       delete this.idIndex_[id.toString()];
     }
     delete this.uidIndex_[featureKey];
-    this.dispatchEvent(
-      new VectorSourceEvent(VectorEventType.REMOVEFEATURE, feature),
-    );
+    if (this.hasListener(VectorEventType.REMOVEFEATURE)) {
+      this.dispatchEvent(
+        new VectorSourceEvent(VectorEventType.REMOVEFEATURE, feature),
+      );
+    }
     return feature;
   }

Be able to remove multiple features from vector source without triggering events.
@ger-benjamin ger-benjamin force-pushed the feature-15642-vector-source-remove-feature-silently branch from 51ae7b4 to c9de02a Compare March 18, 2024 10:25
@ger-benjamin
Copy link
Contributor Author

ger-benjamin commented Mar 18, 2024

I've done the asked changes, but in this way, I've to duplicated the whole removeFeature code into the removeFeatures method. Are you sure that's the right thing to do ?

Or I can migrate:

      if (featureKey in this.nullGeometryFeatures_) {
        delete this.nullGeometryFeatures_[featureKey];
      } else {
        if (this.featuresRtree_) {
          this.featuresRtree_.remove(feature);
        }
      }

Into removeFeatureInternal. But Maybe it's useless in some direct call ?

Edit:
(2nd commit )I chose this solution: migrate a part into the removeFeatureInternal to avoid duplicated code. I've adapted the clean part too.

@ahocevar
Copy link
Member

I have to apologize, I somehow managed to garble the diff I had posted above. I edited the post to show the correct diff.

That said, there should be no duplicated code. The change is essentially this new removeFeatures() method:

  removeFeatures(features) {
    const removedFeatures = [];
    for (let i = 0, ii = features.length; i < ii; ++i) {
      const removedFeature = this.removeFeatureInternal(features[i]);
      if (!removedFeature) {
        continue;
      }
      removedFeatures.push(removedFeature);
    }
    if (removedFeatures.length > 0) {
      this.changed();
    }
  }

And to make removeFeatureInternal() more efficient, events are only dispatched if someone listens to them:

    if (this.hasListener(VectorEventType.REMOVEFEATURE)) {
      this.dispatchEvent(
        new VectorSourceEvent(VectorEventType.REMOVEFEATURE, feature),
      );
    }

@ger-benjamin
Copy link
Contributor Author

ger-benjamin commented Mar 18, 2024

I've edited my post above.

But I can't call only edit removeFeatureInternal. Otherwise, this part about deleting nullGeometryFeatures_ and featuresRtree_.remove would be missing and break the feature

@ahocevar
Copy link
Member

Looks like you have messed up something. This is what I had in mind: ahocevar@1e14403

@ger-benjamin ger-benjamin force-pushed the feature-15642-vector-source-remove-feature-silently branch from add514f to 2eb7ba4 Compare March 18, 2024 11:09
@ger-benjamin
Copy link
Contributor Author

ger-benjamin commented Mar 18, 2024

Thanks, but again, If I only apply your changes, the removeFeatures method will miss this part:
https://github.com/openlayers/openlayers/blob/main/src/ol/source/Vector.js#L1083-L1088

And features will be only partially removed / broken.

@ahocevar
Copy link
Member

Oh, sorry again, you're right. The code you marked above can simply be moved from removeFeature() to the top of removeFeatureInternal().

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

And that's what you did. Beautiful! Thanks for the contribution.

@ahocevar ahocevar merged commit 5fe06e0 into openlayers:main Mar 18, 2024
8 checks passed
@ger-benjamin ger-benjamin deleted the feature-15642-vector-source-remove-feature-silently branch March 18, 2024 11:25
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.

2 participants