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

[FEATURE][needs-docs] Svg marker size: change aspect ratio #4912

Merged
merged 6 commits into from
Oct 4, 2017

Conversation

rldhont
Copy link
Contributor

@rldhont rldhont commented Jul 24, 2017

Description

Qt gives us the ability to display SVG marker with a different aspect ratio than the one defined in the SVG file.
This PR updates the way to display SVG marker and gives the ability to user to update the aspect ratio.

This PR has been funded by Ville d'Avignon

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@rldhont rldhont added this to the QGIS 3 milestone Jul 24, 2017
Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Cool feature! Code looks good - just one concern I've noted above.

@@ -2041,13 +2068,13 @@ void QgsSvgMarkerSymbolLayer::renderPoint( QPointF point, QgsSymbolRenderContext
double QgsSvgMarkerSymbolLayer::calculateSize( QgsSymbolRenderContext &context, bool &hasDataDefinedSize ) const
{
double scaledSize = mSize;
hasDataDefinedSize = mDataDefinedProperties.isActive( QgsSymbolLayer::PropertySize );
hasDataDefinedSize = mDataDefinedProperties.isActive( QgsSymbolLayer::PropertyWidth );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is going to break data defined marker sizes (e.g. setting a data defined size for the whole symbol). What's the motivation behind changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I had to do like that.

@@ -1697,13 +1697,16 @@ QgsSvgMarkerSymbolLayer::QgsSvgMarkerSymbolLayer( const QString &path, double si
{
mPath = path;
mSize = size;
mDefaultAspectRatio = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you initialize these in the header instead of here (c++11 style)

@nyalldawson nyalldawson added the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Jul 24, 2017
Copy link
Contributor

@nirvn nirvn left a comment

Choose a reason for hiding this comment

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

@rldhont , nice feature, thanks for working this out.

There's a problem with the .ui file, whereas the minimum with of the svg marker's configuration panel has dramatically expanded.

PR looks good to me after the above .ui issue is fixed.

@rldhont
Copy link
Contributor Author

rldhont commented Jul 25, 2017

@nirvn I have updated the ui, it is the right way ? If not, which line can I update ?

@rldhont
Copy link
Contributor Author

rldhont commented Jul 25, 2017

@nyalldawson thanks for your review. Is my update right ?

@@ -180,7 +180,7 @@
"QgsRangeConfigDlg": ["QgsRangeConfigDlg(QgsVectorLayer *vl, int fieldIdx, QWidget *parent)", "rangeWidgetChanged(int index)"],
"QgsMapMouseEvent": ["SnappingMode"],
"QgsScaleExpression": ["Type", "operator bool() const "],
"QgsSvgMarkerSymbolLayer": ["strokeWidthMapUnitScale() const ", "path() const ", "setStrokeWidthMapUnitScale(const QgsMapUnitScale &scale)", "setStrokeWidth(double w)", "strokeWidthUnit() const ", "QgsSvgMarkerSymbolLayer(const QString &name=DEFAULT_SVGMARKER_NAME, double size=DEFAULT_SVGMARKER_SIZE, double angle=DEFAULT_SVGMARKER_ANGLE, QgsSymbol::ScaleMethod scaleMethod=DEFAULT_SCALE_METHOD)", "create(const QgsStringMap &properties=QgsStringMap())", "setStrokeWidthUnit(QgsSymbol::OutputUnit unit)", "setPath(const QString &path)", "createFromSld(QDomElement &element)", "strokeWidth() const "],
"QgsSvgMarkerSymbolLayer": ["strokeWidthMapUnitScale() const ", "path() const ", "setStrokeWidthMapUnitScale(const QgsMapUnitScale &scale)", "setStrokeWidth(double w)", "strokeWidthUnit() const ", "QgsSvgMarkerSymbolLayer(const QString &name=DEFAULT_SVGMARKER_NAME, double size=DEFAULT_SVGMARKER_SIZE, double angle=DEFAULT_SVGMARKER_ANGLE, QgsSymbol::ScaleMethod scaleMethod=DEFAULT_SCALE_METHOD)", "create(const QgsStringMap &properties=QgsStringMap())", "setStrokeWidthUnit(QgsSymbol::OutputUnit unit)", "setPath(const QString &path)", "createFromSld(QDomElement &element)", "strokeWidth() const ", "fixedAspectRatio() const", "setFixedAspectRatio(double far)", "updateDefaultAspectRatio()", "defaultAspectRatio() const"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

HEY! That's not how the doc test works... add documentation to the newly added members please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But no method has been documented here...

mSvgMarkerLayer->setFixedAspectRatio( 0.5 );
mSvgMarkerLayer->setStrokeWidth( 0.5 );

bool result = imageCheck( QStringLiteral( "svgmarker_dynamicsize_aspectratio" ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This reference image looks fragile due to the ~1 pixel high areas. I'd suggest:

  • remove the outline from the symbol, that'll help avoid antialiasing/blending of fill/outline platform differences
  • boost the minimum size here so that the smallest symbol is at least a couple of pixels thick in its center

mSvgMarkerLayer->setDataDefinedProperty( QgsSymbolLayer::PropertyWidth, QgsProperty::fromExpression( QStringLiteral( "min(\"importance\" * 2, 6)" ) ) );

mMapSettings.setFlag( QgsMapSettings::DrawSymbolBounds, true );
bool result = imageCheck( QStringLiteral( "svgmarker_bounds" ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's happening here? is this supposed to be reusing the existing reference image? I'd have expected it to look different given that it's only the width which is varying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case the aspect ratio has not been explicitly changed with setFixedAspectRatio or with the PreserveAspectRatio data defined property, so the Width data defined property gives the same has the Size data defined property.
I think I have to add some tests.

@nirvn
Copy link
Contributor

nirvn commented Jul 26, 2017

@rldhont , the width issue is still present:
screenshot from 2017-07-26 10-15-09

@rldhont
Copy link
Contributor Author

rldhont commented Jul 26, 2017

@nirvn I apology but I don't understand the issue ?
I didn't see in the diff of .ui file where I have modified the minimum width of the svg marker's configuration panel. How do you want I fix it ?

@nirvn
Copy link
Contributor

nirvn commented Jul 26, 2017

@rldhont , try a master vs. your PR build, and notice how the minimum width (before a scrollbar appears) of the svg marker panel in your PR is much wider than on master.

It's not clear to me which change in the .ui file led to that.

@rldhont
Copy link
Contributor Author

rldhont commented Jul 26, 2017

@nirvn it is not just because this PR adds 2 new inputs and the panel it self is not scrollable ?

@nirvn
Copy link
Contributor

nirvn commented Jul 26, 2017

@rldhont , problem has to do with the [x] preserve aspect ratio checkbox. The minimum width goes back to normal when I reduce the string to "P..." (test only, to dissect the issue):
screenshot from 2017-07-26 16-11-43

@rldhont
Copy link
Contributor Author

rldhont commented Jul 26, 2017

@nirvn, thanks ! I'll update the .ui to fix it

@nyalldawson
Copy link
Collaborator

@nirvn seems like a perfect place for your new ratio lock widget?

@rldhont
Copy link
Contributor Author

rldhont commented Jul 28, 2017

@nyalldawson I have updated documentations and tests.
@nirvn I have updated the ui

@rldhont
Copy link
Contributor Author

rldhont commented Jul 29, 2017

Do I rebase to reorganize commits ?

@daydinodakent
Copy link

daydinodakent commented Jul 31, 2017

@nyalldawson
I follow nightly edition of QGIS 2.99
There are three problems in showing point features as SVG file.
1- If you try to show point features as SVG marker and give any "Rotation Angle"
2- Make SVG marker settings in "MapUnit"/"Inches"/"Meters" (more than 1000 point symbols), [in "Milimeter"/"Pixels"/"Points" units there is no problem]
3- If you zoom in more than 1:25 to SVG symbol [in "Milimeter"/"Pixels/"Points" units there is no problem]
then QGIS stop working and close.

@rldhont rldhont force-pushed the svg-marker-size branch 2 times, most recently from 36a786f to 87febcf Compare August 7, 2017 16:11
@rldhont
Copy link
Contributor Author

rldhont commented Aug 7, 2017

@nyalldawson and @nirvn do you have any objection to merge ?

@nyalldawson
Copy link
Collaborator

Building now for a test...

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

@rldhont I just tested and it's working very (very!) nicely. My one concern is with the UI here. What I'm wondering is whether the data defined control for preserving the aspect ratio is really necessary here? If it isn't, we should use @nirvn's new QgsRatioLockButton instead of a checkbox for the ratio lock widget. This will keep consistency between different parts of the UI, and is a lot nicer than a plain checkbox.

I also think the units combo box should be vertically centered between the width and height spin boxes. At the moment it's bottom aligned, so it doesn't look associated with the width spin box.

* \param far Fixed Aspect Ratio
* \see fixedAspectRatio() QgsSvgCache
*/
void setFixedAspectRatio( double far ) { mFixedAspectRatio = far; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

far -> ratio

QString mPath;

double mDefaultAspectRatio = 0.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are protected members, they'll need doc strings (or make them private)

@@ -437,7 +437,9 @@ class GUI_EXPORT QgsSvgMarkerSymbolLayerWidget : public QgsSymbolLayerWidget, pr
public slots:
void setName( const QModelIndex &idx );
void populateIcons( const QModelIndex &idx );
void setSize();
void setWidth();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make these private slots? I don't like how all these widgets expose a lot of their internal workings publicly (and hence become part of stable API). If not, they need docstrings added

@rldhont rldhont force-pushed the svg-marker-size branch 2 times, most recently from d7ac750 to 2ab222a Compare August 20, 2017 14:17
@rldhont
Copy link
Contributor Author

rldhont commented Aug 23, 2017

@m-kuhn, @nirvn I have updated the UI and code as you requested it.
Is it ok for you ?

@nirvn
Copy link
Contributor

nirvn commented Aug 23, 2017

@rldhont the checkbox label is being chopped off now:
screenshot from 2017-08-23 17-10-32

@DelazJ
Copy link
Contributor

DelazJ commented Aug 23, 2017

Also, how about left aligning the "Preserve..." checkbox with "Height" and "Width" labels (capitalize first letter)? afaics, they are of the same level: subitems of the Size parameter.

@nyalldawson
Copy link
Collaborator

My view would still be to use the ratio luck button - it's much much less visual clutter then a checkbox with a lengthy label

@nyalldawson
Copy link
Collaborator

Luck=lock

@rldhont
Copy link
Contributor Author

rldhont commented Aug 23, 2017

@nyalldawson @nirvn does the ratio lock button have documentation ? Is it already available in QGIS source tree ?

@nyalldawson
Copy link
Collaborator

QgsRatioLockButton. It's in master and should be suggested to use (API docs are there)

@rldhont
Copy link
Contributor Author

rldhont commented Aug 27, 2017

Hi devs,
@nyalldawson QgsRatioLockButton added
@DelazJ labels updated
@nirvn Preserve Aspect Ratio checkbox removed
Telle me if it is good for you ?

@nirvn
Copy link
Contributor

nirvn commented Aug 28, 2017

@rldhont , looking much better with the radio lock button:
screenshot from 2017-08-28 10-33-40

There's an issue with the ratio lock button's pressed state, whereas the state is still pressed even though the ratio lock is disabled. Check the save as image dialog to see how it should look.

Otherwise, LGTM and can't wait for it to be merged.

@rldhont
Copy link
Contributor Author

rldhont commented Aug 29, 2017

@dmarteau can you take a look ?

@rldhont
Copy link
Contributor Author

rldhont commented Sep 7, 2017

@nirvn I don't understand how to fix this ui point

@rldhont
Copy link
Contributor Author

rldhont commented Sep 11, 2017

@nirvn I have updated the way Size, Height and lock button is used. But I have an issue with the setDown button, if the button is firstly displayed as locked. Can we fix this after ?

@rldhont
Copy link
Contributor Author

rldhont commented Sep 28, 2017

@nirvn the setDown is fixed! Can I merge ?

@rldhont
Copy link
Contributor Author

rldhont commented Oct 4, 2017

I'll merge this PR tomorrow if I've no more comments in the meantime.

@nirvn
Copy link
Contributor

nirvn commented Oct 4, 2017

@rldhont , sorry for the delay -- the PR is fine by me. Thanks, great feature addition.

@rldhont rldhont merged commit 3e45f99 into qgis:master Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Requires Tests! Waiting on the submitter to add unit tests before eligible for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants