Conversation
kannes
left a comment
kannes
left a comment
There was a problem hiding this comment.
Oh wow, thank you so much!
I only had the chance for a very brief look, no testing and no overall review on structure etc. yet. Sorry! I hope we will find more time in the coming weeks.
Still I already saw some functional issues that would definitely need fixing, so I decided to submit this quick review :)
helpers.py
Outdated
| def getEntityTypeFromSensorThingsLayer(layer: QgsVectorLayer): | ||
| decoded_url = QgsProviderRegistry.instance().decodeUri(layer.providerType(), layer.dataProvider().dataSourceUri()) | ||
| return decoded_url.get('entity') |
There was a problem hiding this comment.
Maybe you could directly access layer.dataProvider().uri().param('typename') instead? I am not sure if that would be the same information. :)
There was a problem hiding this comment.
You are right, this is much shorter:
layer.dataProvider().uri().param('entity')
| source_crs = QgsCoordinateReferenceSystem(source_crs_epsg) | ||
| target_crs = QgsCoordinateReferenceSystem(target_crs_epsg) |
There was a problem hiding this comment.
Maybe have a quick if source_crs == target_crs: return wkt here. Probably not really that much of a performance improvement overall, but wouldn't hurt :D
There was a problem hiding this comment.
You are right, this is more elegant 👍
controller.py
Outdated
| self.mapTool.deactivate() | ||
|
|
||
| def onSketchFinished(self, geometry: QgsGeometry): | ||
| geometry.convertToSingleType() # SensorThings Filter only supports single geometry type |
There was a problem hiding this comment.
Other providers are fine with multi-geometries and iirc it works fine if one selects an existing multipolygon as filter geometry. Being able to use multi-geometries is also a wish from the community and something that will probably be added some day.
Any provider-specific limitations should be handled only for that provider, and trigger a warning visible to the user.
There was a problem hiding this comment.
Ok, than I will convert it to single Geometry in a later step. For example here:
filters.py -> filterString()
if layer.storageType() == SENSORTHINGS_STORAGE_TYPE:
### convert wkt back to QgsGeometry
### convert to singleType
filters.py
Outdated
| spatial_predicate = spatial_predicate.lower() # sensorthings specification uses lower case | ||
| entity_str = getEntityTypeFromSensorThingsLayer(layer) | ||
| entity_type = QgsSensorThingsUtils.stringToEntity(entity_str) | ||
| geom_field = QgsSensorThingsUtils.geometryFieldForEntityType(entity_type) |
There was a problem hiding this comment.
I'd prefer the existing getLayerGeomName method to be extended if possible.
There was a problem hiding this comment.
I could add a condition in getLayerGeomName like this, what do you think?:
def getLayerGeomName(layer: QgsVectorLayer):
if layer.storageType() == SENSORTHINGS_STORAGE_TYPE:
entity_str = layer.dataProvider().uri().param('entity')
entity_type = QgsSensorThingsUtils.stringToEntity(entity_str)
geom_field = QgsSensorThingsUtils.geometryFieldForEntityType(entity_type)
return geom_field
return layer.dataProvider().uri().geometryColumn() or getLayerGeomNameOgr(layer)
filters.py
Outdated
| layer_srid=layer.crs().postgisSrid() | ||
|
|
||
| if layer.storageType() == SENSORTHINGS_STORAGE_TYPE: | ||
| reprojected_wkt = reproject_wkt_geometry(wkt, srid, layer_srid) |
There was a problem hiding this comment.
A comment why client-side reprojection is necessary (no ST_Transform available for SensorThings I presume?) would be nice.
There was a problem hiding this comment.
To explain this I added a comment on line 41, where i define the filter template. Of course, i can add it here, too :)
# sensorthings filter does not support reprojection (st_transform)
# reprojection happens in helpers.py -> addFilterToLayer
FILTERSTRING_TEMPLATE_SENSORTHINGS = "{spatial_predicate}({geom_name}, geography'{wkt}')"
helpers.py
Outdated
| newFilter = currentFilter[:start_index] + currentFilter[stop_index:] | ||
| layer.setSubsetString(newFilter) | ||
| if layer.storageType() == SENSORTHINGS_STORAGE_TYPE: | ||
| layer.setSubsetString('') # sensorthings filter does not support comments (FILTER_COMMENT_START) |
There was a problem hiding this comment.
Would it be possible to use a different magic string for SensorThings, maybe some string comparison that always evaluates true and gets added with AND? Something like
'{FILTER_COMMENT_START}' == '{FILTER_COMMENT_START}' AND ... filter ... AND '{FILTER_COMMENT_STOP}' == '{FILTER_COMMENT_STOP}'
Otherwise it would not be possible to support additional filters set by the user, right?
There was a problem hiding this comment.
That seems like a nice work-around. Good idea :)
Unfortunaly comments are not allowed
… type for sensorthings filter
|
I tried to adress all your requested changes. I tested it with QGIS 3.40 and it seemed to work. |
|
I am so sorry for still not having worked on this. If there is a external pressure for you to get this merged, please shout and I will try to squeeze it in. Otherwise it will probably be a little longer :] |
|
Dont worry, take your time! |
2 participants
Adressing #29
Tested with USGS Waterdata Sensorthings sample data: https://labs.waterdata.usgs.gov/sta/v1.1/
Testdata
Spatialfilter with Intersection spatial predicate
Spatialfilter with Disjoint spatial predicate