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

Tests and fix to read sld:Rotation when does not have ogc sub tags #4049

Merged
merged 4 commits into from
Feb 16, 2017

Conversation

luipir
Copy link
Contributor

@luipir luipir commented Jan 24, 2017

Added fix to read correctly sld:Rotation tags from SLD

@luipir
Copy link
Contributor Author

luipir commented Jan 24, 2017

backport to 2.18: #4050
backport to 2.14: #4051

@luipir
Copy link
Contributor Author

luipir commented Jan 24, 2017

@jgrocha @aaime just a little contribution to read back sld generated by qgis

test_qgssymbollayer_readsld.py
=======
test_qgssymbollayerv2_readsld.py
>>>>>>> Tests and fix to read sld:Rotation when does not have ogc sub tags
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge conflict!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gulp! tnx

@jgrocha
Copy link
Member

jgrocha commented Jan 24, 2017

Nice work @luipir
I was able to write a style as SLD and read it back preserving the rotation (just checked out this pr/4049).
Tests are ok, and the provided SLD files for testing are well formed and valid.
I'll do the next improvement regarding SLD interoperability ;-) We have a lot to do!

@jgrocha
Copy link
Member

jgrocha commented Jan 24, 2017

If you have 1 minute more, there are two minor and irrelevant typos.
typos.txt

if ( elem.hasChildNodes() &&
elem.firstChild().nodeType() == QDomNode::TextNode )
{
function = elem.firstChild().nodeValue();
Copy link
Member

@m-kuhn m-kuhn Feb 3, 2017

Choose a reason for hiding this comment

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

Doesn't this need to be sent through QgsOgcUtils::expressionFromOgcFilter( ... );?

Copy link
Member

Choose a reason for hiding this comment

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

Or possibly this logic should be moved into expressionFromOgcFilter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense... I supposed the expressionFromOgcFilter would manage the parsing of an aexpression logic that is more complex. I'll give a check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right... is simple and possibile... I'll do it! tnx @m-kuhn to point it out.
In this moment assume that the expression build does not consider a literal constant othan expressed with ogc:Literal

@luipir
Copy link
Contributor Author

luipir commented Feb 10, 2017

@m-kuhn done! I wasn't sure if it was the correct way to proceed. The OGC filter does not consider the possibility to have operators outside ogc tag. Btw I implemented as requested because IMHO there is no a correct semantic solution.

@luipir
Copy link
Contributor Author

luipir commented Feb 16, 2017

@m-kuhn @sbrunner any problem to approve this PR?

@m-kuhn m-kuhn merged commit f46b25b into qgis:master Feb 16, 2017
@m-kuhn
Copy link
Member

m-kuhn commented Feb 16, 2017

Thanks @luipir and @sbrunner

@luipir
Copy link
Contributor Author

luipir commented Feb 16, 2017

thank you... there are the relative backports

backport to 2.18: #4050
backport to 2.14: #4051

@luipir
Copy link
Contributor Author

luipir commented Feb 16, 2017

just a note... code for 2.18/2.14 is slightly different. due to different api on QgsExpression where there is not setExpression method introduced in Master

@luipir luipir deleted the master-SldRotationFix branch February 17, 2017 08:19
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.

4 participants