Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Update to custom CRS management #315

Merged
merged 4 commits into from May 12, 2013

Conversation

Projects
None yet
3 participants
Contributor

leyan commented Nov 4, 2012

Hello developers,

I work a lot with custom CRS (not by choice !), and the current interface is quite cumbersome to use: the CRS can only be seen one by one, the saving behavior is quite unintuitive, parameters from an existing CRS cannot be seen easily, etc.

I tried to improve the UI and to update the code to use the QgsCoordinateReferenceSystem class. This reduces the code size and duplication, should improve the CRS validity check and later easily allow inputs other than proj4.

One issueI ran into is that QgsCoordinateReferenceSystem is not very fit to work with CRS not yet saved. The createFromProj4() function will automatically save a CRS if it is not found in the database, behavior that I had to slightly change by adding an optional argument to the function.

Currently, I minimized the changes to QgsCoordinateReferenceSystem. If more changes are allowed, I think it would be cleaner to separate the creation and saving to database: essentially make a function saveAsUserCRS(QString name) publicly available. The name could be used to convey more information than the current "Generated ..." default message.

Any comment ? Criticism ? On the code, the UI, the coding style, the pull request process ?

@ghost ghost assigned timlinux Nov 4, 2012

Contributor

mhugent commented Nov 8, 2012

Hi

I agree it would be good to separate query and creation (otherwise, some unexpected behaviour might occure). It is a good idea to change QgsCoordinateReferenceSystem a bit more as you suggested. Looking forward to a follow-up patch.

Contributor

leyan commented Nov 18, 2012

Is the follow-up patch needed to apply this one ?

Contributor

mhugent commented Nov 20, 2012

Yes, I prefer to apply all the changes in one go.

Contributor

leyan commented Apr 9, 2013

Sorry, I was busy with real life and away from Qgis for some time, then I completely forgot about this pull request.

The proposed patch changes the behavior of createFromProj4 so that it does not automatically save as user CRS if the CRS does not exist already. It also makes a saveAsUserCRS function public. This amounts to a change in the API.

If I make a smaller patch with only this API change, would it be possible to insert it into the API cleanup for 2.0? I will then add the actual interface changes and new features for 2.1.

Contributor

mhugent commented Apr 13, 2013

Yes, it is still possible to change the API. It might be good if you provide a patch to change the API (as the API cannot easily be changed after 2.0).

@leyan leyan referenced this pull request Apr 16, 2013

Merged

Custom crs api #533

Contributor

leyan commented Apr 16, 2013

I sent the pull request for the API change. I will update the pull request for the more complete feature when 2.0 has been released.

Sorry again for the bad timing.

Owner

timlinux commented May 1, 2013

Hi @leyan

I would like to merge this before 2.0 - could you take a look at the merge conflicts introduced by merging #533 ?

Thanks

Tim

Contributor

leyan commented May 2, 2013

Hi @timlinux

#533 seems to be merged successfully, so I am not sure what you mean. Is there still a problem with #533 or do you mean you want to merge the whole #315 ? As it introduces new features and changes UI, I thought it would be too late for 2.0.

Owner

timlinux commented May 2, 2013

Hi
On 02 May 2013 6:23 AM, "leyan" notifications@github.com wrote:

Hi @timlinux

#533 seems to be merged successfully, so I am not wat you mean. Is there
still a problem with #533 or do you mean you want to merge the whole #315 ?
As it introduces new features and changes UI, I thought it would be too
late for 2.0.

The feature freeze does not apply to preexisting pull requests so I would
like to apply any valid pull requests before we release.

Regards

Tim


Reply to this email directly or view it on GitHub.

Contributor

leyan commented May 3, 2013

No problem, I will update it and clean it up this weekend.

Contributor

leyan commented May 5, 2013

Hi timlinux,

Here is an updated version.

Also, I think it would be useful to have a way in the QgsCoordinateReferenceSystem class to update a CRS. Currently, the only way to write in the database is with saveAsUserCRS, which will always insert a new CRS. I can update it to overwrite an existing user CRS if the srsid is already existing, or we can add another function updateCRS().

Because of this, I still write directly to the database when saving a user CRS, this should be in QgsCoordinateReferenceSystem.

I still feel like I am trying to make QgsCoordinateReferenceSystem work differently from the way it was intended to be used. I was looking for a simple class allowing to use CRS (dealing with projection, datum, checking validity, converting between representations like proj4, WKT, etc.) while QgsCoordinateReferenceSystem is more a way to access the CRS existing in the database (using parameters lookup to link a newly created CRS to an existing one, not allowing to modify a CRS once it is created, etc.).

@timlinux timlinux merged commit 6e3e89b into qgis:master May 12, 2013

Owner

timlinux commented May 12, 2013

Hi

Thanks very much for this @leyan !

Regards

Tim

@leyan leyan deleted the leyan:customCRS branch Sep 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment