Export parametric SVG, will fallback symbols for the systems that cannot understand them

[aaime]

This pull request makes the SVG parameters explicit, but at the same time leverages SLD fallback symbol support to add a non parametric SVG fallback, as well as a simple mark with the right color and outline for systems that cannot deal with SVG, or lack the symbol on their file system.

[aaime]

This was removed as the svg outline is not the same thing as the polygon fill outline

[nyalldawson]

A few minor suggestions, but this looks good in general to me! Will be very nice to have!

[nyalldawson]

I'd suggest:

1. Rename color to fillColor
2. Make this return the QDomElement instead of passing it as an argument

[aaime]

Renaming to fill color, not a problem.
The second I'm not 100% syre, as I (thought I) was just following an established pattern, e.g.:
https://github.com/qgis/QGIS/blob/master/src/core/symbology-ng/qgssymbollayerutils.h#L242
https://github.com/qgis/QGIS/blob/master/src/core/symbology-ng/qgssymbollayerutils.h#L248
https://github.com/qgis/QGIS/blob/master/src/core/symbology-ng/qgssymbollayerutils.h#L257

However I also see smaller helper methods returning the element indeed:
https://github.com/qgis/QGIS/blob/master/src/core/symbology-ng/qgssymbollayerutils.h#L320
https://github.com/qgis/QGIS/blob/master/src/core/symbology-ng/qgssymbollayerutils.h#L323

So I'll follow your advice for this one too

[nyalldawson]

I personally don't have a strong preference for either, but the vast majority of the xml writing methods in QGIS return the element. So my vote goes to consistency :)

[aaime]

I had another look and believe it should be kept as is. Here is an example of calling code:

void QgsSvgMarkerSymbolLayer::writeSldMarker( QDomDocument &doc, QDomElement &element, const QgsStringMap& props ) const
{
  // <Graphic>
  QDomElement graphicElem = doc.createElement( QStringLiteral( "se:Graphic" ) );
  element.appendChild( graphicElem );

  // encode a parametric SVG reference
  double size = QgsSymbolLayerUtils::rescaleUom( mSize, mSizeUnit, props );
  double outlineWidth = QgsSymbolLayerUtils::rescaleUom( mOutlineWidth, mOutlineWidthUnit, props );
  QgsSymbolLayerUtils::parametricSvgToSld( doc, graphicElem, mPath, mColor, size, mOutlineColor, outlineWidth );

  // <Rotation>
  QString angleFunc;
  bool ok;
  double angle = props.value( QStringLiteral( "angle" ), QStringLiteral( "0" ) ).toDouble( &ok );
  if ( !ok )
  {
    angleFunc = QStringLiteral( "%1 + %2" ).arg( props.value( QStringLiteral( "angle" ), QStringLiteral( "0" ) ) ).arg( mAngle );
  }
  else if ( !qgsDoubleNear( angle + mAngle, 0.0 ) )
  {
    angleFunc = QString::number( angle + mAngle );
  }

  QgsSymbolLayerUtils::createRotationElement( doc, graphicElem, angleFunc );

  // <Displacement>
  QPointF offset = QgsSymbolLayerUtils::rescaleUom( mOffset, mOffsetUnit, props );
  QgsSymbolLayerUtils::createDisplacementElement( doc, graphicElem, offset );
}

As you can see the pattern of passing "doc and element" is spread across the whole method, and parametricSvgToSld is not the only contributor to the creation of graphic.

[aaime]

@nyalldawson what do you think? I can still change that to break the existing pattern and build the element instead of accepting it... it would just look odd in the context where it's used, not the end of the world I presume ;-)

[nyalldawson]

@aaime - no, it's all good. your reasoning is sound!

[nyalldawson]

It's possible that an svg could use the outline-width parameter without having an outline-color. Actually that applies to all parameters, eg fill opacity without paramaterised fill colors.

[aaime]

Will fix that

[aaime]

Pushed some fixes:

• fillColor instead of color
• encode width param anyways
• if a color is not valid in svg params, use black as a fallback to ensure all params are always encoded.

By the way, I noticed that the SVG param expansion code seems to assume colors are never invalid, I don't see checks.

[nyalldawson]

These also should be split up - eg you could potentially use 'fill-opacity' without any 'fill' parameter for a multi-colored fill svg. (same with outline opacity).

[aaime]

Hum hum... how is that done though? The existing callers only have a QColor, which embodies both color and opacity. You mean it would be better to have a string (encoded color) and a opacity (number) for each color, so that the caller can decide which parameters are going to be replaced?

As noted before, the actual rendering code seems to just assume colors are always present and valid, which is a bit strange, but I did not yet play with the UI and see if the fill can be excluded, and how.

[nyalldawson]

ah... ignore me.... i was confused.

[aaime]

Thank you @nyalldawson !