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

Fixes syncDb() that improperly sets "Imported from GDAL" as name for all GEOCCS #7009

Closed
wants to merge 1 commit into from

Conversation

agiudiceandrea
Copy link
Contributor

@agiudiceandrea agiudiceandrea commented May 17, 2018

Like for PR #6153, this commit allow syncDb() to properly set the name of each Geocentric Coordinate System ("GEOCCS") imported in srs.db from GDAL *.csv support files during postinstall, instead of wrongly setting it to "Imported from GDAL".
The bug is also in 3.0 and master.

Fixes #18968

Description

Include a few sentences describing the overall goals for this PR (pull request). If applicable also add screenshots.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

…all GEOCCS

Allow syncDb() to properly set the name of each Geocentric Coordinate System ("GEOCCS") imported in srs.db from GDAL *.csv support files during postinstall, instead of wrongly setting it to "Imported from GDAL".

Fixes #18968
Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Great catch! Could you add a unit test checking the name of one of the affected crs?

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented May 18, 2018

hi @nyalldawson
I'm sorry, but I think I'm not able to do it by myself.
Anyway, this is an epsg of geocentric cs with it's name:
EPSG:6704
name:RDN2008

@nyalldawson
Copy link
Collaborator

That's fine - I'll add one of you can advise me of some crs which are fixed by this issue?

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented May 19, 2018

Thanks @nyalldawson
This is a list of all 163 GEOCCS that will be imported in srs.db, if not already present in it, with their proper name instead of "Imported from GDAL".
Since syncDb does not update the names of the crs already present in srs.db (it updates only ther proj parameters), we need to upload a fixed version of srs.db for both 2.18 a 3 to fix the names of the crs already present in it.
In fact there are 114 GEOCCS in srs.db shipped with QGIS 2.18, so syncDb will only import 49 GEOCCS (including the one already reported in the previous message) with the right name. While in srs.db shipped with QGIS 3 already contains all the 163 GEOCCS without a proper name.
I'll post a specific message on QGIS - Developer addressing all the fixing I think would be useful to do to srs.db after the fix of bugs: 17941 (fixed for 2.18 and 3), 18896 (fixed for 3), 18905 (not yet fixed), 18968 (this PR for 2.18), 18969 (PR #7011 for 2.18).

@jef-n
Copy link
Member

jef-n commented May 19, 2018

7576ae1 also updates the descriptions. With GDAL 2.3 this leaves no rows with 'Imported from GDAL', but 72 with GDAL 2.2. So a test probably must be dependant on the GDAL versions.

@agiudiceandrea
Copy link
Contributor Author

It's great to also update the description, so it shouldn't be necessary to "manually" fix srs.db!
I wonder why the name of these 72 crs are not updated / imported properly.

@jef-n
Copy link
Member

jef-n commented May 19, 2018

@agiudiceandrea Because they also didn't exist in GDAL 2.2

@nyalldawson
Copy link
Collaborator

@agiudiceandrea can this (and #7011 ) be safely closed now?

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented May 22, 2018

@jef-n I've just tested QGIS 3.1.0-21 weekly snapshot (310bfb1) (with GDAL 2.2.4) on Windows and it seems all ok for me:
in tbl_srs there are now 6567 crs (included the 21 crs, 1 from esri_extra.wkt and 20 from cubewerx_extra.wkt, that were previously missing) and all the 285 crs (122 geographic and 163 geocentric) that before had description="Imported from GDAL" now have their proper name.

The 72 crs you previously mentioned (I see them in srs-template.db in the last qgis-providers-common deb package) are all in esri_extra.wkt, that seems missing in gdal-data deb package on Linux: so now they have a proper name on Windows but non on Linux.

@jef-n
Copy link
Member

jef-n commented May 22, 2018

@agiudiceandrea The license of the wkt files were clarified and will be included in the debian packages of 2.3.

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

3 participants