-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
Conversation
inforithmics
commented
May 14, 2024
- Added a sample
- moved wfs Test samples to the Wfs Category
- Fixed Bug without the Bugfix only one area is visible with the bugfix both areas are visible
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks