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

[symbology] respect mixed unit when applying symbol from list widget #3792

Merged
merged 1 commit into from
Nov 22, 2016

Conversation

nirvn
Copy link
Contributor

@nirvn nirvn commented Nov 21, 2016

@nyalldawson , while improving the style manager, I stumbled on this irritating bug whereas a saved symbol containing mixed unit types (say a market with a map unit size and a millimeter outline) would fail to apply properly when selecting the saved symbol using the symbols list widget (i.e. our main way of serving symbols).

This PR fixes the situation by not calling setOutputUnit() when the unit type identifies a mixed-unit scenario.

@nyalldawson
Copy link
Collaborator

What's the even there for? Is it trying to respect the current size unit setting?

@nirvn
Copy link
Contributor Author

nirvn commented Nov 21, 2016

@nyalldawson , funny you'd ask, I was wondering the same :) I didn't feel confident enough to simply assume we can get rid of it. That said, if you think it should go altogether, we can go down that road.

@nyalldawson
Copy link
Collaborator

Well.. it seems dangerous to me. A symbol designed with sizes in mm and then suddenly set to map units will almost always result in sizes too big or too little.

@nirvn
Copy link
Contributor Author

nirvn commented Nov 21, 2016

@nyalldawson , that's indeed what was happening here (the other way around, a 500 map unit (meter) transformed into a 500mm).

Basically, as far as I understand, the units are already set up when layers are created and appended to the main symbol. The setOutputType() seems redundant.

@nirvn
Copy link
Contributor Author

nirvn commented Nov 21, 2016

Actually, the call was added by @wonder-sk in this commit: 8b1f537

edit: fixing a bug you reported: http://hub.qgis.org/issues/10595 😄 --

@nirvn
Copy link
Contributor Author

nirvn commented Nov 21, 2016

@nyalldawson , I'm wondering whether other underlying changes has made setOutputType() irrelevant. I've tried to remove the call altogether, and it very much looks like symbol loading is fine. Switching from mix units to mm-only to map unit-only works flawlessly.

@wonder-sk
Copy link
Member

If the original bug is cannot be reproduced without setting unit type, let's remove it. Looking at the code again, I can't see a reason why the unit type should be explicitly set...

@nirvn
Copy link
Contributor Author

nirvn commented Nov 22, 2016

@wonder-sk , commit updated to get rid of the setOutputUnit() call. Cheers.

@wonder-sk wonder-sk merged commit 0c58555 into qgis:master Nov 22, 2016
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.

None yet

3 participants