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

Fixed: MultiPolygons in WFS Services only loaded the First Polygon #2633

Merged
merged 15 commits into from
Jun 3, 2024

Conversation

inforithmics
Copy link
Contributor

  1. Added a sample
  2. moved wfs Test samples to the Wfs Category
  3. Fixed Bug without the Bugfix only one area is visible with the bugfix both areas are visible

Copy link
Member

@pauldendulk pauldendulk 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 fixing the bug. I request to move the filter provider to the samples.


public class GeometryFilterProvider(IProvider provider, Func<IFeature, bool> filter) : IProvider, IProviderExtended
{
private DateTime _lastUpdate;
Copy link
Member

Choose a reason for hiding this comment

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

I have some reservations wrt the GeometryFilterProvider. Perhaps it could be added as part of the samples (Perhaps a nested class in the sample class) so that we can continue with this PR.

In general I think we should support less and support it better. If we add the GeometryFilterProvider we would also need to explain users how it works, that it caches things for a 20 seconds. We will probably get request to make it configurable. If we decide to change it we would introduce a breaking change. All reasons to not add non essential things like this.

Some questions and remarks:

  • Could you explain a bit more why this was added? What problem were you working on that made you add it?
  • This introduces caching. Caching often causes problems which are hard to track. We should try to reduce the number of places where we use caching and it should be completely clear to us and easy to explain to our users on what levels we cache things and it should be easy to see how many items are in the cache.
  • A separate caching provider could also be an option, so that it could be stacked on the filtering provider. The advantage is that the user chooses to add it or not, understands it better and is responsible for it. If we add this it should also be first as sample.
  • WFS itself also supports filtering. The advantage of that is that it reduces the number of features fetched from the server which may be a more useful kind of filtering because the http request will cause the biggest load. Perhaps this is the thing that users would prefer to use. Not easy to add though, and I think this is not something we should work on, but it is good to have in mind when working on a IProvider filter. See OGC filters and ECQL filters in Geoserver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced it for debugging purposes. The Ogc or ecql filters I didn’t know. So I move it to the sample or use an WFs feature

Copy link
Member

Choose a reason for hiding this comment

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

Just move it to the samples. I prefer to put all the classes needed for a sample in the main sample file, otherwise users can not get it to work without downloading the entire source code. If you have a simpler way (less code) to demonstrate the filtering that is preferred.

I don't think the WFS filtering is supported in Mapsui, adding it will be a big effort. The WFS code is not great, so don't think you should go for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the GeometryFilterProvider to the sample for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that there is an Ogc Filter Property in the WfsProvider so, maybe I will do a sample that uses this.

@@ -14,7 +14,7 @@ namespace Mapsui.Samples.Common.Maps.DataFormats;
public class Wfs2_0Sample : ISample
{
public string Name => "WFS 2.0";
public string Category => "Tests";
public string Category => "WFS";
Copy link
Member

Choose a reason for hiding this comment

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

Good to have a WFS category

Copy link
Member

@pauldendulk pauldendulk left a comment

Choose a reason for hiding this comment

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

Great, thanks

@pauldendulk pauldendulk merged commit cd341b4 into Mapsui:main Jun 3, 2024
5 checks passed
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.

None yet

2 participants