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

Preserve 0/NULL values from postgres int columns #2207

Closed
wants to merge 3 commits into from

Conversation

m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented Jul 10, 2015

Actually, the headline only says half of it. I think this patch is fine to apply but it does not solve the underlying problem.

It seems that the check on this line here fails to properly distinguish 0 from NULL in the attributes
https://github.com/qgis/QGIS/blob/master/src/core/qgsfeature.cpp#L97

We could as well remove the check and have some unnecessary detaches happen once in a while (not sure how often or bad that would be).

The underlying problem is that IIRC Qt returns true for QVariant(int) == QVariant(0) a thing that should be solved upstream. We could maybe implement our own helper method to compare QVariants and have it take care of such things.

@jef-n assigning you since you are the postgres provider wizard.

Thoughts?

@nyalldawson
Copy link
Collaborator

@m-kuhn should we also add an equality operator to QgsAttributes which can work around this upstream issue?

@nyalldawson
Copy link
Collaborator

BTW, thanks for looking into this. I'd also encountered this earlier in the week and was totally confused by what was happening...

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 11, 2015

@nyalldawson like so?

@nyalldawson
Copy link
Collaborator

@m-kuhn looks good to me... I'm curious though, why go through the attributes backwards?

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 11, 2015

@nyalldawson because the Qt QVector implementation does it that way :)

@nyalldawson
Copy link
Collaborator

@m-kuhn OK fair enough... Thought there may have been some complex c++ reason behind it :p the test + attribute changes should be safe to merge now, right?

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 11, 2015

still struggling with sip...

@m-kuhn m-kuhn force-pushed the pgfix branch 4 times, most recently from ca6b939 to 69be0c1 Compare July 12, 2015 07:42
@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 13, 2015

Merged in 156a0e9..c6f7873

@m-kuhn
Copy link
Member Author

m-kuhn commented Jul 13, 2015

Backported to

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.

3 participants