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

Remove deprecated Qgis::WKBType and API cleanup #3325

Merged
merged 13 commits into from Aug 4, 2016

Conversation

m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented Jul 22, 2016

Renames QgsWKBTypes to QgsWkbTypes

Replaces usage of the enums:

  • Qgis::WKBType with QgsWkbTypes::Type
  • Qgis::GeometryType with QgsWkbTypes::GeometryType

Their values should be forward compatible (a fact that was already
explited up to now by casting between the types)

Renames some SSLxxx to SslXxx and URIxxx to UriXxx

@m-kuhn m-kuhn added For Review Requires Changes! Waiting on the submitter to make requested changes API Break! Breaks stable API. Proceed with extreme caution!! labels Jul 22, 2016
@m-kuhn m-kuhn added this to the QGIS 3 milestone Jul 22, 2016
@nyalldawson
Copy link
Collaborator

Nice! I'm guessing we'll need follow ups here - ie I suspect that layers will not be correctly returning Z/M types until we update the providers to do this?

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 22, 2016

Yes, we'll need follow ups I guess.
But I think that postgis has already been returning them by just casting to QGis::WKBType and some parts which were aware of that just casted it to the newer equivalent.

@nyalldawson
Copy link
Collaborator

My only thoughts - in #3314 it's discussed whether we should move global enums back into the Qgis/QGis/QGIS/qgis/QuantumGIS class, and make it an analog of Qt's "Qt". What do you think?

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 22, 2016

whether we should move global enums back into the Qgis/QGis/QGIS/qgis/QuantumGIS class

No strong opinion. Qgis.Point is shorter to write than QgsWkbTypes.Point, so that sounds good. Also one less import to do.

One thing is that QGis.Point was for Geometry type (the simple enum with 5 values) whereas the new QgsWkbTypes.Point is for the complex wkb type and the equivalent for the former QGis.Point is QgsWkbTypes.PointGeometry. I wonder if we can somehow prevent this confusion...

@jef-n
Copy link
Member

jef-n commented Jul 22, 2016

What's the rationale behind collecting all the enums in one header? Doesn't that just cause full rebuilds when a enum changes and separates the enum from where they are used? What are the advantages?

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 22, 2016

@nyalldawson during rebase I also did some more QGis => Qgis fixes.

I think there are still some removed methods used in the oracle provider...

@nyalldawson
Copy link
Collaborator

@jef-n My (incomplete) understanding:

Advantages:

  • it's a nicer api, since it avoids the guess work of knowing where a enum sits
  • it mirrors Qt's design/structure
  • ??

Disadvantages:

  • slow recompilation when things change
  • mixes everything together
  • ??

I was tending towards grouping the common ones, but the more I think about it I think I'm more in favour of keeping them in separate classes. But I'm happy to go either way...

m-kuhn referenced this pull request Jul 22, 2016
git grep -l "QGis::" src/ | xargs perl -pe "s/QGis::/Qgis::/g" -i.bak
@m-kuhn m-kuhn force-pushed the Qgis3GeomUpdates branch 2 times, most recently from e7268f3 to b6686b8 Compare July 22, 2016 16:03
@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 22, 2016

I started to write a fixer for module renames. It's by no means finished, it was probably quite naive to start that on the day before leaving office for a week ;)

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 22, 2016

Hmmm... I am failing big time with these fixers and there is not much documentation to be found. How did you get that signal magic done, @jef-n ?

@nyalldawson
Copy link
Collaborator

What's the rationale behind collecting all the enums in one header? Doesn't that just cause full rebuilds when a enum changes and separates the enum from where they are used? What are the advantages

@jef-n @wonder-sk @m-kuhn can we make a call either way here? Collect enums or not?

@wonder-sk
Copy link
Member

I do not not have a strong preference towards any of those options. Happy with whatever others choose.

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 31, 2016

I do not not have a strong preference towards any of those options. Happy with whatever others choose.

Same here.

@nyalldawson
Copy link
Collaborator

@m-kuhn if it's not too much to ask, can you hold off on rebasing this for now? I'm mid-way through a nightmare mega-patch which changes the getters/setters for feature geometry to just QgsGeometry QgsFeature::geometry() const and void QgsFeature::setGeometry( const QgsGeometry& geometry ), and i absolutely dread the thought of merge conflicts with this ;)

@nyalldawson
Copy link
Collaborator

@m-kuhn ok, all done here... I'm moving on to other code areas.

@m-kuhn m-kuhn force-pushed the Qgis3GeomUpdates branch 4 times, most recently from a5c900a to 2468394 Compare August 3, 2016 10:25
Renames QgsWKBTypes to QgsWkbTypes

Replaces usage of the enums:

* Qgis::WKBType with QgsWkbTypes::Type
* Qgis::GeometryType with QgsWkbTypes::GeometryType

Their values should be forward compatible (a fact that was already
explited up to now by casting between the types)

Renames some SSLxxx to SslXxx and URIxxx to UriXxx
@m-kuhn m-kuhn force-pushed the Qgis3GeomUpdates branch 3 times, most recently from daa7e17 to 2a357c2 Compare August 3, 2016 14:25
@m-kuhn m-kuhn force-pushed the Qgis3GeomUpdates branch 2 times, most recently from 7da4d46 to 54123a0 Compare August 3, 2016 22:22
@m-kuhn m-kuhn merged commit bb79d13 into qgis:master Aug 4, 2016
@nyalldawson
Copy link
Collaborator

Woohoo!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Break! Breaks stable API. Proceed with extreme caution!! Requires Changes! Waiting on the submitter to make requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants