-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Nominatim Geocoder Locator filter - add countryCodes configuration option #60719
base: master
Are you sure you want to change the base?
Conversation
Backport also to 3.42 |
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
|
||
// Load existing settings | ||
QgsSettings settings; | ||
countryCodesEdit->setText( settings.value( "locator_filters/nominatim_geocoder/country_codes", "", QgsSettings::App ).toString() ); |
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.
Maybe use new settings tree API here and in other places?
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.
Don't know, I used the code from other Locator filters as example. Like this
QgsSettings settings; |
I can check this, but not sure what will be better. Is approach I used deprecated?
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.
It is not deprecated, but IMHO for new code we should use new API. Also with this API new setting will be nicely exposed in the advanced settings editor.
// Check if we have QgsNominatimLocatorFilter instance | ||
QgsNominatimGeocoder *nominatimGeocoder = dynamic_cast<QgsNominatimGeocoder *>( mGeocoder ); | ||
if ( nominatimGeocoder ) | ||
{ | ||
// Read countryCodes from settings | ||
QgsSettings settings; | ||
QString countryCodes = settings.value( "locator_filters/nominatim_geocoder/country_codes", "", QgsSettings::App ).toString().trimmed(); | ||
|
||
nominatimGeocoder->setCountryCodes( countryCodes ); | ||
} |
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.
Instead of the dynamic_cast here, it would be cleaner to override fetchResults in QgsNominatimLocatorFilter and set the country code there, and then call this base class QgsAbstractGeocoderLocatorFilter::fetchResults method
Co-authored-by: Nyall Dawson <[email protected]>
Co-authored-by: Nyall Dawson <[email protected]>
This PR adds Configuration option to Nominatim Geocoder Locator filter for filtering results to one or more countries. (Two letter country codes, comma separated). Configuration value is optional, if left blank it will work as now.
Nominatim Geocoder already supports countryCodes parameter so only GUI part and linking setting to request was missing.
Test also supports this parameter so no new tests are created for this.
This change does not effect Batch Nominatim Geocoder processing tool.
Please backport to 3.40
Needs documentation