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.

Sign up for GitHub

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

Jump to bottom

Export map level scale based dependencies in most vector symbology #3436

Merged
merged 1 commit into from
Sep 1, 2016
Merged

Export map level scale based dependencies in most vector symbology #3436

merged 1 commit into from
Sep 1, 2016

Conversation

aaime
Copy link
Contributor

@aaime aaime commented Aug 26, 2016 •
edited
Loading

@aaime
Copy link
Contributor Author

aaime commented Aug 28, 2016

@m-kuhn, took a while to get the build going, but should be good for review now :-)

nyalldawson
nyalldawson reviewed Aug 29, 2016
View reviewed changes
src/core/symbology-ng/qgsrenderer.h
@@ -263,7 +263,7 @@ class CORE_EXPORT QgsFeatureRenderer
Q_DECL_DEPRECATED virtual QDomElement writeSld( QDomDocument& doc, const QgsVectorLayer &layer ) const;
//! create the SLD UserStyle element following the SLD v1.1 specs with the given name
//! @note added in 2.8
virtual QDomElement writeSld( QDomDocument& doc, const QString& styleName ) const;
virtual QDomElement writeSld( QDomDocument& doc, const QString& styleName, QgsStringMap props = QgsStringMap() ) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is an api break it will need to be documented in the doc/api_break.dox file (and also unfortunately makes the commit ineligible for backporting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed the approach a bit during the FOSS4G code sprint, because I wanted to keep this one backportable. I probably have misunderstood (limited c++ experience), what if I use another method in overload, would that make the patch backportable instead?

For the benefit of my understanding, what makes the current approach non backportable? Since the parameter has a default value, it would seem it's transparent to a source compile, but I'm guessing it's maybe not binary compatible?

Copy link
Member

Choose a reason for hiding this comment

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

It changes the API because it's virtual, so any override of it will no longer match in signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @m-kuhn. Would adding a parallel method with the extra argument be any better? I guess its default implementation would just call onto the older version. And all existing subclasses would override the new method, in order to get the scale ranges and encode them in the rules.

Copy link
Member

Choose a reason for hiding this comment

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

For the 3.0 version I like the current approach.

But for a backported version this should be ok.

@aaime
Copy link
Contributor Author

aaime commented Aug 31, 2016

Pull request updated, backwards incompatible change documented.

@m-kuhn m-kuhn merged commit 665df5f into qgis:master Sep 1, 2016
@m-kuhn
Copy link
Member

m-kuhn commented Sep 1, 2016

Thanks a lot
Would be great if you could give @raymondnijssen some backup once he's back to work

@aaime
Copy link
Contributor Author

aaime commented Sep 1, 2016

Thank you @m-kuhn. Before leaving FOSS4G me and @raymondnijssen discussed about the text symbolizer, the plan was that he was going to make a pull request with what he had (incomplete, but functional enough) and then we can work on it toghether. I also told him to contact me directly for any help regarding TextSymbolizer. So, everything already in motion :-)

@m-kuhn
Copy link
Member

m-kuhn commented Sep 1, 2016

I've seen the symbolizer in action. And a picture from Raymond on
vacation. :)

On 01/09/16 14:09, Andrea Aime wrote:

Thank you @m-kuhn https://github.com/m-kuhn. Before leaving FOSS4G
me and @raymondnijssen https://github.com/raymondnijssen discussed
about the text symbolizer, the plan was that he was going to make a
pull request with what he had (incomplete, but functional enough) and
then we can work on it toghether. I also told him to contact me
directly for any help regarding TextSymbolizer. So, everything already
in motion :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3436 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AAj6d1yw2KyD-7HW04PMkh-M_W5tbGCYks5qlsB1gaJpZM4Jt60U.

@rldhont rldhont mentioned this pull request Sep 13, 2016
Merged
rldhont added a commit to rldhont/QGIS that referenced this pull request Sep 20, 2016
@rldhont
Fix cherry-pickings
929ad8d 
Do not generate a ogc:Filter where a ogc:Expression is expected. qgis#3366
Emit valid SLD when mm are used as symbol unit qgis#3414
Export map level scale based dependencies in most vector symbology qgis#3436
rldhont added a commit to rldhont/QGIS that referenced this pull request Sep 21, 2016
@rldhont
Fix cherry-pickings
30110a7 
Do not generate a ogc:Filter where a ogc:Expression is expected. qgis#3366
Emit valid SLD when mm are used as symbol unit qgis#3414
Export map level scale based dependencies in most vector symbology qgis#3436
@rldhont rldhont mentioned this pull request Sep 30, 2016
Merged
rldhont added a commit to rldhont/QGIS that referenced this pull request Oct 1, 2016
@rldhont
Fix cherry-pickings
4aef2db 
Do not generate a ogc:Filter where a ogc:Expression is expected. qgis#3366
Emit valid SLD when mm are used as symbol unit qgis#3414
Export map level scale based dependencies in most vector symbology qgis#3436
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aaime @m-kuhn @nyalldawson